merging pull requests

merging pull requests

[Kees Cook]

Hi!

I'm just realized that all my workflows have been entirely mbox patch
sets, and I finally have a pull request to take, and ... I have no idea
how to do it "right". :P

I see "b4 pr", but I was hoping for some kind of three-step process:
1) get the code
2) read/verify/test code
3) merge code into main brain

And that it would end up looking like what I see from Linus (i.e. naming
branching after the tags sent, etc):

6e439bbd436e Merge branch 'linus' of git://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6
a4e6f95a891a Merge tag 'pinctrl-v5.15-2' of git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl
62da74a73570 Merge tag 'vfio-v5.15-rc4' of git://github.com/awilliam/linux-vfio
e7bd807e8c9e Merge tag 'm68k-for-v5.15-tag3' of git://git.kernel.org/pub/scm/linux/kernel/git/geert/linux-m68k
dca50f08a03e Merge tag 'nios2_fixes_for_v5.15_part1' of git://git.kernel.org/pub/scm/linux/kernel/git/dinguyen/linux

It seems like "b4 pr" does part of step 1, but doesn't build a branch
from the tag (though one can use "-b").

And for step 3, when I do a "git merge --no-ff $branch", all the tag and
origin (e.g. "tag 'pinctrl-v5.15-2' of git://..." is missing).

Either I'm missing something or everyone manually adjusts the merge
commit subjects?

How are other folks doing this, and should b4 help?

-- 
Kees Cook

Re: merging pull requests

[Konstantin Ryabitsev]

On Thu, Sep 30, 2021 at 10:33:56AM -0700, Kees Cook wrote:
> Hi!
> 
> I'm just realized that all my workflows have been entirely mbox patch
> sets, and I finally have a pull request to take, and ... I have no idea
> how to do it "right". :P
> 
> I see "b4 pr", but I was hoping for some kind of three-step process:
> 1) get the code
> 2) read/verify/test code
> 3) merge code into main brain

Glad you're still around and doing well, professor Nightingale. ;)

I'm going to cc this to users@, since it's likely to reach more
maintainers' eyes (and brains) there.

> And that it would end up looking like what I see from Linus (i.e. naming
> branching after the tags sent, etc):
> 
> 6e439bbd436e Merge branch 'linus' of git://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6
> a4e6f95a891a Merge tag 'pinctrl-v5.15-2' of git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl
> 62da74a73570 Merge tag 'vfio-v5.15-rc4' of git://github.com/awilliam/linux-vfio
> e7bd807e8c9e Merge tag 'm68k-for-v5.15-tag3' of git://git.kernel.org/pub/scm/linux/kernel/git/geert/linux-m68k
> dca50f08a03e Merge tag 'nios2_fixes_for_v5.15_part1' of git://git.kernel.org/pub/scm/linux/kernel/git/dinguyen/linux
> 
> It seems like "b4 pr" does part of step 1, but doesn't build a branch
> from the tag (though one can use "-b").
> 
> And for step 3, when I do a "git merge --no-ff $branch", all the tag and
> origin (e.g. "tag 'pinctrl-v5.15-2' of git://..." is missing).

It shouldn't be if you leave it in FETCH_HEAD, which is what b4 does by
default. So, if you do "b4 pr" and then follow it by "git merge", it should
retain the pull request origin and make it the default subject there.

I just added some merge message template processing for "b4 shazam", so we can
probably do the same for "b4 pr" if that makes sense. Should b4 grab the
contents of the pull request and pre-stuff them into a file you can pass to
git-merge?

-K

Re: merging pull requests

[Kees Cook]

On Thu, Sep 30, 2021 at 04:00:02PM -0400, Konstantin Ryabitsev wrote:
> On Thu, Sep 30, 2021 at 10:33:56AM -0700, Kees Cook wrote:
> > Hi!
> > 
> > I'm just realized that all my workflows have been entirely mbox patch
> > sets, and I finally have a pull request to take, and ... I have no idea
> > how to do it "right". :P
> > 
> > I see "b4 pr", but I was hoping for some kind of three-step process:
> > 1) get the code
> > 2) read/verify/test code
> > 3) merge code into main brain
> 
> Glad you're still around and doing well, professor Nightingale. ;)

3 weeks of virtual conferences is wrecking my branch.

> I'm going to cc this to users@, since it's likely to reach more
> maintainers' eyes (and brains) there.

Good idea; thanks!

> > And that it would end up looking like what I see from Linus (i.e. naming
> > branching after the tags sent, etc):
> > 
> > 6e439bbd436e Merge branch 'linus' of git://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6
> > a4e6f95a891a Merge tag 'pinctrl-v5.15-2' of git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl
> > 62da74a73570 Merge tag 'vfio-v5.15-rc4' of git://github.com/awilliam/linux-vfio
> > e7bd807e8c9e Merge tag 'm68k-for-v5.15-tag3' of git://git.kernel.org/pub/scm/linux/kernel/git/geert/linux-m68k
> > dca50f08a03e Merge tag 'nios2_fixes_for_v5.15_part1' of git://git.kernel.org/pub/scm/linux/kernel/git/dinguyen/linux
> > 
> > It seems like "b4 pr" does part of step 1, but doesn't build a branch
> > from the tag (though one can use "-b").
> > 
> > And for step 3, when I do a "git merge --no-ff $branch", all the tag and
> > origin (e.g. "tag 'pinctrl-v5.15-2' of git://..." is missing).
> 
> It shouldn't be if you leave it in FETCH_HEAD, which is what b4 does by
> default. So, if you do "b4 pr" and then follow it by "git merge", it should
> retain the pull request origin and make it the default subject there.

I guess it depends on the expected order of operations. What do other
maintainers do to process a PR? I would think it would be:

- pull remote branch (to FETCH_HEAD)
- review (in FETCH_HEAD? in a "real" branch?)
- merge into my topic branch (where the commit subject should retain details
  of the pull location, body has notes from the signed tag, etc...)
- review, build, and test
- push to public tree

> I just added some merge message template processing for "b4 shazam", so we can
> probably do the same for "b4 pr" if that makes sense. Should b4 grab the
> contents of the pull request and pre-stuff them into a file you can pass to
> git-merge?

I think so -- that seems to be sort of what Linus does? There seem to be
a few templates (tag, branch, and "patches from Andrew"), e.g.:

Merge tag 'char-misc-5.15-rc1-2' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc
Merge branch 'misc.namei' of git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs
Merge branch 'akpm' (patches from Andrew)

Though I'd love to know why there's a distinction between "tag" and
"branch" here. Are "branch" pulls not checked for signatures?

