* merging pull requests @ 2021-09-30 17:33 Kees Cook 2021-09-30 20:00 ` Konstantin Ryabitsev 0 siblings, 1 reply; 25+ messages in thread From: Kees Cook @ 2021-09-30 17:33 UTC (permalink / raw) To: tools 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: merging pull requests 2021-09-30 17:33 merging pull requests Kees Cook @ 2021-09-30 20:00 ` Konstantin Ryabitsev 2021-09-30 23:09 ` Kees Cook 0 siblings, 1 reply; 25+ messages in thread From: Konstantin Ryabitsev @ 2021-09-30 20:00 UTC (permalink / raw) To: Kees Cook; +Cc: tools, users 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: merging pull requests 2021-09-30 20:00 ` Konstantin Ryabitsev @ 2021-09-30 23:09 ` Kees Cook 2021-09-30 23:22 ` Stephen Rothwell ` (3 more replies) 0 siblings, 4 replies; 25+ messages in thread From: Kees Cook @ 2021-09-30 23:09 UTC (permalink / raw) To: Konstantin Ryabitsev; +Cc: tools, users 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: merging pull requests 2021-09-30 23:09 ` Kees Cook @ 2021-09-30 23:22 ` Stephen Rothwell 2021-09-30 23:29 ` Kees Cook 2021-09-30 23:29 ` Stephen Rothwell ` (2 subsequent siblings) 3 siblings, 1 reply; 25+ messages in thread From: Stephen Rothwell @ 2021-09-30 23:22 UTC (permalink / raw) To: Kees Cook; +Cc: Konstantin Ryabitsev, tools, users [-- 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 --] ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: merging pull requests 2021-09-30 23:22 ` Stephen Rothwell @ 2021-09-30 23:29 ` Kees Cook 0 siblings, 0 replies; 25+ messages in thread From: Kees Cook @ 2021-09-30 23:29 UTC (permalink / raw) To: Stephen Rothwell; +Cc: Konstantin Ryabitsev, tools, users 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: merging pull requests 2021-09-30 23:09 ` Kees Cook 2021-09-30 23:22 ` Stephen Rothwell @ 2021-09-30 23:29 ` Stephen Rothwell 2021-09-30 23:42 ` Kees Cook 2021-09-30 23:31 ` Olof Johansson 2021-10-01 18:26 ` Konstantin Ryabitsev 3 siblings, 1 reply; 25+ messages in thread From: Stephen Rothwell @ 2021-09-30 23:29 UTC (permalink / raw) To: Kees Cook; +Cc: Konstantin Ryabitsev, tools, users [-- 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 --] ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: merging pull requests 2021-09-30 23:29 ` Stephen Rothwell @ 2021-09-30 23:42 ` Kees Cook 2021-10-01 11:59 ` Jason Gunthorpe ` (3 more replies) 0 siblings, 4 replies; 25+ messages in thread From: Kees Cook @ 2021-09-30 23:42 UTC (permalink / raw) To: Stephen Rothwell; +Cc: Konstantin Ryabitsev, tools, users 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: merging pull requests 2021-09-30 23:42 ` Kees Cook @ 2021-10-01 11:59 ` Jason Gunthorpe 2021-10-02 0:15 ` Kees Cook 2021-10-01 17:01 ` Steven Rostedt ` (2 subsequent siblings) 3 siblings, 1 reply; 25+ messages in thread From: Jason Gunthorpe @ 2021-10-01 11:59 UTC (permalink / raw) To: Kees Cook; +Cc: Stephen Rothwell, Konstantin Ryabitsev, tools, users 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: merging pull requests 2021-10-01 11:59 ` Jason Gunthorpe @ 2021-10-02 0:15 ` Kees Cook 0 siblings, 0 replies; 25+ messages in thread From: Kees Cook @ 2021-10-02 0:15 UTC (permalink / raw) To: Jason Gunthorpe; +Cc: Stephen Rothwell, Konstantin Ryabitsev, tools, users 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: merging pull requests 2021-09-30 23:42 ` Kees Cook 2021-10-01 11:59 ` Jason Gunthorpe @ 2021-10-01 17:01 ` Steven Rostedt 2021-10-01 17:07 ` James Bottomley 2021-10-01 17:19 ` Konstantin Ryabitsev 3 siblings, 0 replies; 25+ messages in thread From: Steven Rostedt @ 2021-10-01 17:01 UTC (permalink / raw) To: Kees Cook; +Cc: Stephen Rothwell, Konstantin Ryabitsev, tools, users 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: merging pull requests 2021-09-30 23:42 ` Kees Cook 2021-10-01 11:59 ` Jason Gunthorpe 2021-10-01 17:01 ` Steven Rostedt @ 2021-10-01 17:07 ` James Bottomley 2021-10-02 0:17 ` Kees Cook 2021-10-01 17:19 ` Konstantin Ryabitsev 3 siblings, 1 reply; 25+ messages in thread From: James Bottomley @ 2021-10-01 17:07 UTC (permalink / raw) To: Kees Cook, Stephen Rothwell; +Cc: Konstantin Ryabitsev, tools, users 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: merging pull requests 2021-10-01 17:07 ` James Bottomley @ 2021-10-02 0:17 ` Kees Cook 0 siblings, 0 replies; 25+ messages in thread From: Kees Cook @ 2021-10-02 0:17 UTC (permalink / raw) To: James Bottomley; +Cc: Stephen Rothwell, Konstantin Ryabitsev, tools, users 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: merging pull requests 2021-09-30 23:42 ` Kees Cook ` (2 preceding siblings ...) 2021-10-01 17:07 ` James Bottomley @ 2021-10-01 17:19 ` Konstantin Ryabitsev 2021-10-02 2:35 ` Kees Cook 3 siblings, 1 reply; 25+ messages in thread From: Konstantin Ryabitsev @ 2021-10-01 17:19 UTC (permalink / raw) To: Kees Cook; +Cc: Stephen Rothwell, tools, users 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: merging pull requests 2021-10-01 17:19 ` Konstantin Ryabitsev @ 2021-10-02 2:35 ` Kees Cook 0 siblings, 0 replies; 25+ messages in thread From: Kees Cook @ 2021-10-02 2:35 UTC (permalink / raw) To: Konstantin Ryabitsev; +Cc: Stephen Rothwell, tools, users 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: merging pull requests 2021-09-30 23:09 ` Kees Cook 2021-09-30 23:22 ` Stephen Rothwell 2021-09-30 23:29 ` Stephen Rothwell @ 2021-09-30 23:31 ` Olof Johansson 2021-10-01 0:09 ` Kees Cook 2021-10-01 18:26 ` Konstantin Ryabitsev 3 siblings, 1 reply; 25+ messages in thread From: Olof Johansson @ 2021-09-30 23:31 UTC (permalink / raw) To: Kees Cook; +Cc: Konstantin Ryabitsev, tools, users 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: merging pull requests 2021-09-30 23:31 ` Olof Johansson @ 2021-10-01 0:09 ` Kees Cook 2021-10-01 0:27 ` Olof Johansson 0 siblings, 1 reply; 25+ messages in thread From: Kees Cook @ 2021-10-01 0:09 UTC (permalink / raw) To: Olof Johansson; +Cc: Konstantin Ryabitsev, tools, users 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: merging pull requests 2021-10-01 0:09 ` Kees Cook @ 2021-10-01 0:27 ` Olof Johansson 2021-10-01 17:05 ` Steven Rostedt 0 siblings, 1 reply; 25+ messages in thread From: Olof Johansson @ 2021-10-01 0:27 UTC (permalink / raw) To: Kees Cook; +Cc: Konstantin Ryabitsev, tools, users 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: merging pull requests 2021-10-01 0:27 ` Olof Johansson @ 2021-10-01 17:05 ` Steven Rostedt 2021-10-02 0:12 ` Kees Cook 0 siblings, 1 reply; 25+ messages in thread From: Steven Rostedt @ 2021-10-01 17:05 UTC (permalink / raw) To: Olof Johansson; +Cc: Kees Cook, Konstantin Ryabitsev, tools, users 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: merging pull requests 2021-10-01 17:05 ` Steven Rostedt @ 2021-10-02 0:12 ` Kees Cook 0 siblings, 0 replies; 25+ messages in thread From: Kees Cook @ 2021-10-02 0:12 UTC (permalink / raw) To: Steven Rostedt; +Cc: Olof Johansson, Konstantin Ryabitsev, tools, users 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: merging pull requests 2021-09-30 23:09 ` Kees Cook ` (2 preceding siblings ...) 2021-09-30 23:31 ` Olof Johansson @ 2021-10-01 18:26 ` Konstantin Ryabitsev 2021-10-01 18:47 ` Linus Torvalds 2021-10-02 0:11 ` Kees Cook 3 siblings, 2 replies; 25+ messages in thread From: Konstantin Ryabitsev @ 2021-10-01 18:26 UTC (permalink / raw) To: Kees Cook; +Cc: tools, users 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: merging pull requests 2021-10-01 18:26 ` Konstantin Ryabitsev @ 2021-10-01 18:47 ` Linus Torvalds 2021-10-01 19:30 ` Konstantin Ryabitsev 2021-10-02 6:22 ` Willy Tarreau 2021-10-02 0:11 ` Kees Cook 1 sibling, 2 replies; 25+ messages in thread From: Linus Torvalds @ 2021-10-01 18:47 UTC (permalink / raw) To: Konstantin Ryabitsev; +Cc: Kees Cook, tools, users 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: merging pull requests 2021-10-01 18:47 ` Linus Torvalds @ 2021-10-01 19:30 ` Konstantin Ryabitsev 2021-10-02 0:08 ` Kees Cook 2021-10-02 6:22 ` Willy Tarreau 1 sibling, 1 reply; 25+ messages in thread From: Konstantin Ryabitsev @ 2021-10-01 19:30 UTC (permalink / raw) To: Linus Torvalds; +Cc: Kees Cook, tools, users 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: merging pull requests 2021-10-01 19:30 ` Konstantin Ryabitsev @ 2021-10-02 0:08 ` Kees Cook 0 siblings, 0 replies; 25+ messages in thread From: Kees Cook @ 2021-10-02 0:08 UTC (permalink / raw) To: Konstantin Ryabitsev; +Cc: Linus Torvalds, tools, users 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: merging pull requests 2021-10-01 18:47 ` Linus Torvalds 2021-10-01 19:30 ` Konstantin Ryabitsev @ 2021-10-02 6:22 ` Willy Tarreau 1 sibling, 0 replies; 25+ messages in thread From: Willy Tarreau @ 2021-10-02 6:22 UTC (permalink / raw) To: Linus Torvalds; +Cc: Konstantin Ryabitsev, Kees Cook, tools, users 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: merging pull requests 2021-10-01 18:26 ` Konstantin Ryabitsev 2021-10-01 18:47 ` Linus Torvalds @ 2021-10-02 0:11 ` Kees Cook 1 sibling, 0 replies; 25+ messages in thread From: Kees Cook @ 2021-10-02 0:11 UTC (permalink / raw) To: Konstantin Ryabitsev; +Cc: tools, users 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2021-10-02 6:28 UTC | newest] Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-30 17:33 merging pull requests Kees Cook 2021-09-30 20:00 ` Konstantin Ryabitsev 2021-09-30 23:09 ` Kees Cook 2021-09-30 23:22 ` Stephen Rothwell 2021-09-30 23:29 ` Kees Cook 2021-09-30 23:29 ` Stephen Rothwell 2021-09-30 23:42 ` Kees Cook 2021-10-01 11:59 ` Jason Gunthorpe 2021-10-02 0:15 ` Kees Cook 2021-10-01 17:01 ` Steven Rostedt 2021-10-01 17:07 ` James Bottomley 2021-10-02 0:17 ` Kees Cook 2021-10-01 17:19 ` Konstantin Ryabitsev 2021-10-02 2:35 ` Kees Cook 2021-09-30 23:31 ` Olof Johansson 2021-10-01 0:09 ` Kees Cook 2021-10-01 0:27 ` Olof Johansson 2021-10-01 17:05 ` Steven Rostedt 2021-10-02 0:12 ` Kees Cook 2021-10-01 18:26 ` Konstantin Ryabitsev 2021-10-01 18:47 ` Linus Torvalds 2021-10-01 19:30 ` Konstantin Ryabitsev 2021-10-02 0:08 ` Kees Cook 2021-10-02 6:22 ` Willy Tarreau 2021-10-02 0:11 ` Kees Cook
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.