sign: Add templater methods to show signature info

Disclaimer: this is the work of @necauqua and @julienvincent (see #3141). I simply materialized the changes by rebasing them on latest `main` and making the necessary adjustments to pass CI. --- I had to fix an issue in `TestSignatureBackend::sign()`. The following test was failing: ``` ---- test_signature_templates::test_signature_templates stdout ---- ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Snapshot Summary ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Snapshot: signature_templates Source: cli/tests/test_signature_templates.rs:28 ──────────────────────────────────────────────────────────────────────────────── Expression: stdout ──────────────────────────────────────────────────────────────────────────────── -old snapshot +new results ────────────┬─────────────────────────────────────────────────────────────────── 0 0 │ @ Commit ID: 05ac066d05701071af20e77506a0f2195194cbc9 1 1 │ │ Change ID: qpvuntsmwlqtpsluzzsnyyzlmlwvmlnu 2 2 │ │ Author: Test User <[email protected]> (2001-02-03 08:05:07) 3 3 │ │ Committer: Test User <[email protected]> (2001-02-03 08:05:07) 4 │-│ Signature: Good test signature 4 │+│ Signature: Bad test signature 5 5 │ │ 6 6 │ │ (no description set) 7 7 │ │ 8 8 │ ◆ Commit ID: 0000000000000000000000000000000000000000 ────────────┴─────────────────────────────────────────────────────────────────── ``` Print debugging revealed that the signature was bad, because of a missing trailing `\n` in `TestSignatureBackend::sign()`. ```diff diff --git a/lib/src/test_signing_backend.rs b/lib/src/test_signing_backend.rs index d47fef1086..0ba249e358 100644 --- a/lib/src/test_signing_backend.rs +++ b/lib/src/test_signing_backend.rs @@ -59,6 +59,8 @@ let key = (!key.is_empty()).then_some(std::str::from_utf8(key).unwrap().to_owned()); let sig = self.sign(data, key.as_deref())?; + dbg!(&std::str::from_utf8(&signature).unwrap()); + dbg!(&std::str::from_utf8(&sig).unwrap()); if sig == signature { Ok(Verification::new( SigStatus::Good, ``` ``` [lib/src/test_signing_backend.rs:62:9] &std::str::from_utf8(&signature).unwrap() = \"--- JJ-TEST-SIGNATURE ---\\nKEY: \\n5300977ff3ecda4555bd86d383b070afac7b7459c07f762af918943975394a8261d244629e430c8554258904f16dd9c18d737f8969f2e7d849246db0d93cc004\\n\" [lib/src/test_signing_backend.rs:63:9] &std::str::from_utf8(&sig).unwrap() = \"--- JJ-TEST-SIGNATURE ---\\nKEY: \\n5300977ff3ecda4555bd86d383b070afac7b7459c07f762af918943975394a8261d244629e430c8554258904f16dd9c18d737f8969f2e7d849246db0d93cc004\" ``` Thankfully, @yuja pointed out that libgit2 appends a trailing newline (see bfb7613). Co-authored-by: necauqua <[email protected]> Co-authored-by: julienvincent <[email protected]>
jj-vcs · Jan 3, 2025 · 64de0ae · 64de0ae
1 parent a4e36da
commit 64de0ae
Show file tree

Hide file tree

Showing 9 changed files with 323 additions and 33 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -18,6 +18,8 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 * `jj evolog` now accepts `--reversed`.
 
+* Add templater support for rendering commit signatures.
+
 ### Fixed bugs
 
 ## [0.25.0] - 2025-01-01

diff --git a/cli/src/commit_templater.rs b/cli/src/commit_templater.rs
@@ -49,6 +49,10 @@ use jj_lib::revset::RevsetDiagnostics;
 use jj_lib::revset::RevsetModifier;
 use jj_lib::revset::RevsetParseContext;
 use jj_lib::revset::UserRevsetExpression;
+use jj_lib::signing::SigStatus;
+use jj_lib::signing::SignError;
+use jj_lib::signing::SignResult;
+use jj_lib::signing::Verification;
 use jj_lib::store::Store;
 use once_cell::unsync::OnceCell;
 
@@ -243,6 +247,19 @@ impl<'repo> TemplateLanguage<'repo> for CommitTemplateLanguage<'repo> {
                 let build = template_parser::lookup_method(type_name, table, function)?;
                 build(self, diagnostics, build_ctx, property, function)
             }
