support for openpgp, ssh, x509 signatures with tests #1538

mhammerly · 2025-04-19T17:19:42Z

yet another PR enters the mix for #465

mostly based on:

[WIP] Support GPG-signing commits #966
- mostly i rebased, made a couple changes to reading config
wip: supports gpg signing #1531
- did not change this at all, just stole the smartlog stuff

my additions:

updating the MSRV to 1.80, in line with the git2-ext MSRV
fixed existing snapshot tests
adds integration tests for creating commit signatures using gpg, ssh-keygen, and gpgsm.

there are three integration tests:

one for openpgp signatures, which is gated by a test_gpg feature flag and requires a TEST_GPG env var to point at a gpg binary
one for ssh signatures, which is gated by a test_ssh feature flag and requires a TEST_SSH_KEYGEN env var to point at an ssh-keygen binary
one for x509 signatures, which is gated by a test_x509 feature flag and requires a TEST_GPGSM env var to point at a gpgsm binary

the openpgp and x509 tests actually expect GnuPG tools because their keyring format or whatever is checked in (meaning, no smimesign for mac)

run them with:

$ cargo test -p git-branchless-lib --test test_sign --all-features

claytonrcarter · 2025-04-19T17:59:27Z

this PR is not ready to merge. git-branchless depends on git2 version 0.20.0 whereas the new dependency git2-ext caps its dependency at 0.18.0. we need to resolve that before merging

If I understand the deps situation correctly, we are waiting on:

git-fixture is updated but needs a release
git2-ext (which depends on git-fixture) needs an update and release

The good news is that I believe @epage maintains both of those and has contributed to this project in the past. So, if you can open a PR to update git2-ext, we may be able to beg a favor to get the deps straightened out.

I don't have any knowledge or experience w/ gpg or commit signing, but I may be able to do some code review to shepherd this along. It's a much requested feature!

mhammerly · 2025-04-19T19:42:30Z

i pinged ed on #465 (comment) to ask whether there was a reason the dependency version was capped at 0.18 and request these updates. i suppose opening a PR to do the git2 update is easy enough, though we'll need another PR to update git-fixture when that gets released

EDIT i forgot i tried to do that earlier. can't update the git2 dep without also updating git-fixture because the current latest release locks 0.18 as well. whatever, i can just update the PR when a new release is pushed gitext-rs/git2-ext#92

epage · 2025-04-21T15:30:10Z

Sorry, no one had told me they were moving forward with this and needed a release with support for new git2 versions.

I've released v0.6.3.

mhammerly · 2025-04-21T17:18:02Z

no apology needed! thank you for implementing the signing functionality in the first place, and for doing this little chore to unblock integrating it! i'm using it at work every day and it works great!

Originally implemented by Thomas Ip in PR 966. Updated by Matt. Co-authored-by: Thomas Ip <[email protected]>

mhammerly · 2025-04-21T18:18:36Z

alright, updated git2-ext, quoted the rustc version strings because yaml was parsing 1.80 to 1.8, set the paths for gpg, gpgsm, ssh-keygen in test invocations

remaining issue is apparently gpg.format ssh support was only added in git >= 2.34 so we can't use --all-features on older versions. i'll deal with that later. but the other two tests pass, and if the 2.37.3 test job goes first then all three should pass

claytonrcarter

Thank you for shepherding this work through, and to the previous authors for getting it started. Most of this looked pretty straightforward and made sense to me, but there were a few places I requested some changes, most of which were to refactor this code into a similar style as the rest of the code base.

I'll take another look when that's done. Thanks again!

claytonrcarter · 2025-04-28T01:48:39Z

git-branchless-smartlog/tests/test_smartlog.rs

+    // Run smartlog without --show-signature flag
+    {
+        let stdout = git.smartlog()?;
+        // Without the flag, the signature indicators shouldn't be visible
+        // However, we can't reliably check for absence of brackets since they might be used elsewhere
+        // So we'll just ensure the command runs successfully
+        assert!(!stdout.is_empty(), "Smartlog output should not be empty");
+    }
+
+    // Run smartlog with --show-signature flag
+    {
+        let (stdout, _) = git.branchless("smartlog", &["--show-signature"])?;
+        // With the flag, the command should run successfully
+        assert!(!stdout.is_empty(), "Smartlog output should not be empty");
+        // The signature status indicators should be present in some form,
+        // but we don't validate exactly which status since that depends on the environment
+    }


