Re: merging pull requests

[Kees Cook <keescook@chromium.org>]

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