From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752588AbdHPXVq (ORCPT ); Wed, 16 Aug 2017 19:21:46 -0400 Received: from bombadil.infradead.org ([65.50.211.133]:59338 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752382AbdHPXVo (ORCPT ); Wed, 16 Aug 2017 19:21:44 -0400 Date: Wed, 16 Aug 2017 16:21:39 -0700 From: Darren Hart To: Olof Johansson , Arnd Bergmann Cc: Stephen Rothwell , Linux-Next Mailing List , Linux Kernel Mailing List , "Gustavo A. R. Silva" , Andy Shevchenko , Linus Torvalds , Dan Carpenter Subject: Re: linux-next: Signed-off-by missing for commit in the drivers-x86 tree Message-ID: <20170816232139.GB14728@fury> References: <20170803063743.1d50a5d2@canb.auug.org.au> <20170802235740.GB27974@fury> <20170803102810.37f7c6b0@canb.auug.org.au> <20170803155006.GA2234@fury> <20170805215829.GC1277@fury> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170805215829.GC1277@fury> User-Agent: Mutt/1.8.0 (2017-02-23) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org +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 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