jj git push: safety checks when pushing branch sideways, similar to git push --force-with-lease

CHANGELOG.md

@@ -51,6 +51,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed bugs
 
+* Previously, `jj git push` only made sure that the branch is in the expected
+  location on the remote server when pushing a branch forward (as opposed to
+  sideways or backwards). Now, `jj git push` makes a safety check in all cases
+  and fails whenever `jj git fetch` would have introduced a conflict.
+
+  In other words, previously branches that moved sideways or backward were
+  pushed similarly to Git's `git push --force`; now they have protections
+  similar to `git push --force-with-lease` (though not identical to it, to match
+  the behavior of `jj git fetch`). Note also that because of the way `jj git
+  fetch` works, `jj` does not suffer from the same problems as Git's `git push
+  --force-with-lease` in situations when `git fetch` is run in the background.
+
 * When the working copy commit becomes immutable, a new one is automatically created on top of it 
 to avoid letting the user edit the immutable one.
 

cli/src/commands/git.rs

@@ -899,6 +899,10 @@ fn cmd_git_push(
     }
 
     let mut new_heads = vec![];
+    // Short-term TODO: `force_pushed_branches` no longer has an effect on how
+    // branches are actually pushed. Messages about "Moving" vs "Forcing"
+    // branches are now misleading. Change this logic so that we print whether
+    // the branch moved forward, backward, or sideways.
     let mut force_pushed_branches = hashset! {};
     for (branch_name, update) in &branch_updates {
         if let Some(new_target) = &update.new_target {
@@ -1001,10 +1005,7 @@ fn cmd_git_push(
         return Ok(());
     }
 
-    let targets = GitBranchPushTargets {
-        branch_updates,
-        force_pushed_branches,
-    };
+    let targets = GitBranchPushTargets { branch_updates };
     let mut writer = GitSidebandProgressMessageWriter::new(ui);
     let mut sideband_progress_callback = |progress_message: &[u8]| {
         _ = writer.write(ui, progress_message);
@@ -1014,8 +1015,12 @@ fn cmd_git_push(
     })
     .map_err(|err| match err {
         GitPushError::InternalGitError(err) => map_git_error(err),
-        GitPushError::NotFastForward => user_error_with_hint(
-            "The push conflicts with changes made on the remote (it is not fast-forwardable).",
+        GitPushError::RefInUnexpectedLocation(refs) => user_error_with_hint(
+            format!(
+                "Refusing to push a branch that unexpectedly moved on the remote. Affected refs: \
+                 {}",
+                refs.join(", ")
+            ),
             "Try fetching from the remote, then make the branch point to where you want it to be, \
              and push again.",
         ),

cli/tests/test_git_push.rs

@@ -14,7 +14,7 @@
 
 use std::path::{Path, PathBuf};
 
-use crate::common::{get_stderr_string, get_stdout_string, TestEnvironment};
+use crate::common::TestEnvironment;
 
 fn set_up() -> (TestEnvironment, PathBuf) {
     let test_env = TestEnvironment::default();
@@ -196,7 +196,14 @@ fn test_git_push_other_remote_has_branch() {
     Warning: No branches found in the default push revset: remote_branches(remote=origin)..@
     Nothing changed.
     "###);
-    // But it will still get pushed to another remote
+    // The branch was moved on the "other" remote as well (since it's actually the
+    // same remote), but `jj` is not aware of that since it thinks this is a
+    // different remote. So, the push should fail.
+    //
+    // But it succeeds! That's because the branch is created at the same location as
+    // it is on the remote. This would also work for a descendant.
+    //
+    // TODO: Saner test?
     let (stdout, stderr) = test_env.jj_cmd_ok(&workspace_root, &["git", "push", "--remote=other"]);
     insta::assert_snapshot!(stdout, @"");
     insta::assert_snapshot!(stderr, @r###"
@@ -206,7 +213,7 @@ fn test_git_push_other_remote_has_branch() {
 }
 
 #[test]
-fn test_git_push_not_fast_forward() {
+fn test_git_push_forward_unexpectedly_moved() {
     let (test_env, workspace_root) = set_up();
 
     // Move branch1 forward on the remote
@@ -222,15 +229,114 @@ fn test_git_push_not_fast_forward() {
     test_env.jj_cmd_ok(&workspace_root, &["branch", "set", "branch1"]);
 
     // Pushing should fail
-    let assert = test_env
-        .jj_cmd(&workspace_root, &["git", "push"])
-        .assert()
-        .code(1);
-    insta::assert_snapshot!(get_stdout_string(&assert), @"");
-    insta::assert_snapshot!(get_stderr_string(&assert), @r###"
+    let stderr = test_env.jj_cmd_failure(&workspace_root, &["git", "push"]);
+    insta::assert_snapshot!(stderr, @r###"
     Branch changes to push to origin:
       Move branch branch1 from 45a3aa29e907 to c35839cb8e8c
-    Error: The push conflicts with changes made on the remote (it is not fast-forwardable).
+    Error: Refusing to push a branch that unexpectedly moved on the remote. Affected refs: refs/heads/branch1
+    Hint: Try fetching from the remote, then make the branch point to where you want it to be, and push again.
+    "###);
+}
+
+#[test]
+fn test_git_push_sideways_unexpectedly_moved() {
+    let (test_env, workspace_root) = set_up();
+
+    // Move branch1 forward on the remote
+    let origin_path = test_env.env_root().join("origin");
+    test_env.jj_cmd_ok(&origin_path, &["new", "branch1", "-m=remote"]);
+    std::fs::write(origin_path.join("remote"), "remote").unwrap();
+    test_env.jj_cmd_ok(&origin_path, &["branch", "set", "branch1"]);
+    insta::assert_snapshot!(get_branch_output(&test_env, &origin_path), @r###"
+    branch1: vruxwmqv fb645b4b remote
+      @git (behind by 1 commits): qpvuntsm 45a3aa29 (empty) description 1
+    branch2: zsuskuln 8476341e (empty) description 2
+      @git: zsuskuln 8476341e (empty) description 2
+    "###);
+    test_env.jj_cmd_ok(&origin_path, &["git", "export"]);
+
+    // Move branch1 sideways to another commit locally
+    test_env.jj_cmd_ok(&workspace_root, &["new", "root()", "-m=local"]);
+    std::fs::write(workspace_root.join("local"), "local").unwrap();
+    test_env.jj_cmd_ok(
+        &workspace_root,
+        &["branch", "set", "branch1", "--allow-backwards"],
+    );
+    insta::assert_snapshot!(get_branch_output(&test_env, &workspace_root), @r###"
+    branch1: kmkuslsw 0f8bf988 local
+      @origin (ahead by 1 commits, behind by 1 commits): lzmmnrxq 45a3aa29 (empty) description 1
+    branch2: rlzusymt 8476341e (empty) description 2
+      @origin: rlzusymt 8476341e (empty) description 2
+    "###);
+
+    let stderr = test_env.jj_cmd_failure(&workspace_root, &["git", "push"]);
+    insta::assert_snapshot!(stderr, @r###"
+    Branch changes to push to origin:
+      Force branch branch1 from 45a3aa29e907 to 0f8bf988588e
+    Error: Refusing to push a branch that unexpectedly moved on the remote. Affected refs: refs/heads/branch1
+    Hint: Try fetching from the remote, then make the branch point to where you want it to be, and push again.
+    "###);
+}
+
+// This tests whether the push checks that the remote branches are in expected
+// positions.
+#[test]
+fn test_git_push_deletion_unexpectedly_moved() {
+    let (test_env, workspace_root) = set_up();
+
+    // Move branch1 forward on the remote
+    let origin_path = test_env.env_root().join("origin");
+    test_env.jj_cmd_ok(&origin_path, &["new", "branch1", "-m=remote"]);
+    std::fs::write(origin_path.join("remote"), "remote").unwrap();
+    test_env.jj_cmd_ok(&origin_path, &["branch", "set", "branch1"]);
+    insta::assert_snapshot!(get_branch_output(&test_env, &origin_path), @r###"
+    branch1: vruxwmqv fb645b4b remote
+      @git (behind by 1 commits): qpvuntsm 45a3aa29 (empty) description 1
+    branch2: zsuskuln 8476341e (empty) description 2
+      @git: zsuskuln 8476341e (empty) description 2
+    "###);
+    test_env.jj_cmd_ok(&origin_path, &["git", "export"]);
+
+    // Delete branch1 locally
+    test_env.jj_cmd_ok(&workspace_root, &["branch", "delete", "branch1"]);
+    insta::assert_snapshot!(get_branch_output(&test_env, &workspace_root), @r###"
+    branch1 (deleted)
+      @origin: lzmmnrxq 45a3aa29 (empty) description 1
+    branch2: rlzusymt 8476341e (empty) description 2
+      @origin: rlzusymt 8476341e (empty) description 2
+    "###);
+
+    let stderr = test_env.jj_cmd_failure(&workspace_root, &["git", "push", "--branch", "branch1"]);
+    insta::assert_snapshot!(stderr, @r###"
+    Branch changes to push to origin:
+      Delete branch branch1 from 45a3aa29e907
+    Error: Refusing to push a branch that unexpectedly moved on the remote. Affected refs: refs/heads/branch1
+    Hint: Try fetching from the remote, then make the branch point to where you want it to be, and push again.
+    "###);
+}
+
+#[test]
+fn test_git_push_creation_unexpectedly_already_exists() {
+    let (test_env, workspace_root) = set_up();
+
+    // Forget branch1 locally
+    test_env.jj_cmd_ok(&workspace_root, &["branch", "forget", "branch1"]);
+
+    // Create a new branh1
+    test_env.jj_cmd_ok(&workspace_root, &["new", "root()", "-m=new branch1"]);
+    std::fs::write(workspace_root.join("local"), "local").unwrap();
+    test_env.jj_cmd_ok(&workspace_root, &["branch", "create", "branch1"]);
+    insta::assert_snapshot!(get_branch_output(&test_env, &workspace_root), @r###"
+    branch1: yostqsxw 4c595cf9 new branch1
+    branch2: rlzusymt 8476341e (empty) description 2
+      @origin: rlzusymt 8476341e (empty) description 2
+    "###);
+
+    let stderr = test_env.jj_cmd_failure(&workspace_root, &["git", "push"]);
+    insta::assert_snapshot!(stderr, @r###"
+    Branch changes to push to origin:
+      Add branch branch1 to 4c595cf9ac0a
+    Error: Refusing to push a branch that unexpectedly moved on the remote. Affected refs: refs/heads/branch1
     Hint: Try fetching from the remote, then make the branch point to where you want it to be, and push again.
     "###);
 }