From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752719AbdHEV6d (ORCPT ); Sat, 5 Aug 2017 17:58:33 -0400 Received: from bombadil.infradead.org ([65.50.211.133]:54874 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751662AbdHEV6b (ORCPT ); Sat, 5 Aug 2017 17:58:31 -0400 Date: Sat, 5 Aug 2017 14:58:29 -0700 From: Darren Hart To: Linus Torvalds Cc: Stephen Rothwell , Linux-Next Mailing List , Linux Kernel Mailing List , "Gustavo A. R. Silva" , Andy Shevchenko , Dan Carpenter Subject: Re: linux-next: Signed-off-by missing for commit in the drivers-x86 tree Message-ID: <20170805215829.GC1277@fury> References: <20170803063743.1d50a5d2@canb.auug.org.au> <20170802235740.GB27974@fury> <20170803102810.37f7c6b0@canb.auug.org.au> <20170803155006.GA2234@fury> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170803155006.GA2234@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 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