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\" ``` I still have no idea, where in the call chain that trailing `\n` is added to `signature`. I tried to retrace `signature`'s steps. However, it seemed to be returned from `TestSignatureBackend::sign()`, which was even more mind-boggling to me since `sig` is also returned from `TestSignatureBackend::sign()`. How can they be different?
martinvonz · Nov 23, 2024 · 4e6ba53 · 4e6ba53
1 parent dec683b
commit 4e6ba53
Show file tree

Hide file tree

Showing 19 changed files with 446 additions and 35 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -958,6 +958,9 @@ Thanks to the people who made this release happen!
 * New function `working_copies()` for revsets to show the working copy commits
   of all workspaces.
 
+* Add templater support for rendering commit signatures and added new builtin templates which
+  show commit signatures.
+
 ### Fixed bugs
 
 None.

diff --git a/cli/src/commit_templater.rs b/cli/src/commit_templater.rs
@@ -47,6 +47,9 @@ 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::Verification;
 use jj_lib::store::Store;
 use once_cell::unsync::OnceCell;
 
@@ -72,6 +75,7 @@ use crate::templater::PlainTextFormattedProperty;
 use crate::templater::SizeHint;
 use crate::templater::Template;
 use crate::templater::TemplateFormatter;
+use crate::templater::TemplateFunction;
 use crate::templater::TemplateProperty;
 use crate::templater::TemplatePropertyError;
 use crate::templater::TemplatePropertyExt as _;
@@ -237,6 +241,11 @@ 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::CommitSignature(property) => {
+                let table = &self.build_fn_table.build_commit_signature_method;
+                let build = template_parser::lookup_method("CommitSignature", table, function)?;
+                build(self, diagnostics, build_ctx, property, function)
+            }
         }
     }
 }
@@ -313,6 +322,13 @@ impl<'repo> CommitTemplateLanguage<'repo> {
     ) -> CommitTemplatePropertyKind<'repo> {
         CommitTemplatePropertyKind::TreeDiff(Box::new(property))
     }
+
+    fn wrap_commit_signature(
+        &self,
+        property: impl TemplateProperty<Output = CommitSignature> + 'repo,
+    ) -> CommitTemplatePropertyKind<'repo> {
+        CommitTemplatePropertyKind::CommitSignature(Box::new(property))
+    }
 }
 
 pub enum CommitTemplatePropertyKind<'repo> {
@@ -326,6 +342,7 @@ 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>),
+    CommitSignature(Box<dyn TemplateProperty<Output = CommitSignature> + 'repo>),
 }
 
 impl<'repo> IntoTemplateProperty<'repo> for CommitTemplatePropertyKind<'repo> {
@@ -341,6 +358,7 @@ impl<'repo> IntoTemplateProperty<'repo> for CommitTemplatePropertyKind<'repo> {
             CommitTemplatePropertyKind::CommitOrChangeId(_) => "CommitOrChangeId",
             CommitTemplatePropertyKind::ShortestIdPrefix(_) => "ShortestIdPrefix",
             CommitTemplatePropertyKind::TreeDiff(_) => "TreeDiff",
+            CommitTemplatePropertyKind::CommitSignature(_) => "CommitSignature",
         }
     }
 
@@ -366,6 +384,7 @@ 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::CommitSignature(_) => None,
         }
     }
 
@@ -402,6 +421,7 @@ impl<'repo> IntoTemplateProperty<'repo> for CommitTemplatePropertyKind<'repo> {
                 Some(property.into_template())
             }
             CommitTemplatePropertyKind::TreeDiff(_) => None,
+            CommitTemplatePropertyKind::CommitSignature(_) => None,
         }
     }
 
@@ -420,6 +440,7 @@ impl<'repo> IntoTemplateProperty<'repo> for CommitTemplatePropertyKind<'repo> {
             (CommitTemplatePropertyKind::CommitOrChangeId(_), _) => None,
             (CommitTemplatePropertyKind::ShortestIdPrefix(_), _) => None,
             (CommitTemplatePropertyKind::TreeDiff(_), _) => None,
+            (CommitTemplatePropertyKind::CommitSignature(_), _) => None,
         }
     }
 }
@@ -436,6 +457,7 @@ 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 build_commit_signature_method: CommitTemplateBuildMethodFnMap<'repo, CommitSignature>,
 }
 
 impl<'repo> CommitTemplateBuildFnTable<'repo> {
@@ -448,6 +470,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(),
+            build_commit_signature_method: build_commit_signature_method(),
         }
     }
 
