From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id E43F6108F for ; Wed, 18 Jan 2017 05:39:40 +0100 (CET) Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga103.jf.intel.com with ESMTP; 17 Jan 2017 20:39:39 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,247,1477983600"; d="scan'208";a="54277476" Received: from yliu-dev.sh.intel.com (HELO yliu-dev) ([10.239.67.162]) by orsmga005.jf.intel.com with ESMTP; 17 Jan 2017 20:39:38 -0800 Date: Wed, 18 Jan 2017 12:41:50 +0800 From: Yuanhan Liu To: Thomas Monjalon Cc: Ferruh Yigit , dev@dpdk.org, pablo.de.lara.guarch@intel.com, bruce.richardson@intel.com Message-ID: <20170118044150.GN10293@yliu-dev.sh.intel.com> References: <1484664872-26859-1-git-send-email-thomas.monjalon@6wind.com> <1860075.ojoIIAvjcn@xps13> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1860075.ojoIIAvjcn@xps13> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-dev] [PATCH] devtools: check stable tag in fixes 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: , X-List-Received-Date: Wed, 18 Jan 2017 04:39:41 -0000 On Tue, Jan 17, 2017 at 07:42:33PM +0100, Thomas Monjalon wrote: > 2017-01-17 18:15, Ferruh Yigit: > > On 1/17/2017 2:54 PM, Thomas Monjalon wrote: > > > The tag "Cc: stable@dpdk.org" must be set when the commit must be > > > backported to a stable branch. > > > > > > It must be located just below the "Fixes:" tag (without blank line) > > > and followed by a blank line, separated from SoB and review tags below. > > > > I am OK to keep it if it will help stable tree maintenance, but I still > > not clear about why we need this. > > > > If a patch is a fix, it should already have "Fixes:" line, so this can > > be used to parse git history. Same answer (as I have already replied to you in another email): not all fix patches should be picked to stable branch. (I gave some examples below) > That's a question for Yuanhan. My comments below: > > Some fixes are not candidate for the stable branch because the bug was > not in the previous release. These fixes are filtered out by the script > devtools/git-log-fixes.sh > Some fixes are not so important and we can decide they do not fit in > the stable branch. Yes, I see no reason to pick patches that fix a typo, a comment issue, a coding style issue, or even, a hypothetic bug. Usually, it should be a patch that fixes a solid bug, like a crash, or something like "it behaves abnormally without the fix". That's the basic rule. Upon that, I think we could lower the bar a bit case by case. For example, I picked a doc update to v16.07.2: commit 92b70d21ee29bad92766699d0b45f579a2ff9adc Author: Jingjing Wu Date: Fri Sep 30 14:46:23 2016 +0800 doc: add limitations for i40e PMD [ upstream commit 972cc03ac4e30a7df8f734a77021bb15d0419b55 ] This patch adds "Limitations or Known issues" section for i40e PMD, including two items: 1. MPLS packet classification on X710/XL710 2. 16 Byte Descriptor cannot be used on DPDK VF 3. Link down with i40e kernel driver after DPDK application exist Signed-off-by: Jingjing Wu Acked-by: John McNamara It adds some limitations to the code introduced in last release: I think it's an important note the user might want to know if he sticks to that stable release. Talking about that, I think this patch makes abuse of the "Cc: stable" tag if a developer has to (or must) add such tag for a fix patch, no matter what it fixes. > Who make this decision? Relying on this Cc tag would > mean the committers decide which patch to backport. Ideally, it's the developer to make such decision: he knows the best what he is trying to fix, he also knows which version is vulnerable. But that's not something a new contributor might be aware of, and that's what the maintainer (not the committer) could be helpful here: tell him it's a candidate for a stable release and guide him on howto. The reason I want to stress the point of "the maintainer but not the committer" is, normally, the maintainer knows the code better. > > If patch is a feature, as far as I know still can go to stable tree, but > > for this case stable tree maintainer decides this, and author putting > > "Cc: stable@dpdk.org" tag not so useful. Author can put this tag just > > for recommendation, but if so why we are saving this into git history? > > No feature should be backported. > > > Initially this was to be sure fixes CC'ed to stable mail list, now > > meaning is changing I guess. For the case author already cc'ed the > > stable tree but not put Cc: tag into git commit, should committer add > > this explicitly or ask from author a new version of the patch? > > Yuanhan was suggesting that the committer can do it if an author forget. Yes, it's just part of the committer job, say, adding Tested-by, Reviewed-by, that kind of stuff. > > > Last thing, if this tag will remain in the commit log, is this only for > > stable tree, or any "Cc: " can stay in the history? > > I do not see the benefit of keeping other Cc in the history. Again, I really don't know why you bother to remove it, manually, commit by commit. What's the gain here? It just adds more burden to a committer. Honestly, I never do that. It actually has benefits. Keeping the cc tags allows us to cc them when we find a bug in that patch and make a fix patch later: they are likely to want to know any follow updates on that original patch. It also helps when I send out a stable patch review: I send a copy to all guys on the Cc list, on the Ack/Review, SoB list. I think they might also want to know that patch is a candidate for a specific branch. He may even give some comments, something like "if you pick this one, you might want to pick another one", or "why bother to port it to a stable release", that kind of thing. Honestly, for a commit log, besides the very basic format issues, like too long lines, words are tightly organized, I just care (and care a lot) one thing only: does the commit log states thing clearly? If it's a bug fix, does it state how the bug comes from and how to fix it, very specifically? If it's a feature patch (or set), does it describes what the feature is, what's it for, how it's implemented, with great details? If yes, I'm very happy about it. If no, I'd complain about it and ask him to reword it. After all, that's something I really want to know when I look back the git history (it normally happens when I want to know more explanations than the code could provide me). I don't care if it has few more Cc lines, few more fix lines, few more SoBs, etc. I don't even care it has typos, don't even to say he writes "rx" but not "Rx". Sorry, too much "complains" :) --yliu > It is just a convenience for authors when using git-send-email.