From 777da995337fad4c5009e9858cb946c616d37905 Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Tue, 16 Apr 2024 19:13:48 -0700 Subject: [PATCH] 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()), }],