-- 
Kees Cook

Re: merging pull requests

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 1334 bytes --]

Hi Kees,

On Thu, 30 Sep 2021 16:09:13 -0700 Kees Cook <keescook@chromium.org> wrote:
>
> I think so -- that seems to be sort of what Linus does? There seem to be
> a few templates (tag, branch, and "patches from Andrew"), e.g.:
> 
> Merge tag 'char-misc-5.15-rc1-2' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc
> Merge branch 'misc.namei' of git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs
> Merge branch 'akpm' (patches from Andrew)
> 
> Though I'd love to know why there's a distinction between "tag" and
> "branch" here. Are "branch" pulls not checked for signatures?

I assume it depends on what Linus is asked to pull/merge.  i.e. if the
pull request refers to a tag then Linus will just merge that tag.  Some
people just ask Linus to merge a branch from their tree.  In Andrew's
case, he sends patch series which Linus would import into a local
branch ("akpm") and then merge that.

The messages seem to mostly be the default git message for a merge
(sometimes maybe editted a bit).

For linux-next, I fetch all the branches into remotes in my tree and
then merge them later.  I have recently changed my merges to include the
source tree and branch in the merge message (previously, all you saws
was the local name of the remote branch).

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: merging pull requests

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 992 bytes --]

Hi Kees,

On Thu, 30 Sep 2021 16:09:13 -0700 Kees Cook <keescook@chromium.org> wrote:
>
> I guess it depends on the expected order of operations. What do other
> maintainers do to process a PR? I would think it would be:
> 
> - pull remote branch (to FETCH_HEAD)
> - review (in FETCH_HEAD? in a "real" branch?)

Our workflow usually requires that all patches are posted to an
appropriate mailing list for review.  If the developer then also has a
git branch/tag containing the patch series (after updating for
Reviewed-bys etc) then that is a separate issue and (probably) requires
a level of trust on the part of the maintainer (or a recheck) that the
patches have not been changed since the final review.

> - merge into my topic branch (where the commit subject should retain details
>   of the pull location, body has notes from the signed tag, etc...)
> - review, build, and test

"rinse and repeat" :-)

> - push to public tree

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: merging pull requests

[Kees Cook]

On Fri, Oct 01, 2021 at 09:22:31AM +1000, Stephen Rothwell wrote:
> Hi Kees,
> 
> On Thu, 30 Sep 2021 16:09:13 -0700 Kees Cook <keescook@chromium.org> wrote:
> >
> > I think so -- that seems to be sort of what Linus does? There seem to be
> > a few templates (tag, branch, and "patches from Andrew"), e.g.:
> > 
> > Merge tag 'char-misc-5.15-rc1-2' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc
> > Merge branch 'misc.namei' of git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs
> > Merge branch 'akpm' (patches from Andrew)
> > 
> > Though I'd love to know why there's a distinction between "tag" and
> > "branch" here. Are "branch" pulls not checked for signatures?
> 
> I assume it depends on what Linus is asked to pull/merge.  i.e. if the
> pull request refers to a tag then Linus will just merge that tag.  Some
> people just ask Linus to merge a branch from their tree.  In Andrew's
> case, he sends patch series which Linus would import into a local
> branch ("akpm") and then merge that.

Right, I assume for akpm's it's a local branch populated by an mbox,
etc. For the others, I guess it's just a retention of details? i.e. while
a tag points to a branch, it's name usually contains more information.

> The messages seem to mostly be the default git message for a merge
> (sometimes maybe editted a bit).

I couldn't get git to put anything like "git://" in the default subject;
it looks like "git merge" always works against a local tag/branch.

> For linux-next, I fetch all the branches into remotes in my tree and
> then merge them later.  I have recently changed my merges to include the
> source tree and branch in the merge message (previously, all you saws
> was the local name of the remote branch).

Yeah, this is what I was thinking -- a maintainer would have some kind
of mapping of remote tag/branch to local branch, and then do the reviews
and merges from those.

-- 
Kees Cook

Re: merging pull requests

[Olof Johansson]

On Thu, Sep 30, 2021 at 4:09 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Thu, Sep 30, 2021 at 04:00:02PM -0400, Konstantin Ryabitsev wrote:
> > On Thu, Sep 30, 2021 at 10:33:56AM -0700, Kees Cook wrote:
> > > Hi!
> > >
> > > I'm just realized that all my workflows have been entirely mbox patch
> > > sets, and I finally have a pull request to take, and ... I have no idea
> > > how to do it "right". :P
> > >
> > > I see "b4 pr", but I was hoping for some kind of three-step process:
> > > 1) get the code
> > > 2) read/verify/test code
> > > 3) merge code into main brain
> >
> > Glad you're still around and doing well, professor Nightingale. ;)
>
> 3 weeks of virtual conferences is wrecking my branch.
>
> > I'm going to cc this to users@, since it's likely to reach more
> > maintainers' eyes (and brains) there.
>
> Good idea; thanks!
>
> > > And that it would end up looking like what I see from Linus (i.e. naming
> > > branching after the tags sent, etc):
> > >
> > > 6e439bbd436e Merge branch 'linus' of git://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6
> > > a4e6f95a891a Merge tag 'pinctrl-v5.15-2' of git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl
> > > 62da74a73570 Merge tag 'vfio-v5.15-rc4' of git://github.com/awilliam/linux-vfio
> > > e7bd807e8c9e Merge tag 'm68k-for-v5.15-tag3' of git://git.kernel.org/pub/scm/linux/kernel/git/geert/linux-m68k
> > > dca50f08a03e Merge tag 'nios2_fixes_for_v5.15_part1' of git://git.kernel.org/pub/scm/linux/kernel/git/dinguyen/linux
> > >
> > > It seems like "b4 pr" does part of step 1, but doesn't build a branch
> > > from the tag (though one can use "-b").
> > >
> > > And for step 3, when I do a "git merge --no-ff $branch", all the tag and
> > > origin (e.g. "tag 'pinctrl-v5.15-2' of git://..." is missing).
> >
> > It shouldn't be if you leave it in FETCH_HEAD, which is what b4 does by
> > default. So, if you do "b4 pr" and then follow it by "git merge", it should
> > retain the pull request origin and make it the default subject there.
>
> I guess it depends on the expected order of operations. What do other
> maintainers do to process a PR? I would think it would be:
>
> - pull remote branch (to FETCH_HEAD)
> - review (in FETCH_HEAD? in a "real" branch?)
> - merge into my topic branch (where the commit subject should retain details
>   of the pull location, body has notes from the signed tag, etc...)
> - review, build, and test
> - push to public tree

