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