[dpdk-dev] [PATCH] devtools: check stable tag in fixes

Yuanhan Liu yuanhan.liu at linux.intel.com
Wed Jan 18 05:41:50 CET 2017


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 at 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 <jingjing.wu at intel.com>
    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 <jingjing.wu at intel.com>
        Acked-by: John McNamara <john.mcnamara at intel.com>
    
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 at 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: <mail_address>" 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.


More information about the dev mailing list