diff --git a/CHANGELOG.md b/CHANGELOG.md index b40e31ba7..03321e7e7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -69,6 +69,10 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). * `jj config edit` will now roll back to previous version if a syntax error has been introduced in the new config. +* The `git.sign-on-push` config option has been changed from a boolean to a + string containing the revset of commits to be signed when `jj git push` is + run. The default value is `none()`. If `git.sign-on-push` was previously set + to `true`, it will be set to `mine()`. ### Fixed bugs diff --git a/cli/src/commands/git/push.rs b/cli/src/commands/git/push.rs index 56376b08a..95b76a367 100644 --- a/cli/src/commands/git/push.rs +++ b/cli/src/commands/git/push.rs @@ -400,34 +400,20 @@ pub fn cmd_git_push( return Ok(()); } - let sign_behavior = if tx.settings().get_bool("git.sign-on-push")? { - Some(SignBehavior::Own) - } else { - None - }; let commits_to_sign = - validate_commits_ready_to_push(ui, &bookmark_updates, remote, &tx, args, sign_behavior)?; + validate_commits_ready_to_push(ui, &bookmark_updates, remote, &tx, args)?; if !args.dry_run && !commits_to_sign.is_empty() { - if let Some(sign_behavior) = sign_behavior { - let num_updated_signatures = commits_to_sign.len(); - let num_rebased_descendants; - (num_rebased_descendants, bookmark_updates) = sign_commits_before_push( - &mut tx, - commits_to_sign, - sign_behavior, - bookmark_updates, - )?; - if let Some(mut formatter) = ui.status_formatter() { + let num_commits_signed = commits_to_sign.len(); + let num_rebased_descendants; + (num_rebased_descendants, bookmark_updates) = + sign_commits_before_push(&mut tx, commits_to_sign, bookmark_updates)?; + if let Some(mut formatter) = ui.status_formatter() { + writeln!(formatter, "Signed {num_commits_signed} commits")?; + if num_rebased_descendants > 0 { writeln!( formatter, - "Updated signatures of {num_updated_signatures} commits" + "Rebased {num_rebased_descendants} descendant commits" )?; - if num_rebased_descendants > 0 { - writeln!( - formatter, - "Rebased {num_rebased_descendants} descendant commits" - )?; - } } } } @@ -513,7 +499,6 @@ fn validate_commits_ready_to_push( remote: &RemoteName, tx: &WorkspaceCommandTransaction, args: &GitPushArgs, - sign_behavior: Option, ) -> Result, CommandError> { let workspace_helper = tx.base_workspace_helper(); let repo = workspace_helper.repo(); @@ -538,11 +523,11 @@ fn validate_commits_ready_to_push( .parse_revset(ui, &private_revset_str)? .evaluate()? .containing_fn(); - let sign_settings = sign_behavior.map(|sign_behavior| { - let mut sign_settings = settings.sign_settings(); - sign_settings.behavior = sign_behavior; - sign_settings - }); + let sign_on_push_rev = RevisionArg::from(settings.get_string("git.sign-on-push")?); + let should_sign_on_push = workspace_helper + .parse_revset(ui, &sign_on_push_rev)? + .evaluate()? + .containing_fn(); let mut commits_to_sign = vec![]; @@ -591,10 +576,8 @@ fn validate_commits_ready_to_push( } return Err(error); } - if let Some(sign_settings) = &sign_settings { - if !commit.is_signed() && sign_settings.should_sign(commit.store_commit()) { - commits_to_sign.push(commit); - } + if !commit.is_signed() && should_sign_on_push(commit.id())? { + commits_to_sign.push(commit); } } Ok(commits_to_sign) @@ -607,7 +590,6 @@ fn validate_commits_ready_to_push( fn sign_commits_before_push( tx: &mut WorkspaceCommandTransaction, commits_to_sign: Vec, - sign_behavior: SignBehavior, bookmark_updates: Vec<(RefNameBuf, BookmarkPushUpdate)>, ) -> Result<(usize, Vec<(RefNameBuf, BookmarkPushUpdate)>), CommandError> { let commit_ids: IndexSet = commits_to_sign.iter().ids().cloned().collect(); @@ -619,7 +601,7 @@ fn sign_commits_before_push( if commit_ids.contains(&old_commit_id) { let commit = rewriter .reparent() - .set_sign_behavior(sign_behavior) + .set_sign_behavior(SignBehavior::Force) .write()?; old_to_new_commits_map.insert(old_commit_id, commit.id().clone()); } else { diff --git a/cli/src/config-schema.json b/cli/src/config-schema.json index d7811c4c0..039a655dd 100644 --- a/cli/src/config-schema.json +++ b/cli/src/config-schema.json @@ -470,9 +470,8 @@ "default": "origin" }, "sign-on-push": { - "type": "boolean", - "description": "Whether jj should sign commits before pushing", - "default": false + "type": "string", + "description": "Revset of commits to sign before pushing" }, "subprocess": { "type": "boolean", diff --git a/cli/src/config.rs b/cli/src/config.rs index 12c4e389b..06cd50606 100644 --- a/cli/src/config.rs +++ b/cli/src/config.rs @@ -653,6 +653,28 @@ pub fn default_config_migrations() -> Vec { Ok(format!(r#""{escaped}""#).into()) }, ), + // TODO: Delete in jj 0.34+ + ConfigMigrationRule::custom( + |layer| { + if let Ok(Some(value)) = layer.look_up_item("git.sign-on-push") { + value.is_bool() + } else { + false + } + }, + |layer| match layer.look_up_item("git.sign-on-push") { + Ok(Some(value)) => { + let old_value = value.as_bool().unwrap(); + let new_value = if old_value { "mine()" } else { "none()" }; + layer.set_value("git.sign-on-push", new_value.to_string())?; + Ok(format!( + "git.sign-on-push = {old_value} is updated to git.sign-on-push = \ + '{new_value}'", + )) + } + _ => unreachable!(), + }, + ), ] } diff --git a/cli/src/config/misc.toml b/cli/src/config/misc.toml index b5407d175..c685d8a4f 100644 --- a/cli/src/config/misc.toml +++ b/cli/src/config/misc.toml @@ -17,7 +17,7 @@ context = 3 private-commits = "none()" push-bookmark-prefix = "push-" push-new-bookmarks = false -sign-on-push = false +sign-on-push = "none()" [ui] always-allow-large-revsets = false diff --git a/cli/tests/test_git_push.rs b/cli/tests/test_git_push.rs index e1b1dc701..c3f9b835c 100644 --- a/cli/tests/test_git_push.rs +++ b/cli/tests/test_git_push.rs @@ -2380,13 +2380,31 @@ fn test_git_push_sign_on_push() { work_dir .run_jj(["new", "-m", "commit which should not be signed 1"]) .success(); + work_dir.run_jj(["new"]).success(); work_dir - .run_jj(["new", "-m", "commit which should not be signed 2"]) + .run_jj([ + "describe", + "-m", + "commit which should not be signed 2 with different author", + "--author=Foo ", + ]) .success(); + work_dir.run_jj(["new"]).success(); + work_dir + .run_jj([ + "describe", + "-m", + "commit with different author", + "--author=Foo ", + ]) + .success(); + work_dir.run_jj(["new", "-m", "final commit"]).success(); // There should be no signed commits initially let output = work_dir.run_jj(["log", "-T", template]); insta::assert_snapshot!(output, @r" - @ commit which should not be signed 2 + @ final commit + ○ commit with different author + ○ commit which should not be signed 2 with different author ○ commit which should not be signed 1 ○ commit to be signed 2 ○ commit to be signed 1 @@ -2400,7 +2418,7 @@ fn test_git_push_sign_on_push() { r#" signing.backend = "test" signing.key = "impeccable" - git.sign-on-push = true + git.sign-on-push = "mine()" "#, ); let output = work_dir.run_jj(["git", "push", "--dry-run"]); @@ -2414,7 +2432,9 @@ fn test_git_push_sign_on_push() { // There should be no signed commits after performing a dry run let output = work_dir.run_jj(["log", "-T", template]); insta::assert_snapshot!(output, @r" - @ commit which should not be signed 2 + @ final commit + ○ commit with different author + ○ commit which should not be signed 2 with different author ○ commit which should not be signed 1 ○ commit to be signed 2 ○ commit to be signed 1 @@ -2427,18 +2447,20 @@ fn test_git_push_sign_on_push() { let output = work_dir.run_jj(["git", "push"]); insta::assert_snapshot!(output, @r" ------- stderr ------- - Updated signatures of 2 commits - Rebased 2 descendant commits + Signed 2 commits + Rebased 4 descendant commits Changes to push to origin: - Move forward bookmark bookmark2 from 8476341eb395 to a6259c482040 - Working copy (@) now at: kmkuslsw b5f47345 (empty) commit which should not be signed 2 - Parent commit (@-) : kpqxywon 90df08d3 (empty) commit which should not be signed 1 + Move forward bookmark bookmark2 from 8476341eb395 to 2ac4cdcf94ef + Working copy (@) now at: nkmrtpmo a4e979ab (empty) final commit + Parent commit (@-) : lylxulpl 2b7318f0 (empty) commit with different author [EOF] "); // Only commits which are being pushed should be signed let output = work_dir.run_jj(["log", "-T", template]); insta::assert_snapshot!(output, @r" - @ commit which should not be signed 2 + @ final commit + ○ commit with different author + ○ commit which should not be signed 2 with different author ○ commit which should not be signed 1 ○ commit to be signed 2 │ Signature: test-display, Status: good, Key: impeccable @@ -2461,28 +2483,113 @@ fn test_git_push_sign_on_push() { ]); insta::assert_snapshot!(output, @r" ------- stderr ------- - Created 1 bookmarks pointing to kpqxywon 90df08d3 bookmark3 | (empty) commit which should not be signed 1 + Created 1 bookmarks pointing to kpqxywon af823162 bookmark3 | (empty) commit which should not be signed 1 [EOF] "); let output = work_dir.run_jj(["bookmark", "move", "bookmark2", "--to", "bookmark3"]); insta::assert_snapshot!(output, @r" ------- stderr ------- - Moved 1 bookmarks to kpqxywon 90df08d3 bookmark2* bookmark3 | (empty) commit which should not be signed 1 + Moved 1 bookmarks to kpqxywon af823162 bookmark2* bookmark3 | (empty) commit which should not be signed 1 [EOF] "); test_env.add_config(r#"revset-aliases."immutable_heads()" = "bookmark3""#); - let output = work_dir.run_jj(["git", "push"]); + let output = work_dir.run_jj(["git", "push", "-b", "bookmark2"]); insta::assert_snapshot!(output, @r" ------- stderr ------- - Warning: Refusing to create new remote bookmark bookmark3@origin - Hint: Use --allow-new to push new bookmark. Use --remote to specify the remote to push to. Changes to push to origin: - Move forward bookmark bookmark2 from a6259c482040 to 90df08d3d612 + Move forward bookmark bookmark2 from 2ac4cdcf94ef to af8231621fcb [EOF] "); let output = work_dir.run_jj(["log", "-T", template, "-r", "::"]); insta::assert_snapshot!(output, @r" - @ commit which should not be signed 2 + @ final commit + ○ commit with different author + ○ commit which should not be signed 2 with different author + ◆ commit which should not be signed 1 + ◆ commit to be signed 2 + │ Signature: test-display, Status: good, Key: impeccable + ◆ commit to be signed 1 + │ Signature: test-display, Status: good, Key: impeccable + ◆ description 2 + │ ○ description 1 + ├─╯ + ◆ + [EOF] + "); + + // With `git.sign-on-push = "mine()"`, commit with different author should not + // be signed + let output = work_dir.run_jj([ + "bookmark", + "move", + "bookmark2", + "--to", + "description('commit which should not be signed 2')", + ]); + insta::assert_snapshot!(output, @r" + ------- stderr ------- + Moved 1 bookmarks to kmkuslsw 4f13590e bookmark2* | (empty) commit which should not be signed 2 with different author + [EOF] + "); + let output = work_dir.run_jj(["git", "push", "-b", "bookmark2"]); + insta::assert_snapshot!(output, @r" + ------- stderr ------- + Changes to push to origin: + Move forward bookmark bookmark2 from af8231621fcb to 4f13590e2672 + [EOF] + "); + let output = work_dir.run_jj(["log", "-T", template, "-r", "::"]); + insta::assert_snapshot!(output, @r" + @ final commit + ○ commit with different author + ○ commit which should not be signed 2 with different author + ◆ commit which should not be signed 1 + ◆ commit to be signed 2 + │ Signature: test-display, Status: good, Key: impeccable + ◆ commit to be signed 1 + │ Signature: test-display, Status: good, Key: impeccable + ◆ description 2 + │ ○ description 1 + ├─╯ + ◆ + [EOF] + "); + + // With with `behavior = "force"`, commit with different author is signed + test_env.add_config( + r#" + git.sign-on-push = "all()" + "#, + ); + let output = work_dir.run_jj([ + "bookmark", + "move", + "bookmark2", + "--to", + "description('commit with different author')", + ]); + insta::assert_snapshot!(output, @r" + ------- stderr ------- + Moved 1 bookmarks to lylxulpl 2b7318f0 bookmark2* | (empty) commit with different author + [EOF] + "); + let output = work_dir.run_jj(["git", "push", "-b", "bookmark2"]); + insta::assert_snapshot!(output, @r" + ------- stderr ------- + Signed 1 commits + Rebased 1 descendant commits + Changes to push to origin: + Move forward bookmark bookmark2 from 4f13590e2672 to aaed4b63a38a + Working copy (@) now at: nkmrtpmo 10ff3e13 (empty) final commit + Parent commit (@-) : lylxulpl aaed4b63 bookmark2 | (empty) commit with different author + [EOF] + "); + let output = work_dir.run_jj(["log", "-T", template, "-r", "::"]); + insta::assert_snapshot!(output, @r" + @ final commit + ○ commit with different author + │ Signature: test-display, Status: good, Key: impeccable + ○ commit which should not be signed 2 with different author ◆ commit which should not be signed 1 ◆ commit to be signed 2 │ Signature: test-display, Status: good, Key: impeccable diff --git a/docs/config.md b/docs/config.md index 8ffdf3e37..8716fe664 100644 --- a/docs/config.md +++ b/docs/config.md @@ -1349,22 +1349,22 @@ options are: ### Sign commits only on `jj git push` -Instead of signing all commits during creation when `signing.behavior` is -set to `own`, the `git.sign-on-push` configuration can be used to sign -commits only upon running `jj git push`. All mutable unsigned commits -being pushed will be signed prior to pushing. This might be preferred if the -signing backend requires user interaction or is slow, so that signing is -performed in a single batch operation. +If the signing backend requires user interaction for every signature, or is +slow, you might not want to sign each commit at creation time. Instead, the +`git.sign-on-push` configuration can be set to a revset of commits to sign only +upon running `jj git push`. All mutable unsigned commits being pushed which +match the revset will be signed prior to pushing. ```toml # Configure signing backend as before, but lazily signing only on push. [signing] -behavior = "drop" +behavior = "drop" # Avoid doing any signing on commit creation backend = "ssh" key = "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIGj+J6N6SO+4P8dOZqfR1oiay2yxhhHnagH52avUqw5h" [git] -sign-on-push = true +sign-on-push = "all()" # Sign all commits on push +# Could be "mine()", or any other revset ``` ### Manually signing commits