[dpdk-dev] [PATCH 00/51] ethdev: change rte_eth_dev_info_get() return value to int

[Andrew Rybchenko]

On 8/29/19 8:05 PM, Thomas Monjalon wrote:
> 28/08/2019 16:29, Andrew Rybchenko:
>> On 8/28/19 4:42 PM, Aaron Conole wrote:
>>> Andrew Rybchenko <arybchenko at solarflare.com> writes:
>>>> On 8/27/19 11:47 PM, Aaron Conole wrote:
>>>>> Andrew Rybchenko <arybchenko at solarflare.com> writes:
>>>>>
>>>>>> It is the first patch series to get rid of void returning functions
>>>>>> in ethdev in accordance with deprecation notice [1].
>>>>> This is a huge series, and I suggest to combine some of the work, and/or
>>>>> break it up.
>>>> I can send patches for examples separately, but it will not help a lot.
>>>> I can squash changes in examples, but I think it is wrong since it would
>>>> make review harder - different maintainers and different practices to
>>>> handle error in different examples (and we tried to take it into account).
>>> Hrrm?  Not sure what you mean.
>> I mean that it is easier to review many small patches than one huge patch
>> especially when these files are maintained by different people.
>>
>>> Patches should be broken up by logical change.  That way, it is easy to
>>> bisect and isolate what has changed.  This series, it seems like there's
>>> a single logical change, and that's been spread over lots of patches.
>> Single huge patch is worse for both bisect and review. When patch is huge
>> and bisect says that the patch is guilty, you still need to find out which
>> snippet/change is guilty.
>>
>> In this particular case nothing prevents to split the patch make it easier
>> to review and bisect.
>>
>>> I think grouping all the examples and all the app/test together, would
>>> make the series 14 review-able patches.  As it is, stepping through 40+
>>> 10-line emails is much more tedious (not to mention needing to apply
>>> them, check each for build, etc).
>> Yes, less build cycles are required for smaller number of patches, but
>> anyway automation does (should do) it. So, not that important.
>>
>> I disagree that it is easier to review one huge patch. Sorry.
>> I think it is important here that different examples are maintained
>> by different people. Anyway if more reviewers will support the idea
>> to squash examples into once patch, technically it is trial to do.
> When doing same kind of change on multiple applications,
> splitting patches won't help any bisect (they are all different
> applications anyway). And I think squashing can better show the idea that
> every applications got the same kind of change (thanks for that).
> In general, I think patches deserve to be split when there is something
> interesting to say in the commit log. If you want to describe a
> different logic of each app, why not. The other way is to explain
> some exceptions for some applications in a signle patch.

Thanks a lot, makes sense and I agree with explanations.
The only problem is review of such huge patches, when
reviewer needs to find out his snippet. Also split version
is better for patchwork reviewed/acked counters. It is
clear which snippets are reviewed, which are not.
Unfortunately split does not help this patchset to get
more attention from reviewers :(

> My conclusion is that it is often hard to find the good balance,
> between split and squash, and I can understand any motivation :)
> Sometimes I squash some patches on apply after they got all reviewed
> separately. In this case, I didn't look yet :)