I have a script that I pipe the email to, that parses the pull
request, finds it in PW, updates status, checks out the topic branch I
want it for, and merges it there with a commit message with a link to
lore, etc.

I also manually double check merge stats in case the script got something wrong.

It's a messy script, but I can send it over.

We have bots that build the for-next branch after push. I do run a
script iterating on the branch before merging it, looking for the
silly mistakes that otherwise sfr emails about (missing sign-offs,
etc).

For build coverage, I usually let my bot crank through post-merge when
I push out the for-next contents, many of the pull requests we get
have already been in linux-next.

As for review, it depends on who it comes from. Some I only glance at,
others I look at in detail for every single patch (usually with tig,
on the pulled branch). If I have a comment on a patch, I respond
saying I didn't merge due to something, and reply with comments to the
patch. It happens surprisingly rarely.



-Olof

Re: merging pull requests

[Kees Cook]

On Fri, Oct 01, 2021 at 09:29:14AM +1000, Stephen Rothwell wrote:
> Hi Kees,
> 
> On Thu, 30 Sep 2021 16:09:13 -0700 Kees Cook <keescook@chromium.org> wrote:
> >
> > I guess it depends on the expected order of operations. What do other
> > maintainers do to process a PR? I would think it would be:
> > 
> > - pull remote branch (to FETCH_HEAD)
> > - review (in FETCH_HEAD? in a "real" branch?)
> 
> Our workflow usually requires that all patches are posted to an
> appropriate mailing list for review.  If the developer then also has a
> git branch/tag containing the patch series (after updating for
> Reviewed-bys etc) then that is a separate issue and (probably) requires
> a level of trust on the part of the maintainer (or a recheck) that the
> patches have not been changed since the final review.

Right, that's why I'm such a huge fan of "b4" (and patatt). It makes the
integrity chain very easy to maintain. (Though see my P.S. below...)

> > - merge into my topic branch (where the commit subject should retain details
> >   of the pull location, body has notes from the signed tag, etc...)
> > - review, build, and test
> 
> "rinse and repeat" :-)

Indeed. :)

-Kees

P.S.

The only "hole" I see with the integrity checking is that since only tags
or mbox headers are signed, and those aren't part of the merge, there
isn't a easy way that I see to follow the integrity chain for a given
resulting tree. (Which is technically different from the "trust" chain.)

For example, for stuff going into my tree:
- If it's from an mbox, I can easily check that the patches haven't changed
  in flight when the author used b4/patatt to wrap the email delivery.
- If it's from a remote tag, I can check the tag signature.
This is all fine.

Now I publish my tree, and sign a tag for it for a pull request. Whoever
does that pull can only check my tag and has to trust I checked what
went into my tree. At the end of the day, that's exactly what the
tag signature is for: whoever is pulling must trust the PR sender for
all kinds of reasons. But there isn't a way to mechanically perform an
integrity check on the components of those results: the merged mbox with
the signature headers or the remote tag signature aren't associated
with the resulting branch any more.

But given that maintainers may tweak what was sent to them or squash
fixes, there's likely no point in that kind of integrity chain...

-- 
Kees Cook

Re: merging pull requests

[Kees Cook]

On Thu, Sep 30, 2021 at 04:31:20PM -0700, Olof Johansson wrote:
> On Thu, Sep 30, 2021 at 4:09 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Thu, Sep 30, 2021 at 04:00:02PM -0400, Konstantin Ryabitsev wrote:
> > > On Thu, Sep 30, 2021 at 10:33:56AM -0700, Kees Cook wrote:
> > > > Hi!
> > > >
> > > > I'm just realized that all my workflows have been entirely mbox patch
> > > > sets, and I finally have a pull request to take, and ... I have no idea
> > > > how to do it "right". :P
> > > >
> > > > I see "b4 pr", but I was hoping for some kind of three-step process:
> > > > 1) get the code
> > > > 2) read/verify/test code
> > > > 3) merge code into main brain
> > >
> > > Glad you're still around and doing well, professor Nightingale. ;)
> >
> > 3 weeks of virtual conferences is wrecking my branch.
> >
> > > I'm going to cc this to users@, since it's likely to reach more
> > > maintainers' eyes (and brains) there.
> >
> > Good idea; thanks!
> >
> > > > And that it would end up looking like what I see from Linus (i.e. naming
> > > > branching after the tags sent, etc):
> > > >
> > > > 6e439bbd436e Merge branch 'linus' of git://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6
> > > > a4e6f95a891a Merge tag 'pinctrl-v5.15-2' of git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl
> > > > 62da74a73570 Merge tag 'vfio-v5.15-rc4' of git://github.com/awilliam/linux-vfio
> > > > e7bd807e8c9e Merge tag 'm68k-for-v5.15-tag3' of git://git.kernel.org/pub/scm/linux/kernel/git/geert/linux-m68k
> > > > dca50f08a03e Merge tag 'nios2_fixes_for_v5.15_part1' of git://git.kernel.org/pub/scm/linux/kernel/git/dinguyen/linux
> > > >
> > > > It seems like "b4 pr" does part of step 1, but doesn't build a branch
> > > > from the tag (though one can use "-b").
> > > >
> > > > And for step 3, when I do a "git merge --no-ff $branch", all the tag and
> > > > origin (e.g. "tag 'pinctrl-v5.15-2' of git://..." is missing).
> > >
> > > It shouldn't be if you leave it in FETCH_HEAD, which is what b4 does by
> > > default. So, if you do "b4 pr" and then follow it by "git merge", it should
> > > retain the pull request origin and make it the default subject there.
> >
> > I guess it depends on the expected order of operations. What do other
> > maintainers do to process a PR? I would think it would be:
> >
> > - pull remote branch (to FETCH_HEAD)
> > - review (in FETCH_HEAD? in a "real" branch?)
> > - merge into my topic branch (where the commit subject should retain details
> >   of the pull location, body has notes from the signed tag, etc...)
> > - review, build, and test
> > - push to public tree
> 
> I have a script that I pipe the email to, that parses the pull
> request, finds it in PW, updates status, checks out the topic branch I
> want it for, and merges it there with a commit message with a link to
> lore, etc.

Yeah, this is what I'm looking for: the scripts that are driven by a PR.
(And now I have so many more questions...)

What's the PW step here? I've only seen PW used for managing emailed
patches, not handling RPs. Or is that just a check for "was this sent
to a public list"? (And do you end up needing to compare the PR results
against the emailed patches? Does your PW do any checks like the netdev
PW[1] has running for CI[2]?)

