Ask for confirmation before pushing to remote

CHANGELOG.md

@@ -33,6 +33,9 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
   back to prompting the user if the heuristics are inconclusive. It can also run
   in non-interactive mode, which aborts if prompting would be needed.
 
+* `jj git push` can now be configured to ask for confirmation before pushing
+  changes to a remote using `git.confirm-before-push`.
+
 ### Fixed bugs
 
 * `jj arrange` now scrolls the viewport to keep the selected commit visible

cli/src/commands/git/push.rs

@@ -535,6 +535,13 @@ pub async fn cmd_git_push(
         return Ok(());
     }
 
+    let needs_confirm = !args.dry_run
+        && match tx.settings().get("git.confirm-before-push")? {
+            PushConfirmChoice::Always => true,
+            PushConfirmChoice::Never => false,
+            PushConfirmChoice::Auto => ref_updates.bookmarks.len() + ref_updates.tags.len() > 1,
+        };
+
     if !args.dry_run && tx.settings().get_bool("git.sign-on-push")? {
         let to_push_expr = ready_to_push_revset_expression(&tx, remote, &ref_updates);
         ref_updates = sign_commits_before_push(ui, &mut tx, to_push_expr, ref_updates).await?;
@@ -554,6 +561,11 @@ pub async fn cmd_git_push(
         return Ok(());
     }
 
+    if needs_confirm && !ui.prompt_yes_no("Continue?", Some(true))? {
+        writeln!(ui.status(), "Aborting; nothing was changed.")?;
+        return Ok(());
+    }
+
     let git_settings = GitSettings::from_settings(tx.settings())?;
     let options = GitPushOptions {
         remote_push_options: args.option.clone(),
@@ -601,6 +613,21 @@ pub async fn cmd_git_push(
     }
 }
 
+#[derive(Clone, Copy, PartialEq, Eq, Debug, serde::Deserialize)]
+#[serde(rename_all = "kebab-case")]
+pub enum PushConfirmChoice {
+    /// Always ask for confirmation before pushing
+    Always,
+    /// Never prompt the user before pushing a change
+    Never,
+    /// Only prompt if more than one bookmark/tag is about to be pushed
+    ///
+    /// If more than one bookmark or tag is moved in the same push, it is
+    /// possible that some of them were unintentional, so we should give the
+    /// user a chance to correct their mistake.
+    Auto,
+}
+
 #[derive(Clone, Debug)]
 struct RejectedCommitReason {
     commit: Commit,

cli/src/config-schema.json

@@ -559,6 +559,11 @@
                     "enum": ["sha1", "sha256"],
                     "description": "Object hash algorithm used when initializing a new Git repository",
                     "default": "sha1"
+                },
+                "confirm-before-push": {
+                    "enum": ["always", "never", "auto"],
+                    "description": "When to prompt user before pushing changes to remote",
+                    "default": "never"
                 }
             }
         },

cli/src/config/misc.toml

@@ -29,6 +29,7 @@ colocate = true
 object-hash = "sha1"
 private-commits = "none()"
 sign-on-push = false
+confirm-before-push = "never"
 track-default-bookmark-on-clone = true
 
 [ui]

cli/tests/test_git_push.rs

@@ -19,6 +19,7 @@ use testutils::git;
 use crate::common::CommandOutput;
 use crate::common::TestEnvironment;
 use crate::common::TestWorkDir;
