From 760431a21e7270b8126308b1c9d5012902b1eea1 Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Thu, 11 Apr 2024 17:32:34 -0700 Subject: [PATCH] Actual push negotiation TODOs: can NotFastForward happen? Remove `force`? Thanks to https://github.com/rust-lang/git2-rs/pull/926 and @karin0 for making this possible. --- cli/src/commands/git.rs | 12 ++-- cli/tests/test_git_push.rs | 27 +++---- lib/src/git.rs | 143 +++++++++++++++++++++++++++---------- lib/tests/test_git.rs | 11 ++- 4 files changed, 136 insertions(+), 57 deletions(-) diff --git a/cli/src/commands/git.rs b/cli/src/commands/git.rs index 9122399631..3628df2de4 100644 --- a/cli/src/commands/git.rs +++ b/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)?; diff --git a/cli/tests/test_git_push.rs b/cli/tests/test_git_push.rs index d09c36c774..b6207c1633 100644 --- a/cli/tests/test_git_push.rs +++ b/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. "###); } diff --git a/lib/src/git.rs b/lib/src/git.rs index 5e7d1591ee..b017a9c6e6 100644 --- a/lib/src/git.rs +++ b/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>), #[error("Remote rejected the update of some refs (do you have permission to push to {0:?}?)")] RefUpdateRejected(Vec), // 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>, + // 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 to Option. + 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>, pub get_password: Option<&'a mut dyn FnMut(&str, &str) -> Option>, 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> { diff --git a/lib/tests/test_git.rs b/lib/tests/test_git.rs index a78523532f..a44534e14e 100644 --- a/lib/tests/test_git.rs +++ b/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]