Andrew Rybchenko <arybche...@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 <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.
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 :)
