Display commit signatures #4853
Conversation
pylbrecht
force-pushed
the
jj-sign/display
branch
15 times, most recently
from
760fda2 to
b000597
Compare
pylbrecht
force-pushed
the
jj-sign/display
branch
4 times, most recently
from
d1a1a72 to
4e6ba53
Compare
|
Is there anything blocking this PR? It seems to be in a functional state. |
Well, since I simply materialized the changes from @julienvincent and @necauqua, there are still two things missing: Regarding printing hints about dropped signatures, I reached out for help (#4712 (comment), #4712 (comment)). Maybe we could merge without the hints (and create a new issue to track that missing piece to be implemented later)? Then I would focus on getting the docs ready. |
|
Based on the PR title, I would presume that this PR is focused mainly on being able to display commit signatures. So it seems fine to me if printing hints about dropped commit signatures is shifted to a different issue/PR, since it isn't directly related to displaying commit signatures. |
|
Sounds good to me. |
pylbrecht
force-pushed
the
jj-sign/display
branch
from
4e6ba53 to
f4d1de5
Compare
|
I pushed a slightly modified version of 739ddf2. However, I believe it was planned to add a @necauqua, originally this was your work, I think. Was it planned to add |
pylbrecht
force-pushed
the
jj-sign/display
branch
from
f4d1de5 to
b59be44
Compare
We need to make `TestSigningBackend` available for cli tests, as we will add cli tests for signing related functionality (templates for displaying commit signatures, `jj sign`) in upcoming commits. Co-authored-by: julienvincent <[email protected]>
Disclaimer: this is the work of @necauqua and @julienvincent (see jj-vcs#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: necauqua <[email protected]> Co-authored-by: julienvincent <[email protected]>
Co-authored-by: julienvincent <[email protected]>
pylbrecht
force-pushed
the
jj-sign/display
branch
from
b59be44 to
8a8e8b5
Compare
|
@julienvincent, I added you as co-author to the commits. Would you like to provide the email address you used to sign the CLA, so I can add you as a co-author? |
|
Thanks for attribution - I just went ahead and signed the CLA for the email address you used. I reran the check which is now passing 👍🏻 |
As far as I remember - originally this was implemented by modifying the default template to add commit signatures. This was then removed from the default template into a new, named one. This was done for two reasons:
|
|
Yeah, mostly the second point. Also, I vaguely recall that I suggested having something like |
|
Thanks for elaborating on past discussions.
I was wondering about the dormant While it is not actively used at the moment, should we keep On a general note, I would like to get this merged rather sooner than later, dealing with additional work in follow-ups wherever reasonable. I think @bnjmnt4n would be happy to see this merged, as #4712 depends on parts of this PR. |
|
I would suggest splitting builtin template thingy to separate PR (because people have their tastes, and we might have to think about compatibility if the default template contained some emojis.) Regarding template types and methods, we now have comparison operators |
These changes are continuing the work of #3141.
I took over its changes and rebased them on latest
main.This work is required for #4747, as it lays the foundation for writing CLI tests for
jj sign.Checklist
If applicable:
CHANGELOG.md