And now that I'm thinking about it -- do the various PWs by various
subsystems perform the signature checking that b4 can do? Am I really
the only person who has added b4 to their "git send-email" hook? It's so
easy! :)

> I also manually double check merge stats in case the script got something wrong.
> 
> It's a messy script, but I can send it over.

Sure! How much of it could be replaced by b4, I wonder?

> We have bots that build the for-next branch after push. I do run a
> script iterating on the branch before merging it, looking for the
> silly mistakes that otherwise sfr emails about (missing sign-offs,
> etc).

Yeah, I've added "git push" hooks for this now too. I should refresh my
kernel-tools tree[3] with those changes.

> For build coverage, I usually let my bot crank through post-merge when
> I push out the for-next contents, many of the pull requests we get
> have already been in linux-next.
> 
> As for review, it depends on who it comes from. Some I only glance at,
> others I look at in detail for every single patch (usually with tig,
> on the pulled branch). If I have a comment on a patch, I respond
> saying I didn't merge due to something, and reply with comments to the
> patch. It happens surprisingly rarely.

Right -- this seems pretty common. :)

Thanks!

-Kees

[1] https://patchwork.kernel.org/project/netdevbpf/list/
[2] https://github.com/kuba-moo/nipa/tree/master
[3] https://github.com/kees/kernel-tools

-- 
Kees Cook

Re: merging pull requests

[Olof Johansson]

 On Thu, Sep 30, 2021 at 5:09 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Thu, Sep 30, 2021 at 04:31:20PM -0700, Olof Johansson wrote:
> > On Thu, Sep 30, 2021 at 4:09 PM Kees Cook <keescook@chromium.org> wrote:
> > >
> > > On Thu, Sep 30, 2021 at 04:00:02PM -0400, Konstantin Ryabitsev wrote:
> > > > On Thu, Sep 30, 2021 at 10:33:56AM -0700, Kees Cook wrote:
> > > > > Hi!
> > > > >
> > > > > I'm just realized that all my workflows have been entirely mbox patch
> > > > > sets, and I finally have a pull request to take, and ... I have no idea
> > > > > how to do it "right". :P
> > > > >
> > > > > I see "b4 pr", but I was hoping for some kind of three-step process:
> > > > > 1) get the code
> > > > > 2) read/verify/test code
> > > > > 3) merge code into main brain
> > > >
> > > > Glad you're still around and doing well, professor Nightingale. ;)
> > >
> > > 3 weeks of virtual conferences is wrecking my branch.
> > >
> > > > I'm going to cc this to users@, since it's likely to reach more
> > > > maintainers' eyes (and brains) there.
> > >
> > > Good idea; thanks!
> > >
> > > > > And that it would end up looking like what I see from Linus (i.e. naming
> > > > > branching after the tags sent, etc):
> > > > >
> > > > > 6e439bbd436e Merge branch 'linus' of git://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6
> > > > > a4e6f95a891a Merge tag 'pinctrl-v5.15-2' of git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl
> > > > > 62da74a73570 Merge tag 'vfio-v5.15-rc4' of git://github.com/awilliam/linux-vfio
> > > > > e7bd807e8c9e Merge tag 'm68k-for-v5.15-tag3' of git://git.kernel.org/pub/scm/linux/kernel/git/geert/linux-m68k
> > > > > dca50f08a03e Merge tag 'nios2_fixes_for_v5.15_part1' of git://git.kernel.org/pub/scm/linux/kernel/git/dinguyen/linux
> > > > >
> > > > > It seems like "b4 pr" does part of step 1, but doesn't build a branch
> > > > > from the tag (though one can use "-b").
> > > > >
> > > > > And for step 3, when I do a "git merge --no-ff $branch", all the tag and
> > > > > origin (e.g. "tag 'pinctrl-v5.15-2' of git://..." is missing).
> > > >
> > > > It shouldn't be if you leave it in FETCH_HEAD, which is what b4 does by
> > > > default. So, if you do "b4 pr" and then follow it by "git merge", it should
> > > > retain the pull request origin and make it the default subject there.
> > >
> > > I guess it depends on the expected order of operations. What do other
> > > maintainers do to process a PR? I would think it would be:
> > >
> > > - pull remote branch (to FETCH_HEAD)
> > > - review (in FETCH_HEAD? in a "real" branch?)
> > > - merge into my topic branch (where the commit subject should retain details
> > >   of the pull location, body has notes from the signed tag, etc...)
> > > - review, build, and test
> > > - push to public tree
> >
> > I have a script that I pipe the email to, that parses the pull
> > request, finds it in PW, updates status, checks out the topic branch I
> > want it for, and merges it there with a commit message with a link to
> > lore, etc.
>
> Yeah, this is what I'm looking for: the scripts that are driven by a PR.
> (And now I have so many more questions...)
>
> What's the PW step here? I've only seen PW used for managing emailed
> patches, not handling RPs. Or is that just a check for "was this sent
> to a public list"? (And do you end up needing to compare the PR results
> against the emailed patches? Does your PW do any checks like the netdev
> PW[1] has running for CI[2]?)

PW catches pull requests too. Current state hasn't been updated so
there's some stale material in there, but see
https://patchwork.kernel.org/project/linux-soc/list/ for reference.

Our PW does not do any checks today, I've had ambitions to add some
but my personal time for upstream work has been limited in the last
year.

> And now that I'm thinking about it -- do the various PWs by various
> subsystems perform the signature checking that b4 can do? Am I really
> the only person who has added b4 to their "git send-email" hook? It's so
> easy! :)

I don't rely on PW for any checking, I just use it as a state tracker
for patches/pull requests. For maintainers who send to the right
alias/list it gives us a safety net for missed pull requests or
patches as well (but we've had issues with some developers sending us
patches directly that we shouldn't receive, so it's gotten noisy).

> > I also manually double check merge stats in case the script got something wrong.
> >
> > It's a messy script, but I can send it over.
>
> Sure! How much of it could be replaced by b4, I wonder?

I'll be honest, b4 is newer than all of this and I have had zero
cycles to invest in replacing anything with it. Maybe a lot of
overlap, and maybe all my work is redundant. I don't know.

