Implement `jj sign` command #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

jalil-salame · 2024-12-16T15:33:15Z

cli/src/commands/sign.rs

+    key: Option<String>,
+    /// What revision to sign
+    #[arg(long, short, default_value = "@")]
+    revision: RevisionArg,


Wouldn't it make sense to have this be revisions: Vec<RevisionArg>?

This way you can do jj sign -r main..bookmark to sign a whole branch.

That can still be supported with a single argument. The whole main..bookmark string gets passed as a single argument from your shell to the jj binary. The binary then decides how to interpret it, and the jj sign command will presumably want to accept multiple revisions, as most commands do. Commands that want a single revision still accept revsets; they just validate that the revset contains exactly one revision.

It may still be a good idea to allow multiple revision arguments here, so the user can do jj sign -r X -r Y -r Z to sign those three revsets.

Thanks for the clarification c: and yes jj sign -r X -r Y -r Z sounds logical

A reminder about an identical discussion - maybe the issue with rebasing transformations got better since then?

If implemented naively, signing main..bookmark is quadratic - signing an ancestor rebases all of it's descendants, and then we sign the child of that ancesrot rebasing all of its descendants and so on

maybe the issue with rebasing transformations got better since then?

Yes, we have .transform_descendants(roots, |rewriter| ..) API now.

lib/src/commit_builder.rs

Co-authored-by: julienvincent <[email protected]>

Cannot make it accept multiple revisions easily, that'd have to be done with a rebaser

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

necauqua · 2025-01-06T18:14:40Z

cli/src/commands/sign.rs

+            if !args.drop && commit.is_signed() {
+                return Err(user_error(
+                    "Commit is already signed, use --force to sign anyway",
+                ));
+            }
+            if commit.author().email != command.settings().user_email() {
+                return Err(user_error(
+                    "Commit is not authored by you, use --force to sign anyway",
+                ));
+            }


Now that this version can work with multiple commits, those checks should show the offending one.
Or ones, actually, so they should eagerly collect lists of bad commits and print them all at once.

nit: Also if !args.force can be moved outside of the loop

necauqua · 2025-01-06T18:15:32Z

cli/src/commands/sign.rs

