diff --git a/cli/src/commands/git.rs b/cli/src/commands/git.rs index 256aa62364..6ecf3af2e7 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![]; + // TODO(ilyagr): `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 61fdd68212..0d08c08eec 100644 --- a/lib/src/git.rs +++ b/lib/src/git.rs @@ -15,7 +15,7 @@ #![allow(missing_docs)] use std::borrow::Cow; -use std::collections::{BTreeMap, HashMap, HashSet}; +use std::collections::{BTreeMap, HashMap}; use std::default::Default; use std::io::Read; use std::path::PathBuf; @@ -1208,6 +1208,7 @@ pub enum GitPushError { name = REMOTE_NAME_FOR_LOCAL_GIT_REPO )] RemoteReservedForLocalGitRepo, + // TODO: Delete this? It should never trigger #[error("The push conflicts with changes made on the remote (it is not fast-forwardable).")] NotFastForward, #[error("Refusing to push a branch that unexpectedly moved on the remote. {}", @@ -1229,14 +1230,11 @@ fn render_ref_in_unexpected_location_details(refs: &[String]) -> String { #[derive(Clone, Debug)] pub struct GitBranchPushTargets { pub branch_updates: Vec<(String, BranchPushUpdate)>, - pub force_pushed_branches: HashSet, } pub struct GitRefUpdate { pub qualified_name: String, pub expected_current_target: Option, - // Short-term TODO: Delete `force` - pub force: bool, pub new_target: Option, } @@ -1253,7 +1251,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(), }) @@ -1291,13 +1288,14 @@ pub fn push_updates( &update.expected_current_target, ); 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)); } } diff --git a/lib/tests/test_git.rs b/lib/tests/test_git.rs index f1fba12106..97e5760e69 100644 --- a/lib/tests/test_git.rs +++ b/lib/tests/test_git.rs @@ -2560,7 +2560,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(), @@ -2630,7 +2629,6 @@ fn test_push_branches_deletion() { new_target: None, }, )], - force_pushed_branches: hashset! {}, }; let result = git::push_branches( tx.mut_repo(), @@ -2688,7 +2686,6 @@ fn test_push_branches_mixed_deletion_and_addition() { }, ), ], - force_pushed_branches: hashset! {}, }; let result = git::push_branches( tx.mut_repo(), @@ -2748,7 +2745,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(), @@ -2757,7 +2753,13 @@ 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_debug_snapshot!(result, @r###" + Ok( + (), + ) + "###); } #[test] @@ -2775,9 +2777,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(), @@ -2842,7 +2841,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, }]; @@ -3004,7 +3002,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, }]; @@ -3132,7 +3129,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, }]; @@ -3183,7 +3179,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()), }], @@ -3220,7 +3215,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()), }], @@ -3239,7 +3233,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()), }],