> > We have bots that build the for-next branch after push. I do run a
> > script iterating on the branch before merging it, looking for the
> > silly mistakes that otherwise sfr emails about (missing sign-offs,
> > etc).
>
> Yeah, I've added "git push" hooks for this now too. I should refresh my
> kernel-tools tree[3] with those changes.
>
> > For build coverage, I usually let my bot crank through post-merge when
> > I push out the for-next contents, many of the pull requests we get
> > have already been in linux-next.
> >
> > As for review, it depends on who it comes from. Some I only glance at,
> > others I look at in detail for every single patch (usually with tig,
> > on the pulled branch). If I have a comment on a patch, I respond
> > saying I didn't merge due to something, and reply with comments to the
> > patch. It happens surprisingly rarely.
>
> Right -- this seems pretty common. :)

It's... almost as if we had an ad-hoc personal web of trust going.
Strange, isn't it? :)


-Olof

Re: merging pull requests

[Jason Gunthorpe]

On Thu, Sep 30, 2021 at 04:42:58PM -0700, Kees Cook wrote:

> The only "hole" I see with the integrity checking is that since only tags
> or mbox headers are signed, and those aren't part of the merge, there
> isn't a easy way that I see to follow the integrity chain for a given
> resulting tree. (Which is technically different from the "trust" chain.)

The git tag and signature are part of the merge commit:

$ git show --show-signature v5.15-rc3-151-g78c56e53821a7e
commit 78c56e53821a7ec3462ce448c1fe6a8d44358831
merged tag 'for-linus'
gpg: Signature made Wed 29 Sep 2021 09:57:42 PM ADT
gpg:                using RSA key 7C1EC530B87EF10C4BFBA8B7386DF7157E209B1A
gpg: Good signature from "Jason Gunthorpe <jgg@nvidia.com>" [ultimate]
gpg:                 aka "Jason Gunthorpe <jgg@mellanox.com>" [ultimate]
gpg:                 aka "Jason Gunthorpe <jgg@ziepe.ca>" [ultimate]
gpg:                 aka "Jason Gunthorpe <jgunthorpe@obsidianresearch.com>" [ultimate]
gpg:                 aka "Jason Gunthorpe <jgunthorpe@gmail.com>" [ultimate]
gpg:                 aka "Jason Gunthorpe <jgg@kernel.org>" [ultimate]
Merge: 02d5e016800d08 e671f0ecfece14
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Thu Sep 30 12:00:46 2021 -0700
[..]

You can see the raw data like this:

$ git cat-file commit v5.15-rc3-151-g78c56e53821a7e
tree cc120d95622f6363c42b7ee9a759aefb11c4f11a
parent 02d5e016800d082058b3d3b7c3ede136cdc6ddcb
parent e671f0ecfece14940a9bb81981098910ea278cf7
author Linus Torvalds <torvalds@linux-foundation.org> 1633028446 -0700
committer Linus Torvalds <torvalds@linux-foundation.org> 1633028446 -0700
mergetag object e671f0ecfece14940a9bb81981098910ea278cf7
 type commit
 tag for-linus
 tagger Jason Gunthorpe <jgg@nvidia.com> 1632963221 -0300
 
 RDMA v5.15 first rc pull request
 
 Several core bugs and a batch of driver bug fixes:
 
 - Fix compilation problems in qib and hfi1
 
 - Do not corrupt the joined multicast group state when using SEND_ONLY
 
 - Several CMA bugs, a reference leak for listening and two syzkaller
   crashers
 
 - Various bug fixes for irdma
 
 - Fix a Sleeping while atomic bug in usnic
 
 - Properly sanitize kernel pointers in dmesg
 
 - Two bugs in the 64b CQE support for hns
 -----BEGIN PGP SIGNATURE-----
 
 iQIzBAABCgAdFiEEfB7FMLh+8QxL+6i3OG33FX4gmxoFAmFVC4YACgkQOG33FX4g
 mxrBuw//XpgZqcXtAd/p70Qp0pgMULb44p6BNCh0HixyFnBFybsxvy3jsjAI5qkb
 +BszhjWRBdkWxwae/LgbIE30TlTu+mFqWhRgBcATa8HujgPiNFDPOxB/oaNpI4Qb
 SUASou2IcMfTBnxu0T1gZ3v6UVOHhD0RzZJsA86vweVmeReGUNITXzso8QmZtz5Y
 7j5x1mWYbmGY3fQx8sur7iKasMIN4i8fPg3ntj84kDOcNTeSg0ir/sVaAX8iSkHB
 LoF2iXZ6B/2OM0rU238qZVC1bzs3ZXFsfvpRqXs+gR48VH4kKnnWunYeDV5qKLAs
 V/YRvwZ/fdz/qZ8wLBnYjaEL7pOprvR/zHNx1Bj66/pvBADKcpVs+DlBZ4hfTh6T
 Qx//LooadcSU3YW3owSXJy2o2orYQlXuD21kdWx3+RTgOlZxDPcMrn6vQe9eEeaB
 tMt7ueUAch1Dz56ZuxYEPy3RbzHeTeWVQro0j7SEb9vImW8pOnURRSV9WuPn+IeJ
 8tMPbBD+vKv7QxnN161fn4i+WbhMiEUmyu4eEjrZgtXZ4Xq0B7QbhsPpPujpNw/I
 fPs6IHWmRKctMOwBpG337yWpbVQbMJcD8P18A9+rrUHdMvS4q2W/U8mJfApWhF9R
 PuE5W8wL/tWTrbqEcp6hzHWqMMVWd6iTcYU/iF6RwFstjrndHFU=
 =PE1D
 -----END PGP SIGNATURE-----

Merge tag 'for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma

Pull rdma fixes from Jason Gunthorpe:
 "Not much too exciting here, although two syzkaller bugs that seem to
  have 9 lives may have finally been squashed.

  Several core bugs and a batch of driver bug fixes:

   - Fix compilation problems in qib and hfi1

   - Do not corrupt the joined multicast group state when using
     SEND_ONLY

   - Several CMA bugs, a reference leak for listening and two syzkaller
     crashers

   - Various bug fixes for irdma

   - Fix a Sleeping while atomic bug in usnic

   - Properly sanitize kernel pointers in dmesg

   - Two bugs in the 64b CQE support for hns"

* tag 'for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma:
  RDMA/hns: Add the check of the CQE size of the user space
  RDMA/hns: Fix the size setting error when copying CQE in clean_cq()
  RDMA/hfi1: Fix kernel pointer leak
  RDMA/usnic: Lock VF with mutex instead of spinlock
  RDMA/hns: Work around broken constant propagation in gcc 8
  RDMA/cma: Ensure rdma_addr_cancel() happens before issuing more requests
  RDMA/cma: Do not change route.addr.src_addr.ss_family
  RDMA/irdma: Report correct WC error when there are MW bind errors
  RDMA/irdma: Report correct WC error when transport retry counter is exceeded
  RDMA/irdma: Validate number of CQ entries on create CQ
  RDMA/irdma: Skip CQP ring during a reset
  MAINTAINERS: Update Broadcom RDMA maintainers
  RDMA/cma: Fix listener leak in rdma_cma_listen_on_all() failure
  IB/cma: Do not send IGMP leaves for sendonly Multicast groups
  IB/qib: Fix clang confusion of NULL pointer comparison

