feat: `sl pr` support for GPG signatures

[nairb774]

Testing out sapling for the first time, and it seems like the sl pr command doesn't sign commits. Would it be possible to add gpg signing to sl pr? Signed commits are a requirement for the internal repos that I work with so this is currently a blocker on further exploration. My git config has both user.signingkey set and commit.gpgsign=true. I'm open to adjusting configurations, but a quick grep through the code doesn't seem to show any implementation of GPG signing for the sl pr command that I could locate.

I'd consider using sl ghstack which does seem to sign the commits, but the way it sets the base branch of PRs interacts poorly with our CI setup. It seems like https://github.com/ezyang/ghstack/issues/122 might open up this path to being a little less problematic for our setup.

[bolinfest]

This is a good point: I'll investigate!

[nairb774]

I'd be happy to take a stab at this as well if that would help any. I had a really hard time locating where in the code base the actual git commits were being created. I was able to find the relevant code for ghstack, but the other path eluded me. A pointer to the relevant code might help me be able to produce a patch or at least start to dig into the things that might need to change. Additionally, if you know of any sharp corners in the relevant parts of the code base that I'd need to keep an eye out for that could also be helpful.

If this is something that you think would be fairly quick to crank-out with minimal fuss, feel free to keep this on your TODO stack.

[bolinfest]

I suspect this is the right function:

https://github.com/facebook/sapling/blob/5bf764c794e689a7a9390e88ce982afab3f3afae/eden/scm/edenscm/localrepo.py#L2654

[bolinfest]

Here are the appropriate Git docs on the subject:

https://git-scm.com/docs/signature-format

Though I don't see the option to specify a signature in https://libgit2.org/libgit2/#HEAD/group/commit/git_commit_create.

but maybe I'm missing something? We probably need to look at Git's actual commit implementation.

[bolinfest]

Ah, in libgit2, it looks like you have to use:

https://libgit2.org/libgit2/#v1.0.0/group/callback/git_commit_signing_cb

though it is declared in a file named deprecated.h, so that doesn't seem so good...

https://github.com/libgit2/libgit2/blob/HEAD/include/git2/deprecated.h#L276-280

[bolinfest]

Oh, I guess this is the non-deprecated API?

https://libgit2.org/libgit2/#v1.0.0/group/commit/git_commit_create_with_signature

[bolinfest]

I found someone who wrote a unit test that demonstrates how this API is meant to be used:

https://github.com/Vincent2Liu/AsiaInfo/blob/fb4602ee8316b8ebe31b643442c144004401e75a/tests/commit/write.c#L326-L357

[bolinfest]

I checked with @quark-zju and a few things of note.

Here is the place in localrepo.py where we call into changelog.add() to create the commit object:

https://github.com/facebook/sapling/blob/5bf764c794e689a7a9390e88ce982afab3f3afae/eden/scm/edenscm/localrepo.py#L2800-L2810

We are using a Rust crate that wraps libgit2, git2, which has this this commit_signed() method:

https://docs.rs/git2/0.15.0/git2/struct.Repository.html#method.commit_signed

It looks like git commit supports a number of flags related to signing:

       -S[<keyid>], --gpg-sign[=<keyid>], --no-gpg-sign
           GPG-sign commits. The keyid argument is optional and defaults to the committer
           identity; if specified, it must be stuck to the option without a space.
           --no-gpg-sign is useful to countermand both commit.gpgSign configuration variable,
           and earlier --gpg-sign.

as well as a config option to automatically auto-sign commits:

git config --global commit.gpgsign true

On our end, I think we have to:

• Decide on which flags to support in sl commit.
• Decide on what config option to add for automatic signing in sapling.conf.
• Update the Sapling code to use ui.configoverride() to set signing to True when -S is passed on the command line.
• Update the Sapling code to use the config value to determine whether to use commit_signed() in git.rs or wherever the right place is when writing out the Git commit object to the odb.

[bolinfest]

By the time we hit our Rust code, we have the raw bytes for the commit object, so inserting the signature must occur upstream of that. I believe this is the right place to add the signing option:

https://github.com/facebook/sapling/blob/5bf764c794e689a7a9390e88ce982afab3f3afae/eden/scm/edenscm/changelog.py#L295-L316

Or maybe call gitcommittext() as-is and then do the signing here:

https://github.com/facebook/sapling/blob/5bf764c794e689a7a9390e88ce982afab3f3afae/eden/scm/edenscm/changelog2.py#L391-L392

[bolinfest]

Incidentally, here is the commit that added support for GPG signing within Git itself:

https://github.com/git/git/commit/ba3c69a9ee1894de397b60d3b548383e13ef49e3

The do_sign_commit() function seems to be where the action is. In particular, it shows that sign_buffer() is applied to the original commit object, but then the buffer with the commit object contents has to be split on \n\n so the signature generated from sign_buffer() can be written back in before the commit message.

[quark-zju]

By the time we hit our Rust code, we have the raw bytes for the commit object

That was a quick hack that uses hg's commit serialization as the way to represent commit data. It seems in this case it'd be beneficial to have a more structured commit struct. I think if we continue using this hack the hg's "extras" dict might be a reasonable place for passing the extra sign information. Otherwise we might want to refactor related logic so a more structured commit object (defined in Rust) is used.

[bolinfest]

That was a quick hack that uses hg's commit serialization as the way to represent commit data.

I am close to having something working with this architecture, though! I'll put up a diff soon.

[bolinfest]

@nairb774 I just put up https://github.com/facebook/sapling/pull/311 to demonstrate that I am working on this, but it's a bit tricky. What is there "works," but we're still hashing out:

• Refactoring this code so it applies only to commits you create, which happens to work as-written, but could easily get messed up if the contract of changelog changes.
• What the config names should be.
• How to write a proper, maintainable integration test for this logic.

[nairb774]

Really appreciate the preview, I'm building it locally to play with it and see how workable it is. It looks like it ended up being a bit more complicated than I initially guessed, so thank you for continuing to poke at this - it is really appreciated.

Learning a lot about the code following along here as well.

[bolinfest]

Incidentally, I have another commit stacked on top that ensures we use this under the hood when we use commit-tree in sl ghstack and sl pr, which is pretty critical to the overall experience.

[nairb774]

I can confirm #311 works:

Sorry about the small snip, but internal code. I'll run with this for now, and I'll drop any notes here if I stumble across edge cases. Super excited!

[bolinfest]

FYI, I just used the release candidate https://github.com/facebook/sapling/releases/tag/0.1.20221212-142634-r7ae28228 to create https://github.com/facebook/sapling/pull/323, and as you can see, it is signed and verified!

[bolinfest]

Fix is now in the latest release! Docs are here: https://sapling-scm.com/docs/git/signing.