+            CommitTemplatePropertyKind::CryptographicSignatureOpt(property) => {
+                let type_name = "CryptographicSignature";
+                let table = &self.build_fn_table.cryptographic_signature_methods;
+                let build = template_parser::lookup_method(type_name, table, function)?;
+                let inner_property = property.try_unwrap(type_name);
+                build(
+                    self,
+                    diagnostics,
+                    build_ctx,
+                    Box::new(inner_property),
+                    function,
+                )
+            }
         }
     }
 }
@@ -319,6 +336,12 @@ impl<'repo> CommitTemplateLanguage<'repo> {
     ) -> CommitTemplatePropertyKind<'repo> {
         CommitTemplatePropertyKind::TreeDiff(Box::new(property))
     }
+
+    fn wrap_cryptographic_signature_opt(
+        property: impl TemplateProperty<Output = Option<CryptographicSignature>> + 'repo,
+    ) -> CommitTemplatePropertyKind<'repo> {
+        CommitTemplatePropertyKind::CryptographicSignatureOpt(Box::new(property))
+    }
 }
 
 pub enum CommitTemplatePropertyKind<'repo> {
@@ -332,6 +355,9 @@ pub enum CommitTemplatePropertyKind<'repo> {
     CommitOrChangeId(Box<dyn TemplateProperty<Output = CommitOrChangeId> + 'repo>),
     ShortestIdPrefix(Box<dyn TemplateProperty<Output = ShortestIdPrefix> + 'repo>),
     TreeDiff(Box<dyn TemplateProperty<Output = TreeDiff> + 'repo>),
+    CryptographicSignatureOpt(
+        Box<dyn TemplateProperty<Output = Option<CryptographicSignature>> + 'repo>,
+    ),
 }
 
 impl<'repo> IntoTemplateProperty<'repo> for CommitTemplatePropertyKind<'repo> {
@@ -347,6 +373,9 @@ impl<'repo> IntoTemplateProperty<'repo> for CommitTemplatePropertyKind<'repo> {
             CommitTemplatePropertyKind::CommitOrChangeId(_) => "CommitOrChangeId",
             CommitTemplatePropertyKind::ShortestIdPrefix(_) => "ShortestIdPrefix",
             CommitTemplatePropertyKind::TreeDiff(_) => "TreeDiff",
+            CommitTemplatePropertyKind::CryptographicSignatureOpt(_) => {
+                "Option<CryptographicSignature>"
+            }
         }
     }
 
@@ -372,6 +401,9 @@ impl<'repo> IntoTemplateProperty<'repo> for CommitTemplatePropertyKind<'repo> {
             // TODO: boolean cast could be implemented, but explicit
             // diff.empty() method might be better.
             CommitTemplatePropertyKind::TreeDiff(_) => None,
+            CommitTemplatePropertyKind::CryptographicSignatureOpt(property) => {
+                Some(Box::new(property.map(|sig| sig.is_some())))
+            }
         }
     }
 
@@ -408,6 +440,7 @@ impl<'repo> IntoTemplateProperty<'repo> for CommitTemplatePropertyKind<'repo> {
                 Some(property.into_template())
             }
             CommitTemplatePropertyKind::TreeDiff(_) => None,
+            CommitTemplatePropertyKind::CryptographicSignatureOpt(_) => None,
         }
     }
 
@@ -426,6 +459,7 @@ impl<'repo> IntoTemplateProperty<'repo> for CommitTemplatePropertyKind<'repo> {
             (CommitTemplatePropertyKind::CommitOrChangeId(_), _) => None,
             (CommitTemplatePropertyKind::ShortestIdPrefix(_), _) => None,
             (CommitTemplatePropertyKind::TreeDiff(_), _) => None,
+            (CommitTemplatePropertyKind::CryptographicSignatureOpt(_), _) => None,
         }
     }
 
@@ -447,6 +481,7 @@ impl<'repo> IntoTemplateProperty<'repo> for CommitTemplatePropertyKind<'repo> {
             (CommitTemplatePropertyKind::CommitOrChangeId(_), _) => None,
             (CommitTemplatePropertyKind::ShortestIdPrefix(_), _) => None,
             (CommitTemplatePropertyKind::TreeDiff(_), _) => None,
+            (CommitTemplatePropertyKind::CryptographicSignatureOpt(_), _) => None,
         }
     }
 }
@@ -463,6 +498,8 @@ pub struct CommitTemplateBuildFnTable<'repo> {
     pub commit_or_change_id_methods: CommitTemplateBuildMethodFnMap<'repo, CommitOrChangeId>,
     pub shortest_id_prefix_methods: CommitTemplateBuildMethodFnMap<'repo, ShortestIdPrefix>,
     pub tree_diff_methods: CommitTemplateBuildMethodFnMap<'repo, TreeDiff>,