Jason

Re: merging pull requests

[Steven Rostedt]

On Thu, 30 Sep 2021 16:42:58 -0700
Kees Cook <keescook@chromium.org> wrote:

> Now I publish my tree, and sign a tag for it for a pull request. Whoever
> does that pull can only check my tag and has to trust I checked what
> went into my tree. At the end of the day, that's exactly what the
> tag signature is for: whoever is pulling must trust the PR sender for
> all kinds of reasons. But there isn't a way to mechanically perform an
> integrity check on the components of those results: the merged mbox with
> the signature headers or the remote tag signature aren't associated
> with the resulting branch any more.

As I've mentioned before, I have a personalized patchwork instance that
runs against my inbox. I pull from that patchwork to pull into my git
tree (which all patches must at least go to LKML). I also subscribe to
all commits that go into Linus's tree, and those patches run against
the patches in my inbox patchwork database. If a match is found, the
patch changes from "Under Review" (which gets set to that when it's
posted as my "for-next" or "for-linus" emails) to "Accepted", and they
no longer appear in my default view.

Thus, if the patch changes for the time I reviewed it, to the time it
gets into Linus's tree, that patch will not be removed from my
patchwork database. Which causes me to examine further.

> 
> But given that maintainers may tweak what was sent to them or squash
> fixes, there's likely no point in that kind of integrity chain...

I sometimes make slight changes, usually do to conflicts with other
changes. In these cases, I have to manually set the patches in my
database to "Accepted", but I don't do that without manually examining
what is in Linus's tree and what is in patchwork.

-- Steve

Re: merging pull requests

[Steven Rostedt]

On Thu, 30 Sep 2021 17:27:40 -0700
Olof Johansson <olof@lixom.net> wrote:

> > Sure! How much of it could be replaced by b4, I wonder?  
> 
> I'll be honest, b4 is newer than all of this and I have had zero
> cycles to invest in replacing anything with it. Maybe a lot of
> overlap, and maybe all my work is redundant. I don't know.

Sounds like I'm very much in the same boat as Olof.

-- Steve

Re: merging pull requests

[James Bottomley]

On Thu, 2021-09-30 at 16:42 -0700, Kees Cook wrote:
[...]
> The only "hole" I see with the integrity checking is that since only
> tags or mbox headers are signed, and those aren't part of the merge,
> there isn't a easy way that I see to follow the integrity chain for a
> given resulting tree. (Which is technically different from the
> "trust" chain.)
> 
> For example, for stuff going into my tree:
> - If it's from an mbox, I can easily check that the patches haven't
> changed
>   in flight when the author used b4/patatt to wrap the email
> delivery.
> - If it's from a remote tag, I can check the tag signature.
> This is all fine.
> 
> Now I publish my tree, and sign a tag for it for a pull request.
> Whoever does that pull can only check my tag and has to trust I
> checked what went into my tree. At the end of the day, that's exactly
> what the tag signature is for: whoever is pulling must trust the PR
> sender for all kinds of reasons. But there isn't a way to
> mechanically perform an integrity check on the components of those
> results: the merged mbox with the signature headers or the remote tag
> signature aren't associated with the resulting branch any more.
> 
> But given that maintainers may tweak what was sent to them or squash
> fixes, there's likely no point in that kind of integrity chain...

Well, I think you need to re-examine what it is we're attesting to
cryptographically.  We already have an attestation process, it's called
the Signed-off-by: chain.  The DCO is very specific, either: it's your
contribution; a contribution with attribution you modified or it's an
unmodified contribution from another.  This is the base level of legal
attestation the maintainers do and which we're very careful to get
right.  If you want to try and express this crytpographically, that's
fine, but it must match the workflow we currently use.

James

Re: merging pull requests

[Konstantin Ryabitsev]

On Thu, Sep 30, 2021 at 04:42:58PM -0700, Kees Cook wrote:
> The only "hole" I see with the integrity checking is that since only tags
> or mbox headers are signed, and those aren't part of the merge, there
> isn't a easy way that I see to follow the integrity chain for a given
> resulting tree. (Which is technically different from the "trust" chain.)

This is unavoidable with the current workflow because commit messages will
necessarily get modified when S-o-b and other trailers get injected. I do not
think attempting the "commits must remain immutable" approach is worthwhile,
as I think there's more value in allowing maintainers to tweak the code they
receive.

> For example, for stuff going into my tree:
> - If it's from an mbox, I can easily check that the patches haven't changed
>   in flight when the author used b4/patatt to wrap the email delivery.
> - If it's from a remote tag, I can check the tag signature.
> This is all fine.
> 
> Now I publish my tree, and sign a tag for it for a pull request. Whoever
> does that pull can only check my tag and has to trust I checked what
> went into my tree.

I think there are two different attestation targets here. What I've been
working on is in-transit attestation, with the goal to make patches
tamper-evident. What you're describing sounds to me more like
cryptographically-backed signoffs. I think tools like sigstore [1] and
external attestation documents are actually better suited for this.

I don't want to wander too far down that path at the moment, but it's perhaps
something to consider in the future. With patatt in-header signatures, you
*can* perform attestation if each commit includes a Link: to the original
patch message (and that message is cryptographically attested).

[1] https://www.sigstore.dev/

-K

Re: merging pull requests

[Konstantin Ryabitsev]

On Thu, Sep 30, 2021 at 04:09:13PM -0700, Kees Cook wrote:
> I think so -- that seems to be sort of what Linus does? There seem to be
> a few templates (tag, branch, and "patches from Andrew"), e.g.:
> 
> Merge tag 'char-misc-5.15-rc1-2' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc
> Merge branch 'misc.namei' of git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs
> Merge branch 'akpm' (patches from Andrew)
> 
> Though I'd love to know why there's a distinction between "tag" and
> "branch" here. Are "branch" pulls not checked for signatures?

That's just what git does when you merge FETCH_HEAD. It takes the contents of
.git/FETCH_HEAD, which describe from where the last fetch happened. If you
fetch a tag, it will say:

    [tipsha]\t\ttag 'tagname' of url://of/remote/tree

