Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1702453 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  linux-next: Signed-off-by missing for commit in the drivers-x86  tree Stephen Rothwell <sfr@canb.auug.org.au> - 2017-08-02 22:40 +0200
    Re: linux-next: Signed-off-by missing for commit in the drivers-x86  tree Darren Hart <dvhart@infradead.org> - 2017-08-03 02:00 +0200
      Re: linux-next: Signed-off-by missing for commit in the drivers-x86  tree Stephen Rothwell <sfr@canb.auug.org.au> - 2017-08-03 02:30 +0200
        Re: linux-next: Signed-off-by missing for commit in the drivers-x86 tree Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-03 03:10 +0200
          Re: linux-next: Signed-off-by missing for commit in the drivers-x86  tree Darren Hart <dvhart@infradead.org> - 2017-08-03 18:00 +0200
            Re: linux-next: Signed-off-by missing for commit in the drivers-x86  tree Darren Hart <dvhart@infradead.org> - 2017-08-06 00:00 +0200
          Re: linux-next: Signed-off-by missing for commit in the drivers-x86 tree Junio C Hamano <gitster@pobox.com> - 2017-08-04 19:50 +0200
            Re: linux-next: Signed-off-by missing for commit in the drivers-x86  tree Darren Hart <dvhart@infradead.org> - 2017-08-04 19:50 +0200
    Re: linux-next: Signed-off-by missing for commit in the drivers-x86  tree Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-08-03 10:20 +0200
      Re: linux-next: Signed-off-by missing for commit in the drivers-x86  tree Stephen Rothwell <sfr@canb.auug.org.au> - 2017-08-03 11:30 +0200

#1702453 — linux-next: Signed-off-by missing for commit in the drivers-x86 tree

Hi Darren,

Commits

  890f658c101d ("platform/x86: peaq-wmi: silence a static checker warning")
  6d8d55626296 ("platform/x86: msi-wmi: remove unnecessary static in msi_wmi_notify()")
  cd0223c64c60 ("platform/x86: ibm_rtl: remove unnecessary static in ibm_rtl_write()")

are missing Signed-off-by's from their commiter.

-- 
Cheers,
Stephen Rothwell

[toc] | [next] | [standalone]

#1702580

On Thu, Aug 03, 2017 at 06:37:43AM +1000, Stephen Rothwell wrote:
> Hi Darren,
> 
> Commits
> 
>   890f658c101d ("platform/x86: peaq-wmi: silence a static checker warning")
>   6d8d55626296 ("platform/x86: msi-wmi: remove unnecessary static in msi_wmi_notify()")
>   cd0223c64c60 ("platform/x86: ibm_rtl: remove unnecessary static in ibm_rtl_write()")
> 
> are missing Signed-off-by's from their commiter.

So each of these was originally committed by Andy... but appear as
committed by me. This must have occured as a rebase of our testing
branch I suppose. Nothing has been out of the ordinary this development
cycle, so I wonder that we haven't received such a report previously.
Hrm.

Is this a new check Stephen?

Is there any statement regarding maintainer teams that we must abide by
this?  e.g. any time a rebase in a testing branch is made, the
maintainer must also ensure a SOB is on each patch?

I just want to get a clear picture of what the failure was so we can
update our tooling so this doesn't repeat itself.

-- 
Darren Hart
VMware Open Source Technology Center

[toc] | [prev] | [next] | [standalone]

#1702588

Hi Darren,

On Wed, 2 Aug 2017 16:57:40 -0700 Darren Hart <dvhart@infradead.org> wrote:
>
> Is this a new check Stephen?

Yes :-)

> Is there any statement regarding maintainer teams that we must abide by
> this?  e.g. any time a rebase in a testing branch is made, the
> maintainer must also ensure a SOB is on each patch?

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. (cc'ing
Linus to see if he has an opinion.  Linus, the case here is a patch
originally committed by one maintainer and then rebased by the other.)

> I just want to get a clear picture of what the failure was so we can
> update our tooling so this doesn't repeat itself.

"git rebase" does have a "--signoff" option.

-- 
Cheers,
Stephen Rothwell

[toc] | [prev] | [next] | [standalone]

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

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.

> "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.

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.

              Linus

[toc] | [prev] | [next] | [standalone]

#1703220

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.
> 
> > "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).

Thanks,

-- 
Darren Hart
VMware Open Source Technology Center

[toc] | [prev] | [next] | [standalone]

#1704715

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

[toc] | [prev] | [next] | [standalone]

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

Linus Torvalds <torvalds@linux-foundation.org> writes:

> 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.
>
>> "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".

Git avoids duplication only when your SoB appears as the last
existing one, so that we can capture a flow of a patch which you
originally signed off, picked up and tweaked further by somebody
else, which comes back to you and you sign it off again.

We may drop yours even when yours is not the last in the existing
chain, but that would be a bug; at least the above is what we try to
do.

> 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.
>
> 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.

When those involved are using push/pull as a replacement for
e-mailed patch exchange, then such a workflow should be OK.  There
needs to be a shared understanding that the branch(es) used for such
exchange are unstable and should not be built directly on to be
merged, of course.

[toc] | [prev] | [next] | [standalone]

#1704139

On Fri, Aug 04, 2017 at 10:44:31AM -0700, Junio C Hamano wrote:
> Linus Torvalds <torvalds@linux-foundation.org> writes:
> 
> > 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.
> >
> >> "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".
> 
> Git avoids duplication only when your SoB appears as the last
> existing one, so that we can capture a flow of a patch which you
> originally signed off, picked up and tweaked further by somebody
> else, which comes back to you and you sign it off again.
> 
> We may drop yours even when yours is not the last in the existing
> chain, but that would be a bug; at least the above is what we try to
> do.
> 
> > 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.
> >
> > 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.
> 
> When those involved are using push/pull as a replacement for
> e-mailed patch exchange, then such a workflow should be OK.  There
> needs to be a shared understanding that the branch(es) used for such
> exchange are unstable and should not be built directly on to be
> merged, of course.
> 

Thanks Junio,

I don't think I correctly parsed "should not be built directly on to be
merged", can you rephrase?

-- 
Darren Hart
VMware Open Source Technology Center

[toc] | [prev] | [next] | [standalone]

#1702763

On Thu, 2017-08-03 at 06:37 +1000, Stephen Rothwell wrote:
> Hi Darren,
> 
> Commits
> 
>   890f658c101d ("platform/x86: peaq-wmi: silence a static checker
> warning")
>   6d8d55626296 ("platform/x86: msi-wmi: remove unnecessary static in
> msi_wmi_notify()")
>   cd0223c64c60 ("platform/x86: ibm_rtl: remove unnecessary static in
> ibm_rtl_write()")
> 
> are missing Signed-off-by's from their commiter.
> 

I just checked what we have in our for-next branch and mentioned commits
have mine SoB. Should they have something else?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[toc] | [prev] | [next] | [standalone]

#1702831

Hi Andy,

On Thu, 03 Aug 2017 11:17:03 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>
> I just checked what we have in our for-next branch and mentioned commits
> have mine SoB. Should they have something else?

In Darren's response it became clear that even though you had initially
commited the patches, he had rebased them so he is now the committer.
So he should have added a Signed-off-by tag.


-- 
Cheers,
Stephen Rothwell

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web