From 031eb919741f86a47a4565ed71a3e453a1cb6689 Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Mon, 8 Apr 2024 14:53:56 -0700 Subject: [PATCH 1/8] test_git_push: demo behavior we'd like to be safer Adds two tests where pushing should fail, but currently succeeds. --- cli/tests/test_git_push.rs | 90 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 90 insertions(+) diff --git a/cli/tests/test_git_push.rs b/cli/tests/test_git_push.rs index 5651fb0606..0756547d21 100644 --- a/cli/tests/test_git_push.rs +++ b/cli/tests/test_git_push.rs @@ -235,6 +235,96 @@ fn test_git_push_not_fast_forward() { "###); } +// 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(); + + // Move branch1 forward on the remote + let origin_path = test_env.env_root().join("origin"); + test_env.jj_cmd_ok(&origin_path, &["new", "branch1", "-m=remote"]); + std::fs::write(origin_path.join("remote"), "remote").unwrap(); + test_env.jj_cmd_ok(&origin_path, &["branch", "set", "branch1"]); + insta::assert_snapshot!(get_branch_output(&test_env, &origin_path), @r###" + branch1: vruxwmqv fb645b4b remote + @git (behind by 1 commits): qpvuntsm 45a3aa29 (empty) description 1 + branch2: zsuskuln 8476341e (empty) description 2 + @git: zsuskuln 8476341e (empty) description 2 + "###); + test_env.jj_cmd_ok(&origin_path, &["git", "export"]); + + // Move branch1 sideways to another commit locally + test_env.jj_cmd_ok(&workspace_root, &["new", "root()", "-m=local"]); + std::fs::write(workspace_root.join("local"), "local").unwrap(); + test_env.jj_cmd_ok( + &workspace_root, + &["branch", "set", "branch1", "--allow-backwards"], + ); + insta::assert_snapshot!(get_branch_output(&test_env, &workspace_root), @r###" + branch1: kmkuslsw 0f8bf988 local + @origin (ahead by 1 commits, behind by 1 commits): lzmmnrxq 45a3aa29 (empty) description 1 + branch2: rlzusymt 8476341e (empty) description 2 + @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###" + Branch changes to push to origin: + Force branch branch1 from 45a3aa29e907 to 0f8bf988588e + "###); +} + +// 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_deletion_unexpectedly_moved() { + let (test_env, workspace_root) = set_up(); + + // Move branch1 forward on the remote + let origin_path = test_env.env_root().join("origin"); + test_env.jj_cmd_ok(&origin_path, &["new", "branch1", "-m=remote"]); + std::fs::write(origin_path.join("remote"), "remote").unwrap(); + test_env.jj_cmd_ok(&origin_path, &["branch", "set", "branch1"]); + insta::assert_snapshot!(get_branch_output(&test_env, &origin_path), @r###" + branch1: vruxwmqv fb645b4b remote + @git (behind by 1 commits): qpvuntsm 45a3aa29 (empty) description 1 + branch2: zsuskuln 8476341e (empty) description 2 + @git: zsuskuln 8476341e (empty) description 2 + "###); + test_env.jj_cmd_ok(&origin_path, &["git", "export"]); + + // Delete branch1 locally + test_env.jj_cmd_ok(&workspace_root, &["branch", "delete", "branch1"]); + insta::assert_snapshot!(get_branch_output(&test_env, &workspace_root), @r###" + branch1 (deleted) + @origin: lzmmnrxq 45a3aa29 (empty) description 1 + branch2: rlzusymt 8476341e (empty) description 2 + @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###" + Branch changes to push to origin: + Delete branch branch1 from 45a3aa29e907 + "###); +} + #[test] fn test_git_push_locally_created_and_rewritten() { let (test_env, workspace_root) = set_up(); From fa72cac586c5ed30cb40202a7c26af67e548bfca Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Tue, 16 Apr 2024 17:21:05 -0700 Subject: [PATCH 2/8] test_git_push: add a test for creating unexpectedly existing branch This tests `git push` attempting to create a branch when the branch already unexpectedly exists on the remote. This should (and does) fail. Also changes another test to use `jj_cmd_failure`. --- cli/tests/test_git_push.rs | 34 ++++++++++++++++++++++++++++------ 1 file changed, 28 insertions(+), 6 deletions(-) diff --git a/cli/tests/test_git_push.rs b/cli/tests/test_git_push.rs index 0756547d21..78439d1086 100644 --- a/cli/tests/test_git_push.rs +++ b/cli/tests/test_git_push.rs @@ -222,12 +222,8 @@ fn test_git_push_not_fast_forward() { test_env.jj_cmd_ok(&workspace_root, &["branch", "set", "branch1"]); // Pushing should fail - let assert = test_env - .jj_cmd(&workspace_root, &["git", "push"]) - .assert() - .code(1); - 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: Move branch branch1 from 45a3aa29e907 to c35839cb8e8c Error: The push conflicts with changes made on the remote (it is not fast-forwardable). @@ -325,6 +321,32 @@ fn test_git_push_deletion_unexpectedly_moved() { "###); } +#[test] +fn test_git_push_creation_unexpectedly_already_exists() { + let (test_env, workspace_root) = set_up(); + + // Forget branch1 locally + test_env.jj_cmd_ok(&workspace_root, &["branch", "forget", "branch1"]); + + // Create a new branh1 + test_env.jj_cmd_ok(&workspace_root, &["new", "root()", "-m=new branch1"]); + std::fs::write(workspace_root.join("local"), "local").unwrap(); + test_env.jj_cmd_ok(&workspace_root, &["branch", "create", "branch1"]); + insta::assert_snapshot!(get_branch_output(&test_env, &workspace_root), @r###" + branch1: yostqsxw 4c595cf9 new branch1 + branch2: rlzusymt 8476341e (empty) description 2 + @origin: rlzusymt 8476341e (empty) description 2 + "###); + + let stderr = test_env.jj_cmd_failure(&workspace_root, &["git", "push"]); + 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). + Hint: Try fetching from the remote, then make the branch point to where you want it to be, and push again. + "###); +} + #[test] fn test_git_push_locally_created_and_rewritten() { let (test_env, workspace_root) = set_up(); From 74a5856ca7bdc0192233b027beda3645ddda824b Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Wed, 8 May 2024 21:26:36 -0700 Subject: [PATCH 3/8] test_git.rs: add two more commits to PushSetup, rename them The new names are a bit clearer. Soem tests already use a `sideways_commit`, `parent_of_main_commit` will be needed in an upcoming test. --- lib/tests/test_git.rs | 99 ++++++++++++++++++++++++++----------------- 1 file changed, 59 insertions(+), 40 deletions(-) diff --git a/lib/tests/test_git.rs b/lib/tests/test_git.rs index 802a3c83c6..5cd18be504 100644 --- a/lib/tests/test_git.rs +++ b/lib/tests/test_git.rs @@ -2483,16 +2483,34 @@ fn test_fetch_no_such_remote() { struct PushTestSetup { source_repo_dir: PathBuf, jj_repo: Arc, - initial_commit: Commit, - new_commit: Commit, -} - + main_commit: Commit, + child_of_main_commit: Commit, + _parent_of_main_commit: Commit, + sideways_commit: Commit, +} + +/// Set up a situation where `main` is at `main_commit`, the child of +/// `parent_of_main_commit`, both in the source repo and in jj's clone of the +/// repo. In jj's clone, there are also two more commits, `child_of_main_commit` +/// and `sideways_commit`, arranged as follows: +/// +/// o child_of_main_commit +/// o main_commit +/// o parent_of_main_commit +/// | o sideways_commit +/// |/ +/// ~ root fn set_up_push_repos(settings: &UserSettings, temp_dir: &TempDir) -> PushTestSetup { let source_repo_dir = temp_dir.path().join("source"); let clone_repo_dir = temp_dir.path().join("clone"); let jj_repo_dir = temp_dir.path().join("jj"); let source_repo = git2::Repository::init_bare(&source_repo_dir).unwrap(); - let initial_git_commit = empty_git_commit(&source_repo, "refs/heads/main", &[]); + let parent_of_initial_git_commit = empty_git_commit(&source_repo, "refs/heads/main", &[]); + let initial_git_commit = empty_git_commit( + &source_repo, + "refs/heads/main", + &[&parent_of_initial_git_commit], + ); let clone_repo = git2::Repository::clone(source_repo_dir.to_str().unwrap(), clone_repo_dir).unwrap(); std::fs::create_dir(&jj_repo_dir).unwrap(); @@ -2516,24 +2534,29 @@ fn set_up_push_repos(settings: &UserSettings, temp_dir: &TempDir) -> PushTestSet get_git_backend(&jj_repo) .import_head_commits(&[jj_id(&initial_git_commit)]) .unwrap(); - let initial_commit = jj_repo + let main_commit = jj_repo .store() .get_commit(&jj_id(&initial_git_commit)) .unwrap(); + let parent_of_main_commit = jj_repo + .store() + .get_commit(&jj_id(&parent_of_initial_git_commit)) + .unwrap(); let mut tx = jj_repo.start_transaction(settings); - let new_commit = create_random_commit(tx.mut_repo(), settings) - .set_parents(vec![initial_commit.id().clone()]) + let sideways_commit = write_random_commit(tx.mut_repo(), settings); + let child_of_main_commit = create_random_commit(tx.mut_repo(), settings) + .set_parents(vec![main_commit.id().clone()]) .write() .unwrap(); tx.mut_repo().set_git_ref_target( "refs/remotes/origin/main", - RefTarget::normal(initial_commit.id().clone()), + RefTarget::normal(main_commit.id().clone()), ); tx.mut_repo().set_remote_branch( "main", "origin", RemoteRef { - target: RefTarget::normal(initial_commit.id().clone()), + target: RefTarget::normal(main_commit.id().clone()), // Caller expects the main branch is tracked. The corresponding local branch will // be created (or left as deleted) by caller. state: RemoteRefState::Tracking, @@ -2543,8 +2566,10 @@ fn set_up_push_repos(settings: &UserSettings, temp_dir: &TempDir) -> PushTestSet PushTestSetup { source_repo_dir, jj_repo, - initial_commit, - new_commit, + main_commit, + child_of_main_commit, + _parent_of_main_commit: parent_of_main_commit, + sideways_commit, } } @@ -2560,8 +2585,8 @@ fn test_push_branches_success() { branch_updates: vec![( "main".to_owned(), BranchPushUpdate { - old_target: Some(setup.initial_commit.id().clone()), - new_target: Some(setup.new_commit.id().clone()), + old_target: Some(setup.main_commit.id().clone()), + new_target: Some(setup.child_of_main_commit.id().clone()), }, )], force_pushed_branches: hashset! {}, @@ -2581,7 +2606,7 @@ fn test_push_branches_success() { .find_reference("refs/heads/main") .unwrap() .target(); - let new_oid = git_id(&setup.new_commit); + let new_oid = git_id(&setup.child_of_main_commit); assert_eq!(new_target, Some(new_oid)); // Check that the ref got updated in the cloned repo. This just tests our @@ -2597,12 +2622,12 @@ fn test_push_branches_success() { let view = tx.mut_repo().view(); assert_eq!( *view.get_git_ref("refs/remotes/origin/main"), - RefTarget::normal(setup.new_commit.id().clone()), + RefTarget::normal(setup.child_of_main_commit.id().clone()), ); assert_eq!( *view.get_remote_branch("main", "origin"), RemoteRef { - target: RefTarget::normal(setup.new_commit.id().clone()), + target: RefTarget::normal(setup.child_of_main_commit.id().clone()), state: RemoteRefState::Tracking, }, ); @@ -2630,7 +2655,7 @@ fn test_push_branches_deletion() { branch_updates: vec![( "main".to_owned(), BranchPushUpdate { - old_target: Some(setup.initial_commit.id().clone()), + old_target: Some(setup.main_commit.id().clone()), new_target: None, }, )], @@ -2680,7 +2705,7 @@ fn test_push_branches_mixed_deletion_and_addition() { ( "main".to_owned(), BranchPushUpdate { - old_target: Some(setup.initial_commit.id().clone()), + old_target: Some(setup.main_commit.id().clone()), new_target: None, }, ), @@ -2688,7 +2713,7 @@ fn test_push_branches_mixed_deletion_and_addition() { "topic".to_owned(), BranchPushUpdate { old_target: None, - new_target: Some(setup.new_commit.id().clone()), + new_target: Some(setup.child_of_main_commit.id().clone()), }, ), ], @@ -2709,7 +2734,7 @@ fn test_push_branches_mixed_deletion_and_addition() { .find_reference("refs/heads/topic") .unwrap() .target(); - assert_eq!(new_target, Some(git_id(&setup.new_commit))); + assert_eq!(new_target, Some(git_id(&setup.child_of_main_commit))); // Check that the main ref got deleted in the source repo assert!(source_repo.find_reference("refs/heads/main").is_err()); @@ -2720,12 +2745,12 @@ fn test_push_branches_mixed_deletion_and_addition() { assert!(view.get_remote_branch("main", "origin").is_absent()); assert_eq!( *view.get_git_ref("refs/remotes/origin/topic"), - RefTarget::normal(setup.new_commit.id().clone()), + RefTarget::normal(setup.child_of_main_commit.id().clone()), ); assert_eq!( *view.get_remote_branch("topic", "origin"), RemoteRef { - target: RefTarget::normal(setup.new_commit.id().clone()), + target: RefTarget::normal(setup.child_of_main_commit.id().clone()), state: RemoteRefState::Tracking, }, ); @@ -2741,18 +2766,15 @@ fn test_push_branches_mixed_deletion_and_addition() { fn test_push_branches_not_fast_forward() { 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 new_commit = write_random_commit(tx.mut_repo(), &settings); - setup.jj_repo = tx.commit("test"); + let setup = set_up_push_repos(&settings, &temp_dir); let mut tx = setup.jj_repo.start_transaction(&settings); let targets = GitBranchPushTargets { branch_updates: vec![( "main".to_owned(), BranchPushUpdate { - old_target: Some(setup.initial_commit.id().clone()), - new_target: Some(new_commit.id().clone()), + old_target: Some(setup.main_commit.id().clone()), + new_target: Some(setup.sideways_commit.id().clone()), }, )], force_pushed_branches: hashset! {}, @@ -2771,18 +2793,15 @@ fn test_push_branches_not_fast_forward() { fn test_push_branches_not_fast_forward_with_force() { 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 new_commit = write_random_commit(tx.mut_repo(), &settings); - setup.jj_repo = tx.commit("test"); + let setup = set_up_push_repos(&settings, &temp_dir); let mut tx = setup.jj_repo.start_transaction(&settings); let targets = GitBranchPushTargets { branch_updates: vec![( "main".to_owned(), BranchPushUpdate { - old_target: Some(setup.initial_commit.id().clone()), - new_target: Some(new_commit.id().clone()), + old_target: Some(setup.main_commit.id().clone()), + new_target: Some(setup.sideways_commit.id().clone()), }, )], force_pushed_branches: hashset! { @@ -2804,7 +2823,7 @@ fn test_push_branches_not_fast_forward_with_force() { .find_reference("refs/heads/main") .unwrap() .target(); - assert_eq!(new_target, Some(git_id(&new_commit))); + assert_eq!(new_target, Some(git_id(&setup.sideways_commit))); } #[test] @@ -2819,7 +2838,7 @@ fn test_push_updates_success() { &[GitRefUpdate { qualified_name: "refs/heads/main".to_string(), force: false, - new_target: Some(setup.new_commit.id().clone()), + new_target: Some(setup.child_of_main_commit.id().clone()), }], git::RemoteCallbacks::default(), ); @@ -2831,7 +2850,7 @@ fn test_push_updates_success() { .find_reference("refs/heads/main") .unwrap() .target(); - let new_oid = git_id(&setup.new_commit); + let new_oid = git_id(&setup.child_of_main_commit); assert_eq!(new_target, Some(new_oid)); // Check that the ref got updated in the cloned repo. This just tests our @@ -2855,7 +2874,7 @@ fn test_push_updates_no_such_remote() { &[GitRefUpdate { qualified_name: "refs/heads/main".to_string(), force: false, - new_target: Some(setup.new_commit.id().clone()), + new_target: Some(setup.child_of_main_commit.id().clone()), }], git::RemoteCallbacks::default(), ); @@ -2873,7 +2892,7 @@ fn test_push_updates_invalid_remote() { &[GitRefUpdate { qualified_name: "refs/heads/main".to_string(), force: false, - new_target: Some(setup.new_commit.id().clone()), + new_target: Some(setup.child_of_main_commit.id().clone()), }], git::RemoteCallbacks::default(), ); From 0ed2a7c81fcaf7e490e44661cb3b7d2160a3ea88 Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Thu, 11 Apr 2024 17:32:34 -0700 Subject: [PATCH 4/8] jj git push: no-op push negotiation, plumb data to it Hopefully, splitting the no-op portion will make the following commits easier to review. --- lib/src/git.rs | 55 ++++++++++++++++++++++++++++++++++++------- lib/tests/test_git.rs | 3 +++ 2 files changed, 50 insertions(+), 8 deletions(-) diff --git a/lib/src/git.rs b/lib/src/git.rs index 4dbe7a38f4..e672ed92aa 100644 --- a/lib/src/git.rs +++ b/lib/src/git.rs @@ -1240,9 +1240,12 @@ pub struct GitBranchPushTargets { pub struct GitRefUpdate { pub qualified_name: String, - // TODO: We want this to be a `current_target: Option` for the expected current - // commit on the remote. It's a blunt "force" option instead until git2-rs supports the - // "push negotiation" callback (https://github.com/rust-lang/git2-rs/issues/733). + /// Expected position on the remote or None if we expect the ref to not + /// exist on the remote + /// + /// This is sourced from the local remote-tracking branch. + pub expected_current_target: Option, + // Short-term TODO: Delete `force` pub force: bool, pub new_target: Option, } @@ -1261,6 +1264,7 @@ pub fn push_branches( .map(|(branch_name, update)| GitRefUpdate { qualified_name: format!("refs/heads/{branch_name}"), force: targets.force_pushed_branches.contains(branch_name), + expected_current_target: update.old_target.clone(), new_target: update.new_target.clone(), }) .collect_vec(); @@ -1289,10 +1293,13 @@ 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.as_ref(), + ); if let Some(new_target) = &update.new_target { refspecs.push(format!( "{}{}:{}", @@ -1310,7 +1317,7 @@ pub fn push_updates( push_refs( git_repo, remote_name, - &qualified_remote_refs, + &qualified_remote_refs_expected_locations, &refspecs, callbacks, ) @@ -1319,7 +1326,7 @@ 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>>, refspecs: &[String], callbacks: RemoteCallbacks<'_>, ) -> Result<(), GitPushError> { @@ -1333,12 +1340,44 @@ fn push_refs( GitPushError::InternalGitError(err) } })?; - let mut remaining_remote_refs: HashSet<_> = qualified_remote_refs.iter().copied().collect(); + let mut remaining_remote_refs: HashSet<_> = qualified_remote_refs_expected_locations + .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() { diff --git a/lib/tests/test_git.rs b/lib/tests/test_git.rs index 5cd18be504..0814f847d2 100644 --- a/lib/tests/test_git.rs +++ b/lib/tests/test_git.rs @@ -2838,6 +2838,7 @@ fn test_push_updates_success() { &[GitRefUpdate { qualified_name: "refs/heads/main".to_string(), force: false, + expected_current_target: Some(setup.main_commit.id().clone()), new_target: Some(setup.child_of_main_commit.id().clone()), }], git::RemoteCallbacks::default(), @@ -2874,6 +2875,7 @@ fn test_push_updates_no_such_remote() { &[GitRefUpdate { qualified_name: "refs/heads/main".to_string(), force: false, + expected_current_target: Some(setup.main_commit.id().clone()), new_target: Some(setup.child_of_main_commit.id().clone()), }], git::RemoteCallbacks::default(), @@ -2892,6 +2894,7 @@ fn test_push_updates_invalid_remote() { &[GitRefUpdate { qualified_name: "refs/heads/main".to_string(), force: false, + expected_current_target: Some(setup.main_commit.id().clone()), new_target: Some(setup.child_of_main_commit.id().clone()), }], git::RemoteCallbacks::default(), From 99e10315b6c1e1634a1f03531d3c96ca196e11be Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Wed, 8 May 2024 19:31:19 -0700 Subject: [PATCH 5/8] 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 920ca1ede6..2213cb76d6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,6 +51,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), From 1a1d8e7d83e041da85aca087776949de9fd994c5 Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Tue, 16 Apr 2024 19:13:48 -0700 Subject: [PATCH 6/8] jj git push: always force-push, all safety logic now in push_negotiation This should be a no-op, though that is not necessarily obvious in corner cases. Note that libgit2 already performs the push negotiation even when pushing without force (without `+` in the refspec). --- cli/src/commands/git.rs | 9 +++++---- lib/src/git.rs | 32 ++++++++++---------------------- lib/tests/test_git.rs | 17 +++-------------- 3 files changed, 18 insertions(+), 40 deletions(-) diff --git a/cli/src/commands/git.rs b/cli/src/commands/git.rs index 6545932ed9..a8b4cf0129 100644 --- a/cli/src/commands/git.rs +++ b/cli/src/commands/git.rs @@ -899,6 +899,10 @@ fn cmd_git_push( } let mut new_heads = vec![]; + // Short-term TODO: `force_pushed_branches` no longer has an effect on how + // branches are actually pushed. Messages about "Moving" vs "Forcing" + // branches are now misleading. Change this logic so that we print whether + // the branch moved forward, backward, or sideways. let mut force_pushed_branches = hashset! {}; for (branch_name, update) in &branch_updates { if let Some(new_target) = &update.new_target { @@ -1001,10 +1005,7 @@ fn cmd_git_push( return Ok(()); } - let targets = GitBranchPushTargets { - branch_updates, - force_pushed_branches, - }; + let targets = GitBranchPushTargets { branch_updates }; let mut writer = GitSidebandProgressMessageWriter::new(ui); let mut sideband_progress_callback = |progress_message: &[u8]| { _ = writer.write(ui, progress_message); diff --git a/lib/src/git.rs b/lib/src/git.rs index b7e80ad47b..3a90501055 100644 --- a/lib/src/git.rs +++ b/lib/src/git.rs @@ -1223,6 +1223,7 @@ pub enum GitPushError { name = REMOTE_NAME_FOR_LOCAL_GIT_REPO )] RemoteReservedForLocalGitRepo, + // Short-term TODO: Delete this; it should never trigger #[error("Push is not fast-forwardable")] NotFastForward, #[error("Refs in unexpected location: {0:?}")] @@ -1238,7 +1239,6 @@ pub enum GitPushError { #[derive(Clone, Debug)] pub struct GitBranchPushTargets { pub branch_updates: Vec<(String, BranchPushUpdate)>, - pub force_pushed_branches: HashSet, } pub struct GitRefUpdate { @@ -1248,8 +1248,6 @@ pub struct GitRefUpdate { /// /// This is sourced from the local remote-tracking branch. pub expected_current_target: Option, - // Short-term TODO: Delete `force` - pub force: bool, pub new_target: Option, } @@ -1266,7 +1264,6 @@ pub fn push_branches( .iter() .map(|(branch_name, update)| GitRefUpdate { qualified_name: format!("refs/heads/{branch_name}"), - force: targets.force_pushed_branches.contains(branch_name), expected_current_target: update.old_target.clone(), new_target: update.new_target.clone(), }) @@ -1305,13 +1302,14 @@ pub fn push_updates( update.expected_current_target.as_ref(), ); if let Some(new_target) = &update.new_target { - refspecs.push(format!( - "{}{}:{}", - (if update.force { "+" } else { "" }), - new_target.hex(), - update.qualified_name - )); + // We always force-push. We use the push_negotiation callback in + // `push_refs` to check that the refs did not unexpectedly move on + // the remote. + refspecs.push(format!("+{}:{}", new_target.hex(), update.qualified_name)); } else { + // Prefixing this with `+` to force-push or not should make no + // difference. The push negotiation happens regardless, and wouldn't + // allow creating a branch if it's not a fast-forward. refspecs.push(format!(":{}", update.qualified_name)); } } @@ -1506,22 +1504,12 @@ fn allow_push( // https://github.com/martinvonz/jj/blob/c9b44f382824301e6c0fdd6f4cbc52bb00c50995/lib/src/merge.rs#L92. PushAllowReason::UnexpectedNoop } else { - // The assertions follow from our ref merge rules: + // Due to our ref merge rules, this case should happen if an only + // if: // // 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 { diff --git a/lib/tests/test_git.rs b/lib/tests/test_git.rs index 2a12a06ed9..59c7b9a0ac 100644 --- a/lib/tests/test_git.rs +++ b/lib/tests/test_git.rs @@ -2589,7 +2589,6 @@ fn test_push_branches_success() { new_target: Some(setup.child_of_main_commit.id().clone()), }, )], - force_pushed_branches: hashset! {}, }; let result = git::push_branches( tx.mut_repo(), @@ -2659,7 +2658,6 @@ fn test_push_branches_deletion() { new_target: None, }, )], - force_pushed_branches: hashset! {}, }; let result = git::push_branches( tx.mut_repo(), @@ -2717,7 +2715,6 @@ fn test_push_branches_mixed_deletion_and_addition() { }, ), ], - force_pushed_branches: hashset! {}, }; let result = git::push_branches( tx.mut_repo(), @@ -2777,7 +2774,6 @@ fn test_push_branches_not_fast_forward() { new_target: Some(setup.sideways_commit.id().clone()), }, )], - force_pushed_branches: hashset! {}, }; let result = git::push_branches( tx.mut_repo(), @@ -2786,7 +2782,9 @@ fn test_push_branches_not_fast_forward() { &targets, git::RemoteCallbacks::default(), ); - assert_eq!(result, Err(GitPushError::NotFastForward)); + // Short-term TODO: This test is now equivalent to the following one, and + // will be removed in a follow-up commit. + assert_eq!(result, Ok(())); } #[test] @@ -2804,9 +2802,6 @@ fn test_push_branches_not_fast_forward_with_force() { new_target: Some(setup.sideways_commit.id().clone()), }, )], - force_pushed_branches: hashset! { - "main".to_owned(), - }, }; let result = git::push_branches( tx.mut_repo(), @@ -2850,7 +2845,6 @@ fn test_push_updates_unexpectedly_moved_sideways_on_remote() { 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, }]; @@ -2918,7 +2912,6 @@ fn test_push_updates_unexpectedly_moved_forward_on_remote() { 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, }]; @@ -2977,7 +2970,6 @@ fn test_push_updates_unexpectedly_exists_on_remote() { 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, }]; @@ -3014,7 +3006,6 @@ fn test_push_updates_success() { "origin", &[GitRefUpdate { qualified_name: "refs/heads/main".to_string(), - force: false, expected_current_target: Some(setup.main_commit.id().clone()), new_target: Some(setup.child_of_main_commit.id().clone()), }], @@ -3052,7 +3043,6 @@ fn test_push_updates_no_such_remote() { "invalid-remote", &[GitRefUpdate { qualified_name: "refs/heads/main".to_string(), - force: false, expected_current_target: Some(setup.main_commit.id().clone()), new_target: Some(setup.child_of_main_commit.id().clone()), }], @@ -3072,7 +3062,6 @@ fn test_push_updates_invalid_remote() { "http://invalid-remote", &[GitRefUpdate { qualified_name: "refs/heads/main".to_string(), - force: false, expected_current_target: Some(setup.main_commit.id().clone()), new_target: Some(setup.child_of_main_commit.id().clone()), }], From ed72dfea20e30ae8ef6fb579c290010f65f9b8ba Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Sun, 12 May 2024 21:17:03 -0700 Subject: [PATCH 7/8] test_git.rs: delete now-useless test --- lib/tests/test_git.rs | 28 ---------------------------- 1 file changed, 28 deletions(-) diff --git a/lib/tests/test_git.rs b/lib/tests/test_git.rs index 59c7b9a0ac..959b303490 100644 --- a/lib/tests/test_git.rs +++ b/lib/tests/test_git.rs @@ -2766,34 +2766,6 @@ fn test_push_branches_not_fast_forward() { let setup = set_up_push_repos(&settings, &temp_dir); let mut tx = setup.jj_repo.start_transaction(&settings); - let targets = GitBranchPushTargets { - branch_updates: vec![( - "main".to_owned(), - BranchPushUpdate { - old_target: Some(setup.main_commit.id().clone()), - new_target: Some(setup.sideways_commit.id().clone()), - }, - )], - }; - let result = git::push_branches( - tx.mut_repo(), - &get_git_repo(&setup.jj_repo), - "origin", - &targets, - git::RemoteCallbacks::default(), - ); - // Short-term TODO: This test is now equivalent to the following one, and - // will be removed in a follow-up commit. - assert_eq!(result, Ok(())); -} - -#[test] -fn test_push_branches_not_fast_forward_with_force() { - let settings = testutils::user_settings(); - let temp_dir = testutils::new_temp_dir(); - let setup = set_up_push_repos(&settings, &temp_dir); - let mut tx = setup.jj_repo.start_transaction(&settings); - let targets = GitBranchPushTargets { branch_updates: vec![( "main".to_owned(), From 7dc80263eda71369a6e5e16e2b3dd920ba5061e4 Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Wed, 17 Apr 2024 21:59:33 -0700 Subject: [PATCH 8/8] jj git push: remove the NotFastForward error Now that we always force push, it should not occur in practice. --- cli/src/commands/git.rs | 5 ----- lib/src/git.rs | 12 +----------- 2 files changed, 1 insertion(+), 16 deletions(-) diff --git a/cli/src/commands/git.rs b/cli/src/commands/git.rs index a8b4cf0129..994868fbf6 100644 --- a/cli/src/commands/git.rs +++ b/cli/src/commands/git.rs @@ -1015,11 +1015,6 @@ 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::RefInUnexpectedLocation(refs) => user_error_with_hint( format!( "Refusing to push a branch that unexpectedly moved on the remote. Affected refs: \ diff --git a/lib/src/git.rs b/lib/src/git.rs index 3a90501055..6b8e132b41 100644 --- a/lib/src/git.rs +++ b/lib/src/git.rs @@ -1223,9 +1223,6 @@ pub enum GitPushError { name = REMOTE_NAME_FOR_LOCAL_GIT_REPO )] RemoteReservedForLocalGitRepo, - // Short-term TODO: Delete this; it should never trigger - #[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:?}?)")] @@ -1422,14 +1419,7 @@ fn push_refs( 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), - }) + remote.push(refspecs, Some(&mut push_options)) }; if !failed_push_negotiations.is_empty() { // If the push negotiation returned an error, `remote.push` would not