if you fetch a branch, it will be

    [tipsha]\t\tbranch 'branchanme' of url://of/remote/tree

When merging FETCH_HEAD, git will just say "Merge %s" with the contents of
.git/FETCH_HEAD file for that tip.

When you merge a local branch instead of FETCH_HEAD, it will default to:

    Merge branch 'localbranchname'

I think Linus just manually adds "(patches from Andrew)" to that.

-K

Re: merging pull requests

[Linus Torvalds]

On Fri, Oct 1, 2021 at 11:26 AM Konstantin Ryabitsev
<konstantin@linuxfoundation.org> wrote:
>
> I think Linus just manually adds "(patches from Andrew)" to that.

I always manually edit my merge messages fairly extensively.

And I would very much discourage anybody from thinking that a merge
commit could be auto-generated. Don't do it. If you do it, you're
doing things wrong.

Merge messages need to be meaningful, and no amount of auto-generation
will ever make them so.

Even when people use signed tags with all the merge message in them to
send me pull requests, I will edit things so that at a minimum there's
proper indentation for me quoting the data, but also to try to make
merges look somewhat consistent (different people use very different
formats for listing changes).

I'll spend time making my merge messages look more legible, including
fixing spelling and grammar for people, and sometimes going to look at
the individual commits themselves if there is some explanation that
doesn't make sense to me.

Yeah, some trivial pull requests with a couple of simple changes,
maybe the auto-generated shortlog feature ends up being sufficient.
but honestly, that's _very_ rare. And sometimes the message might be
just "Small x86 fixes" like in the recent kvm pull I did.

But if the pull has anything even remotely more subtle, I will
complain to whoever sent me the pull request if they didn't give me
enough information.

And honestly, when _I_ merge stuff, I generally will need to explain
things _less_ than most other people, for one very simple and
fundamental reason: my tree is simply the known upstream tree. So me
merging some networking pull should explain _what_ I merge, but at the
same time, I don't really need to explain why it is _I_ that am
merging it.

When you see me merging networking code into my tree, a simple "Merge
networking fixes from David/Jakub" is already enough information.
There's no question about "why is Linus merging from the networking
maintainers".

So then I get very upset when I see other people merge stuff, and they
have NO explanations at all.

If you aren't the upstream maintainer for whatever you are merging,
you should not only explain *what* you are merging, you should explain
*why* you are merging it.

So put simply: I put a fair amount of effort into making my merge
messages be meaningful.

Anybody else who does merges should do at LEAST the same, and in
situations where you're merging something that you aren't the obvious
maintainer for (like doing back-merges of my tree), you should in fact
put a lot *more* effort into it.

End result: Please don't make some crazy "b4 shazam" tool that handles
pull requests and then makes it trivial for people to do automated
merges too. That's absolutely the last thing we want to see.

Merges need *more* explanation than regular commits. The "why" is
often very important for a merge, even if you'll sendom see it in my
merges where the "why" is kind of implicit.

Don't try to do something that avoids that very important manual
editing step to explain what is going on.

I've said it many times before, and I'll sadly probably have to say it
many times again: if you can't be bothered to write a good merge
message, you shouldn't be doing the merge.

It really is that simple.

No automated merges outside of pure test-trees (ie where the merges
will never be seen by anybody else), because they fundamentally
violate that very basic rule.

             Linus

Re: merging pull requests

[Konstantin Ryabitsev]

On Fri, Oct 01, 2021 at 11:47:12AM -0700, Linus Torvalds wrote:
> End result: Please don't make some crazy "b4 shazam" tool that handles
> pull requests and then makes it trivial for people to do automated
> merges too. That's absolutely the last thing we want to see.

Okay, it will for sure not do any automated merges. At most, it will provide
the contents of the cover letter (if present) as a convenient starting point.

What I like about "b4 shazam" is that it can make patch series work more like
pull requests. Once it's fetched and placed into FETCH_HEAD, maintainers can
either opt to do a merge, or rebase with their working branch and ff.

I expect that merges would be rare and only for large features that do require
a merge commit with a proper explanation (as you describe).

If any of these assumptions are wrong and I'm going down the wrong path,
please let me know.

-K

Re: merging pull requests

[Kees Cook]

On Fri, Oct 01, 2021 at 03:30:22PM -0400, Konstantin Ryabitsev wrote:
> On Fri, Oct 01, 2021 at 11:47:12AM -0700, Linus Torvalds wrote:
> > End result: Please don't make some crazy "b4 shazam" tool that handles
> > pull requests and then makes it trivial for people to do automated
> > merges too. That's absolutely the last thing we want to see.
> 
> Okay, it will for sure not do any automated merges. At most, it will provide
> the contents of the cover letter (if present) as a convenient starting point.
> 
> What I like about "b4 shazam" is that it can make patch series work more like
> pull requests. Once it's fetched and placed into FETCH_HEAD, maintainers can
> either opt to do a merge, or rebase with their working branch and ff.

Right -- I was just trying to save on the copy/paste mechanical process
of going from a PR email to a local branch I can review, build, test,
etc.

> I expect that merges would be rare and only for large features that do require
> a merge commit with a proper explanation (as you describe).

Totally, yes. I hope I didn't come across in my initial email as want to
automatic the commit itself. I did talk a bit about the subject being
mechanical, but the rest of the log needs as much detail as possible.

-Kees

-- 
Kees Cook

Re: merging pull requests

[Kees Cook]

On Fri, Oct 01, 2021 at 02:26:15PM -0400, Konstantin Ryabitsev wrote:
> On Thu, Sep 30, 2021 at 04:09:13PM -0700, Kees Cook wrote:
> > I think so -- that seems to be sort of what Linus does? There seem to be
> > a few templates (tag, branch, and "patches from Andrew"), e.g.:
> > 
> > Merge tag 'char-misc-5.15-rc1-2' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc
> > Merge branch 'misc.namei' of git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs
> > Merge branch 'akpm' (patches from Andrew)
> > 
> > Though I'd love to know why there's a distinction between "tag" and
> > "branch" here. Are "branch" pulls not checked for signatures?
> 
> That's just what git does when you merge FETCH_HEAD. It takes the contents of
> .git/FETCH_HEAD, which describe from where the last fetch happened. If you
> fetch a tag, it will say:
> 
>     [tipsha]\t\ttag 'tagname' of url://of/remote/tree
> 
> if you fetch a branch, it will be
> 
>     [tipsha]\t\tbranch 'branchanme' of url://of/remote/tree
> 
> When merging FETCH_HEAD, git will just say "Merge %s" with the contents of
> .git/FETCH_HEAD file for that tip.
> 
> When you merge a local branch instead of FETCH_HEAD, it will default to:
> 
>     Merge branch 'localbranchname'

