Implement `jj sign` and `jj unsign` commands #4747

pylbrecht · 2024-11-01T09:57:04Z

Checklist

If applicable:

I have updated CHANGELOG.md
I have updated the documentation (README.md, docs/, demos/)
~~I have updated the config schema (cli/src/config-schema.json)~~
I have added tests to cover my changes

cli/src/commands/sign.rs

lib/src/commit_builder.rs

pylbrecht · 2025-01-06T13:48:22Z

This is still WIP, but I would love to get some feedback on the general approach and if this has any design flaws that I need to address before polishing.
@yuja, @necauqua, since you have been involved in my last PR, maybe you have some suggestions?
(Of course anyone else is also welcome to leave feedback!)

// edit

Some things, which have been discussed in jj sign related issues/PRs, are still missing: showing hints if signatures have been dropped during a rebase, or builtin template(s) for displaying signatures.
I don't know if we want to make them part of this PR or address them separately.

necauqua

Actually left a couple of comments.

Dropped signatures hint and builtin templates can be separate PRs, yes.

Will reiterate my idea for the templates here:

There should be a some_consistent_name_for_crypt_sig template that's used in the default log template and by default it's empty.
Then there should be some_other_name template that's unused, but is present by default, with my pretty checkmarks or whatever.
And then in FAQ or in docs, there should be a tip "you can run this jj config command to set that first template to equal that second one so that you'll get checkmarks in all the default templates".

"Or you can set that template to if(signature, "[•]", "") to just differentiate signed commits from unsigned in the logs, which is very fast compared to actually verifying them"

Another thing that could be in some tips and tricks section, is an alias for jj log that has --config= set that template thing to show signatures temporarily.

Also for the builtin_log_detailed there was a change somewhere that showed a Signature: extra line - maybe do the same thing with empty default one + unused verifying one?. Or have the detailed log always verify. Maybe Yuja has an opinion there

cli/src/commands/sign.rs

necauqua · 2025-01-06T18:21:35Z

lib/src/commit_builder.rs

    pub fn set_sign_key(&mut self, sign_key: Option<String>) -> &mut Self {
-        self.sign_settings.key = sign_key;
+        if sign_key.is_some() {
+            self.sign_settings.key = sign_key;
+        }
        self
    }


nit: looking at the overall method now (my previous nit was a mechanical refactor to avoid an unnecessary clone) - set_sign_key does nothing if given option is None, which is a bit.. meh, idk - could change it to accept sign_key: String directly and make the callers do the if let Some(..) = ..?

Since sign_settings.key is Option<String>, wouldn't we eliminate the case, where the caller wants to explicitly set sign_settings.key = None?
Or is this case nonsense and sign_settings.key should never be set to None after initialization (SignSettings::from_settings())?

@necauqua, I reverted the change as I honestly do not recall why I made it in the first place. 🤔
Let me know if you have any objections.

Well, I never loaded the entire context of this specific PR into my head, so I don't have any objections whatsoever
If it works, it works, and if it also happens to make sense, the better)

cli/src/commands/sign.rs

martinvonz · 2025-01-06T18:55:07Z

cli/tests/test_sign_command.rs

Make most of these tests sign multiple commits to make sure that that works as expected

random suggestion, ignore if it doesn't make sense:
Snapshot the evolog to ensure no quadratic rebases that I feared?.
This is probably handled by tests of transform_descendants already.

cli/src/commands/sign.rs

pylbrecht · 2025-01-06T20:20:04Z

Wow, thanks a lot for your feedback, @necauqua and @martinvonz!
This helps a lot. Being new to the codebase (and the VCS domain), I struggle with what is expected a fair bit. It's great to receive some guidance here.

Now I am all set to return to my dungeon and work on the how.

// edit
Also, still wrapping my head around Rust, so.. please bear with me. 😅

necauqua · 2025-01-06T21:17:34Z

