cli: git push: do not push new bookmarks by default

If you have multiple remotes to push to, you might want to keep some changes
(such as security patches) in your private fork. Git CLI has one upstream remote
per branch, but jj supports multiple tracking remotes, and therefore "jj git
push" can start tracking new remotes automatically.

This patch makes new bookmarks not eligible for push by default. I considered
adding a warning, but it's not always possible to interrupt the push shortly
after a warning is emitted.

--all implies --allow-new because otherwise it's equivalent to --tracked. It's
also easier to write a conflict rule with --all/--deleted/--tracked than with
two of them.

-c/--change doesn't require --allow-new because it is the flag to create new
tracking bookmark.

jj-vcs#1278

CHANGELOG.md

@@ -12,6 +12,9 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 * `jj move` has been removed. It was deprecated in 0.16.0.
 
+* `jj git push` no longer pushes new bookmarks by default. Use `--allow-new` to
+  bypass this restriction.
+
 ### Deprecations
 
 ### New features

cli/src/commands/git/push.rs

@@ -57,7 +57,7 @@ use crate::ui::Ui;
 
 /// Push to a Git remote
 ///
-/// By default, pushes any bookmarks pointing to
+/// By default, pushes tracking bookmarks pointing to
 /// `remote_bookmarks(remote=<remote>)..@`. Use `--bookmark` to push specific
 /// bookmarks. Use `--all` to push all bookmarks. Use `--change` to generate
 /// bookmark names based on the change IDs of specific commits.
@@ -98,7 +98,7 @@ pub struct GitPushArgs {
         add = ArgValueCandidates::new(complete::local_bookmarks),
     )]
     bookmark: Vec<StringPattern>,
-    /// Push all bookmarks (including deleted bookmarks)
+    /// Push all bookmarks (including new and deleted bookmarks)
     #[arg(long)]
     all: bool,
     /// Push all tracked bookmarks (including deleted bookmarks)
