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

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 8/29/2019 6: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@solarflare.com> writes:
>>>> On 8/27/19 11:47 PM, Aaron Conole wrote:
>>>>> Andrew Rybchenko <arybchenko@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.
> 
> 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 :)
> 

I would like to get this early if possible, before end of this week when I
expect there will be a peak of patches before the deadline.

Overall patchset looks good to me, only one comment on the testpmd one, it also
passes automated builds/checks (including patch by patch builds).
(ring_pmd_autotest needs to be fixed of course)

For the patchset, I agree with Andrew's argument, yes logically same change and
apps/examples/tests can be merged into single commit but it would be big &
harder to review. Since it is already split, for me it looks good as it is.