Re: linux-next: Signed-off-by missing for commit in the drivers-x86 tree

[Darren Hart]

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@xxxxxxxxxxxxxxxx> 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