From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Monjalon Subject: Re: [PATCH] devtools: check stable tag in fixes Date: Wed, 18 Jan 2017 09:32:22 +0100 Message-ID: <21531262.6kCXYB22gk@xps13> References: <1484664872-26859-1-git-send-email-thomas.monjalon@6wind.com> <1860075.ojoIIAvjcn@xps13> <20170118044150.GN10293@yliu-dev.sh.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7Bit Cc: Ferruh Yigit , dev@dpdk.org, pablo.de.lara.guarch@intel.com, bruce.richardson@intel.com To: Yuanhan Liu Return-path: Received: from mail-wm0-f44.google.com (mail-wm0-f44.google.com [74.125.82.44]) by dpdk.org (Postfix) with ESMTP id 90B0D106A for ; Wed, 18 Jan 2017 09:32:25 +0100 (CET) Received: by mail-wm0-f44.google.com with SMTP id f73so45276238wmf.1 for ; Wed, 18 Jan 2017 00:32:25 -0800 (PST) In-Reply-To: <20170118044150.GN10293@yliu-dev.sh.intel.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 2017-01-18 12:41, Yuanhan Liu: > 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. So it should be stressed in the contribution guide that it is the responsibility of the author and the maintainer to put this Cc: tag. > > > 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. I had the personal feeling that if the Cc: person is not in SoB, Ack or Review tags, it means he's not interested in this patch. After thinking more, I was probably wrong. > 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. OK Yuanhan, thanks for the explanations. I will send a v2 to relax the constraint of blank line below Fixes: tag, and change the warning when Cc: tag is missing. Or remove the warning? What do you think of: "Reminder: is it a candidate for stable@dpdk.org backport?"