Message-ID: <20170816232139.GB14728@fury>
Date:   Wed, 16 Aug 2017 16:21:39 -0700
From:   Darren Hart <dvhart@...radead.org>
To:     Olof Johansson <olof@...om.net>, Arnd Bergmann <arnd@...db.de>
Cc:     Stephen Rothwell <sfr@...b.auug.org.au>,
        Linux-Next Mailing List <linux-next@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "Gustavo A. R. Silva" <gustavo@...eddedor.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Dan Carpenter <dan.carpenter@...cle.com>
Subject: Re: linux-next: Signed-off-by missing for commit in the drivers-x86
 tree

+Olof and Arnd,

I am curious how you handle the situation below as a maintainer team.

This problem arose from a new for-next test which triggers on the
committer not having a SOB tag in the patch (which can happen when a
shared branch is rebased to drop a patch).

Do you have a branch that you both use for testing and automated testing
which you occasionally need to drop patches from before moving them to a
publication branch like "for-next" or "fixes"? I understand you tend to
pull from sub-maintainers, so perhaps our contexts are fairly different.

Andy and I have brainstormed various approaches to addressing this, and
all of the cures appear worse than the disease from a scalability and/or
chance of error perspective (outlined below).

Linus has been clear he sees "rebase --signoff" to be the wrong thing to
do for "publicized branches" (see my comment below on published vs.
collaboration branches).

On Sat, Aug 05, 2017 at 02:58:29PM -0700, Darren Hart wrote:
> On Thu, Aug 03, 2017 at 08:50:06AM -0700, Darren Hart wrote:
> > On Wed, Aug 02, 2017 at 06:06:20PM -0700, Linus Torvalds wrote:
> > > On Wed, Aug 2, 2017 at 5:28 PM, Stephen Rothwell <sfr@...b.auug.org.au> wrote:
> > > >
> > > > I would say that if you rebase someone's commit(s), then you are on the
> > > > "patch's delivery path" and so should add a Signed-off-by tag.
> > > 
> > > Yeah, I agree. Rebasing really is pretty much the exact same thing as
> > > applying a patch.
> 
> I will be away for a few days, but will follow up on this when I return.
> In the meantime, my plan is to leave the current for-next branch alone
> rather than rebasing it to fix the previous rebase which resulted in the
> mixed committer/signoff issue Stephen's new test identified.
> 
> I just want it to be clear I'm not ignoring the issue, but rather
> planning on addressing it in commits going forward - based on the
> results of the discussion below.
> 
> Thanks,
> 
> > > 
> > > > "git rebase" does have a "--signoff" option.
> > > 
> > > I think you end up signing off twice using that. I don't think it's
> > > smart enough to say "oh, you already did it once".
> > > 
> > > But I didn't check. Sometimes git is a lot smarter than I remember it
> > > being, simply because I don't worry about it. Junio does a good job.
> > > 
> > > And in general, you simply should never rebase commits that have
> > > already been publicized. And the fact that you didn't commit them in
> > > the first place definitely means that they've been public somewhere.
> > 
> > For the platform driver x86 subsystem, Andy I have defined our "testing"
> > branch as mutable. It's the place where our CI pulls from, as well as
> > the first place 0day pulls from, and where we stage things prior to
> > going to the publication branches ("for-next" and then sometimes
> > "fixes"). We find it valuable to let the robots have a chance to catch
> > issues we may have missed before pushing patches to a publication
> > branch, but to do that, we need the testing branch to be accessible to
> > them.
> > 
> > The usual case that would land us in the situation here is we discover a
> > bug in a patch and revert it before going to a publication branch.
> > Generally, this will involve one file (most patches here are isolated),
> > which we drop via rebase, and the rest are entirely unaffected in terms
> > of code, but as the tree changed under them, they get "re-committed".
> > 
> > This seems like a reasonable way to handle a tree with more than one
> > maintainer and take advantage of some automation. Andy and I do need a
> > common tree to work from, and I prefer to sync with him as early in the
> > process as possible, rather than have him and I work with two private
> > testing branches and have to negotiate who takes which patches. It would
> > slow us down and wouldn't improve quality in any measurable way. Even if
> > we did this work in an access controlled repository, we would still have
> > this problem.
> > 
> > With more and more maintainer teams, I think we need to distinguish
> > between "published" branches and "collaboration" branches. I suspect
> > maintainer teams will expose this rebasing behavior, but I don't believe
> > it is new or unique to us. To collaborate, we need a common branch,
> > which a lone maintainer doesn't need, and the committer/sign-off delta
> > makes this discoverable, whereas it was invisible with a lone
> > maintainer.
> > 
> > Note: A guiding principle behind our process is that of not introducing
> > bugs into mainline. Rather than reverting bad patches in testing, we
> > drop them, and replace them with a fixed version. The idea being we
> > don't want to introduce git bisect breakage, and we don't want to open
> > the window for stable/distro maintainers to pull a bad patch and forget
> > the revert or the fixup. If we can correct it before it goes to Linus,
> > we do.
> > 
> > > So I would definitely suggest against the "git rebase --signoff"
> > > model, even if git were to do the "right thing". It's simply
> > > fundamentally the wrong thing to do. Either you already committed them
> > > (and hopefully signed off correctly the first time), or you didn't
> > > (and you shouldn't be rebasing). So in neither case is "git rebase
> > > --signoff" sensible.
> > 
> > So in light of the above, we can:
> > 
> > a) Keep doing what we're doing
> > b) Sign off whenever we rebase
> > c) Add our signoff whenever we move patches from testing to for-next
> >    (I hadn't considered this until now... this might be the most
> >     compatible with maintainer teams while strictly tracking the
> >     "patches" delivery path")
> > d) Redefine testing as immutable and revert patches rather than drop
> >    them, introducing bugs into mainline.
> > e) Make each maintainer work from a private set of branches (this just
> >    masks the problem by making the rebase invisible)
> > 
> > Whatever we decide, I'd like to add this to some documentation for
> > maintainer teams (which I'm happy to prepare and submit).

-- 
Darren Hart
VMware Open Source Technology Center

Powered by blists - more mailing lists