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

[Aaron Conole <aconole@redhat.com>]

Andrew Rybchenko <arybchenko@solarflare.com> writes:

> 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@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.
>
> 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.

At least, hopefully the reviewer knows from the changestat what to look
for to narrow it down.

> 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 :(

That's a problem for which I have no solution.  If you solve it please
let me know how. :)

>  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 :)