Very offtopic, sorry, but there's no chance in hell I'd have contributed to jj if is was cpp or something.
Even if it was C (that I don't dislike per se) every single project has it's own very specific patterns etc. and the build systems are plentiful.
Here in Rust we have cargo that just works, we have rustfmt, and we have the borrowchecker to not be scared of your code, and to not learn again how the particular project handles memory.

cli/src/commands/sign.rs

pylbrecht · 2025-01-07T05:41:28Z

Thanks @yuja!

pylbrecht · 2025-01-22T07:36:29Z

docs/config.md

+### Manually signing commits
+
+You can use [`jj sign`](./cli-reference.md#jj-sign)/[`jj
+unsign`](./cli-reference.md#jj-unsign)to sign/unsign commits manually.


@yuja, we can extend this section with an example for the workflow you mentioned in #4747 (comment) when implementing #5428.

pylbrecht · 2025-01-22T07:45:30Z

CHANGELOG.md

+* The new `jj sign` command allows signing commits.
+
+* The new `jj unsign` command allows unsigning commits.
+


These do feel a bit scarce, but maybe that's ok? I am open to suggestions of things to add here.

cli/src/commands/sign.rs

yuja · 2025-01-22T13:27:41Z

cli/src/commands/sign.rs

+    let bad_commits = commits
+        .into_iter()
+        .filter(|commit| commit.author().email != user_email)


nit: Can't we use SignSettings::should_sign()?

BTW, I'm not sure if this should be an error. If a warning is good enough, warnings can be emitted within/after transform_descendants().

@necauqua any thoughts?

My thoughts about jj philosophy are that you can always jj undo any rewrite - at least if jj gabe you a warning/notice that something unusual has happened.

So, in that case, error => warning is good imo

//offtop
I guess the same applies to removing all: btw, even though I like it and didn't like the idea of getting rid of it - if jj brings your attention to the fact that rebase happened to affect multiple revs so that you can undo it if it was not what you expected - the all: stumble it not needed

I really like the idea of embracing jj undo. Warnings it is then!

I need some guidance on how to place (and format) these warnings.
I found ui.warning_default(), but it doesn't format commits as nicely as ui.status_formatter().

I started with something like this:

diff --git a/cli/src/commands/sign.rs b/cli/src/commands/sign.rs index 19853838d7..5fa3991425 100644 --- a/cli/src/commands/sign.rs +++ b/cli/src/commands/sign.rs @@ -19,6 +19,7 @@ use jj_lib::commit::CommitIteratorExt; use jj_lib::signing::SignBehavior; +use crate::cli_util::short_commit_hash; use crate::cli_util::CommandHelper; use crate::cli_util::RevisionArg; use crate::command_error::CommandError; @@ -58,6 +59,15 @@ |rewriter| { let authored_by_me = rewriter.old_commit().author().email == command.settings().user_email(); + + if !authored_by_me { + writeln!( + ui.warning_default(), + "signing commit {}, which was not authored by you", + short_commit_hash(rewriter.old_commit().id()) + )?; + } + if rewriter.old_commit().is_signed() && authored_by_me { // Don't resign commits, which are already signed by me. For people using // hardware devices for signatures, resigning commits is

Besides being unsure what id to display (commit hash or change hash?), I couldn't write the warning within transform_descendants():

error[E0277]: `?` couldn't convert the error to `jj_lib::backend::BackendError` --> cli/src/commands/sign.rs:68:18 | 68 | )?; | ^ the trait `std::convert::From<std::io::Error>` is not implemented for `jj_lib::backend::BackendError` | = note: the question mark operation (`?`) implicitly performs a conversion on the error value using the `From` trait = help: the trait `From<std::io::Error>` is not implemented for `jj_lib::backend::BackendError` but trait `From<jj_lib::git_backend::GitBackendError>` is implemented for it = help: for that trait implementation, expected `jj_lib::git_backend::GitBackendError`, found `std::io::Error` = note: required for `std::result::Result<(), jj_lib::backend::BackendError>` to implement `std::ops::FromResidual<std::result::Result<std::convert::Infallible, std::io::Error>>`

All right, then let's do it outside of transform_descendants() and deal with mapping errors later when polishing a working solution:

diff --git a/cli/src/commands/sign.rs b/cli/src/commands/sign.rs index 19853838d7..28d561f0d1 100644 --- a/cli/src/commands/sign.rs +++ b/cli/src/commands/sign.rs @@ -19,6 +19,7 @@ use jj_lib::commit::CommitIteratorExt; use jj_lib::signing::SignBehavior; +use crate::cli_util::short_commit_hash; use crate::cli_util::CommandHelper; use crate::cli_util::RevisionArg; use crate::command_error::CommandError; @@ -53,11 +54,17 @@ let mut tx = workspace_command.start_transaction(); let mut signed_commits = vec![]; + let mut foreign_commits = vec![]; tx.repo_mut().transform_descendants( commits.iter().ids().cloned().collect_vec(), |rewriter| { let authored_by_me = rewriter.old_commit().author().email == command.settings().user_email(); + + if !authored_by_me { + foreign_commits.push(rewriter.old_commit().clone()); + } + if rewriter.old_commit().is_signed() && authored_by_me { // Don't resign commits, which are already signed by me. For people using // hardware devices for signatures, resigning commits is @@ -77,6 +84,24 @@ }, )?; + match &*foreign_commits { + [] => {} + [commit] => { + writeln!( + ui.warning_default(), + "signed commit {}, which was not authored by you", + short_commit_hash(commit.id()) + )?; + } + commits => { + writeln!( + ui.warning_default(), + "signed the following commits, which were not authored by you:\n {}", + commits.iter().ids().map(short_commit_hash).join("\n ") + )?; + } + } + if let Some(mut formatter) = ui.status_formatter() { match &*signed_commits { [] => {}

(keep in mind I am still in the stage of getting something working; clone() being a sign of that)

Great, it compiles! Let's get a basic test going, so we can see the actual output in a snapshot diff:

diff --git a/cli/tests/test_sign_command.rs b/cli/tests/test_sign_command.rs index 5086b3ae03..f5dec988c7 100644 --- a/cli/tests/test_sign_command.rs +++ b/cli/tests/test_sign_command.rs @@ -72,7 +72,33 @@ } #[test] -#[should_panic] fn test_warn_about_signing_commits_not_authored_by_me() { - todo!() + let test_env = TestEnvironment::default(); + + test_env.add_config( + r#" +[signing] +sign-all = false +backend = "test" +"#, + ); + + test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]); + let repo_path = test_env.env_root().join("repo"); + test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "one"]); + test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "two"]); + test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "three"]); + + test_env.jj_cmd_ok( + &repo_path, + &[ + "desc", + "--author", + "Someone Else <[email protected]>", + "--no-edit", + "..@-", + ], + ); + let (_, stderr) = test_env.jj_cmd_ok(&repo_path, &["sign", "-r", "..@-"]); + insta::assert_snapshot!(stderr, @r""); }

-old snapshot +new results ────────────┬───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── 0 │+Warning: signed the following commits, which were not authored by you: 1 │+ 4bcea9a68424 2 │+ e14d78359449 3 │+ ef6b36d10836 4 │+Signed the following commits: 5 │+ qpvuntsm hidden 82f99921 (empty) one 6 │+ rlvkpnrz hidden 715131ae (empty) two 7 │+ kkmpptxz hidden 60618621 (empty) three 8 │+Rebased 1 descendant commits 9 │+Working copy now at: zsuskuln 5a1d05b3 (empty) (no description set) 10 │+Parent commit : kkmpptxz 60618621 (empty) three ────────────┴─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────

While it does the job, I feel it would be nice to format the warning like the output of signed commits.

But I am not sure if (ab)using ui.status_formatter() is the way to go or if I am overthinking this. Having spent enough time in that rabbit hole already, I felt it's time to reach out.

Any suggestions or some prevalent examples in the code you can point me to?

I think warnings can be printed later (or earlier) with the short summary line as warning, and the list of commit summaries as status output.

Warning: Signed N commits ... not authored by you <commit summary #0> <commit summary #1> ...

yuja · 2025-01-22T13:31:20Z

cli/src/commands/sign.rs

+            }
+
+            if commits.contains(rewriter.old_commit()) {
+                let commit_builder = rewriter.reparent();


We'll probably need to do always .reparent(). See cmd_describe() for example.

.reparent() can be skipped when !rewriter.parents_changed() && no_need_to_sign_the_current_commit. cmd_describe() excludes uninteresting commits prior to transform_descendants(). I'm not sure which is better here, but prefiltering might simplify the condition to do .reparent().

While we can reduce the roots passed to transform_descendants(), I think we still need to apply the same checks (e.g. no_need_to_sign_the_current_commit) in transform_descendants()'s callback.

Yes, we'll need to check if the commit should be signed or not. What we can remove is the check for reparent or not.

Mhh, I am not sure I fully grokked what you mean by "the check for reparent or not".
Why would we always need to reparent?

Maybe you could have a sneak peek into the recently pushed changes I made in transform_descendants()'s callback. I do not always reparent, but the test snapshots look reasonable to me (at least with my somewhat superficial knowledge).

For context, if we always reparent, we end up with this instead:

diff --git a/cli/tests/test_sign_command.rs b/cli/tests/test_sign_command.rs index 18e3862265..16322afcef 100644 --- a/cli/tests/test_sign_command.rs +++ b/cli/tests/test_sign_command.rs @@ -52,7 +52,6 @@ qpvuntsm hidden 8174ec98 (empty) one rlvkpnrz hidden 6500b275 (empty) two kkmpptxz hidden bcfaa4c3 (empty) three - Rebased 1 descendant commits Working copy now at: zsuskuln eeb8c985 (empty) (no description set) Parent commit : kkmpptxz bcfaa4c3 (empty) three "); @@ -109,7 +108,6 @@ qpvuntsm hidden 82f99921 (empty) one rlvkpnrz hidden 715131ae (empty) two kkmpptxz hidden 60618621 (empty) three - Rebased 1 descendant commits Working copy now at: zsuskuln 5a1d05b3 (empty) (no description set) Parent commit : kkmpptxz 60618621 (empty) three ");

Maybe that's what we actually want? I am leaning onto your judgement here.

See cmd_describe() for why. In short, it will save extra cost of redundant rewrites. I think it's more important here, because signature will be invalidated if the signed commit is rebased thereafter.

Thanks, that helps!

I will play with incorporating into a test (even if only locally and for the sake of my own understanding), that we do not invalidate signatures if the signed commit is rebased thereafter.

cli/src/commands/sign.rs

cli/tests/[email protected]

cli/src/commands/sign.rs

cli/src/commands/unsign.rs

cli/tests/test_sign_command.rs

Commits to be signed, which are not authored by the user, require `--allow-not-mine`. We do not resign commits, which are already signed. For people using hardware devices for signatures, resigning commits is cumbersome (e.g. having to touch your YubiKey for each signature). The output of `jj sign` is based on that of `jj abandon`. --- Co-authored-by: julienvincent <[email protected]> Co-authored-by: necauqua <[email protected]>

Commits to be unsigned, which are signed and not authored by the user, require `--allow-not-mine`. The output of `jj unsign` is based on that of `jj abandon`. --- Co-authored-by: julienvincent <[email protected]> Co-authored-by: necauqua <[email protected]>

pylbrecht force-pushed the pylbrecht/push-tokrxypvsmqw branch 3 times, most recently from 0753a24 to 46731eb Compare November 8, 2024 06:00

This was referenced Nov 11, 2024

FR: Add a command for signing commits (when signing.sign-all = false) #4712

Open

Add templater support for rendering commit signatures #4853

Merged

jalil-salame reviewed Dec 16, 2024

View reviewed changes

cli/src/commands/sign.rs Outdated Show resolved Hide resolved

martinvonz mentioned this pull request Dec 17, 2024

FR: don't sign commits without a description #5108

Closed

julienvincent mentioned this pull request Jan 2, 2025

Add jj sign command #3142

Closed

7 tasks

pylbrecht force-pushed the pylbrecht/push-tokrxypvsmqw branch 5 times, most recently from 517e230 to 45be04d Compare January 4, 2025 13:14

necauqua reviewed Jan 4, 2025

View reviewed changes

lib/src/commit_builder.rs Outdated Show resolved Hide resolved

pylbrecht force-pushed the pylbrecht/push-tokrxypvsmqw branch 9 times, most recently from 6b049ab to 2c49a16 Compare January 5, 2025 11:31

necauqua reviewed Jan 6, 2025

View reviewed changes

martinvonz reviewed Jan 6, 2025

View reviewed changes

yuja reviewed Jan 7, 2025

View reviewed changes

pylbrecht force-pushed the pylbrecht/push-tokrxypvsmqw branch 7 times, most recently from b534a86 to 29a8933 Compare January 22, 2025 07:33

pylbrecht commented Jan 22, 2025

View reviewed changes

pylbrecht force-pushed the pylbrecht/push-tokrxypvsmqw branch from 29a8933 to 57cf2eb Compare January 22, 2025 07:37

pylbrecht commented Jan 22, 2025

View reviewed changes

pylbrecht mentioned this pull request Jan 22, 2025

FR: make default revisions of jj sign configurable #5428

Open

pylbrecht force-pushed the pylbrecht/push-tokrxypvsmqw branch from 57cf2eb to bb917db Compare January 22, 2025 08:01

pylbrecht marked this pull request as ready for review January 22, 2025 08:18

yuja reviewed Jan 22, 2025

View reviewed changes

cli/tests/test_sign_command.rs Outdated Show resolved Hide resolved

pylbrecht force-pushed the pylbrecht/push-tokrxypvsmqw branch 7 times, most recently from 1804079 to acf89a3 Compare January 29, 2025 20:28

pylbrecht force-pushed the pylbrecht/push-tokrxypvsmqw branch 5 times, most recently from eb1737a to d4a94b0 Compare February 2, 2025 08:12

necauqua and others added 2 commits February 2, 2025 09:16

pylbrecht force-pushed the pylbrecht/push-tokrxypvsmqw branch from d4a94b0 to 113b5e8 Compare February 2, 2025 08:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `jj sign` and `jj unsign` commands #4747

Implement `jj sign` and `jj unsign` commands #4747

pylbrecht commented Nov 1, 2024 •

edited

Loading

pylbrecht commented Jan 6, 2025 •

edited

Loading

necauqua left a comment •

edited

Loading

necauqua Jan 6, 2025

pylbrecht Jan 20, 2025

pylbrecht Jan 22, 2025 •

edited

Loading

necauqua Jan 23, 2025

martinvonz Jan 6, 2025

necauqua Jan 6, 2025 •

edited

Loading

pylbrecht commented Jan 6, 2025 •

edited

Loading

necauqua commented Jan 6, 2025

pylbrecht commented Jan 7, 2025

pylbrecht Jan 22, 2025 •

edited

Loading

pylbrecht Jan 22, 2025

yuja Jan 22, 2025

necauqua Jan 23, 2025

pylbrecht Jan 23, 2025

pylbrecht Jan 27, 2025

yuja Jan 28, 2025

yuja Jan 22, 2025 •

edited

Loading

pylbrecht Feb 2, 2025

yuja Feb 2, 2025

pylbrecht Feb 2, 2025

yuja Feb 2, 2025

pylbrecht Feb 2, 2025

		* The new `jj sign` command allows signing commits.

		* The new `jj unsign` command allows unsigning commits.

Implement jj sign and jj unsign commands #4747

Are you sure you want to change the base?

Implement jj sign and jj unsign commands #4747

Conversation

pylbrecht commented Nov 1, 2024 • edited Loading

Checklist

pylbrecht commented Jan 6, 2025 • edited Loading

necauqua left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pylbrecht Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

necauqua Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

pylbrecht commented Jan 6, 2025 • edited Loading

necauqua commented Jan 6, 2025

pylbrecht commented Jan 7, 2025

pylbrecht Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuja Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Implement `jj sign` and `jj unsign` commands #4747

Implement `jj sign` and `jj unsign` commands #4747

pylbrecht commented Nov 1, 2024 •

edited

Loading

pylbrecht commented Jan 6, 2025 •

edited

Loading

necauqua left a comment •

edited

Loading

pylbrecht Jan 22, 2025 •

edited

Loading

necauqua Jan 6, 2025 •

edited

Loading

pylbrecht commented Jan 6, 2025 •

edited

Loading

pylbrecht Jan 22, 2025 •

edited

Loading

yuja Jan 22, 2025 •

edited

Loading