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).
jj-vcs · May 15, 2024 · 1947218 · 1947218
1 parent 6dc7ca2
commit 1947218
Show file tree

Hide file tree

Showing 3 changed files with 21 additions and 29 deletions.
diff --git 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
@@ -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<String>,
 }
 
 pub struct GitRefUpdate {
     pub qualified_name: String,
     pub expected_current_target: Option<CommitId>,
-    // Short-term TODO: Delete `force`
-    pub force: bool,
     pub new_target: Option<CommitId>,
 }
 
@@ -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
@@ -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<CommitId>| {
         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<CommitId>| {
         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<CommitId>| {
         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()),
         }],