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