Actual push negotiation TODOs: can NotFastForward happen? Remove `for…

…ce`?

Thanks to rust-lang/git2-rs#926 and @karin0 for
making this possible.

cli/src/commands/git.rs

@@ -1014,11 +1014,13 @@ 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).",
-            "Try fetching from the remote, then make the branch point to where you want it to be, \
-             and push again.",
-        ),
+        GitPushError::NotFastForward | GitPushError::RefInUnexpectedLocation(_) => {
+            user_error_with_hint(
+                err.to_string(),
+                "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

@@ -199,12 +199,15 @@ 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
-    let (stdout, stderr) = test_env.jj_cmd_ok(&workspace_root, &["git", "push", "--remote=other"]);
-    insta::assert_snapshot!(stdout, @"");
+    // 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 fails.
+    let stderr = test_env.jj_cmd_failure(&workspace_root, &["git", "push", "--remote=other"]);
     insta::assert_snapshot!(stderr, @r###"
     Branch changes to push to other:
       Add branch branch1 to 50421a29358a
+    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.
     "###);
 }
 
@@ -233,15 +236,11 @@ fn test_git_push_not_fast_forward() {
     insta::assert_snapshot!(get_stderr_string(&assert), @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();
@@ -283,16 +282,16 @@ fn test_git_push_sideways_unexpectedly_moved() {
     "###);
     insta::assert_snapshot!(get_stderr_string(&assert), @"");
 
-    // BUG: Pushing should fail. Currently, it succeeds because moving the branch
-    // sideways causes `jj` to use the analogue of `git push --force` when pushing.
     let assert = test_env
         .jj_cmd(&workspace_root, &["git", "push"])
         .assert()
-        .success();
+        .failure();
     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.
     "###);
 }
 
@@ -341,11 +340,13 @@ fn test_git_push_deletion_unexpectedly_moved() {
     let assert = test_env
         .jj_cmd(&workspace_root, &["git", "push", "--branch", "branch1"])
         .assert()
-        .success();
+        .failure();
     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.
     "###);
 }
 
@@ -379,7 +380,7 @@ fn test_git_push_creation_unexpectedly_already_exists() {
     insta::assert_snapshot!(get_stderr_string(&assert), @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

@@ -1208,8 +1208,16 @@ pub enum GitPushError {
         name = REMOTE_NAME_FOR_LOCAL_GIT_REPO
     )]
     RemoteReservedForLocalGitRepo,
-    #[error("Push is not fast-forwardable")]
+    #[error("The push conflicts with changes made on the remote (it is not fast-forwardable).")]
     NotFastForward,
+    #[error("Refusing to push a branch that unexpectedly moved on the remote. Affected refs: {:?}",
+    .0
+                .iter()
+                .sorted()
+                .map(|option| option.clone().unwrap_or_default())
+                .collect_vec()
+)]
+    RefInUnexpectedLocation(Vec<Option<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,
@@ -1275,11 +1283,15 @@ pub fn push_updates(
     updates: &[GitRefUpdate],
     callbacks: RemoteCallbacks<'_>,
 ) -> Result<(), GitPushError> {
-    let mut qualified_remote_refs = vec![];
+    let mut qualified_remote_refs_expected_locations = HashMap::new();
     let mut refspecs = vec![];
     for update in updates {
-        qualified_remote_refs.push(update.qualified_name.as_str());
+        qualified_remote_refs_expected_locations.insert(
+            update.qualified_name.as_str(),
+            &update.expected_current_target,
+        );
         if let Some(new_target) = &update.new_target {
+            // TODO: What branches should be force-pushed after force-with-lease??
             refspecs.push(format!(
                 "{}{}:{}",
                 (if update.force { "+" } else { "" }),
@@ -1296,7 +1308,7 @@ pub fn push_updates(
     push_refs(
         git_repo,
         remote_name,
-        &qualified_remote_refs,
+        &qualified_remote_refs_expected_locations,
         &refspecs,
         callbacks,
     )
@@ -1305,49 +1317,103 @@ pub fn push_updates(
 fn push_refs(
     git_repo: &git2::Repository,
     remote_name: &str,
-    qualified_remote_refs: &[&str],
+    qualified_remote_refs_expected_locations: &HashMap<&str, &Option<CommitId>>,
+    // expected_remote_ref_locations
     refspecs: &[String],
     callbacks: RemoteCallbacks<'_>,
 ) -> Result<(), GitPushError> {
-    if remote_name == REMOTE_NAME_FOR_LOCAL_GIT_REPO {
-        return Err(GitPushError::RemoteReservedForLocalGitRepo);
-    }
-    let mut remote = git_repo.find_remote(remote_name).map_err(|err| {
-        if is_remote_not_found_err(&err) {
-            GitPushError::NoSuchRemote(remote_name.to_string())
-        } else {
-            GitPushError::InternalGitError(err)
+    let mut remaining_remote_refs_expected_locations =
+        qualified_remote_refs_expected_locations.clone();
+    let mut failed_push_negotiations = vec![];
+    let push_result = {
+        if remote_name == REMOTE_NAME_FOR_LOCAL_GIT_REPO {
+            return Err(GitPushError::RemoteReservedForLocalGitRepo);
         }
-    })?;
-    let mut remaining_remote_refs: HashSet<_> = qualified_remote_refs.iter().copied().collect();
-    let mut push_options = git2::PushOptions::new();
-    let mut proxy_options = git2::ProxyOptions::new();
-    proxy_options.auto();
-    push_options.proxy_options(proxy_options);
-    let mut callbacks = callbacks.into_git();
-    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
+        let mut remote = git_repo.find_remote(remote_name).map_err(|err| {
+            if is_remote_not_found_err(&err) {
+                GitPushError::NoSuchRemote(remote_name.to_string())
+            } else {
+                GitPushError::InternalGitError(err)
             }
-            _ => GitPushError::InternalGitError(err),
         })?;
-    drop(push_options);
-    if remaining_remote_refs.is_empty() {
+        let mut callbacks = callbacks.into_git();
+        let mut push_options = git2::PushOptions::new();
+        let mut proxy_options = git2::ProxyOptions::new();
+        proxy_options.auto();
+        push_options.proxy_options(proxy_options);
+        let failed_push_negotiations_ref = &mut failed_push_negotiations;
+        callbacks.push_negotiation(|updates| {
+            for update in updates {
+                // TODO: What would it mean if dst_refname was empty? AH, NEED TO USE dst
+                // INSTEAD!
+                let expected_location = update.dst_refname().and_then(|refname| {
+                    #[allow(suspicious_double_ref_op)]
+                    // TODO: Suggestions to remove the double clone are welcome.
+                    // First, Rust lulls you into a sense of security. Then, out
+                    // of nowhere, an hour is spent looking for an idiomatic way
+                    // of converting &&Option<CommitId> to Option<CommitId>.
+                    qualified_remote_refs_expected_locations
+                        .get(refname)
+                        .expect("Push is trying to move a ref it wasn't asked to move")
+                        .clone()
+                        .clone()
+                });
+                /* dbg!(
+                    update.src_refname(),
+                    update.dst_refname(),
+                    update.src(),
+                    &expected_location
+                ); */
+                // src is the current position on target or 00000000000 if it's
+                // not on the remote. `src_refname`` seems to be the destination
+                // where `dst_refname` is set to.
+                if Some(update.src())
+                    .filter(|oid| !oid.is_zero())
+                    .map(|oid| oid.as_bytes().to_vec())
+                    != expected_location.map(|commitid| commitid.as_bytes().to_vec())
+                /* TODO: Check if src and src_refname point to different things, if not this is a
+                 * no-op */
+                {
+                    failed_push_negotiations_ref.push(update.dst_refname().map(|s| s.to_string()));
+                }
+            }
+            if failed_push_negotiations_ref.is_empty() {
+                Ok(())
+            } else {
+                Err(git2::Error::from_str("failed push negotiation"))
+            }
+        });
+        callbacks.push_update_reference(|refname, status| {
+            // The status is Some if the ref update was rejected
+            if status.is_none() {
+                remaining_remote_refs_expected_locations.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 remaining_remote_refs_expected_locations.is_empty() {
+        push_result?;
         Ok(())
+    } else if !failed_push_negotiations.is_empty() {
+        // TODO: move this into `map_err` above (if possible) once
+        // https://github.com/rust-lang/git2-rs/issues/1042 is fully resolved.
+        assert!(push_result.is_err());
+        Err(GitPushError::RefInUnexpectedLocation(
+            failed_push_negotiations,
+        ))
     } else {
         Err(GitPushError::RefUpdateRejected(
-            remaining_remote_refs
-                .iter()
+            remaining_remote_refs_expected_locations
+                .keys()
                 .sorted()
                 .map(|name| name.to_string())
                 .collect(),
@@ -1364,6 +1430,7 @@ pub struct RemoteCallbacks<'a> {
     pub get_ssh_keys: Option<&'a mut dyn FnMut(&str) -> Vec<PathBuf>>,
     pub get_password: Option<&'a mut dyn FnMut(&str, &str) -> Option<String>>,
     pub get_username_password: Option<&'a mut dyn FnMut(&str) -> Option<(String, String)>>,
+    // pub push_negotiation: Option<&'a mut dyn FnMut(&[git2::PushUpdate<'_>]) -> bool>,
 }
 
 impl<'a> RemoteCallbacks<'a> {

lib/tests/test_git.rs

@@ -19,6 +19,7 @@ use std::{fs, iter, thread};
 
 use assert_matches::assert_matches;
 use git2::Oid;
+use insta::assert_debug_snapshot;
 use itertools::Itertools;
 use jj_lib::backend::{BackendError, ChangeId, CommitId, MillisSinceEpoch, Signature, Timestamp};
 use jj_lib::commit::Commit;
@@ -2719,7 +2720,15 @@ fn test_push_branches_not_fast_forward() {
         &targets,
         git::RemoteCallbacks::default(),
     );
-    assert_eq!(result, Err(GitPushError::NotFastForward));
+    assert_debug_snapshot!(result, @r###"
+    Err(
+        RefUpdateRejected(
+            [
+                "refs/heads/main",
+            ],
+        ),
+    )
+    "###);
 }
 
 #[test]