+    pub cryptographic_signature_methods:
+        CommitTemplateBuildMethodFnMap<'repo, CryptographicSignature>,
 }
 
 impl<'repo> CommitTemplateBuildFnTable<'repo> {
@@ -475,6 +512,7 @@ impl<'repo> CommitTemplateBuildFnTable<'repo> {
             commit_or_change_id_methods: builtin_commit_or_change_id_methods(),
             shortest_id_prefix_methods: builtin_shortest_id_prefix_methods(),
             tree_diff_methods: builtin_tree_diff_methods(),
+            cryptographic_signature_methods: builtin_cryptographic_signature_methods(),
         }
     }
 
@@ -486,6 +524,7 @@ impl<'repo> CommitTemplateBuildFnTable<'repo> {
             commit_or_change_id_methods: HashMap::new(),
             shortest_id_prefix_methods: HashMap::new(),
             tree_diff_methods: HashMap::new(),
+            cryptographic_signature_methods: HashMap::new(),
         }
     }
 
@@ -497,6 +536,7 @@ impl<'repo> CommitTemplateBuildFnTable<'repo> {
             commit_or_change_id_methods,
             shortest_id_prefix_methods,
             tree_diff_methods,
+            cryptographic_signature_methods,
         } = extension;
 
         self.core.merge(core);
@@ -511,6 +551,10 @@ impl<'repo> CommitTemplateBuildFnTable<'repo> {
             shortest_id_prefix_methods,
         );
         merge_fn_map(&mut self.tree_diff_methods, tree_diff_methods);
+        merge_fn_map(
+            &mut self.cryptographic_signature_methods,
+            cryptographic_signature_methods,
+        );
     }
 }
 
@@ -621,6 +665,14 @@ fn builtin_commit_methods<'repo>() -> CommitTemplateBuildMethodFnMap<'repo, Comm
             Ok(L::wrap_boolean(out_property))
         },
     );
+    map.insert(
+        "signature",
+        |_language, _diagnostics, _build_ctx, self_property, function| {
+            function.expect_no_arguments()?;
+            let out_property = self_property.map(CryptographicSignature::new);
+            Ok(L::wrap_cryptographic_signature_opt(out_property))
+        },
+    );
     map.insert(
         "working_copies",
         |language, _diagnostics, _build_ctx, self_property, function| {
@@ -1671,3 +1723,88 @@ fn builtin_tree_diff_methods<'repo>() -> CommitTemplateBuildMethodFnMap<'repo, T
     // TODO: add files() or map() to support custom summary-like formatting?
     map
 }
+
+#[derive(Debug)]
+pub struct CryptographicSignature {
+    commit: Commit,
+}
+
+impl CryptographicSignature {
+    fn new(commit: Commit) -> Option<Self> {
+        commit.is_signed().then_some(Self { commit })
+    }
+
+    fn verify(&self) -> SignResult<Verification> {
+        self.commit
+            .verification()
+            .transpose()
+            .expect("must have signature")
+    }
+
+    fn status(&self) -> SignResult<SigStatus> {
+        self.verify().map(|verification| verification.status)
+    }
+
+    /// Defaults to empty string if key is not present.
+    fn key(&self) -> SignResult<String> {
+        self.verify()
+            .map(|verification| verification.key.unwrap_or_default())
+    }
+
+    /// Defaults to empty string if display is not present.
+    fn display(&self) -> SignResult<String> {
+        self.verify()
+            .map(|verification| verification.display.unwrap_or_default())
+    }
+
+    /// Defaults to empty string if backend is not present.
+    fn backend(&self) -> SignResult<String> {
+        self.verify()
+            .map(|verification| verification.backend().unwrap_or_default().into())
+    }
+}
+
+pub fn builtin_cryptographic_signature_methods<'repo>(
+) -> CommitTemplateBuildMethodFnMap<'repo, CryptographicSignature> {
+    type L<'repo> = CommitTemplateLanguage<'repo>;
+    // Not using maplit::hashmap!{} or custom declarative macro here because
+    // code completion inside macro is quite restricted.
+    let mut map = CommitTemplateBuildMethodFnMap::<CryptographicSignature>::new();
+    map.insert(
+        "status",
+        |_language, _diagnostics, _build_ctx, self_property, function| {
+            function.expect_no_arguments()?;
+            let out_property = self_property.and_then(|sig| match sig.status() {
+                Ok(status) => Ok(status.to_string()),
+                Err(SignError::InvalidSignatureFormat) => Ok("invalid".to_string()),
+                Err(err) => Err(err.into()),
+            });
+            Ok(L::wrap_string(out_property))
+        },
+    );
+    map.insert(
+        "key",
+        |_language, _diagnostics, _build_ctx, self_property, function| {
+            function.expect_no_arguments()?;
+            let out_property = self_property.and_then(|sig| Ok(sig.key()?));
+            Ok(L::wrap_string(out_property))
+        },
+    );
+    map.insert(
+        "display",
+        |_language, _diagnostics, _build_ctx, self_property, function| {
+            function.expect_no_arguments()?;
+            let out_property = self_property.and_then(|sig| Ok(sig.display()?));
+            Ok(L::wrap_string(out_property))
+        },
+    );
+    map.insert(
+        "backend",
+        |_language, _diagnostics, _build_ctx, self_property, function| {
+            function.expect_no_arguments()?;
+            let out_property = self_property.and_then(|sig| Ok(sig.backend()?));
+            Ok(L::wrap_string(out_property))
+        },
+    );
+    map
+}
diff --git a/cli/tests/test_commit_template.rs b/cli/tests/test_commit_template.rs
@@ -1209,3 +1209,29 @@ fn test_log_diff_predefined_formats() {
     +c
     "###);
 }
