From: Darren Hart <dvhart@infradead.org>
To: Olof Johansson <olof@lixom.net>, Arnd Bergmann <arnd@arndb.de>
Cc: Stephen Rothwell <sfr@canb.auug.org.au>,
Linux-Next Mailing List <linux-next@vger.kernel.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
"Gustavo A. R. Silva" <gustavo@embeddedor.com>,
Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
Linus Torvalds <torvalds@linux-foundation.org>,
Dan Carpenter <dan.carpenter@oracle.com>
Subject: Re: linux-next: Signed-off-by missing for commit in the drivers-x86 tree
Date: Wed, 16 Aug 2017 16:21:39 -0700 [thread overview]
Message-ID: <20170816232139.GB14728@fury> (raw)
In-Reply-To: <20170805215829.GC1277@fury>
+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@canb.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
next prev parent reply other threads:[~2017-08-16 23:21 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-02 20:37 linux-next: Signed-off-by missing for commit in the drivers-x86 tree Stephen Rothwell
2017-08-02 23:57 ` Darren Hart
2017-08-03 0:28 ` Stephen Rothwell
2017-08-03 1:06 ` Linus Torvalds
2017-08-03 15:50 ` Darren Hart
2017-08-05 21:58 ` Darren Hart
2017-08-16 23:21 ` Darren Hart [this message]
2017-08-24 20:56 ` Darren Hart
2017-08-04 17:44 ` Junio C Hamano
2017-08-04 17:47 ` Darren Hart
2017-08-03 8:17 ` Andy Shevchenko
2017-08-03 9:27 ` Stephen Rothwell
2018-06-01 11:36 Stephen Rothwell
2018-06-01 11:40 ` Andy Shevchenko
2018-06-01 12:08 ` Stephen Rothwell
2018-06-01 14:33 ` dvhart
2018-06-01 14:55 ` Andy Shevchenko
2018-06-01 14:38 ` dvhart
2018-06-01 15:26 ` Stephen Rothwell
2018-06-01 16:43 ` Darren Hart
2018-06-01 14:45 ` Andy Shevchenko
2018-08-18 14:35 Stephen Rothwell
2018-08-19 8:21 ` Hans de Goede
2018-08-19 8:48 ` Stephen Rothwell
2019-02-23 14:19 Stephen Rothwell
2019-02-23 17:10 ` Darren Hart
2019-02-23 17:52 ` Darren Hart
2019-02-23 22:56 ` Stephen Rothwell
2019-05-06 13:22 Stephen Rothwell
2019-05-06 14:50 ` Andy Shevchenko
2021-04-08 12:13 Stephen Rothwell
2021-04-08 14:18 ` Hans de Goede
2021-04-14 13:51 Stephen Rothwell
2021-04-14 13:55 ` Hans de Goede
2023-02-02 21:33 Stephen Rothwell
2023-02-03 9:06 ` Hans de Goede
2023-06-07 23:15 Stephen Rothwell
2023-06-08 9:02 ` Hans de Goede
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170816232139.GB14728@fury \
--to=dvhart@infradead.org \
--cc=andriy.shevchenko@linux.intel.com \
--cc=arnd@arndb.de \
--cc=dan.carpenter@oracle.com \
--cc=gustavo@embeddedor.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-next@vger.kernel.org \
--cc=olof@lixom.net \
--cc=sfr@canb.auug.org.au \
--cc=torvalds@linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).