[please change] Many tests in this project use insta snapshots to capture the output of commands, and I think that it would be best for these tests to follow suit. Snapshots can be tedious to keep updated, but they tend to be much more pleasant to debug. You can see the test above (lines 800-810) for an example.

The first assertion here, that the smartlog works without the flag, may not be needed, since the rest of this file already tests that. It can be handy to have when running a single test though; in that case, it should also use a snapshot.

claytonrcarter · 2025-04-28T02:01:34Z

git-branchless-lib/src/git/repo.rs

@@ -23,6 +23,7 @@ use chrono::{DateTime, Utc};
 use cursive::theme::BaseColor;
 use cursive::utils::markup::StyledString;
 use git2::DiffOptions;
+use git2_ext::ops::Sign;


[suggestion (maybe/probably change)] I think that it would be best to wrap our own type around this (for example crate::git::sign::Signer) to reduce the number of places we may have to rely on git2_ext and make future refactoring easier. (eg if we ever switch to signing support directly in git2, we can just update our implementation of Signer (hopefully) without having to update all of the args, params, etc that depend on it.)

Here's an example of how this is done for Commit:

git-branchless/git-branchless-lib/src/git/object.rs

Lines 19 to 23 in b1f7ae5

/// Represents a commit object in the Git object database.

#[derive(Clone, Debug)]

pub struct Commit<'repo> {

pub(super) inner: git2::Commit<'repo>,

}

git-branchless-lib/src/git/object.rs

claytonrcarter · 2025-04-28T02:20:16Z

git-branchless-record/src/lib.rs

+        let mut args = vec!["commit"];
+
+        args.extend(messages.iter().flat_map(|message| ["--message", message]));
+
+        if working_copy_changes_type == WorkingCopyChangesType::Unstaged {
+            args.push("--all");
+        }
+
+        let sign_flag = sign_option.as_git_flag();
+        if let Some(flag) = &sign_flag {
+            args.push(flag);
+        }
+


[nitpick] Can you restore the block expression syntax (in other words let args = { ... }) while including the new signing lines? That way args can remain immutable in the else.

claytonrcarter · 2025-04-28T02:21:32Z

git-branchless-record/src/lib.rs

+    let mut args = vec!["commit"];
+
+    if !message.is_empty() {
+        args.extend(["--message", &message]);
+    }
+
+    let sign_flag = sign_option.as_git_flag();
+    if let Some(flag) = &sign_flag {
+        args.push(flag);
+    }
+


[please change] Same here, please: let mut args = ...; => let args = { ... }

claytonrcarter · 2025-04-28T02:23:23Z

git-branchless-record/src/lib.rs

+    let mut args = vec!["commit"];
+
+    if !message.is_empty() {
+        args.extend(["--message", &message]);
+    }
+
+    let sign_flag = sign_option.as_git_flag();
+    if let Some(flag) = &sign_flag {
+        args.push(flag);
+    }
+


[please change] Same here, please: let mut args = ...; => let args = { ... }

claytonrcarter · 2025-04-28T02:33:27Z

git-branchless-lib/src/core/node_descriptors.rs

+                    let mut result = StyledString::new();
+                    result.append_styled("[G]", BaseColor::Green.light());
+                    Ok(Some(result))


[nitpick (maybe change)] It looks like other uses of StyledString in this file seem to use let result = StyledString::styled("[G]", BaseColor::Green.light());. To be consistent, can you update these uses of ::new() to match. Or is there a reason that won't work in this context?

claytonrcarter · 2025-04-28T02:38:04Z

git-branchless-smartlog/tests/test_smartlog.rs

+    // Just create a regular commit without trying to sign it
+    git.commit_file("signed_file", 1)?;
+    git.commit_file("unsigned_file", 2)?;


[question] Are these filenames/comments correct? I don't know much about signing commits, but it seems like something is missing to actually do the signing.

claytonrcarter · 2025-04-28T02:39:09Z

git-branchless-smartlog/tests/test_smartlog.rs

