diff --git a/cli/src/commands/git.rs b/cli/src/commands/git.rs index 78a39045d8..256aa62364 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( - err.to_string(), - "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 1ccd203a48..5b62dff230 100644 --- a/cli/tests/test_git_push.rs +++ b/cli/tests/test_git_push.rs @@ -200,7 +200,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###" @@ -234,15 +241,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 3571d60e8503 to c5ab2d309152 - 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(); @@ -284,16 +287,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 3571d60e8503 to eb921361206c + 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. "###); } @@ -338,15 +341,16 @@ fn test_git_push_deletion_unexpectedly_moved() { "###); insta::assert_snapshot!(get_stderr_string(&assert), @""); - // BUG: Pushing should fail because the branch was moved on the remote 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 3571d60e8503 + 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. "###); } @@ -393,11 +397,13 @@ fn test_git_push_sideways_unexpectedly_deleted() { 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 3571d60e8503 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. "###); } @@ -431,7 +437,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 95344a1ab28a - 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. "###); } @@ -548,15 +554,16 @@ fn test_git_push_fastforward_but_unexpectedly_moved_sideways_on_remote() { insta::assert_snapshot!(get_stderr_string(&assert), @""); test_env.jj_cmd_ok(&origin_path, &["git", "export"]); - // Pushing succeeds, but for a wrong reason? let assert = test_env .jj_cmd(&workspace_root, &["git", "push", "-bbranch1"]) .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 a0c93395c0a3 to 7a722cf0a4d8 + 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. "###); } @@ -574,10 +581,6 @@ fn test_git_push_fastforward_but_unexpectedly_moved_sideways_on_remote() { /// | /// o root /// -/// This currently causes `jj` to treat this as a fast-forward, which succeeds -/// because the remote doesn't care about the location of our local -/// remote-tracking branch. -/// /// AFAIU, regular Git would also consider this a fast-forward and succeed on a /// `git push`, but `git push --force-with-lease` would fail. /// @@ -678,7 +681,6 @@ fn test_git_push_fastforward_but_unexpectedly_moved_up_on_remote() { insta::assert_snapshot!(get_stderr_string(&assert), @""); test_env.jj_cmd_ok(&origin_path, &["git", "export"]); - // Pushing succeeds, but for a wrong reason? let assert = test_env .jj_cmd(&workspace_root, &["git", "push", "-bbranch1"]) .assert() diff --git a/lib/src/git.rs b/lib/src/git.rs index b8847ca7ca..7ffd36b04e 100644 --- a/lib/src/git.rs +++ b/lib/src/git.rs @@ -1210,6 +1210,14 @@ pub enum GitPushError { RemoteReservedForLocalGitRepo, #[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,53 +1317,153 @@ 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: 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); + 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); } - 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() { - Ok(()) - } else { - Err(GitPushError::RefUpdateRejected( - remaining_remote_refs - .iter() - .sorted() - .map(|name| name.to_string()) - .collect(), + 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 { + let dst_refname = update + .dst_refname() + .expect("Expect reference name to be valid UTF-8"); + let expected_location = (*qualified_remote_refs_expected_locations + .get(dst_refname) + .expect("Push is trying to move a ref it wasn't asked to move")) + .clone(); + let expected_location_oid = expected_location + .map(|id| git2::Oid::from_bytes(id.as_bytes()).unwrap()) + .unwrap_or(git2::Oid::zero()); + /* dbg!( + update.src_refname(), // Position to update the branch to (!) + update.dst_refname(), // Branch to update + update.dst(), // Commit id of `src_refname` (!) + update.src(), // Current position + &expected_location + ); */ + // See also https://github.com/git/git/blob/master/templates/hooks--pre-push.sample + // https://github.com/libgit2/libgit2/commit/efc2fec50e3bb725b2b244fdeacfb2dfad6ee78e + // 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 update.src() != expected_location_oid { + // There are two corner cases where we allow the push even + // though the branch is in an unexpected location. This + // happpens when either: + // + // 1) the destination is the same as the unexpected location (so the push + // turns out to be a no-op) or + // 2) the unexpected location is an ancestor of the push destination (so the + // push is a fast-forward in Git parlance) **and** it is a descendant of + // the expected location. + // + // This is slightly different from Git's `git push` which + // would consider the situation a fast-forward as long as + // the unexpected location is an ancestor of the destination + // (or equal to it), regardless of where the expected + // location is. The difference seems like it would matter + // rarely in practice. Our rule is also different from Git's + // `git push --force-with-lease`, which would AFAIK not + // allow any corner cases at all. TODO(ilyagr): Double-check + // last statement. + // + // The reasoning for our rule is that we essentially have a + // branch conflict with sides `update.dst()` (our branch + // location) and `update.src()` (their branch location), and + // with base `expected_location`. In fact, if the user + // performed a `jj git fetch` instead of `jj git push` they + // are performing, they would get exactly this branch + // conflict. We decide to allow the push **if and only if** + // `jj` would auto-resolve this conflict to our location, + // `update.dst()`. Conveniently, this also means that the + // push will be successful **if and only if** `jj git fetch` + // is a no-op for this branch (and, in particular, would not + // create a conflict). + // + // The logic from the previous paragraph allows exactly the + // two corner case described above, as can be seen in + // `test_refs.rs`. This is true even if we also consider the + // cases where some of the three members of the branch + // conflict do not exist, e.g. branch creation or deletion. + // + // TODO: Double-check this with tests, refer to them in the + // comment. + if update.src() == update.dst() + || (git_repo + .graph_descendant_of(update.src(), expected_location_oid) + .unwrap_or(false) + && git_repo + .graph_descendant_of(update.dst(), update.src()) + .unwrap_or(false)) + { + continue; + } + 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 !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 { + push_result?; + if remaining_remote_refs_expected_locations.is_empty() { + Ok(()) + } else { + Err(GitPushError::RefUpdateRejected( + remaining_remote_refs_expected_locations + .keys() + .sorted() + .map(|name| name.to_string()) + .collect(), + )) + } } } @@ -1364,6 +1476,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 b7a334b348..1e885d15d0 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; @@ -2440,7 +2441,6 @@ struct PushTestSetup { jj_repo: Arc, initial_commit: Commit, child_of_initial_commit: Commit, - #[allow(dead_code)] // For next commit parent_of_initial_commit: Commit, } @@ -2777,6 +2777,172 @@ fn test_push_branches_not_fast_forward_with_force() { assert_eq!(new_target, Some(git_id(&new_commit))); } +#[test] +fn test_push_branches_unexpectedly_moved_sideways_on_remote() { + let settings = testutils::user_settings(); + let temp_dir = testutils::new_temp_dir(); + let mut setup = set_up_push_repos(&settings, &temp_dir); + let mut tx = setup.jj_repo.start_transaction(&settings); + let sideways_commit = write_random_commit(tx.mut_repo(), &settings); + setup.jj_repo = tx.commit("test"); + let mut tx = setup.jj_repo.start_transaction(&settings); + + // The main branch is actually at `initial_commit` on the remote. If we expected + // it to be at `sideways_commit`, 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`. + + let targets = GitBranchPushTargets { + branch_updates: vec![( + "main".to_owned(), + BranchPushUpdate { + old_target: Some(sideways_commit.id().clone()), + new_target: None, + }, + )], + force_pushed_branches: hashset! { + "main".to_owned(), + }, + }; + let result = git::push_branches( + tx.mut_repo(), + &get_git_repo(&setup.jj_repo), + "origin", + &targets, + git::RemoteCallbacks::default(), + ); + assert_debug_snapshot!(result, @r###" + Err( + RefInUnexpectedLocation( + [ + Some( + "refs/heads/main", + ), + ], + ), + ) + "###); + + let targets = GitBranchPushTargets { + branch_updates: vec![( + "main".to_owned(), + BranchPushUpdate { + old_target: Some(sideways_commit.id().clone()), + new_target: Some(setup.child_of_initial_commit.id().clone()), + }, + )], + force_pushed_branches: hashset! { + "main".to_owned(), + }, + }; + let result = git::push_branches( + tx.mut_repo(), + &get_git_repo(&setup.jj_repo), + "origin", + &targets, + git::RemoteCallbacks::default(), + ); + assert_debug_snapshot!(result, @r###" + Err( + RefInUnexpectedLocation( + [ + Some( + "refs/heads/main", + ), + ], + ), + ) + "###); + + let targets = GitBranchPushTargets { + branch_updates: vec![( + "main".to_owned(), + BranchPushUpdate { + old_target: Some(sideways_commit.id().clone()), + new_target: Some(sideways_commit.id().clone()), + }, + )], + force_pushed_branches: hashset! { + "main".to_owned(), + }, + }; + let result = git::push_branches( + tx.mut_repo(), + &get_git_repo(&setup.jj_repo), + "origin", + &targets, + git::RemoteCallbacks::default(), + ); + assert_debug_snapshot!(result, @r###" + Err( + RefInUnexpectedLocation( + [ + Some( + "refs/heads/main", + ), + ], + ), + ) + "###); + + let targets = GitBranchPushTargets { + branch_updates: vec![( + "main".to_owned(), + BranchPushUpdate { + old_target: Some(sideways_commit.id().clone()), + new_target: Some(setup.parent_of_initial_commit.id().clone()), + }, + )], + force_pushed_branches: hashset! { + "main".to_owned(), + }, + }; + let result = git::push_branches( + tx.mut_repo(), + &get_git_repo(&setup.jj_repo), + "origin", + &targets, + git::RemoteCallbacks::default(), + ); + assert_debug_snapshot!(result, @r###" + Err( + RefInUnexpectedLocation( + [ + Some( + "refs/heads/main", + ), + ], + ), + ) + "###); + + // Moving the branch to the same place it already is is OK. + let targets = GitBranchPushTargets { + branch_updates: vec![( + "main".to_owned(), + BranchPushUpdate { + old_target: Some(sideways_commit.id().clone()), + new_target: Some(setup.initial_commit.id().clone()), + }, + )], + force_pushed_branches: hashset! { + "main".to_owned(), + }, + }; + let result = git::push_branches( + tx.mut_repo(), + &get_git_repo(&setup.jj_repo), + "origin", + &targets, + git::RemoteCallbacks::default(), + ); + assert_debug_snapshot!(result, @r###" + Ok( + (), + ) + "###); +} + #[test] fn test_push_updates_success() { let settings = testutils::user_settings(); diff --git a/lib/tests/test_refs.rs b/lib/tests/test_refs.rs index d56d1fb65a..0b5ca2be4b 100644 --- a/lib/tests/test_refs.rs +++ b/lib/tests/test_refs.rs @@ -89,7 +89,13 @@ 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