+
+#[test]
+fn test_signature_templates() {
+    let test_env = TestEnvironment::default();
+
+    test_env.add_config(r#"signing.sign-all = true"#);
+    test_env.add_config(r#"signing.backend = "test""#);
+
+    test_env.jj_cmd_ok(test_env.env_root(), &["init", "repo", "--git"]);
+    let repo_path = test_env.env_root().join("repo");
+
+    let template = r#"if(signature,
+                         signature.status() ++ " " ++
+                         signature.display() ++ " " ++
+                         signature.backend(),
+                         "no"
+                      ) ++ " signature""#;
+
+    let stdout = test_env.jj_cmd_success(&repo_path, &["log", "-T", template]);
+    insta::assert_snapshot!(stdout, @r"
+        @  good test-display test signature
+        ◆  no signature");
+
+    let stdout = test_env.jj_cmd_success(&repo_path, &["show", "-T", template]);
+    insta::assert_snapshot!(stdout, @"good test-display test signature");
+}
diff --git a/docs/config.md b/docs/config.md
@@ -1027,6 +1027,17 @@ as follows:
 backends.ssh.allowed-signers = "/path/to/allowed-signers"
 ```
 
+## Commit Signature Verification
+
+By default signature verification and display is **disabled** as it incurs a
+performance cost when rendering medium to large change logs.
+
+If you want to display commit signatures in your templates, you can use
+`commit.signature()` (see [Commit type](./templates.md#commit-type)). The
+returned [CryptographicSignature
+Type](./templates.md#cryptographicsignature-type) provides methods to retrieve
+signature details.
+
 ## Git settings
 
 ### Default remotes for `jj git fetch` and `jj git push`

diff --git a/docs/templates.md b/docs/templates.md
@@ -93,6 +93,7 @@ This type cannot be printed. The following methods are defined.
 * `parents() -> List<Commit>`
 * `author() -> Signature`
 * `committer() -> Signature`
+* `signature() -> Option<CryptographicSignature>`
 * `mine() -> Boolean`: Commits where the author's email matches the email of the current
   user.
 * `working_copies() -> String`: For multi-workspace repository, indicate
@@ -129,6 +130,31 @@ The following methods are defined.
 * `.short([len: Integer]) -> String`
 * `.shortest([min_len: Integer]) -> ShortestIdPrefix`: Shortest unique prefix.
 
+### CryptographicSignature type
+
+The following methods are defined.
+
+* `.status() -> String`: The signature's status (`"good"`, `"bad"`, `"unknown"`, `"invalid"`).
+* `.key() -> String`: The signature's key id representation (for GPG, this is the key fingerprint).
+* `.display() -> String`: The signature's display string (for GPG this is the formatted primary user ID).
+* `.backend() -> String`: The signature backend (e.g. `"gpg"`).
+
+!!! warning
+
+    Calling any of `.status()`, `.key()`, `.display()`, or `.backend()` is
+    slow, as it incurs the performance cost of verifying the signature (for
+    example shelling out to `gpg` or `ssh-keygen`). Though consecutive calls
+    will be faster, because the backend caches the verification result.
+
+!!! info
+
+    As opposed to calling any of `.status()`, `.key()`, `.display()`, or
+    `.backend()`, checking for signature presence through boolean coercion is
+    fast:
+    ```
+    if(commit.signature(), "commit has a signature", "commit is unsigned")
+    ```
+
 ### Email type
 
 The following methods are defined.