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-14T08:41:38Z

Things have been a bit hectic at work recently, but I expect to make some progress on this in the next couple of days.

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.

yuja · 2025-01-22T13:24:08Z

cli/src/commands/sign.rs

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


nit: omit the default value for now? I don't think the current default is useful. (the same for unsign)

yuja · 2025-01-22T13:24:50Z

cli/src/commands/sign.rs

+        add = ArgValueCandidates::new(complete::mutable_revisions),
+    )]
+    revisions: Vec<RevisionArg>,
+    /// Sign a commit that is not authored by you.


nit: "Sign revisions that are .."?

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!

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

yuja · 2025-01-22T13:33:07Z

cli/src/commands/sign.rs

+            1 => {
+                write!(formatter, "Signed commit ")?;
+                tx.base_workspace_helper()
+                    .write_commit_summary(formatter.as_mut(), &signed_commits[0])?;


use tx.write_commit_summary() and tx.commit_summary_template() because signed_commits[N] is rewritten commits. (also applies to unsign)

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

cli/tests/[email protected]

@@ -1,6 +1,7 @@
 ---
 source: cli/tests/test_generate_md_cli_help.rs
 description: "AUTO-GENERATED FILE, DO NOT EDIT. This cli reference is generated by a test as an `insta` snapshot. MkDocs includes this snapshot from docs/cli-reference.md."
+snapshot_kind: text


nit: you'll need to upgrade cargo-insta

yuja · 2025-01-22T13:36:03Z

cli/src/commands/sign.rs

+        match signed_commits.len() {
+            0 => (),
+            1 => {


nit: you can also use slice pattern

match &*signed_commits { [] => {} [commit] => ... commits => ... }

Oh, neat! Thanks.

// edit
I have to say, your suggestion blew my mind a little. That was definitely another moment of "Rust is awesome!".

I did not apply that suggestion to this part, yet. I got some compiler errors around .into_iter().ids() and since this part might change (#4747 (comment)), I decided to leave that until tomorrow when my mind is fresh.

yuja · 2025-01-22T13:37:21Z

cli/src/commands/unsign.rs

+    /// Unsign a commit that is not authored by you.
+    #[arg(long, short)]
+    allow_not_mine: bool,


@necauqua Do we need --allow-not-mine for unsign?

Actually, it's the same thing with jj undo - I now think that we don't need this flag at all in both subcommands.

If jj prints a warning that you messed with someone's signature, you can just undo that.

Or, if we keep it, it's better to have it in both ones for consistency

Actually, it's the same thing with jj undo - I now think that we don't need this flag at all in both subcommands.

If jj prints a warning that you messed with someone's signature, you can just undo that.

Couldn't agree more on removing the flag entirely. That also leaves us with a simpler interface.

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

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

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

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

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

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

pylbrecht changed the title ~~Implement jj sign command~~ Implement jj sign and jj unsign commands Jan 11, 2025

pylbrecht force-pushed the pylbrecht/push-tokrxypvsmqw branch 10 times, most recently from 1866a0b to 3edbfa3 Compare January 20, 2025 19:58

pylbrecht force-pushed the pylbrecht/push-tokrxypvsmqw branch 6 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 from bb917db to 6842c60 Compare January 22, 2025 20:40

necauqua and others added 2 commits January 22, 2025 21:49

pylbrecht force-pushed the pylbrecht/push-tokrxypvsmqw branch from 6842c60 to 5ce99e0 Compare January 22, 2025 20:49

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 commented Jan 14, 2025

pylbrecht Jan 22, 2025 •

edited

Loading

pylbrecht Jan 22, 2025

yuja Jan 22, 2025

yuja Jan 22, 2025

yuja Jan 22, 2025

necauqua Jan 23, 2025

pylbrecht Jan 23, 2025

yuja Jan 22, 2025 •

edited

Loading

yuja Jan 22, 2025

yuja Jan 22, 2025

yuja Jan 22, 2025

pylbrecht Jan 22, 2025 •

edited

Loading

pylbrecht Jan 22, 2025 •

edited

Loading

yuja Jan 22, 2025

necauqua Jan 23, 2025

pylbrecht Jan 23, 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 commented Jan 14, 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

pylbrecht Jan 22, 2025 • edited Loading

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

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

pylbrecht Jan 22, 2025 •

edited

Loading

pylbrecht Jan 22, 2025 •

edited

Loading