Skip to content

Commit

Permalink
sign: Add templater methods to show signature info
Browse files Browse the repository at this point in the history
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?

---

Co-authored-by: julienvincent <[email protected]>
Co-authored-by: pylbrecht <[email protected]>
  • Loading branch information
3 people committed Nov 23, 2024
1 parent dec683b commit d1a1a72
Show file tree
Hide file tree
Showing 19 changed files with 446 additions and 35 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
166 changes: 166 additions & 0 deletions cli/src/commit_templater.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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 _;
Expand Down Expand Up @@ -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)
}
}
}
}
Expand Down Expand Up @@ -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> {
Expand All @@ -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> {
Expand All @@ -341,6 +358,7 @@ impl<'repo> IntoTemplateProperty<'repo> for CommitTemplatePropertyKind<'repo> {
CommitTemplatePropertyKind::CommitOrChangeId(_) => "CommitOrChangeId",
CommitTemplatePropertyKind::ShortestIdPrefix(_) => "ShortestIdPrefix",
CommitTemplatePropertyKind::TreeDiff(_) => "TreeDiff",
CommitTemplatePropertyKind::CommitSignature(_) => "CommitSignature",
}
}

Expand All @@ -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,
}
}

Expand Down Expand Up @@ -402,6 +421,7 @@ impl<'repo> IntoTemplateProperty<'repo> for CommitTemplatePropertyKind<'repo> {
Some(property.into_template())
}
CommitTemplatePropertyKind::TreeDiff(_) => None,
CommitTemplatePropertyKind::CommitSignature(_) => None,
}
}

Expand All @@ -420,6 +440,7 @@ impl<'repo> IntoTemplateProperty<'repo> for CommitTemplatePropertyKind<'repo> {
(CommitTemplatePropertyKind::CommitOrChangeId(_), _) => None,
(CommitTemplatePropertyKind::ShortestIdPrefix(_), _) => None,
(CommitTemplatePropertyKind::TreeDiff(_), _) => None,
(CommitTemplatePropertyKind::CommitSignature(_), _) => None,
}
}
}
Expand All @@ -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> {
Expand All @@ -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(),
}
}

Expand All @@ -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(),
}
}

Expand All @@ -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);
Expand All @@ -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,
);
}
}

Expand Down Expand Up @@ -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| {
Expand Down Expand Up @@ -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
}
7 changes: 7 additions & 0 deletions cli/src/config/colors.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
58 changes: 58 additions & 0 deletions cli/src/config/templates.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
)
'''
1 change: 1 addition & 0 deletions cli/tests/runner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
3 changes: 3 additions & 0 deletions cli/tests/test_evolog_command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 3 additions & 0 deletions cli/tests/test_log_command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading

0 comments on commit d1a1a72

Please sign in to comment.