From 8d3dd17b5102a4c9494618c981ab5f1c76e077aa Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Wed, 8 May 2024 19:31:19 -0700 Subject: [PATCH] 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 | 12 ++ cli/src/commands/git.rs | 9 ++ cli/tests/test_git_push.rs | 48 ++++--- lib/src/git.rs | 249 +++++++++++++++++++++++++++---------- lib/tests/test_git.rs | 183 ++++++++++++++++++++++++++- lib/tests/test_refs.rs | 19 ++- 6 files changed, 426 insertions(+), 94 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8c41f6b353..031a8c796c 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/cli/src/commands/git.rs b/cli/src/commands/git.rs index 2e1cbe86e7..6545932ed9 100644 --- a/cli/src/commands/git.rs +++ b/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)?; diff --git a/cli/tests/test_git_push.rs b/cli/tests/test_git_push.rs index 78439d1086..26191476cd 100644 --- a/cli/tests/test_git_push.rs +++ b/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 - // 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(); - 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: 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 -// push --force-with-lease` +// positions. #[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 assert = test_env - .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###" + 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. "###); } @@ -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. "###); } diff --git a/lib/src/git.rs b/lib/src/git.rs index e672ed92aa..b7e80ad47b 100644 --- a/lib/src/git.rs +++ b/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), #[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, @@ -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 - // don't inline for now. If it's after 2024-06-01 or so, ilyagr may have - // forgotten to delete this comment. + // TODO(ilyagr): `push_refs`, or parts of it, should probably be inlined. This + // requires adjusting some tests. 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 proxy_options = git2::ProxyOptions::new(); - proxy_options.auto(); - push_options.proxy_options(proxy_options); - let mut callbacks = callbacks.into_git(); - 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_remote_location = *qualified_remote_refs_expected_locations - .get(dst_refname) - .expect("Push is trying to move a ref it wasn't asked to move"); - - // `update.src()` is the current actual position of the branch - // `dst_refname` on the remote. `update.dst()` is the position - // we are trying to push the branch to (AKA our local branch - // location). 0000000000 is a valid value for either, indicating - // branch creation or deletion. - 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()); - - // Short-term TODO: Replace this with actual rules of when to permit the push - 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 + let mut failed_push_negotiations = vec![]; + let push_result = { + 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_negotiation(|updates| { + for update in updates { + let dst_refname = update + .dst_refname() + .expect("Expect reference name to be valid UTF-8"); + 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()); + + match allow_push( + repo.index(), + actual_remote_location.as_ref(), + expected_remote_location, + local_location.as_ref(), + ) { + Ok(PushAllowReason::NormalMatch) => {} + Ok(PushAllowReason::UnexpectedNoop) => { + 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:?}", + ); + + failed_push_negotiations.push(dst_refname.to_string()); + } + } } - _ => 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(), + if failed_push_negotiations.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.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() { + // 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 { + 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(()) } } diff --git a/lib/tests/test_git.rs b/lib/tests/test_git.rs index 0814f847d2..2a12a06ed9 100644 --- a/lib/tests/test_git.rs +++ b/lib/tests/test_git.rs @@ -2485,7 +2485,7 @@ struct PushTestSetup { jj_repo: Arc, 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| { + 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| { + 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| { + 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 { diff --git a/lib/tests/test_refs.rs b/lib/tests/test_refs.rs index d56d1fb65a..59fa06758a 100644 --- a/lib/tests/test_refs.rs +++ b/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),