git: force push when not known to be a fast-forward

With this change, we no longer fail if the user moves a branch
sideways or backwards and then push.

The push should ideally only succeed if the remote branch is where we
thought it was (like `git push --force-with-lease`), but that requires
rust-lang/git2-rs#733 to be fixed first.
This commit is contained in:
Martin von Zweigbergk 2021-08-04 14:30:06 -07:00
parent d555e0c326
commit 81ba65e3a5
3 changed files with 66 additions and 5 deletions

View file

@ -173,6 +173,10 @@ pub fn push_commit(
target: &Commit,
remote_name: &str,
remote_branch: &str,
// TODO: We want this to be an Option<CommitId> 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).
force: bool,
) -> Result<(), GitPushError> {
// Create a temporary ref to work around https://github.com/libgit2/libgit2/issues/3178
let temp_ref_name = format!("refs/jj/git-push/{}", target.id().hex());
@ -184,7 +188,12 @@ pub fn push_commit(
)?;
// Need to add "refs/heads/" prefix due to https://github.com/libgit2/libgit2/issues/1125
let qualified_remote_branch = format!("refs/heads/{}", remote_branch);
let refspec = format!("{}:{}", temp_ref_name, qualified_remote_branch);
let refspec = format!(
"{}{}:{}",
(if force { "+" } else { "" }),
temp_ref_name,
qualified_remote_branch
);
let result = push_ref(git_repo, remote_name, &qualified_remote_branch, &refspec);
// TODO: Figure out how to do the equivalent of absl::Cleanup for
// temp_ref.delete().
@ -196,6 +205,8 @@ pub fn delete_remote_branch(
git_repo: &git2::Repository,
remote_name: &str,
remote_branch: &str,
/* TODO: Similar to push_commit(), we want an CommitId for the expected current commit on
* the remote. */
) -> Result<(), GitPushError> {
// Need to add "refs/heads/" prefix due to https://github.com/libgit2/libgit2/issues/1125
let qualified_remote_branch = format!("refs/heads/{}", remote_branch);

View file

@ -353,7 +353,7 @@ fn test_push_commit_success() {
let temp_dir = tempfile::tempdir().unwrap();
let setup = set_up_push_repos(&settings, &temp_dir);
let clone_repo = setup.jj_repo.store().git_repo().unwrap();
let result = git::push_commit(&clone_repo, &setup.new_commit, "origin", "main");
let result = git::push_commit(&clone_repo, &setup.new_commit, "origin", "main", false);
assert_eq!(result, Ok(()));
// Check that the ref got updated in the source repo
@ -388,10 +388,38 @@ fn test_push_commit_not_fast_forward() {
&new_commit,
"origin",
"main",
false,
);
assert_eq!(result, Err(GitPushError::NotFastForward));
}
#[test]
fn test_push_commit_not_fast_forward_with_force() {
let settings = testutils::user_settings();
let temp_dir = tempfile::tempdir().unwrap();
let mut setup = set_up_push_repos(&settings, &temp_dir);
let new_commit = testutils::create_random_commit(&settings, &setup.jj_repo)
.write_to_new_transaction(&setup.jj_repo, "test");
setup.jj_repo = setup.jj_repo.reload();
let result = git::push_commit(
&setup.jj_repo.store().git_repo().unwrap(),
&new_commit,
"origin",
"main",
true,
);
assert_eq!(result, Ok(()));
// Check that the ref got updated in the source repo
let source_repo = git2::Repository::open(&setup.source_repo_dir).unwrap();
let new_target = source_repo
.find_reference("refs/heads/main")
.unwrap()
.target();
let new_oid = Oid::from_bytes(&new_commit.id().0).unwrap();
assert_eq!(new_target, Some(new_oid));
}
#[test]
fn test_push_commit_no_such_remote() {
let settings = testutils::user_settings();
@ -402,6 +430,7 @@ fn test_push_commit_no_such_remote() {
&setup.new_commit,
"invalid-remote",
"main",
false,
);
assert!(matches!(result, Err(GitPushError::NoSuchRemote(_))));
}
@ -416,6 +445,7 @@ fn test_push_commit_invalid_remote() {
&setup.new_commit,
"http://invalid-remote",
"main",
false,
);
assert!(matches!(result, Err(GitPushError::NoSuchRemote(_))));
}

View file

@ -2585,7 +2585,8 @@ fn cmd_git_push(
}
let branch_target = maybe_branch_target.unwrap();
if branch_target.local_target.as_ref() == branch_target.remote_targets.get(remote_name) {
let maybe_remote_target = branch_target.remote_targets.get(remote_name);
if branch_target.local_target.as_ref() == maybe_remote_target {
writeln!(
ui,
"Branch {}@{} already matches {}",
@ -2610,8 +2611,27 @@ fn cmd_git_push(
"Won't push open commit".to_string(),
));
}
git::push_commit(&git_repo, &new_target_commit, remote_name, branch_name)
.map_err(|err| CommandError::UserError(err.to_string()))?;
let force = match maybe_remote_target {
None => false,
Some(RefTarget::Conflict { .. }) => {
return Err(CommandError::UserError(format!(
"Branch {}@{} is conflicted",
branch_name, remote_name
)));
}
Some(RefTarget::Normal(old_target_id)) => {
!repo.index().is_ancestor(old_target_id, new_target_id)
}
};
git::push_commit(
&git_repo,
&new_target_commit,
remote_name,
branch_name,
force,
)
.map_err(|err| CommandError::UserError(err.to_string()))?;
}
}
} else {