+    };
+    let mut signed_commits = vec![];
+    tx.repo_mut().transform_descendants(
+        commits.iter().ids().cloned().collect_vec(),


Use commit_ids here?

Hmm, if check_rewritable consumes the vector that sounds meh, it should accept &[CommitId].
If it really does need to consume the vector for some reason ignore this comment then 🤷
(Or maybe it accepts any iterable and you just need to borrow there, I haven't looked at the code in a year already)

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(..) = ..?

martinvonz · 2025-01-06T18:51:34Z

cli/src/commands/sign.rs

@@ -0,0 +1,117 @@
+// Copyright 2023 The Jujutsu Authors


nit: maybe 2024 or 2025? or maybe this work was started in 2023 so it should be 2023?

martinvonz · 2025-01-06T18:53:53Z

cli/src/commands/sign.rs

+    let mut workspace_command = command.workspace_helper(ui)?;
+
+    let commits = workspace_command
+        .resolve_some_revsets_default_single(ui, &args.revisions)?


I think we want parse_union_revsets() here. I think it's pretty expected to want to sign multiple commits at once.

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

martinvonz · 2025-01-06T18:57:53Z

cli/src/commands/sign.rs

+        if args.drop {
+            write!(formatter, "Signature was dropped: ")?;
+        } else {
+            write!(formatter, "Commit was signed: ")?;
+        }


I would expect these messages to depend on the commit's previous signature, especially "Signature was dropped" is confusing for commits that were not signed to start with.

This made sense with a singular commit, since pre-check would short-circuit the situation where args.drop=true and the commit was not signed

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.

yuja · 2025-01-07T03:14:18Z

cli/src/commands/sign.rs

+    /// What revision(s) to sign
+    #[arg(
+        long, short,
+        default_value = "@",


nit: it might be better to make the default configurable. I think jj sign will be used in the context where we do jj fix && jj sign .. && jj git push.

(can be addressed separately)

yuja · 2025-01-07T03:16:23Z

cli/src/commands/sign.rs

+    /// Sign a commit that is not authored by you or was already signed.
+    #[arg(long, short)]
+    force: bool,


nit: --force tends to mean arbitrary things. Maybe --allow-not-mine or something is better?

yuja · 2025-01-07T03:19:09Z

cli/src/commands/sign.rs

+    /// Drop the signature, explicitly "un-signing" the commit.
+    #[arg(long, short = 'D', conflicts_with = "force")]
+    drop: bool,


nit: just dropping idea: jj unsign or jj signature drop.
(It's unlikely we'll add more sub commands, so namespaced command might be overkill.)

I like jj unsign.
Splitting off the drop from jj sign would make cmd_sign() a little simpler, as well.

yuja · 2025-01-07T03:20:29Z

cli/src/commands/sign.rs

+                    .write()?;
+                signed_commits.push(new_commit);
+            }
+            Ok(())


maybe you'll need to do rewriter.reparent(). cmd_describe() does rewriter.rebase(), but I think .reparent() works there, too.

yuja · 2025-01-07T03:21:08Z

cli/src/commands/sign.rs

+        long, short,
+        default_value = "@",
+        value_name = "REVSETS",
+        add = ArgValueCandidates::new(complete::all_revisions),


nit: complete::mutable_revisions?

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

Thanks @yuja!

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

martinvonz mentioned this pull request Dec 17, 2024

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

Open

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 7 times, most recently from 415d744 to 9184331 Compare January 5, 2025 06:11

sign: add tests for the jj sign command

c8bc16b

Co-authored-by: julienvincent <[email protected]>

pylbrecht force-pushed the pylbrecht/push-tokrxypvsmqw branch from 9184331 to 6b049ab Compare January 5, 2025 06:15

sign: Implement the jj sign command

2c49a16

Cannot make it accept multiple revisions easily, that'd have to be done with a rebaser

pylbrecht force-pushed the pylbrecht/push-tokrxypvsmqw branch 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `jj sign` command #4747

Implement `jj sign` command #4747

pylbrecht commented Nov 1, 2024

jalil-salame Dec 16, 2024

martinvonz Dec 16, 2024

jalil-salame Dec 16, 2024

necauqua Jan 2, 2025

yuja Jan 3, 2025

pylbrecht commented Jan 6, 2025 •

edited

Loading

necauqua left a comment •

edited

Loading

necauqua Jan 6, 2025

necauqua Jan 6, 2025 •

edited

Loading

necauqua Jan 6, 2025

necauqua Jan 6, 2025

martinvonz Jan 6, 2025

martinvonz Jan 6, 2025

martinvonz Jan 6, 2025

necauqua Jan 6, 2025 •

edited

Loading

martinvonz Jan 6, 2025

necauqua Jan 6, 2025

pylbrecht commented Jan 6, 2025 •

edited

Loading

necauqua commented Jan 6, 2025

yuja Jan 7, 2025

yuja Jan 7, 2025

yuja Jan 7, 2025

pylbrecht Jan 7, 2025

yuja Jan 7, 2025

yuja Jan 7, 2025

pylbrecht commented Jan 7, 2025

Implement jj sign command #4747

Are you sure you want to change the base?

Implement jj sign command #4747

Conversation

pylbrecht commented Nov 1, 2024

Checklist

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

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

necauqua Jan 6, 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

necauqua Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pylbrecht commented Jan 6, 2025 • edited Loading

necauqua commented Jan 6, 2025

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

pylbrecht commented Jan 7, 2025

Implement `jj sign` command #4747

Implement `jj sign` command #4747

pylbrecht commented Jan 6, 2025 •

edited

Loading

necauqua left a comment •

edited

Loading

necauqua Jan 6, 2025 •

edited

Loading

necauqua Jan 6, 2025 •

edited

Loading

pylbrecht commented Jan 6, 2025 •

edited

Loading