cli: git push: make git.sign-on-push accept a revset

This allows `git.sign-on-push` to sign other commits based on the given
revset, not just the user's own commits.

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
 

cli/src/commands/git/push.rs

@@ -400,28 +400,15 @@ 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_commits_signed = commits_to_sign.len();
-            let num_updated_signatures = commits_to_sign.len();
         let num_rebased_descendants;
-            (num_rebased_descendants, bookmark_updates) = sign_commits_before_push(
+        (num_rebased_descendants, bookmark_updates) =
-                &mut tx,
+            sign_commits_before_push(&mut tx, commits_to_sign, bookmark_updates)?;
-                commits_to_sign,
-                sign_behavior,
-                bookmark_updates,
-            )?;
         if let Some(mut formatter) = ui.status_formatter() {
-                writeln!(
+            writeln!(formatter, "Signed {num_commits_signed} commits")?;
-                    formatter,
-                    "Updated signatures of {num_updated_signatures} commits"
-                )?;
             if num_rebased_descendants > 0 {
                 writeln!(
                     formatter,
@@ -430,7 +417,6 @@ pub fn cmd_git_push(
             }
         }
     }
-    }
 
     if let Some(mut formatter) = ui.status_formatter() {
         writeln!(
@@ -513,7 +499,6 @@ fn validate_commits_ready_to_push(
     remote: &RemoteName,
     tx: &WorkspaceCommandTransaction,
     args: &GitPushArgs,
-    sign_behavior: Option<SignBehavior>,
 ) -> Result<Vec<Commit>, 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 sign_on_push_rev = RevisionArg::from(settings.get_string("git.sign-on-push")?);
-        let mut sign_settings = settings.sign_settings();
+    let should_sign_on_push = workspace_helper
-        sign_settings.behavior = sign_behavior;
+        .parse_revset(ui, &sign_on_push_rev)?
-        sign_settings
+        .evaluate()?
-    });
+        .containing_fn();
 
     let mut commits_to_sign = vec![];
 
@@ -591,12 +576,10 @@ fn validate_commits_ready_to_push(
             }
             return Err(error);
         }
-        if let Some(sign_settings) = &sign_settings {
+        if !commit.is_signed() && should_sign_on_push(commit.id())? {
-            if !commit.is_signed() && sign_settings.should_sign(commit.store_commit()) {
             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<Commit>,
-    sign_behavior: SignBehavior,
     bookmark_updates: Vec<(RefNameBuf, BookmarkPushUpdate)>,
 ) -> Result<(usize, Vec<(RefNameBuf, BookmarkPushUpdate)>), CommandError> {
     let commit_ids: IndexSet<CommitId> = 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 {

cli/src/config-schema.json

@@ -470,9 +470,8 @@
                     "default": "origin"
                 },
                 "sign-on-push": {
-                    "type": "boolean",
+                    "type": "string",
-                    "description": "Whether jj should sign commits before pushing",
+                    "description": "Revset of commits to sign before pushing"
-                    "default": false
                 },
                 "subprocess": {
                     "type": "boolean",

cli/src/config.rs

@@ -653,6 +653,28 @@ pub fn default_config_migrations() -> Vec<ConfigMigrationRule> {
                 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!(),
+            },
+        ),
     ]
 }
 

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

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 <foo@example.org>",
+        ])
         .success();
+    work_dir.run_jj(["new"]).success();
+    work_dir
+        .run_jj([
+            "describe",
+            "-m",
+            "commit with different author",
+            "--author=Foo <foo@example.org>",
+        ])
+        .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
+    Signed 2 commits
-    Rebased 2 descendant commits
+    Rebased 4 descendant commits
     Changes to push to origin:
-      Move forward bookmark bookmark2 from 8476341eb395 to a6259c482040
+      Move forward bookmark bookmark2 from 8476341eb395 to 2ac4cdcf94ef
-    Working copy  (@) now at: kmkuslsw b5f47345 (empty) commit which should not be signed 2
+    Working copy  (@) now at: nkmrtpmo a4e979ab (empty) final commit
-    Parent commit (@-)      : kpqxywon 90df08d3 (empty) commit which should not be signed 1
+    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

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
+If the signing backend requires user interaction for every signature, or is
-set to `own`, the `git.sign-on-push` configuration can be used to sign
+slow, you might not want to sign each commit at creation time. Instead, the
-commits only upon running `jj git push`. All mutable unsigned commits
+`git.sign-on-push` configuration can be set to a revset of commits to sign only
-being pushed will be signed prior to pushing. This might be preferred if the
+upon running `jj git push`. All mutable unsigned commits being pushed which
-signing backend requires user interaction or is slow, so that signing is
+match the revset will be signed prior to pushing.
-performed in a single batch operation.
 
 ```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