+#[test]
+fn test_show_signature_snapshot() -> eyre::Result<()> {
+    let git = make_git()?;
+
+    git.init_repo()?;
+
+    // Create regular commits without trying to sign them
+    git.commit_file("signed_file", 1)?;
+
+    // Create a branch and make another commit
+    git.run(&["checkout", "-b", "branch1", "master"])?;
+    git.commit_file("another_file", 2)?;
+
+    // Run smartlog with --show-signature flag
+    let (stdout, _) = git.branchless("smartlog", &["--show-signature"])?;
+
+    // Simply verify the command runs without error
+    assert!(!stdout.is_empty(), "Smartlog output should not be empty");
+
+    Ok(())
+}


[please change] This seems to be very similar to the previous test. Can you double to check and confirm that they're both really needed?

claytonrcarter · 2025-04-28T02:40:50Z

git-branchless-smartlog/tests/test_smartlog.rs

+#[test]
+fn test_show_signature_argument_parsing() -> eyre::Result<()> {
+    let git = make_git()?;
+    git.init_repo()?;
+
+    // Test that invalid flag combination reports error
+    // For example, combine with a flag that doesn't make sense (if any)
+    // If there are no incompatible flags, we can at least test that the flag is recognized
+
+    // Test help output includes the flag
+    let (help_stdout, _) = git.branchless("smartlog", &["--help"])?;
+    assert!(
+        help_stdout.contains("--show-signature"),
+        "Help output should include --show-signature option"
+    );
+
+    // Test that the flag is recognized (should execute without error)
+    let result = git.branchless("smartlog", &["--show-signature"]);
+    assert!(
+        result.is_ok(),
+        "The --show-signature flag should be recognized"
+    );
+
+    // Test that the flag works when combined with other flags
+    let result = git.branchless("smartlog", &["--show-signature", "--exact"]);
+    assert!(
+        result.is_ok(),
+        "The --show-signature flag should work with --exact"
+    );
+
+    let result = git.branchless("smartlog", &["--show-signature", "--reverse"]);
+    assert!(
+        result.is_ok(),
+        "The --show-signature flag should work with --reverse"
+    );
+
+    Ok(())
+}


[please change] I also think that this test can be removed; mostly it seems to be testing that clap is working as expected. If I'm misunderstanding, let's update it to be more clear about which flags may or may not work with the new --show-signature flag.

mhammerly mentioned this pull request Apr 19, 2025

wip: supports gpg signing #1531

Open

mhammerly force-pushed the gpg-support branch 2 times, most recently from 36d6600 to 97c5af9 Compare April 19, 2025 17:47

mhammerly mentioned this pull request Apr 19, 2025

chore: update git2 dep to 0.20, git-fixture dep to latest gitext-rs/git2-ext#92

Closed

mhammerly force-pushed the gpg-support branch 3 times, most recently from ef87113 to 2cd3c0f Compare April 21, 2025 17:59

mhammerly and others added 6 commits April 21, 2025 11:09

breaking: update msrv to 1.80

e1d76d9

feat: support signing commits with gpg, ssh, x509

c4d0b0b

Originally implemented by Thomas Ip in PR 966. Updated by Matt. Co-authored-by: Thomas Ip <[email protected]>

feat: add support for git branchless --show-signature

1a85de7

feat: adds smartlog tests

1b27a5b

fix: update test snapshots to include show_signature argument

ceb95f2

create tests for commit signatures

7d36b95

mhammerly force-pushed the gpg-support branch from 2cd3c0f to 7d36b95 Compare April 21, 2025 18:09

claytonrcarter requested changes Apr 28, 2025

View reviewed changes

	/// Represents a commit object in the Git object database.
	#[derive(Clone, Debug)]
	pub struct Commit<'repo> {
	pub(super) inner: git2::Commit<'repo>,
	}

Uh oh!

support for openpgp, ssh, x509 signatures with tests #1538

Are you sure you want to change the base?

support for openpgp, ssh, x509 signatures with tests #1538

Uh oh!

Conversation

mhammerly commented Apr 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claytonrcarter commented Apr 19, 2025

Uh oh!

mhammerly commented Apr 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

epage commented Apr 21, 2025

Uh oh!

mhammerly commented Apr 21, 2025

Uh oh!

mhammerly commented Apr 21, 2025

Uh oh!

claytonrcarter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mhammerly commented Apr 19, 2025 •

edited

Loading

mhammerly commented Apr 19, 2025 •

edited

Loading