From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 6C6A8A2E1B for ; Tue, 3 Sep 2019 16:08:58 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 992D91EC76; Tue, 3 Sep 2019 15:59:44 +0200 (CEST) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id 3D58D1EAD6 for ; Tue, 3 Sep 2019 15:58:11 +0200 (CEST) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 4E8411089044; Tue, 3 Sep 2019 13:58:10 +0000 (UTC) Received: from dhcp-25.97.bos.redhat.com (unknown [10.18.25.8]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 826E660A35; Tue, 3 Sep 2019 13:58:09 +0000 (UTC) From: Aaron Conole To: Andrew Rybchenko Cc: Thomas Monjalon , Ferruh Yigit , "dev\@dpdk.org" , "Ivan Ilchenko" References: <1566915962-5472-1-git-send-email-arybchenko@solarflare.com> <660e1152-9ae5-8698-5e1f-bca45943cfd9@solarflare.com> <13169824.Gphp0D0tZG@xps> <66023bf0-b49d-452e-d6a0-f9cb84cab86e@solarflare.com> Date: Tue, 03 Sep 2019 09:58:08 -0400 In-Reply-To: <66023bf0-b49d-452e-d6a0-f9cb84cab86e@solarflare.com> (Andrew Rybchenko's message of "Tue, 3 Sep 2019 15:27:25 +0300") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.6.2 (mx1.redhat.com [10.5.110.64]); Tue, 03 Sep 2019 13:58:10 +0000 (UTC) Subject: Re: [dpdk-dev] [PATCH 00/51] ethdev: change rte_eth_dev_info_get() return value to int X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Andrew Rybchenko 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 writes: > > On 8/27/19 11:47 PM, Aaron Conole wrote: > > Andrew Rybchenko 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 :)