From 9e0b9e6dc8ffcdff077d3fc63a9f5d925e1455f3 Mon Sep 17 00:00:00 2001
From: Yuya Nishihara <yuya@tcha.org>
Date: Mon, 16 Oct 2023 02:43:45 +0900
Subject: [PATCH] refs: error out push if non-tracking remote branches exist

We can provide more actionable error message than "not fast-forwardable". If
the push was fast-forwardable, "jj branch track" should be able to merge the
remote branch without conflicts, so the added step would be minimal.
---
 CHANGELOG.md               |  3 +++
 cli/src/commands/git.rs    |  3 +++
 cli/tests/test_git_push.rs | 12 +++++-------
 lib/src/refs.rs            | 10 ++++++----
 4 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 717dcb42f..4306789ab 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -38,6 +38,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   See [automatic local branch creation](docs/config.md#automatic-local-branch-creation)
   for details.
 
+* It's not allowed to push branches if non-tracking remote branches of the same
+  name exist.
+
 * Pushing deleted/moved branches no longer abandons the local commits referenced
   by the remote branches.
 
diff --git a/cli/src/commands/git.rs b/cli/src/commands/git.rs
index 5812d9724..6eaf53b1b 100644
--- a/cli/src/commands/git.rs
+++ b/cli/src/commands/git.rs
@@ -1018,6 +1018,9 @@ fn classify_branch_update(
         BranchPushAction::RemoteConflicted => {
             Err(format!("Branch {branch_name}@{remote_name} is conflicted"))
         }
+        BranchPushAction::RemoteUntracked => Err(format!(
+            "Non-tracking remote branch {branch_name}@{remote_name} exists"
+        )),
         BranchPushAction::Update(update) => Ok(Some(update)),
     }
 }
diff --git a/cli/tests/test_git_push.rs b/cli/tests/test_git_push.rs
index 996ae802b..7deb4eb47 100644
--- a/cli/tests/test_git_push.rs
+++ b/cli/tests/test_git_push.rs
@@ -741,8 +741,8 @@ fn test_git_push_moved_forward_untracked() {
     test_env.jj_cmd_ok(&workspace_root, &["branch", "untrack", "branch1@origin"]);
     let (_stdout, stderr) = test_env.jj_cmd_ok(&workspace_root, &["git", "push"]);
     insta::assert_snapshot!(stderr, @r###"
-    Branch changes to push to origin:
-      Add branch branch1 to a25f24af0d0e
+    Non-tracking remote branch branch1@origin exists
+    Nothing changed.
     "###);
 }
 
@@ -756,12 +756,10 @@ fn test_git_push_moved_sideways_untracked() {
         &["branch", "set", "--allow-backwards", "branch1"],
     );
     test_env.jj_cmd_ok(&workspace_root, &["branch", "untrack", "branch1@origin"]);
-    let stderr = test_env.jj_cmd_failure(&workspace_root, &["git", "push"]);
+    let (_stdout, stderr) = test_env.jj_cmd_ok(&workspace_root, &["git", "push"]);
     insta::assert_snapshot!(stderr, @r###"
-    Branch changes to push to origin:
-      Add branch branch1 to cfbae7608334
-    Error: The push conflicts with changes made on the remote (it is not fast-forwardable).
-    Hint: Try fetching from the remote, then make the branch point to where you want it to be, and push again.
+    Non-tracking remote branch branch1@origin exists
+    Nothing changed.
     "###);
 }
 
diff --git a/lib/src/refs.rs b/lib/src/refs.rs
index 1b64a994f..6b68cd94b 100644
--- a/lib/src/refs.rs
+++ b/lib/src/refs.rs
@@ -132,6 +132,7 @@ pub enum BranchPushAction {
     AlreadyMatches,
     LocalConflicted,
     RemoteConflicted,
+    RemoteUntracked,
 }
 
 /// Figure out what changes (if any) need to be made to the remote when pushing
@@ -145,6 +146,8 @@ pub fn classify_branch_push_action(targets: TrackingRefPair) -> BranchPushAction
         BranchPushAction::LocalConflicted
     } else if remote_target.has_conflict() {
         BranchPushAction::RemoteConflicted
+    } else if targets.remote_ref.is_present() && !targets.remote_ref.is_tracking() {
+        BranchPushAction::RemoteUntracked
     } else {
         BranchPushAction::Update(BranchPushUpdate {
             old_target: remote_target.as_normal().cloned(),
@@ -237,6 +240,8 @@ mod tests {
 
     #[test]
     fn test_classify_branch_push_action_removed_untracked() {
+        // This is not RemoteUntracked error since non-tracking remote branches
+        // have no relation to local branches, and there's nothing to push.
         let commit_id1 = CommitId::from_hex("11");
         let targets = TrackingRefPair {
             local_target: RefTarget::absent_ref(),
@@ -258,10 +263,7 @@ mod tests {
         };
         assert_eq!(
             classify_branch_push_action(targets),
-            BranchPushAction::Update(BranchPushUpdate {
-                old_target: None,
-                new_target: Some(commit_id2),
-            })
+            BranchPushAction::RemoteUntracked
         );
     }