@@ -459,6 +482,7 @@ impl<'repo> CommitTemplateBuildFnTable<'repo> {
             commit_or_change_id_methods: HashMap::new(),
             shortest_id_prefix_methods: HashMap::new(),
             tree_diff_methods: HashMap::new(),
+            build_commit_signature_method: HashMap::new(),
         }
     }
 
@@ -470,6 +494,7 @@ impl<'repo> CommitTemplateBuildFnTable<'repo> {
             commit_or_change_id_methods,
             shortest_id_prefix_methods,
             tree_diff_methods,
+            build_commit_signature_method,
         } = extension;
 
         self.core.merge(core);
@@ -484,6 +509,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.build_commit_signature_method,
+            build_commit_signature_method,
+        );
     }
 }
 
@@ -594,6 +623,22 @@ 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()?;
+            Ok(
+                language.wrap_commit_signature(TemplateFunction::new(self_property, |commit| {
+                    match commit.verification() {
+                        Ok(Some(v)) => Ok(CommitSignature::Present(v)),
+                        Err(SignError::InvalidSignatureFormat) => Ok(CommitSignature::Invalid),
+                        Ok(None) => Ok(CommitSignature::Absent),
+                        Err(e) => Err(e.into()),
+                    }
+                })),
+            )
+        },
+    );
     map.insert(
         "working_copies",
         |language, _diagnostics, _build_ctx, self_property, function| {
@@ -1633,3 +1678,124 @@ fn builtin_tree_diff_methods<'repo>() -> CommitTemplateBuildMethodFnMap<'repo, T
     // TODO: add files() or map() to support custom summary-like formatting?
     map
 }
+
+#[derive(Clone, Debug, Eq, PartialEq)]
+pub enum CommitSignature {
+    Present(Verification),
+    Absent,
+    Invalid,
+}
+
+impl CommitSignature {
+    pub fn is_present(&self) -> bool {
+        matches!(self, CommitSignature::Present(_))
+    }
+
+    pub fn is_invalid(&self) -> bool {
+        matches!(self, CommitSignature::Invalid)
+    }
+
+    pub fn is(&self, status: SigStatus) -> bool {
+        match self {
+            CommitSignature::Present(v) => v.status == status,
+            _ => false,
+        }
+    }
+
+    pub fn key(self) -> String {
+        match self {
+            CommitSignature::Present(v) => v.key.unwrap_or_default(),
+            _ => Default::default(),
+        }
+    }
+
+    pub fn display(self) -> String {
+        match self {
+            CommitSignature::Present(v) => v.display.unwrap_or_default(),
+            _ => Default::default(),
+        }
+    }
+
+    pub fn backend(self) -> String {
+        match self {
+            CommitSignature::Present(v) => v.backend().unwrap_or_default().to_owned(),
+            _ => Default::default(),
+        }
+    }
+}
+
+fn build_commit_signature_method<'repo>() -> CommitTemplateBuildMethodFnMap<'repo, CommitSignature>
+{
+    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::<CommitSignature>::new();
+    map.insert(
+        "present",
+        |_language, _diagnostics, _build_ctx, self_property, function| {
+            function.expect_no_arguments()?;
+            let out_property = TemplateFunction::new(self_property, |sig| Ok(sig.is_present()));
+            Ok(L::wrap_boolean(out_property))
+        },
+    );
+    map.insert(
+        "good",
+        |_language, _diagnostics, _build_ctx, self_property, function| {
+            function.expect_no_arguments()?;
+            let out_property =
+                TemplateFunction::new(self_property, |sig| Ok(sig.is(SigStatus::Good)));
+            Ok(L::wrap_boolean(out_property))
+        },
+    );
+    map.insert(
+        "unknown",
+        |_language, _diagnostics, _build_ctx, self_property, function| {
+            function.expect_no_arguments()?;
+            let out_property =
+                TemplateFunction::new(self_property, |sig| Ok(sig.is(SigStatus::Unknown)));
+            Ok(L::wrap_boolean(out_property))
+        },
+    );
+    map.insert(
+        "bad",
+        |_language, _diagnostics, _build_ctx, self_property, function| {
+            function.expect_no_arguments()?;
+            let out_property =
+                TemplateFunction::new(self_property, |sig| Ok(sig.is(SigStatus::Bad)));
+            Ok(L::wrap_boolean(out_property))
+        },
+    );
+    map.insert(
+        "invalid",
+        |_language, _diagnostics, _build_ctx, self_property, function| {
+            function.expect_no_arguments()?;
+            let out_property = TemplateFunction::new(self_property, |sig| Ok(sig.is_invalid()));
+            Ok(L::wrap_boolean(out_property))
+        },
+    );
+    map.insert(
+        "key",
+        |_language, _diagnostics, _build_ctx, self_property, function| {
+            function.expect_no_arguments()?;
+            let out_property = TemplateFunction::new(self_property, |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 = TemplateFunction::new(self_property, |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 = TemplateFunction::new(self_property, |sig| Ok(sig.backend()));
+            Ok(L::wrap_string(out_property))
+        },
+    );
+    map
+}
diff --git a/cli/src/config/colors.toml b/cli/src/config/colors.toml
@@ -110,3 +110,10 @@
 "node current_operation" = { fg = "green", bold = true }
 "node immutable" = { fg = "bright cyan", bold = true }
 "node conflict" = { fg = "red", bold = true }
+
+"signature good" = "green"
+"signature unknown" = "bright black"
+"signature bad" = "red"
+"signature invalid" = "yellow"
+"signature key" = "blue"
+"signature display" = "yellow"
diff --git a/cli/src/config/templates.toml b/cli/src/config/templates.toml
@@ -317,3 +317,61 @@ coalesce(
   "o",
 )
 '''
+
+'builtin_log_detailed_with_sig' = '''
+concat(
+  "Commit ID: " ++ commit_id ++ "\n",
+  "Change ID: " ++ change_id ++ "\n",
+  surround("Bookmarks: ", "\n", separate(" ", local_bookmarks, remote_bookmarks)),
+  surround("Tags: ", "\n", tags),
+  "Author: " ++ format_detailed_signature(author) ++ "\n",
+  "Committer: " ++ format_detailed_signature(committer)  ++ "\n",
+  builtin_sig_detailed,
+  "\n",
+  indent("    ", if(description, description, description_placeholder ++ "\n")),
+  "\n",
+)
+'''
+
+builtin_sig_status = '''
+if(signature.present(),
+  label("signature",
+    concat(
+      "[",
+      label("status",
+        if(signature.good(), label("good", "✓︎"),
+          if(signature.unknown(), label("unknown", "?"),
+            if(signature.bad(), label("bad", "x"),
+              if(signature.invalid(), label("invalid", "x"))
+            )
+          )
+        )
+      ),
+      "]"
+    )
+  )
+)
+'''
+
+builtin_sig_detailed = '''
+if(signature.present(),
+  concat(
+    "Signature: ",
+    label("signature",
+      if(signature.good(), label("good", "Good"),
+        if(signature.unknown(), label("unknown", "Unknown"),
+          if(signature.bad(), label("bad", "Bad"),
+            if(signature.invalid(), label("invalid", "Invalid"))
+          )
+        )
+      )
+    ),
+    if(signature.backend(), " " ++ label("backend", signature.backend()) ++ " signature"),
+    if(signature.display(),
+      ". By " ++ label("display", signature.display()) ++ if(signature.key(), " (key " ++ label("key", signature.key()) ++ ")"),
+      if(signature.key(), ". Key " ++ label("key", signature.key()))
+    ),
+    "\n"
+  )
+)
+'''
diff --git a/cli/tests/runner.rs b/cli/tests/runner.rs
@@ -63,6 +63,7 @@ mod test_revset_output;
 mod test_root;
 mod test_shell_completion;
 mod test_show_command;
+mod test_signature_templates;
 mod test_simplify_parents_command;
 mod test_sparse_command;
 mod test_split_command;

diff --git a/cli/tests/test_evolog_command.rs b/cli/tests/test_evolog_command.rs
@@ -336,13 +336,16 @@ fn test_evolog_with_no_template() {
     - builtin_log_compact
     - builtin_log_compact_full_description
     - builtin_log_detailed
+    - builtin_log_detailed_with_sig
     - builtin_log_node
     - builtin_log_node_ascii
     - builtin_log_oneline
     - builtin_op_log_comfortable
     - builtin_op_log_compact
     - builtin_op_log_node
     - builtin_op_log_node_ascii
+    - builtin_sig_detailed
+    - builtin_sig_status
     - commit_summary_separator
     - description_placeholder
     - email_placeholder

diff --git a/cli/tests/test_log_command.rs b/cli/tests/test_log_command.rs
@@ -45,13 +45,16 @@ fn test_log_with_no_template() {
     - builtin_log_compact
     - builtin_log_compact_full_description
     - builtin_log_detailed
+    - builtin_log_detailed_with_sig
     - builtin_log_node
     - builtin_log_node_ascii
     - builtin_log_oneline
     - builtin_op_log_comfortable
     - builtin_op_log_compact
     - builtin_op_log_node
     - builtin_op_log_node_ascii
+    - builtin_sig_detailed
+    - builtin_sig_status
     - commit_summary_separator
     - description_placeholder
     - email_placeholder