extrawurst
Add support for GPG signing commits
Overview
This PR is an initial stab at resolving #97. This is a recreation of #218, which I accidentally created without marking as a draft PR and I was unable to set it back so I decided to close it.
With these changes, I was able to create this PR's commits signed with gitui:
Details
Unfortunately, libgit2 appears to not provide an interface for creating signed commits as friendly as standard commit creation so there is a lot of code added. The GPG signing must be done manually which brings in dependencies for gpgme. Unlike standard commit creation, there is not an option to automatically update HEAD so it must be done manually when after creating a signed commit. When dealing with a newly initialized repo, this means the default branch has to be created as well! There is no official documentation creating signed commits with libgit2, so credit goes to https://github.com/rust-lang/git2-rs/issues/507 for figuring it out.
I am not sure how test suite should handle this new functionality. The main conditional is currently determined by the user's git config option, so on my computer it will always use the GPG signing. We could add extra tests and manage whether GPG signing should be on for each one or try to run the entire suite with GPG signing on and off. The main commit tests are working, but locally I am getting ~4-6 failures each time. Here is a sample run:
...
failures:
---- sync::commit_details::tests::test_msg_invalid_utf8 stdout ----
thread 'sync::commit_details::tests::test_msg_invalid_utf8' panicked at 'Buffer was not valid UTF-8', asyncgit/src/sync/commit.rs:83:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
---- sync::commits_info::tests::test_invalid_utf8 stdout ----
thread 'sync::commits_info::tests::test_invalid_utf8' panicked at 'Buffer was not valid UTF-8', asyncgit/src/sync/commit.rs:83:13
---- sync::commit::tests::test_tag stdout ----
Error: Gpg(Error { source: Some("gcrypt"), code: 32854, description: "Cannot allocate memory" })
thread 'sync::commit::tests::test_tag' panicked at 'assertion failed: `(left == right)`
left: `1`,
right: `0`: the test returned a termination value with a non-zero status code (1) which indicates a failure', /Users/grantbdev/.rustup/toolchains/stable-x86_64-apple-darwin/lib/rustlib/src/rust/src/libstd/macros.rs:16:9
---- sync::commits_info::tests::test_log stdout ----
thread 'sync::commits_info::tests::test_log' panicked at 'called `Result::unwrap()` on an `Err` value: Gpg(Error { source: Some("Pinentry"), code: 32854, description: "Cannot allocate memory" })', asyncgit/src/sync/commits_info.rs:137:18
---- sync::stash::tests::test_stash_without_2nd_parent stdout ----
Error: Gpg(Error { source: Some("Pinentry"), code: 32854, description: "Cannot allocate memory" })
thread 'sync::stash::tests::test_stash_without_2nd_parent' panicked at 'assertion failed: `(left == right)`
left: `1`,
right: `0`: the test returned a termination value with a non-zero status code (1) which indicates a failure', /Users/grantbdev/.rustup/toolchains/stable-x86_64-apple-darwin/lib/rustlib/src/rust/src/libstd/macros.rs:16:9
...
test result: FAILED. 48 passed; 5 failed; 0 ignored; 0 measured; 0 filtered out
The invalid UTF-8 failures seem to happen every time. There is a point in the GPG commit logic that will panic if the message contains invalid UTF-8 and I guess that differs from the standard commit logic? I'm not sure how that should be resolved other than having different expectations with and without the GPG signing feature.
The rest are random tests failing each run, pointing to gcrypt or Pinentry and saying it couldn't allocate memory. This appears to only happen when the tests are run in parallel by default. cargo test -- --test-threads=1 will eliminate the random failures.
@grantbdev thanks for looking into this! Can we maybe learn something from libgit2 in how they manage to unittest stuff in the presence of configs?
@extrawurst From what I can tell, the libraries tests things at low enough of a level that typically the config isn't a factor. In git2 there are some occasional config setups: https://github.com/rust-lang/git2-rs/blob/23954937d69ed5c3a798ba85f653099fbecf3cf2/src/build.rs#L720-L721
Setting values during a test like this will impact the execution: repo.config().unwrap().set_bool("commit.gpgsign", false).unwrap(); The config values set in the test code will take priority over the git config on the computer. So that seems like a good way to control it which branches to take in gitui's higher-level logic that would depend on the config.
git2 doesn't test GPG signing at all. The tests I see in libgit2 provide pre-determined signatures which isn't really an option here: https://github.com/libgit2/libgit2/blob/02d61a3b66a6e5f5bc0154d780daaf5f7b71ccd9/tests/commit/write.c#L302-L400
@extrawurst Sorry, definitely got distracted from working on this. I know I was getting discouraged that it seems system (non-Rust) libraries were needed to get this to work and I was having trouble getting it set up correctly for the Windows CI build. I have a feeling this could be too burdensome to the project overall to maintain and I'm uncertain about whether it will negatively impact users who don't care about GPG signing, but let me know what you think.
My gut feeling is that libgit2could be making adding this feature much harder than it needs to be. It appears GitX shelled out to the git CLI which made it much easier to add GPG support and while Fork is not open-source I suspect it's doing something similar since it selects installed git instances.
I know that is a pretty radical departure from the current design of your project, but I'm curious if you have considered or experimented with that approach at all. I also wonder if it's something that could be used in some parts rather than a complete overhaul of not using libgit2 for anything.
@grantbdev it would be a shame to see this bitrot away. but I agree adding big bunch of system dependencies is a bit of a downer. did you look into https://gitlab.com/sequoia-pgp/sequoia if that might be a solution here?
@extrawurst I don't think I previously looked at Sequoia, so thanks for linking. I took a short spin at it and from what I can tell I don't think it well help. I think it's generally more low-level than what GPGme provides and I'm not familiar with the implementation details. For instance, I am not sure how to replicate finding a public key from the local keychain which is the first step in using the user's Git GPG settings.
Hey @extrawurst and @grantbdev, What`s the current status of this PR? How I can help to get, this shipped at least at an experimental feature(Experimental Feature Flag or Config). I really want to use it since I use signed commits everywhere.
You can create commit objects with git commit-tree or sign them directly with git commit-tree -S afterward it will return the git commit object id. Probably we can use it in libgit2 to create the final commit.
I would also propose signing commits directly with the git binary because it’s more convenient to configure it and reduce dependencies. Since People who sign commits already have these tools installed on their machine, it works with the git binary properly.
References
https://git-scm.com/docs/git-commit-tree https://jwiegley.github.io/git-from-the-bottom-up/1-Repository/4-how-trees-are-made.html
@solidnerd This PR is effectively abandoned by me at this point. You're welcome to take your own stab at it and use any of my commits if they are helpful. My understanding was libgit2 wouldn't work (at least easily) for this task, and I don't know what @extrawurst's opinion is on utilizing the git binary in this project.
Just if that helps. I'm gitg maintainer and use this PR as reference to confirm steps needed to sign a commit.
libgit2 is agnostic about signing a commit, that's why it offers a commit_create_buffer (with contents for commit) and a commit_create_with_signature which just add this signature to commit object. In the middle you sign with gpgme that content. I hear there's an spec to sign commits with ssh keys, that would be the only part to change here if you want to implement that too.
The tricky part is create_commit_from_ids updates the reference (symbolic like master or real as HEAD) while with this functions you need to do on your own.
I just merge it: https://gitlab.gnome.org/GNOME/gitg/-/merge_requests/198 and libgit2-glib (glib wrapper around libgit2) related PR is linked there.
This is probably a dumb question... but why not just shell out to the git cli to do the commit?
@nigelatdas that's the approach gitg will take. libgit2 has different scopes and goals than git GUI