cli: git push: exclude new bookmarks tracking other remotes if no --r…

…emote set

If you have multiple remotes to push to, you might want to keep some changes
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 if tracked bookmarks
already exists on other remotes. I considered adding a warning, but it's not
always possible to interrupt the push shortly after a warning is emitted. This
check can be turned off by specifying --remote=NAME explicitly. Another stricter
(and simpler in terms of implementation) idea is to force user to pass
--new-bookmark or something to push any local bookmark to new remote.

#1278

CHANGELOG.md

@@ -30,6 +30,9 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 * Default operation log template now shows end times of operations instead of
   start times.
 
+* `jj git push` no longer pushes tracking bookmarks to new remotes. Specify
+  `--remote=NAME` to bypass this restriction.
+
 ### Deprecations
 
 ### New features

cli/src/commands/git/push.rs

@@ -155,14 +155,15 @@ pub fn cmd_git_push(
     } else {
         get_default_push_remote(ui, command.settings(), &git_repo)?
     };
+    let track_new = args.remote.is_some();
 
     let mut tx = workspace_command.start_transaction();
     let view = tx.repo().view();
     let tx_description;
     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) {
+            match classify_bookmark_update(view, bookmark_name, &remote, targets, track_new) {
                 Ok(Some(update)) => bookmark_updates.push((bookmark_name.to_owned(), update)),
                 Ok(None) => {}
                 Err(reason) => reason.print(ui)?,
@@ -174,7 +175,7 @@ pub fn cmd_git_push(
             if !targets.remote_ref.is_tracking() {
                 continue;
             }
-            match classify_bookmark_update(bookmark_name, &remote, targets) {
+            match classify_bookmark_update(view, bookmark_name, &remote, targets, track_new) {
                 Ok(Some(update)) => bookmark_updates.push((bookmark_name.to_owned(), update)),
                 Ok(None) => {}
                 Err(reason) => reason.print(ui)?,
@@ -186,7 +187,7 @@ pub fn cmd_git_push(
             if targets.local_target.is_present() {
                 continue;
             }
-            match classify_bookmark_update(bookmark_name, &remote, targets) {
+            match classify_bookmark_update(view, bookmark_name, &remote, targets, track_new) {
                 Ok(Some(update)) => bookmark_updates.push((bookmark_name.to_owned(), update)),
                 Ok(None) => {}
                 Err(reason) => reason.print(ui)?,
@@ -223,7 +224,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(view, bookmark_name, &remote, targets, track_new) {
                 Ok(Some(update)) => bookmark_updates.push((bookmark_name.to_owned(), update)),
                 Ok(None) => writeln!(
                     ui.status(),
@@ -246,7 +247,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(view, bookmark_name, &remote, targets, track_new) {
                 Ok(Some(update)) => bookmark_updates.push((bookmark_name.to_owned(), update)),
                 Ok(None) => {}
                 Err(reason) => reason.print(ui)?,
@@ -491,9 +492,11 @@ impl From<RejectedBookmarkUpdateReason> for CommandError {
 }
 
 fn classify_bookmark_update(
+    view: &View,
     bookmark_name: &str,
     remote_name: &str,
     targets: LocalAndRemoteRef,
+    track_new: bool,
 ) -> Result<Option<BookmarkPushUpdate>, RejectedBookmarkUpdateReason> {
     let push_action = classify_bookmark_push_action(targets);
     match push_action {
@@ -516,7 +519,44 @@ fn classify_bookmark_update(
                  bookmark."
             )),
         }),
-        BookmarkPushAction::Update(update) => Ok(Some(update)),
+        BookmarkPushAction::Update(update) => {
+            if !targets.remote_ref.is_tracking() && !track_new {
+                reject_tracking_bookmark(view, bookmark_name, remote_name)?;
+            }
+            Ok(Some(update))
+        }
+    }
+}
+
+fn reject_tracking_bookmark(
+    view: &View,
+    bookmark_name: &str,
+    remote_name: &str,
+) -> Result<(), RejectedBookmarkUpdateReason> {
+    let tracking_remote_names = view
+        .remote_bookmarks_matching(
+            &StringPattern::exact(bookmark_name),
+            &StringPattern::everything(),
+        )
+        .filter(|&((_, remote), _)| remote != git::REMOTE_NAME_FOR_LOCAL_GIT_REPO)
+        .filter(|(_, remote_ref)| remote_ref.is_tracking())
+        .map(|((_, remote), _)| remote)
+        .collect_vec();
+    match &*tracking_remote_names {
+        [] => Ok(()),
+        [tracking_remote, ..] => {
+            let remote_bookmarks = tracking_remote_names
+                .iter()
+                .map(|remote| format!("{bookmark_name}@{remote}"))
+                .join(", ");
+            Err(RejectedBookmarkUpdateReason {
+                message: format!("Bookmark {bookmark_name} is already tracking {remote_bookmarks}"),
+                hint: Some(format!(
+                    "Use --remote={tracking_remote} to update the remote bookmark. Use \
+                     --remote={remote_name} to push to new remote."
+                )),
+            })
+        }
     }
 }
 

cli/tests/test_git_push.rs

@@ -1289,6 +1289,105 @@ fn test_git_push_moved_sideways_untracked() {
     "###);
 }
 
+#[test]
+fn test_git_push_already_tracking() {
+    let test_env = TestEnvironment::default();
+    // Set up colocated local workspace, which has the @git remote.
+    test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "--colocate", "local"]);
+    let workspace_root = test_env.env_root().join("local");
+    for remote in ["origin", "other"] {
+        test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", remote]);
+        let path = test_env
+            .env_root()
+            .join(PathBuf::from_iter([remote, ".jj", "repo", "store", "git"]));
+        test_env.jj_cmd_ok(
+            &workspace_root,
+            &["git", "remote", "add", remote, path.to_str().unwrap()],
+        );
+    }
+
+    // Push newly-created bookmark to default
+    test_env.jj_cmd_ok(&workspace_root, &["commit", "-m1"]);
+    test_env.jj_cmd_ok(&workspace_root, &["bookmark", "create", "-r@-", "foo"]);
+    let (_stdout, stderr) = test_env.jj_cmd_ok(&workspace_root, &["git", "push"]);
+    insta::assert_snapshot!(stderr, @r#"
+    Changes to push to origin:
+      Add bookmark foo to 8ee5f4bfcc8f
+    "#);
+
+    // Push newly-created bookmark to other
+    test_env.jj_cmd_ok(&workspace_root, &["commit", "-m2"]);
+    test_env.jj_cmd_ok(&workspace_root, &["bookmark", "create", "-r@-", "bar"]);
+    let (_stdout, stderr) = test_env.jj_cmd_ok(
+        &workspace_root,
+        &["git", "push", "--remote=other", "--bookmark=bar"],
+    );
+    insta::assert_snapshot!(stderr, @r#"
+    Changes to push to other:
+      Add bookmark bar to 6836627dd409
+    "#);
+
+    // Try to push tracking and newly-created bookmarks to default
+    test_env.jj_cmd_ok(&workspace_root, &["commit", "-m2"]);
+    test_env.jj_cmd_ok(&workspace_root, &["bookmark", "move", "--to=@-", "foo"]);
+    test_env.jj_cmd_ok(&workspace_root, &["bookmark", "create", "-r@-", "baz"]);
+    let (_stdout, stderr) = test_env.jj_cmd_ok(&workspace_root, &["git", "push", "--dry-run"]);
+    insta::assert_snapshot!(stderr, @r#"
+    Warning: Bookmark bar is already tracking bar@other
+    Hint: Use --remote=other to update the remote bookmark. Use --remote=origin to push to new remote.
+    Changes to push to origin:
+      Add bookmark baz to 8a0af1306e25
+      Move forward bookmark foo from 8ee5f4bfcc8f to 8a0af1306e25
+    Dry-run requested, not pushing.
+    "#);
+
+    // Try with explicit --remote argument, which bypasses the protection.
+    let (_stdout, stderr) = test_env.jj_cmd_ok(
+        &workspace_root,
+        &["git", "push", "--remote=origin", "--dry-run"],
+    );
+    insta::assert_snapshot!(stderr, @r#"
+    Changes to push to origin:
+      Add bookmark bar to 6836627dd409
+      Add bookmark baz to 8a0af1306e25
+      Move forward bookmark foo from 8ee5f4bfcc8f to 8a0af1306e25
+    Dry-run requested, not pushing.
+    "#);
+
+    // Try with --all, which doesn't bypass the protection right now.
+    // TODO: Maybe okay to push all bookmarks because --all is an explicit flag?
+    let (_stdout, stderr) =
+        test_env.jj_cmd_ok(&workspace_root, &["git", "push", "--all", "--dry-run"]);
+    insta::assert_snapshot!(stderr, @r#"
+    Warning: Bookmark bar is already tracking bar@other
+    Hint: Use --remote=other to update the remote bookmark. Use --remote=origin to push to new remote.
+    Changes to push to origin:
+      Add bookmark baz to 8a0af1306e25
+      Move forward bookmark foo from 8ee5f4bfcc8f to 8a0af1306e25
+    Dry-run requested, not pushing.
+    "#);
+
+    // Try with --tracked, which effectively silences the warning.
+    let (_stdout, stderr) =
+        test_env.jj_cmd_ok(&workspace_root, &["git", "push", "--tracked", "--dry-run"]);
+    insta::assert_snapshot!(stderr, @r#"
+    Changes to push to origin:
+      Move forward bookmark foo from 8ee5f4bfcc8f to 8a0af1306e25
+    Dry-run requested, not pushing.
+    "#);
+
+    // Try with --bookmark=NAME, which is an error. Git users might expect that
+    // the remote to push to is chosen by the tracking bookmark.
+    let stderr = test_env.jj_cmd_failure(
+        &workspace_root,
+        &["git", "push", "--bookmark=bar", "--dry-run"],
+    );
+    insta::assert_snapshot!(stderr, @r#"
+    Error: Bookmark bar is already tracking bar@other
+    Hint: Use --remote=other to update the remote bookmark. Use --remote=origin to push to new remote.
+    "#);
+}
+
 #[test]
 // TODO: This test fails with libgit2 v1.8.1 on Windows.
 #[cfg(not(target_os = "windows"))]