@@ -115,6 +115,11 @@ pub struct GitPushArgs {
     /// correspond to missing local bookmarks.
     #[arg(long)]
     deleted: bool,
+    /// Allow pushing new bookmarks
+    ///
+    /// Newly-created remote bookmarks will be tracked automatically.
+    #[arg(long, conflicts_with = "what")]
+    allow_new: bool,
     /// Allow pushing commits with empty descriptions
     #[arg(long)]
     allow_empty_description: bool,
@@ -127,8 +132,9 @@ pub struct GitPushArgs {
     /// Push this commit by creating a bookmark based on its change ID (can be
     /// repeated)
     ///
-    /// Use the `git.push-bookmark-prefix` setting to change the prefix for
-    /// generated names.
+    /// The created bookmark will be tracked automatically. Use the
+    /// `git.push-bookmark-prefix` setting to change the prefix for generated
+    /// names.
     #[arg(long, short)]
     change: Vec<RevisionArg>,
     /// Only display what will change on the remote
@@ -172,7 +178,8 @@ pub fn cmd_git_push(
     let mut bookmark_updates = vec![];
     if args.all {
         for (bookmark_name, targets) in view.local_remote_bookmarks(&remote) {
-            match classify_bookmark_update(bookmark_name, &remote, targets) {
+            let allow_new = true; // implied by --all
+            match classify_bookmark_update(bookmark_name, &remote, targets, allow_new) {
                 Ok(Some(update)) => bookmark_updates.push((bookmark_name.to_owned(), update)),
                 Ok(None) => {}
                 Err(reason) => reason.print(ui)?,
@@ -184,7 +191,8 @@ pub fn cmd_git_push(
             if !targets.remote_ref.is_tracking() {
                 continue;
             }
-            match classify_bookmark_update(bookmark_name, &remote, targets) {
+            let allow_new = false; // doesn't matter
+            match classify_bookmark_update(bookmark_name, &remote, targets, allow_new) {
                 Ok(Some(update)) => bookmark_updates.push((bookmark_name.to_owned(), update)),
                 Ok(None) => {}
                 Err(reason) => reason.print(ui)?,
@@ -196,7 +204,8 @@ pub fn cmd_git_push(
             if targets.local_target.is_present() {
                 continue;
             }
-            match classify_bookmark_update(bookmark_name, &remote, targets) {
+            let allow_new = false; // doesn't matter
+            match classify_bookmark_update(bookmark_name, &remote, targets, allow_new) {
                 Ok(Some(update)) => bookmark_updates.push((bookmark_name.to_owned(), update)),
                 Ok(None) => {}
                 Err(reason) => reason.print(ui)?,
@@ -228,12 +237,27 @@ pub fn cmd_git_push(
             (bookmark_name.as_ref(), targets)
         });
         let view = tx.repo().view();
+        for (bookmark_name, targets) in change_bookmarks {
+            if !seen_bookmarks.insert(bookmark_name) {
+                continue;
+            }
+            let allow_new = true; // --change implies creation of remote bookmark
+            match classify_bookmark_update(bookmark_name, &remote, targets, allow_new) {
+                Ok(Some(update)) => bookmark_updates.push((bookmark_name.to_owned(), update)),
+                Ok(None) => writeln!(
+                    ui.status(),
+                    "Bookmark {bookmark_name}@{remote} already matches {bookmark_name}",
+                )?,
+                Err(reason) => return Err(reason.into()),
+            }
+        }
+
         let bookmarks_by_name = find_bookmarks_to_push(view, &args.bookmark, &remote)?;
-        for (bookmark_name, targets) in change_bookmarks.chain(bookmarks_by_name.iter().copied()) {
+        for &(bookmark_name, targets) in &bookmarks_by_name {
             if !seen_bookmarks.insert(bookmark_name) {
                 continue;
             }
-            match classify_bookmark_update(bookmark_name, &remote, targets) {
+            match classify_bookmark_update(bookmark_name, &remote, targets, args.allow_new) {
                 Ok(Some(update)) => bookmark_updates.push((bookmark_name.to_owned(), update)),
                 Ok(None) => writeln!(
                     ui.status(),
@@ -256,7 +280,7 @@ pub fn cmd_git_push(
             if !seen_bookmarks.insert(bookmark_name) {
                 continue;
             }
-            match classify_bookmark_update(bookmark_name, &remote, targets) {
+            match classify_bookmark_update(bookmark_name, &remote, targets, args.allow_new) {
                 Ok(Some(update)) => bookmark_updates.push((bookmark_name.to_owned(), update)),
                 Ok(None) => {}
                 Err(reason) => reason.print(ui)?,
@@ -504,6 +528,7 @@ fn classify_bookmark_update(
     bookmark_name: &str,
     remote_name: &str,
     targets: LocalAndRemoteRef,
+    allow_new: bool,
 ) -> Result<Option<BookmarkPushUpdate>, RejectedBookmarkUpdateReason> {
     let push_action = classify_bookmark_push_action(targets);
     match push_action {
@@ -526,6 +551,18 @@ fn classify_bookmark_update(
                  bookmark."
             )),
         }),
+        BookmarkPushAction::Update(update) if update.old_target.is_none() && !allow_new => {
+            Err(RejectedBookmarkUpdateReason {
+                message: format!(
+                    "Refusing to create new remote bookmark {bookmark_name}@{remote_name}"
+                ),
+                hint: Some(
+                    "Use --allow-new to push new bookmark. Use --remote to specify the remote to \
+                     push to."
+                        .to_owned(),
+                ),
+            })
+        }
         BookmarkPushAction::Update(update) => Ok(Some(update)),
     }
 }

cli/tests/cli-reference@.md.snap

@@ -1148,7 +1148,7 @@ Create a new Git backed repo
 
 Push to a Git remote
 
-By default, pushes any bookmarks pointing to `remote_bookmarks(remote=<remote>)..@`. Use `--bookmark` to push specific bookmarks. Use `--all` to push all bookmarks. Use `--change` to generate bookmark names based on the change IDs of specific commits.
+By default, pushes tracking bookmarks pointing to `remote_bookmarks(remote=<remote>)..@`. Use `--bookmark` to push specific bookmarks. Use `--all` to push all bookmarks. Use `--change` to generate bookmark names based on the change IDs of specific commits.
 
 Before the command actually moves, creates, or deletes a remote bookmark, it makes several [safety checks]. If there is a problem, you may need to run `jj git fetch --remote <remote name>` and/or resolve some [bookmark conflicts].
 
@@ -1166,19 +1166,22 @@ Before the command actually moves, creates, or deletes a remote bookmark, it mak
 * `-b`, `--bookmark <BOOKMARK>` — Push only this bookmark, or bookmarks matching a pattern (can be repeated)
 
    By default, the specified name matches exactly. Use `glob:` prefix to select bookmarks by wildcard pattern. For details, see https://martinvonz.github.io/jj/latest/revsets#string-patterns.
-* `--all` — Push all bookmarks (including deleted bookmarks)
+* `--all` — Push all bookmarks (including new and deleted bookmarks)
 * `--tracked` — Push all tracked bookmarks (including deleted bookmarks)
 
    This usually means that the bookmark was already pushed to or fetched from the relevant remote. For details, see https://martinvonz.github.io/jj/latest/bookmarks#remotes-and-tracked-bookmarks
 * `--deleted` — Push all deleted bookmarks
 
    Only tracked bookmarks can be successfully deleted on the remote. A warning will be printed if any untracked bookmarks on the remote correspond to missing local bookmarks.
+* `--allow-new` — Allow pushing new bookmarks
+
+   Newly-created remote bookmarks will be tracked automatically.
 * `--allow-empty-description` — Allow pushing commits with empty descriptions
 * `--allow-private` — Allow pushing commits that are private
 * `-r`, `--revisions <REVISIONS>` — Push bookmarks pointing to these commits (can be repeated)
 * `-c`, `--change <CHANGE>` — Push this commit by creating a bookmark based on its change ID (can be repeated)
 
-   Use the `git.push-bookmark-prefix` setting to change the prefix for generated names.
+   The created bookmark will be tracked automatically. Use the `git.push-bookmark-prefix` setting to change the prefix for generated names.
 * `--dry-run` — Only display what will change on the remote
 
 

cli/tests/test_bookmark_command.rs

@@ -196,7 +196,7 @@ fn test_bookmark_move() {
 
     // Delete bookmark locally, but is still tracking remote
     test_env.jj_cmd_ok(&repo_path, &["describe", "@-", "-mcommit"]);
-    test_env.jj_cmd_ok(&repo_path, &["git", "push", "-r@-"]);
+    test_env.jj_cmd_ok(&repo_path, &["git", "push", "--allow-new", "-r@-"]);
     test_env.jj_cmd_ok(&repo_path, &["bookmark", "delete", "foo"]);
     insta::assert_snapshot!(get_bookmark_output(&test_env, &repo_path), @r###"
     foo (deleted)
@@ -441,7 +441,7 @@ fn test_bookmark_rename() {
     test_env.jj_cmd_ok(&repo_path, &["new"]);
     test_env.jj_cmd_ok(&repo_path, &["describe", "-m=commit-2"]);
     test_env.jj_cmd_ok(&repo_path, &["bookmark", "create", "bremote"]);
-    test_env.jj_cmd_ok(&repo_path, &["git", "push", "-b=bremote"]);
+    test_env.jj_cmd_ok(&repo_path, &["git", "push", "--allow-new", "-b=bremote"]);
     let (_stdout, stderr) =
         test_env.jj_cmd_ok(&repo_path, &["bookmark", "rename", "bremote", "bremote2"]);
     insta::assert_snapshot!(stderr, @r###"
@@ -1081,7 +1081,7 @@ fn test_bookmark_track_conflict() {
     );
     test_env.jj_cmd_ok(&repo_path, &["bookmark", "create", "main"]);
     test_env.jj_cmd_ok(&repo_path, &["describe", "-m", "a"]);
-    test_env.jj_cmd_ok(&repo_path, &["git", "push", "-b", "main"]);
+    test_env.jj_cmd_ok(&repo_path, &["git", "push", "--allow-new", "-b", "main"]);
     test_env.jj_cmd_ok(&repo_path, &["bookmark", "untrack", "main@origin"]);
     test_env.jj_cmd_ok(
         &repo_path,
@@ -1757,6 +1757,7 @@ fn test_bookmark_list_tracked() {
         &[
             "git",
             "push",
+            "--allow-new",
             "--remote",
             "upstream",
             "--bookmark",

cli/tests/test_completion.rs

@@ -49,7 +49,10 @@ fn test_bookmark_names() {
     test_env.jj_cmd_ok(&repo_path, &["desc", "-r", "aaa-tracked", "-m", "x"]);
     test_env.jj_cmd_ok(&repo_path, &["bookmark", "create", "bbb-tracked"]);
     test_env.jj_cmd_ok(&repo_path, &["desc", "-r", "bbb-tracked", "-m", "x"]);
-    test_env.jj_cmd_ok(&repo_path, &["git", "push", "--bookmark", "glob:*-tracked"]);
+    test_env.jj_cmd_ok(
+        &repo_path,
+        &["git", "push", "--allow-new", "--bookmark", "glob:*-tracked"],
+    );
 
     test_env.jj_cmd_ok(&origin_path, &["bookmark", "create", "aaa-untracked"]);
     test_env.jj_cmd_ok(&origin_path, &["desc", "-r", "aaa-untracked", "-m", "x"]);

cli/tests/test_git_private_commits.rs

@@ -67,7 +67,14 @@ fn set_up_remote_at_main(test_env: &TestEnvironment, workspace_root: &Path, remo
     );
     test_env.jj_cmd_ok(
         workspace_root,
-        &["git", "push", "--remote", remote_name, "-b=main"],
+        &[
+            "git",
+            "push",
+            "--allow-new",
+            "--remote",
+            remote_name,
+            "-b=main",
+        ],
     );
 }
 
@@ -155,7 +162,10 @@ fn test_git_private_commits_not_directly_in_line_block_pushing() {
     test_env.jj_cmd_ok(&workspace_root, &["bookmark", "create", "bookmark1"]);
 
     test_env.add_config(r#"git.private-commits = "description(glob:'private*')""#);
-    let stderr = test_env.jj_cmd_failure(&workspace_root, &["git", "push", "-b=bookmark1"]);
+    let stderr = test_env.jj_cmd_failure(
+        &workspace_root,
+        &["git", "push", "--allow-new", "-b=bookmark1"],
+    );
     insta::assert_snapshot!(stderr, @r###"
     Error: Won't push commit f1253a9b1ea9 since it is private
     "###);
@@ -192,8 +202,10 @@ fn test_git_private_commits_already_on_the_remote_do_not_block_push() {
     test_env.jj_cmd_ok(&workspace_root, &["new", "main", "-m=private 1"]);
     test_env.jj_cmd_ok(&workspace_root, &["new", "-m=public 3"]);
     test_env.jj_cmd_ok(&workspace_root, &["bookmark", "set", "main"]);
-    let (_, stderr) =
-        test_env.jj_cmd_ok(&workspace_root, &["git", "push", "-b=main", "-b=bookmark1"]);
+    let (_, stderr) = test_env.jj_cmd_ok(
+        &workspace_root,
+        &["git", "push", "--allow-new", "-b=main", "-b=bookmark1"],
+    );
     insta::assert_snapshot!(stderr, @r#"
     Changes to push to origin:
       Move forward bookmark main from 7eb97bf230ad to fbb352762352
@@ -223,7 +235,10 @@ fn test_git_private_commits_already_on_the_remote_do_not_block_push() {
         &["new", "description('private 1')", "-m=public 4"],
     );
     test_env.jj_cmd_ok(&workspace_root, &["bookmark", "create", "bookmark2"]);
-    let (_, stderr) = test_env.jj_cmd_ok(&workspace_root, &["git", "push", "-b=bookmark2"]);
+    let (_, stderr) = test_env.jj_cmd_ok(
+        &workspace_root,
+        &["git", "push", "--allow-new", "-b=bookmark2"],
+    );
     insta::assert_snapshot!(stderr, @r#"
     Changes to push to origin:
       Add bookmark bookmark2 to ee5b808b0b95