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

@@ -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);

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<String>,
 }
 
 pub struct GitRefUpdate {
@@ -1248,8 +1248,6 @@ pub struct GitRefUpdate {
     ///
     /// This is sourced from the local remote-tracking branch.
     pub expected_current_target: Option<CommitId>,
-    // Short-term TODO: Delete `force`
-    pub force: bool,
     pub new_target: Option<CommitId>,
 }
 
@@ -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 {

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<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,
         }];
@@ -2918,7 +2912,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,
         }];
@@ -2977,7 +2970,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,
         }];
@@ -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()),
         }],