Gotcha. I remain curious about the process of between the pull (which
puts it into FETCH_HEAD) and the merge (with merges FETCH_HEAD into the
current branch), when I would expect there to be an intermediate step of
checking FETCH_HEAD out into a branch for review, build, test, etc.

But, I guess, nothing would actually move FETCH_HEAD in that case, so
that's why it shows up that way.

-- 
Kees Cook

Re: merging pull requests

[Kees Cook]

On Fri, Oct 01, 2021 at 01:05:01PM -0400, Steven Rostedt wrote:
> On Thu, 30 Sep 2021 17:27:40 -0700
> Olof Johansson <olof@lixom.net> wrote:
> 
> > > Sure! How much of it could be replaced by b4, I wonder?  
> > 
> > I'll be honest, b4 is newer than all of this and I have had zero
> > cycles to invest in replacing anything with it. Maybe a lot of
> > overlap, and maybe all my work is redundant. I don't know.
> 
> Sounds like I'm very much in the same boat as Olof.

I just keep ripping big chunks out of my scripts and replacing them with
b4 one-liners. It's worth it. :)

-- 
Kees Cook

Re: merging pull requests

[Kees Cook]

On Fri, Oct 01, 2021 at 08:59:48AM -0300, Jason Gunthorpe wrote:
> On Thu, Sep 30, 2021 at 04:42:58PM -0700, Kees Cook wrote:
> 
> > The only "hole" I see with the integrity checking is that since only tags
> > or mbox headers are signed, and those aren't part of the merge, there
> > isn't a easy way that I see to follow the integrity chain for a given
> > resulting tree. (Which is technically different from the "trust" chain.)
> 
> The git tag and signature are part of the merge commit:
> 
> $ git show --show-signature v5.15-rc3-151-g78c56e53821a7e
> commit 78c56e53821a7ec3462ce448c1fe6a8d44358831
> merged tag 'for-linus'
> gpg: Signature made Wed 29 Sep 2021 09:57:42 PM ADT
> gpg:                using RSA key 7C1EC530B87EF10C4BFBA8B7386DF7157E209B1A
> gpg: Good signature from "Jason Gunthorpe <jgg@nvidia.com>" [ultimate]
> gpg:                 aka "Jason Gunthorpe <jgg@mellanox.com>" [ultimate]
> gpg:                 aka "Jason Gunthorpe <jgg@ziepe.ca>" [ultimate]
> gpg:                 aka "Jason Gunthorpe <jgunthorpe@obsidianresearch.com>" [ultimate]
> gpg:                 aka "Jason Gunthorpe <jgunthorpe@gmail.com>" [ultimate]
> gpg:                 aka "Jason Gunthorpe <jgg@kernel.org>" [ultimate]
> Merge: 02d5e016800d08 e671f0ecfece14
> Author: Linus Torvalds <torvalds@linux-foundation.org>
> Date:   Thu Sep 30 12:00:46 2021 -0700
> [..]
> 
> You can see the raw data like this:
> 
> $ git cat-file commit v5.15-rc3-151-g78c56e53821a7e

Ah-ha! It does! Thank you; I couldn't figure out how to find the tags
internally.

-- 
Kees Cook

Re: merging pull requests

[Kees Cook]

On Fri, Oct 01, 2021 at 10:07:05AM -0700, James Bottomley wrote:
> > [...]
> > But given that maintainers may tweak what was sent to them or squash
> > fixes, there's likely no point in that kind of integrity chain...
> 
> Well, I think you need to re-examine what it is we're attesting to
> cryptographically.  We already have an attestation process, it's called
> the Signed-off-by: chain.  The DCO is very specific, either: it's your
> contribution; a contribution with attribution you modified or it's an
> unmodified contribution from another.  This is the base level of legal
> attestation the maintainers do and which we're very careful to get
> right.  If you want to try and express this crytpographically, that's
> fine, but it must match the workflow we currently use.

Right -- I had digressed off into integrity land. The personal trust and
forensics part has been covered for a long time. I've been thinking more
about "in flight" integrity, which is what things like patatt nail down.

-- 
Kees Cook

Re: merging pull requests

[Kees Cook]

On Fri, Oct 01, 2021 at 01:19:42PM -0400, Konstantin Ryabitsev wrote:
> I don't want to wander too far down that path at the moment, but it's perhaps
> something to consider in the future. With patatt in-header signatures, you
> *can* perform attestation if each commit includes a Link: to the original
> patch message (and that message is cryptographically attested).

Yeah, just getting more folks using patatt would be a great next step. :)

I've proposed adding "patatt validate"[1] to the netdev CI[2], maybe
gfx-ci[3], and others could do the same?

-Kees

[1] https://github.com/kuba-moo/nipa/pull/5
[2] https://github.com/kuba-moo/nipa
    e.g. https://patchwork.kernel.org/project/netdevbpf/patch/20211002003706.11237-3-xiyou.wangcong@gmail.com/
[3] https://patchwork.freedesktop.org/project/intel-gfx-trybot/series/?ordering=-last_updated
    e.g. https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/-/pipelines/411177

-- 
Kees Cook

Re: merging pull requests

[Willy Tarreau]

On Fri, Oct 01, 2021 at 11:47:12AM -0700, Linus Torvalds wrote:
> The "why" is
> often very important for a merge, even if you'll sendom see it in my
> merges where the "why" is kind of implicit.

FWIW I'm facing the same difficulty in other projects where I feel like
I always have to explain this again and again, but when re-reading my
own messages later I notice I'm often not better than others at this
exercise.

This led me to try to understand why I was not doing well what I'm
expecting others to do, and to realize that when developers finish a
series they've been working on for some time, the "why" is so much
obvious to them that it's difficult to write a good justification
for it, as what is non-obvious to others becomes hard to grasp. The
advices I give which seldom work are:

  - try to *sell* me your series. You want me to adopt them and be
    responsible for them, OK but you have to convince me to do so.

  - explain what we'd lose or break in case we'd later need to revert
    the series. Very often this is the direct response to the "why".

The two are more or less equivalent but are different approaches to
the problem, depending on the context or the nature of the changes.
With a bit of practice it can sometimes be as simple as "this is
the preliminary changes required for X that extend the API using
wrappers to temporarily maintain the compatibility with the previous
one", or stuff like that.

Willy