jj git push: safety checks in push negotiation, "force-with-lease"

As explained in the commit, our logic is a bit more complicated than
that of `git push --force-with-lease`. This is to match the behavior of
`jj git fetch` and branch conflict resolution rules.

CHANGELOG.md

@@ -53,6 +53,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

@@ -1019,6 +1019,15 @@ fn cmd_git_push(
             "Try fetching from the remote, then make the branch point to where you want it to be, \
              and push again.",
         ),
+        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.",
+        ),
         _ => user_error(err),
     })?;
     writer.flush(ui)?;

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
@@ -226,15 +233,11 @@ fn test_git_push_not_fast_forward() {
     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.
     "###);
 }
 
-// Short-term TODO: implement this.
-// This tests whether the push checks that the remote branches are in expected
-// positions. Once this is implemented, `jj git push` will be similar to `git
-// push --force-with-lease`
 #[test]
 fn test_git_push_sideways_unexpectedly_moved() {
     let (test_env, workspace_root) = set_up();
@@ -266,23 +269,17 @@ fn test_git_push_sideways_unexpectedly_moved() {
       @origin: rlzusymt 8476341e (empty) description 2
     "###);
 
-    // BUG: Pushing should fail. Currently, it succeeds because moving the branch
+    let stderr = test_env.jj_cmd_failure(&workspace_root, &["git", "push"]);
-    // sideways causes `jj` to use the analogue of `git push --force` when pushing.
+    insta::assert_snapshot!(stderr, @r###"
-    let assert = test_env
-        .jj_cmd(&workspace_root, &["git", "push"])
-        .assert()
-        .success();
-    insta::assert_snapshot!(get_stdout_string(&assert), @"");
-    insta::assert_snapshot!(get_stderr_string(&assert), @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.
     "###);
 }
 
-// Short-term TODO: implement this.
 // This tests whether the push checks that the remote branches are in expected
-// positions. Once this is implemented, `jj git push` will be similar to `git
+// positions.
-// push --force-with-lease`
 #[test]
 fn test_git_push_deletion_unexpectedly_moved() {
     let (test_env, workspace_root) = set_up();
@@ -309,15 +306,12 @@ fn test_git_push_deletion_unexpectedly_moved() {
       @origin: rlzusymt 8476341e (empty) description 2
     "###);
 
-    // BUG: Pushing should fail because the branch was moved on the remote
+    let stderr = test_env.jj_cmd_failure(&workspace_root, &["git", "push", "--branch", "branch1"]);
-    let assert = test_env
+    insta::assert_snapshot!(stderr, @r###"
-        .jj_cmd(&workspace_root, &["git", "push", "--branch", "branch1"])
-        .assert()
-        .success();
-    insta::assert_snapshot!(get_stdout_string(&assert), @"");
-    insta::assert_snapshot!(get_stderr_string(&assert), @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.
     "###);
 }
 
@@ -342,7 +336,7 @@ fn test_git_push_creation_unexpectedly_already_exists() {
     insta::assert_snapshot!(stderr, @r###"
     Branch changes to push to origin:
       Add branch branch1 to 4c595cf9ac0a
-    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.
     "###);
 }

lib/src/git.rs

@@ -29,9 +29,10 @@ use thiserror::Error;
 use crate::backend::{BackendError, CommitId};
 use crate::commit::Commit;
 use crate::git_backend::GitBackend;
+use crate::index::Index;
 use crate::object_id::ObjectId;
 use crate::op_store::{RefTarget, RefTargetOptionExt, RemoteRef, RemoteRefState};
-use crate::refs::BranchPushUpdate;
+use crate::refs::{self, BranchPushUpdate};
 use crate::repo::{MutableRepo, Repo};
 use crate::revset::RevsetExpression;
 use crate::settings::GitSettings;
@@ -1224,6 +1225,8 @@ pub enum GitPushError {
     RemoteReservedForLocalGitRepo,
     #[error("Push is not fast-forwardable")]
     NotFastForward,
+    #[error("Refs in unexpected location: {0:?}")]
+    RefInUnexpectedLocation(Vec<String>),
     #[error("Remote rejected the update of some refs (do you have permission to push to {0:?}?)")]
     RefUpdateRejected(Vec<String>),
     // TODO: I'm sure there are other errors possible, such as transport-level errors,
@@ -1268,7 +1271,7 @@ pub fn push_branches(
             new_target: update.new_target.clone(),
         })
         .collect_vec();
-    push_updates(git_repo, remote_name, &ref_updates, callbacks)?;
+    push_updates(mut_repo, git_repo, remote_name, &ref_updates, callbacks)?;
 
     // TODO: add support for partially pushed refs? we could update the view
     // excluding rejected refs, but the transaction would be aborted anyway
@@ -1288,6 +1291,7 @@ pub fn push_branches(
 
 /// Pushes the specified Git refs without updating the repo view.
 pub fn push_updates(
+    repo: &dyn Repo,
     git_repo: &git2::Repository,
     remote_name: &str,
     updates: &[GitRefUpdate],
@@ -1311,10 +1315,10 @@ pub fn push_updates(
             refspecs.push(format!(":{}", update.qualified_name));
         }
     }
-    // TODO(ilyagr): this function will be reused to implement force-with-lease, so
+    // TODO(ilyagr): `push_refs`, or parts of it, should probably be inlined. This
-    // don't inline for now. If it's after 2024-06-01 or so, ilyagr may have
+    // requires adjusting some tests.
-    // forgotten to delete this comment.
     push_refs(
+        repo,
         git_repo,
         remote_name,
         &qualified_remote_refs_expected_locations,
@@ -1324,6 +1328,7 @@ pub fn push_updates(
 }
 
 fn push_refs(
+    repo: &dyn Repo,
     git_repo: &git2::Repository,
     remote_name: &str,
     qualified_remote_refs_expected_locations: &HashMap<&str, Option<&CommitId>>,
@@ -1344,67 +1349,183 @@ fn push_refs(
         .keys()
         .copied()
         .collect();
-    let mut push_options = git2::PushOptions::new();
+    let mut failed_push_negotiations = vec![];
-    let mut proxy_options = git2::ProxyOptions::new();
+    let push_result = {
-    proxy_options.auto();
+        let mut push_options = git2::PushOptions::new();
-    push_options.proxy_options(proxy_options);
+        let mut proxy_options = git2::ProxyOptions::new();
-    let mut callbacks = callbacks.into_git();
+        proxy_options.auto();
-    callbacks.push_negotiation(|updates| {
+        push_options.proxy_options(proxy_options);
-        for update in updates {
+        let mut callbacks = callbacks.into_git();
-            let dst_refname = update
+        callbacks.push_negotiation(|updates| {
-                .dst_refname()
+            for update in updates {
-                .expect("Expect reference name to be valid UTF-8");
+                let dst_refname = update
-            let expected_remote_location = *qualified_remote_refs_expected_locations
+                    .dst_refname()
-                .get(dst_refname)
+                    .expect("Expect reference name to be valid UTF-8");
-                .expect("Push is trying to move a ref it wasn't asked to move");
+                let expected_remote_location = *qualified_remote_refs_expected_locations
+                    .get(dst_refname)
+                    .expect("Push is trying to move a ref it wasn't asked to move");
+                let oid_to_maybe_commitid =
+                    |oid: git2::Oid| (!oid.is_zero()).then(|| CommitId::from_bytes(oid.as_bytes()));
+                let actual_remote_location = oid_to_maybe_commitid(update.src());
+                let local_location = oid_to_maybe_commitid(update.dst());
 
-            // `update.src()` is the current actual position of the branch
+                match allow_push(
-            // `dst_refname` on the remote. `update.dst()` is the position
+                    repo.index(),
-            // we are trying to push the branch to (AKA our local branch
+                    actual_remote_location.as_ref(),
-            // location). 0000000000 is a valid value for either, indicating
+                    expected_remote_location,
-            // branch creation or deletion.
+                    local_location.as_ref(),
-            let oid_to_maybe_commitid =
+                ) {
-                |oid: git2::Oid| (!oid.is_zero()).then(|| CommitId::from_bytes(oid.as_bytes()));
+                    Ok(PushAllowReason::NormalMatch) => {}
-            let actual_remote_location = oid_to_maybe_commitid(update.src());
+                    Ok(PushAllowReason::UnexpectedNoop) => {
-            let local_location = oid_to_maybe_commitid(update.dst());
+                        tracing::info!(
+                            "The push of {dst_refname} is unexpectedly a no-op, the remote branch \
+                             is already at {actual_remote_location:?}. We expected it to be at \
+                             {expected_remote_location:?}. We don't consider this an error.",
+                        );
+                    }
+                    Ok(PushAllowReason::ExceptionalFastforward) => {
+                        // TODO(ilyagr): We could consider printing a user-facing message at
+                        // this point.
+                        tracing::info!(
+                            "We allow the push of {dst_refname} to {local_location:?}, even \
+                             though it is unexpectedly at {actual_remote_location:?} on the \
+                             server rather than the expected {expected_remote_location:?}. The \
+                             desired location is a descendant of the actual location, and the \
+                             actual location is a descendant of the expected location.",
+                        );
+                    }
+                    Err(()) => {
+                        // While we show debug info in the message with `--debug`,
+                        // there's probably no need to show the detailed commit
+                        // locations to the user normally. They should do a `jj git
+                        // fetch`, and the resulting branch conflicts should contain
+                        // all the information they need.
+                        tracing::info!(
+                            "Cannot push {dst_refname} to {local_location:?}; it is at \
+                             unexpectedly at {actual_remote_location:?} on the server as opposed \
+                             to the expected {expected_remote_location:?}",
+                        );
 
-            // Short-term TODO: Replace this with actual rules of when to permit the push
+                        failed_push_negotiations.push(dst_refname.to_string());
-            tracing::info!(
+                    }
-                "Forcing {dst_refname} to {dst:?}. It was at {src:?} at the server. We expected \
+                }
-                 it to be at {expected_remote_location:?}.",
-                src = actual_remote_location,
-                dst = local_location
-            );
-        }
-        Ok(())
-    });
-    callbacks.push_update_reference(|refname, status| {
-        // The status is Some if the ref update was rejected
-        if status.is_none() {
-            remaining_remote_refs.remove(refname);
-        }
-        Ok(())
-    });
-    push_options.remote_callbacks(callbacks);
-    remote
-        .push(refspecs, Some(&mut push_options))
-        .map_err(|err| match (err.class(), err.code()) {
-            (git2::ErrorClass::Reference, git2::ErrorCode::NotFastForward) => {
-                GitPushError::NotFastForward
             }
-            _ => GitPushError::InternalGitError(err),
+            if failed_push_negotiations.is_empty() {
-        })?;
+                Ok(())
-    drop(push_options);
+            } else {
-    if remaining_remote_refs.is_empty() {
+                Err(git2::Error::from_str("failed push negotiation"))
-        Ok(())
+            }
-    } else {
+        });
-        Err(GitPushError::RefUpdateRejected(
+        callbacks.push_update_reference(|refname, status| {
-            remaining_remote_refs
+            // The status is Some if the ref update was rejected
-                .iter()
+            if status.is_none() {
-                .sorted()
+                remaining_remote_refs.remove(refname);
-                .map(|name| name.to_string())
+            }
-                .collect(),
+            Ok(())
+        });
+        push_options.remote_callbacks(callbacks);
+        remote
+            .push(refspecs, Some(&mut push_options))
+            .map_err(|err| match (err.class(), err.code()) {
+                (git2::ErrorClass::Reference, git2::ErrorCode::NotFastForward) => {
+                    GitPushError::NotFastForward
+                }
+                _ => GitPushError::InternalGitError(err),
+            })
+    };
+    if !failed_push_negotiations.is_empty() {
+        // If the push negotiation returned an error, `remote.push` would not
+        // have pushed anything and would have returned an error, as expected.
+        // However, the error it returns is not necessarily the error we'd
+        // expect. It also depends on the exact versions of `libgit2` and
+        // `git2.rs`. So, we cannot rely on it containing any useful
+        // information. See https://github.com/rust-lang/git2-rs/issues/1042.
+        assert!(push_result.is_err());
+        failed_push_negotiations.sort();
+        Err(GitPushError::RefInUnexpectedLocation(
+            failed_push_negotiations,
         ))
+    } else {
+        push_result?;
+        if remaining_remote_refs.is_empty() {
+            Ok(())
+        } else {
+            Err(GitPushError::RefUpdateRejected(
+                remaining_remote_refs
+                    .iter()
+                    .sorted()
+                    .map(|name| name.to_string())
+                    .collect(),
+            ))
+        }
+    }
+}
+
+#[derive(Debug, Clone, PartialEq, Eq)]
+enum PushAllowReason {
+    NormalMatch,
+    ExceptionalFastforward,
+    UnexpectedNoop,
+}
+
+fn allow_push(
+    index: &dyn Index,
+    actual_remote_location: Option<&CommitId>,
+    expected_remote_location: Option<&CommitId>,
+    destination_location: Option<&CommitId>,
+) -> Result<PushAllowReason, ()> {
+    if actual_remote_location == expected_remote_location {
+        return Ok(PushAllowReason::NormalMatch);
+    }
+
+    // If the remote ref is in an unexpected location, we still allow some
+    // pushes, based on whether `jj git fetch` would result in a conflicted ref.
+    //
+    // For `merge_ref_targets` to work correctly, `actual_remote_location` must
+    // be a commit that we locally know about.
+    //
+    // This does not lose any generality since for `merge_ref_targets` to
+    // resolve to `local_target` below, it is conceptually necessary (but not
+    // sufficient) for the destination_location to be either a descendant of
+    // actual_remote_location or equal to it. Either way, we would know about that
+    // commit locally.
+    if !actual_remote_location.map_or(true, |id| index.has_id(id)) {
+        return Err(());
+    }
+    let remote_target = RefTarget::resolved(actual_remote_location.cloned());
+    let base_target = RefTarget::resolved(expected_remote_location.cloned());
+    // The push destination is the local position of the ref
+    let local_target = RefTarget::resolved(destination_location.cloned());
+    if refs::merge_ref_targets(index, &remote_target, &base_target, &local_target) == local_target {
+        // Fetch would not change the local branch, so the push is OK in spite of
+        // the discrepancy with the expected location. We return some debug info and
+        // verify some invariants before OKing the push.
+        Ok(if actual_remote_location == destination_location {
+            // This is the situation of what we call "A - B + A = A"
+            // conflicts, see also test_refs.rs and
+            // https://github.com/martinvonz/jj/blob/c9b44f382824301e6c0fdd6f4cbc52bb00c50995/lib/src/merge.rs#L92.
+            PushAllowReason::UnexpectedNoop
+        } else {
+            // The assertions follow from our ref merge rules:
+            //
+            // 1. This is a fast-forward.
+            debug_assert!(index.is_ancestor(
+                actual_remote_location.as_ref().unwrap(),
+                destination_location.as_ref().unwrap()
+            ));
+            // 2. The expected location is an ancestor of both the actual location and the
+            //    destination (local position).
+            debug_assert!(
+                expected_remote_location.is_none()
+                    || index.is_ancestor(
+                        expected_remote_location.unwrap(),
+                        actual_remote_location.as_ref().unwrap()
+                    )
+            );
+            PushAllowReason::ExceptionalFastforward
+        })
+    } else {
+        Err(())
     }
 }
 

lib/tests/test_git.rs

@@ -2485,7 +2485,7 @@ struct PushTestSetup {
     jj_repo: Arc<ReadonlyRepo>,
     main_commit: Commit,
     child_of_main_commit: Commit,
-    _parent_of_main_commit: Commit,
+    parent_of_main_commit: Commit,
     sideways_commit: Commit,
 }
 
@@ -2568,7 +2568,7 @@ fn set_up_push_repos(settings: &UserSettings, temp_dir: &TempDir) -> PushTestSet
         jj_repo,
         main_commit,
         child_of_main_commit,
-        _parent_of_main_commit: parent_of_main_commit,
+        parent_of_main_commit,
         sideways_commit,
     }
 }
@@ -2826,6 +2826,182 @@ fn test_push_branches_not_fast_forward_with_force() {
     assert_eq!(new_target, Some(git_id(&setup.sideways_commit)));
 }
 
+// TODO(ilyagr): More tests for push safety checks were originally planned. We
+// may want to add tests for when a branch unexpectedly moved backwards or
+// unexpectedly does not exist for branch deletion.
+
+#[test]
+fn test_push_updates_unexpectedly_moved_sideways_on_remote() {
+    let settings = testutils::user_settings();
+    let temp_dir = testutils::new_temp_dir();
+    let setup = set_up_push_repos(&settings, &temp_dir);
+
+    // The main branch is actually at `main_commit` on the remote. If we expect
+    // it to be at `sideways_commit`, it unexpectedly moved sideways from our
+    // perspective.
+    //
+    // We cannot delete it or move it anywhere else. However, "moving" it to the
+    // same place it already is is OK, following the behavior in
+    // `test_merge_ref_targets`.
+    //
+    // For each test, we check that the push succeeds if and only if the branch
+    // conflict `jj git fetch` would generate resolves to the push destination.
+
+    let attempt_push_expecting_sideways = |target: Option<CommitId>| {
+        let targets = [GitRefUpdate {
+            qualified_name: "refs/heads/main".to_string(),
+            force: true,
+            expected_current_target: Some(setup.sideways_commit.id().clone()),
+            new_target: target,
+        }];
+        git::push_updates(
+            setup.jj_repo.as_ref(),
+            &get_git_repo(&setup.jj_repo),
+            "origin",
+            &targets,
+            git::RemoteCallbacks::default(),
+        )
+    };
+
+    assert_matches!(
+        attempt_push_expecting_sideways(None),
+        Err(GitPushError::RefInUnexpectedLocation(_))
+    );
+
+    assert_matches!(
+        attempt_push_expecting_sideways(Some(setup.child_of_main_commit.id().clone())),
+        Err(GitPushError::RefInUnexpectedLocation(_))
+    );
+
+    // Here, the local branch hasn't moved from `sideways_commit` from our
+    // perspective, but it moved to `main` on the remote. So, the conflict
+    // resolves to `main`.
+    //
+    // `jj` should not actually attempt a push in this case, but if it did, the
+    // push should fail.
+    assert_matches!(
+        attempt_push_expecting_sideways(Some(setup.sideways_commit.id().clone())),
+        Err(GitPushError::RefInUnexpectedLocation(_))
+    );
+
+    assert_matches!(
+        attempt_push_expecting_sideways(Some(setup.parent_of_main_commit.id().clone())),
+        Err(GitPushError::RefInUnexpectedLocation(_))
+    );
+
+    // Moving the branch to the same place it already is is OK.
+    assert_eq!(
+        attempt_push_expecting_sideways(Some(setup.main_commit.id().clone())),
+        Ok(())
+    );
+}
+
+#[test]
+fn test_push_updates_unexpectedly_moved_forward_on_remote() {
+    let settings = testutils::user_settings();
+    let temp_dir = testutils::new_temp_dir();
+    let setup = set_up_push_repos(&settings, &temp_dir);
+
+    // The main branch is actually at `main_commit` on the remote. If we
+    // expected it to be at `parent_of_commit`, it unexpectedly moved forward
+    // from our perspective.
+    //
+    // We cannot delete it or move it sideways. (TODO: Moving it backwards is
+    // also disallowed; there is currently no test for this). However, "moving"
+    // it *forwards* is OK. This is allowed *only* in this test, i.e. if the
+    // actual location is the descendant of the expected location, and the new
+    // location is the descendant of that.
+    //
+    // For each test, we check that the push succeeds if and only if the branch
+    // conflict `jj git fetch` would generate resolves to the push destination.
+
+    let attempt_push_expecting_parent = |target: Option<CommitId>| {
+        let targets = [GitRefUpdate {
+            qualified_name: "refs/heads/main".to_string(),
+            force: true,
+            expected_current_target: Some(setup.parent_of_main_commit.id().clone()),
+            new_target: target,
+        }];
+        git::push_updates(
+            setup.jj_repo.as_ref(),
+            &get_git_repo(&setup.jj_repo),
+            "origin",
+            &targets,
+            git::RemoteCallbacks::default(),
+        )
+    };
+
+    assert_matches!(
+        attempt_push_expecting_parent(None),
+        Err(GitPushError::RefInUnexpectedLocation(_))
+    );
+
+    assert_matches!(
+        attempt_push_expecting_parent(Some(setup.sideways_commit.id().clone())),
+        Err(GitPushError::RefInUnexpectedLocation(_))
+    );
+
+    // Here, the local branch hasn't moved from `parent_of_main_commit`, but it
+    // moved to `main` on the remote. So, the conflict resolves to `main`.
+    //
+    // `jj` should not actually attempt a push in this case, but if it did, the push
+    // should fail.
+    assert_matches!(
+        attempt_push_expecting_parent(Some(setup.parent_of_main_commit.id().clone())),
+        Err(GitPushError::RefInUnexpectedLocation(_))
+    );
+
+    // Moving the branch *forwards* is OK, as an exception matching our branch
+    // conflict resolution rules
+    assert_eq!(
+        attempt_push_expecting_parent(Some(setup.child_of_main_commit.id().clone())),
+        Ok(())
+    );
+}
+
+#[test]
+fn test_push_updates_unexpectedly_exists_on_remote() {
+    let settings = testutils::user_settings();
+    let temp_dir = testutils::new_temp_dir();
+    let setup = set_up_push_repos(&settings, &temp_dir);
+
+    // The main branch is actually at `main_commit` on the remote. In this test, we
+    // expect it to not exist on the remote at all.
+    //
+    // We cannot move the branch backwards or sideways, but we *can* move it forward
+    // (as a special case).
+    //
+    // For each test, we check that the push succeeds if and only if the branch
+    // conflict `jj git fetch` would generate resolves to the push destination.
+
+    let attempt_push_expecting_absence = |target: Option<CommitId>| {
+        let targets = [GitRefUpdate {
+            qualified_name: "refs/heads/main".to_string(),
+            force: true,
+            expected_current_target: None,
+            new_target: target,
+        }];
+        git::push_updates(
+            setup.jj_repo.as_ref(),
+            &get_git_repo(&setup.jj_repo),
+            "origin",
+            &targets,
+            git::RemoteCallbacks::default(),
+        )
+    };
+
+    assert_matches!(
+        attempt_push_expecting_absence(Some(setup.parent_of_main_commit.id().clone())),
+        Err(GitPushError::RefInUnexpectedLocation(_))
+    );
+
+    // We *can* move the branch forward even if we didn't expect it to exist
+    assert_eq!(
+        attempt_push_expecting_absence(Some(setup.child_of_main_commit.id().clone())),
+        Ok(())
+    );
+}
+
 #[test]
 fn test_push_updates_success() {
     let settings = testutils::user_settings();
@@ -2833,6 +3009,7 @@ fn test_push_updates_success() {
     let setup = set_up_push_repos(&settings, &temp_dir);
     let clone_repo = get_git_repo(&setup.jj_repo);
     let result = git::push_updates(
+        setup.jj_repo.as_ref(),
         &clone_repo,
         "origin",
         &[GitRefUpdate {
@@ -2870,6 +3047,7 @@ fn test_push_updates_no_such_remote() {
     let temp_dir = testutils::new_temp_dir();
     let setup = set_up_push_repos(&settings, &temp_dir);
     let result = git::push_updates(
+        setup.jj_repo.as_ref(),
         &get_git_repo(&setup.jj_repo),
         "invalid-remote",
         &[GitRefUpdate {
@@ -2889,6 +3067,7 @@ fn test_push_updates_invalid_remote() {
     let temp_dir = testutils::new_temp_dir();
     let setup = set_up_push_repos(&settings, &temp_dir);
     let result = git::push_updates(
+        setup.jj_repo.as_ref(),
         &get_git_repo(&setup.jj_repo),
         "http://invalid-remote",
         &[GitRefUpdate {

lib/tests/test_refs.rs

@@ -89,12 +89,29 @@ fn test_merge_ref_targets() {
         target4
     );
 
-    // Both added same target
+    // Both moved sideways ("A - B + A" - type conflict)
+    assert_eq!(
+        merge_ref_targets(index, &target4, &target3, &target4),
+        target4
+    );
+
+    // Both added same target ("A - B + A" - type conflict)
     assert_eq!(
         merge_ref_targets(index, &target3, RefTarget::absent_ref(), &target3),
         target3
     );
 
+    // Both removed ("A - B + A" - type conflict)
+    assert_eq!(
+        merge_ref_targets(
+            index,
+            RefTarget::absent_ref(),
+            &target3,
+            RefTarget::absent_ref()
+        ),
+        RefTarget::absent()
+    );
+
     // Left added target, right added descendant target
     assert_eq!(
         merge_ref_targets(index, &target2, RefTarget::absent_ref(), &target3),