+use crate::common::force_interactive;
 use crate::common::to_toml_value;
 
 fn git_repo_dir_for_jj_repo(work_dir: &TestWorkDir<'_>) -> std::path::PathBuf {
@@ -145,6 +146,85 @@ fn test_git_push_default_remote_selection() {
     ");
 }
 
+#[test]
+fn test_git_push_confirm() {
+    let test_env = TestEnvironment::default();
+    set_up(&test_env);
+    test_env.add_config("remotes.origin.auto-track-bookmarks = '*'");
+    test_env.add_config("git.confirm-before-push = 'always'");
+    let work_dir = test_env.work_dir("local");
+    test_env.add_config(r#"revset-aliases."immutable_heads()" = "none()""#);
+    // Update some bookmarks. `bookmark1` is not a current bookmark, but
+    // `bookmark2` and `my-bookmark` are.
+    work_dir
+        .run_jj(["describe", "bookmark1", "-m", "modified bookmark1 commit"])
+        .success();
+    work_dir.run_jj(["new", "bookmark2"]).success();
+    work_dir
+        .run_jj(["bookmark", "set", "bookmark2", "-r@"])
+        .success();
+    work_dir
+        .run_jj(["bookmark", "create", "-r@", "my-bookmark"])
+        .success();
+    work_dir.run_jj(["describe", "-m", "foo"]).success();
+    // Check the setup
+    insta::assert_snapshot!(get_bookmark_output(&work_dir), @"
+    bookmark1: qpvuntsm e5ce6d9a (empty) modified bookmark1 commit
+      @origin (ahead by 1 commits, behind by 1 commits): qpvuntsm/1 9b2e76de (hidden) (empty) description 1
+    bookmark2: yostqsxw 88ca14a7 (empty) foo
+      @origin (behind by 1 commits): zsuskuln 38a20473 (empty) description 2
+    my-bookmark: yostqsxw 88ca14a7 (empty) foo
+      @origin (not created yet)
+    [EOF]
+    ");
+    // Abort push, should not change anything
+    let output = work_dir.run_jj_with(|cmd| {
+        force_interactive(cmd)
+            .args(["git", "push"])
+            .write_stdin("n\n")
+    });
+    insta::assert_snapshot!(output, @"
+    ------- stderr -------
+    Changes to push to origin:
+      bookmark: bookmark2 [move forward from 38a204733702 to 88ca14a7d46f]
+      bookmark: my-bookmark [add to 88ca14a7d46f]
+    Continue? (Yn): Aborting; nothing was changed.
+    [EOF]
+    ");
+    // Make sure nothing has changed
+    insta::assert_snapshot!(get_bookmark_output(&work_dir), @"
+    bookmark1: qpvuntsm e5ce6d9a (empty) modified bookmark1 commit
+      @origin (ahead by 1 commits, behind by 1 commits): qpvuntsm/1 9b2e76de (hidden) (empty) description 1
+    bookmark2: yostqsxw 88ca14a7 (empty) foo
+      @origin (behind by 1 commits): zsuskuln 38a20473 (empty) description 2
+    my-bookmark: yostqsxw 88ca14a7 (empty) foo
+      @origin (not created yet)
+    [EOF]
+    ");
+    // Accept push, should go though to remote
+    let output = work_dir.run_jj_with(|cmd| {
+        force_interactive(cmd)
+            .args(["git", "push"])
+            .write_stdin("y\n")
+    });
+    insta::assert_snapshot!(output, @"
+    ------- stderr -------
+    Changes to push to origin:
+      bookmark: bookmark2 [move forward from 38a204733702 to 88ca14a7d46f]
+      bookmark: my-bookmark [add to 88ca14a7d46f]
+    Continue? (Yn): [EOF]
+    ");
+    insta::assert_snapshot!(get_bookmark_output(&work_dir), @"
+    bookmark1: qpvuntsm e5ce6d9a (empty) modified bookmark1 commit
+      @origin (ahead by 1 commits, behind by 1 commits): qpvuntsm/1 9b2e76de (hidden) (empty) description 1
+    bookmark2: yostqsxw 88ca14a7 (empty) foo
+      @origin: yostqsxw 88ca14a7 (empty) foo
+    my-bookmark: yostqsxw 88ca14a7 (empty) foo
+      @origin: yostqsxw 88ca14a7 (empty) foo
+    [EOF]
+    ");
+}
+
 #[test]
 fn test_git_push_current_bookmark() {
     let test_env = TestEnvironment::default();
@@ -2667,6 +2747,7 @@ fn test_git_push_sign_on_push() {
     signing.backend = "test"
     signing.key = "impeccable"
     git.sign-on-push = true
+    git.confirm-before-push = "always"
     "#,
     );
     let output = work_dir.run_jj(["git", "push", "--dry-run"]);
@@ -2690,15 +2771,46 @@ fn test_git_push_sign_on_push() {
     ◆
     [EOF]
     ");
-    let output = work_dir.run_jj(["git", "push"]);
-    insta::assert_snapshot!(output, @r"
+    let output = work_dir.run_jj_with(|cmd| {
+        force_interactive(cmd)
+            .args(["git", "push"])
+            .write_stdin("n\n")
+    });
+    insta::assert_snapshot!(output, @"
     ------- stderr -------
     Updated signatures of 2 commits.
     Rebased 2 descendant commits.
     Changes to push to origin:
       bookmark: bookmark2 [move forward from 38a204733702 to d45e2adce0ad]
-    Working copy  (@) now at: kmkuslsw 3d5a9465 (empty) commit which should not be signed 2
-    Parent commit (@-)      : kpqxywon 48ea83e9 (empty) commit which should not be signed 1
+    Continue? (Yn): Aborting; nothing was changed.
+    [EOF]
+    ");
+    // There should be no signed commits aborting a push
+    let output = work_dir.run_jj(["log", "-T", template]);
+    insta::assert_snapshot!(output, @"
+    @  commit which should not be signed 2
+    ○  commit which should not be signed 1
+    ○  commit to be signed 2
+    ○  commit to be signed 1
+    ○  description 2
+    │ ○  description 1
+    ├─╯
+    ◆
+    [EOF]
+    ");
+    let output = work_dir.run_jj_with(|cmd| {
+        force_interactive(cmd)
+            .args(["git", "push"])
+            .write_stdin("y\n")
+    });
+    insta::assert_snapshot!(output, @r"
+    ------- stderr -------
+    Updated signatures of 2 commits.
+    Rebased 2 descendant commits.
+    Changes to push to origin:
+      bookmark: bookmark2 [move forward from 38a204733702 to 87d9bf2ca4fb]
+    Continue? (Yn): Working copy  (@) now at: kmkuslsw 50a620ed (empty) commit which should not be signed 2
+    Parent commit (@-)      : kpqxywon f4169ad1 (empty) commit which should not be signed 1
     [EOF]
     ");
     // Only commits which are being pushed should be signed
@@ -2721,18 +2833,22 @@ fn test_git_push_sign_on_push() {
     let output = work_dir.run_jj(["bookmark", "move", "bookmark2", "--to", "@-"]);
     insta::assert_snapshot!(output, @"
     ------- stderr -------
-    Moved 1 bookmarks to kpqxywon 48ea83e9 bookmark2* | (empty) commit which should not be signed 1
+    Moved 1 bookmarks to kpqxywon f4169ad1 bookmark2* | (empty) commit which should not be signed 1
     [EOF]
     ");
     test_env.add_config(r#"revset-aliases."immutable_heads()" = "bookmark2""#);
-    let output = work_dir.run_jj(["git", "push"]);
+    let output = work_dir.run_jj_with(|cmd| {
+        force_interactive(cmd)
+            .args(["git", "push"])
+            .write_stdin("y\n")
+    });
     insta::assert_snapshot!(output, @"
     ------- stderr -------
     Warning: Skipped signing 1 immutable commits:
-      kpqxywon 48ea83e9 bookmark2* | (empty) commit which should not be signed 1
+      kpqxywon f4169ad1 bookmark2* | (empty) commit which should not be signed 1
     Changes to push to origin:
-      bookmark: bookmark2 [move forward from d45e2adce0ad to 48ea83e9499c]
-    [EOF]
+      bookmark: bookmark2 [move forward from 87d9bf2ca4fb to f4169ad1a603]
+    Continue? (Yn): [EOF]
     ");
     let output = work_dir.run_jj(["log", "-T", template, "-r", "::"]);
     insta::assert_snapshot!(output, @"
@@ -2780,14 +2896,18 @@ fn test_git_push_sign_on_push() {
     ◆
     [EOF]
     ");
-    let output = work_dir.run_jj(["git", "push"]);
+    let output = work_dir.run_jj_with(|cmd| {
+        force_interactive(cmd)
+            .args(["git", "push"])
+            .write_stdin("y\n")
+    });
     insta::assert_snapshot!(output, @r"
     ------- stderr -------
     Updated signatures of 1 commits.
     Changes to push to origin:
-      bookmark: bookmark1 [move sideways from 9b2e76de3920 to 0617b6813c01]
-    Working copy  (@) now at: pzsxstzt 0617b681 bookmark1 | (empty) commit to be signed 3
-    Parent commit (@-)      : kmkuslsw 5114df95 (empty) commit which should not be signed 2
+      bookmark: bookmark1 [move sideways from 9b2e76de3920 to 14bd874a5304]
+    Continue? (Yn): Working copy  (@) now at: uuuvxpvw 14bd874a bookmark1 | (empty) commit to be signed 3
+    Parent commit (@-)      : kmkuslsw 78f629cb (empty) commit which should not be signed 2
     [EOF]
     ");
     let output = work_dir.run_jj(["log", "-T", template]);

docs/config.md

@@ -1967,6 +1967,19 @@ git_push_bookmark = '"martinvonz/push-" ++ change_id.short()'
 This template should include expressions like `change_id` to generate unique and
 stable bookmark.
 
+### Ask for confirmation on push
+
+The command `jj git push` can be configured to ask for confirmation before
+applying any changes to a remote using the setting `git.confirm-before-push`.
+Possible values are `always`, `never` and `auto` (default: `never`); a value
+of `auto` will prompt the user only when multiple bookmarks or tags are about
+to be updated in a single push. For example:
+
+```toml
+[git]
+confirm-before-push = 'auto'
+```
+
 ### Set of private commits
 
 You can configure the set of private commits by setting `git.private-commits` to