Skip to content

Commit

Permalink
Match branch conflict rules
Browse files Browse the repository at this point in the history
  • Loading branch information
@ilyagr
ilyagr committed May 9, 2024
1 parent 3ff3980 commit b37ee77
Show file tree
Hide file tree
Showing 5 changed files with 359 additions and 70 deletions.
12 changes: 7 additions & 5 deletions cli/src/commands/git.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1014,11 +1014,13 @@ fn cmd_git_push(
})
.map_err(|err| match err {
GitPushError::InternalGitError(err) => map_git_error(err),
GitPushError::NotFastForward => user_error_with_hint(
err.to_string(),
"Try fetching from the remote, then make the branch point to where you want it to be, \
and push again.",
),
GitPushError::NotFastForward | GitPushError::RefInUnexpectedLocation(_) => {
user_error_with_hint(
err.to_string(),
"Try fetching from the remote, then make the branch point to where you want it to \
be, and push again.",
)
}
_ => user_error(err),
})?;
writer.flush(ui)?;
Expand Down
42 changes: 22 additions & 20 deletions cli/tests/test_git_push.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,14 @@ fn test_git_push_other_remote_has_branch() {
Warning: No branches found in the default push revset: remote_branches(remote=origin)..@
Nothing changed.
"###);
// But it will still get pushed to another remote
// The branch was moved on the "other" remote as well (since it's actually the
// same remote), but `jj` is not aware of that since it thinks this is a
// different remote. So, the push should fail.
//
// But it succeeds! That's because the branch is created at the same location as
// it is on the remote. This would also work for a descendant.
//
// TODO: Saner test?
let (stdout, stderr) = test_env.jj_cmd_ok(&workspace_root, &["git", "push", "--remote=other"]);
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###"
Expand Down Expand Up @@ -234,15 +241,11 @@ fn test_git_push_not_fast_forward() {
insta::assert_snapshot!(get_stderr_string(&assert), @r###"
Branch changes to push to origin:
Move branch branch1 from 3571d60e8503 to c5ab2d309152
Error: The push conflicts with changes made on the remote (it is not fast-forwardable).
Error: Refusing to push a branch that unexpectedly moved on the remote. Affected refs: ["refs/heads/branch1"]
Hint: Try fetching from the remote, then make the branch point to where you want it to be, and push again.
"###);
}

// Short-term TODO: implement this.
// This tests whether the push checks that the remote branches are in expected
// positions. Once this is implemented, `jj git push` will be similar to `git
// push --force-with-lease`
#[test]
fn test_git_push_sideways_unexpectedly_moved() {
let (test_env, workspace_root) = set_up();
Expand Down Expand Up @@ -284,16 +287,16 @@ fn test_git_push_sideways_unexpectedly_moved() {
"###);
insta::assert_snapshot!(get_stderr_string(&assert), @"");

// BUG: Pushing should fail. Currently, it succeeds because moving the branch
// sideways causes `jj` to use the analogue of `git push --force` when pushing.
let assert = test_env
.jj_cmd(&workspace_root, &["git", "push"])
.assert()
.success();
.failure();
insta::assert_snapshot!(get_stdout_string(&assert), @"");
insta::assert_snapshot!(get_stderr_string(&assert), @r###"
Branch changes to push to origin:
Force branch branch1 from 3571d60e8503 to eb921361206c
Error: Refusing to push a branch that unexpectedly moved on the remote. Affected refs: ["refs/heads/branch1"]
Hint: Try fetching from the remote, then make the branch point to where you want it to be, and push again.
"###);
}

Expand Down Expand Up @@ -338,15 +341,16 @@ fn test_git_push_deletion_unexpectedly_moved() {
"###);
insta::assert_snapshot!(get_stderr_string(&assert), @"");

// BUG: Pushing should fail because the branch was moved on the remote
let assert = test_env
.jj_cmd(&workspace_root, &["git", "push", "--branch", "branch1"])
.assert()
.success();
.failure();
insta::assert_snapshot!(get_stdout_string(&assert), @"");
insta::assert_snapshot!(get_stderr_string(&assert), @r###"
Branch changes to push to origin:
Delete branch branch1 from 3571d60e8503
Error: Refusing to push a branch that unexpectedly moved on the remote. Affected refs: ["refs/heads/branch1"]
Hint: Try fetching from the remote, then make the branch point to where you want it to be, and push again.
"###);
}

Expand Down Expand Up @@ -393,11 +397,13 @@ fn test_git_push_sideways_unexpectedly_deleted() {
let assert = test_env
.jj_cmd(&workspace_root, &["git", "push"])
.assert()
.success();
.failure();
insta::assert_snapshot!(get_stdout_string(&assert), @"");
insta::assert_snapshot!(get_stderr_string(&assert), @r###"
Branch changes to push to origin:
Force branch branch1 from 3571d60e8503 to 0f8bf988588e
Error: Refusing to push a branch that unexpectedly moved on the remote. Affected refs: ["refs/heads/branch1"]
Hint: Try fetching from the remote, then make the branch point to where you want it to be, and push again.
"###);
}

Expand Down Expand Up @@ -431,7 +437,7 @@ fn test_git_push_creation_unexpectedly_already_exists() {
insta::assert_snapshot!(get_stderr_string(&assert), @r###"
Branch changes to push to origin:
Add branch branch1 to 95344a1ab28a
Error: The push conflicts with changes made on the remote (it is not fast-forwardable).
Error: Refusing to push a branch that unexpectedly moved on the remote. Affected refs: ["refs/heads/branch1"]
Hint: Try fetching from the remote, then make the branch point to where you want it to be, and push again.
"###);
}
Expand Down Expand Up @@ -548,15 +554,16 @@ fn test_git_push_fastforward_but_unexpectedly_moved_sideways_on_remote() {
insta::assert_snapshot!(get_stderr_string(&assert), @"");
test_env.jj_cmd_ok(&origin_path, &["git", "export"]);

// Pushing succeeds, but for a wrong reason?
let assert = test_env
.jj_cmd(&workspace_root, &["git", "push", "-bbranch1"])
.assert()
.success();
.failure();
insta::assert_snapshot!(get_stdout_string(&assert), @"");
insta::assert_snapshot!(get_stderr_string(&assert), @r###"
Branch changes to push to origin:
Force branch branch1 from a0c93395c0a3 to 7a722cf0a4d8
Error: Refusing to push a branch that unexpectedly moved on the remote. Affected refs: ["refs/heads/branch1"]
Hint: Try fetching from the remote, then make the branch point to where you want it to be, and push again.
"###);
}

Expand All @@ -574,10 +581,6 @@ fn test_git_push_fastforward_but_unexpectedly_moved_sideways_on_remote() {
/// |
/// o root
///
/// This currently causes `jj` to treat this as a fast-forward, which succeeds
/// because the remote doesn't care about the location of our local
/// remote-tracking branch.
///
/// AFAIU, regular Git would also consider this a fast-forward and succeed on a
/// `git push`, but `git push --force-with-lease` would fail.
///
Expand Down Expand Up @@ -678,7 +681,6 @@ fn test_git_push_fastforward_but_unexpectedly_moved_up_on_remote() {
insta::assert_snapshot!(get_stderr_string(&assert), @"");
test_env.jj_cmd_ok(&origin_path, &["git", "export"]);

// Pushing succeeds, but for a wrong reason?
let assert = test_env
.jj_cmd(&workspace_root, &["git", "push", "-bbranch1"])
.assert()
Expand Down
199 changes: 156 additions & 43 deletions lib/src/git.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1210,6 +1210,14 @@ pub enum GitPushError {
RemoteReservedForLocalGitRepo,
#[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. Affected refs: {:?}",
.0
.iter()
.sorted()
.map(|option| option.clone().unwrap_or_default())
.collect_vec()
)]
RefInUnexpectedLocation(Vec<Option<String>>),
#[error("Remote rejected the update of some refs (do you have permission to push to {0:?}?)")]
RefUpdateRejected(Vec<String>),
// TODO: I'm sure there are other errors possible, such as transport-level errors,
Expand Down Expand Up @@ -1275,11 +1283,15 @@ pub fn push_updates(
updates: &[GitRefUpdate],
callbacks: RemoteCallbacks<'_>,
) -> Result<(), GitPushError> {
let mut qualified_remote_refs = vec![];
let mut qualified_remote_refs_expected_locations = HashMap::new();
let mut refspecs = vec![];
for update in updates {
qualified_remote_refs.push(update.qualified_name.as_str());
qualified_remote_refs_expected_locations.insert(
update.qualified_name.as_str(),
&update.expected_current_target,
);
if let Some(new_target) = &update.new_target {
// TODO: What branches should be force-pushed after force-with-lease??
refspecs.push(format!(
"{}{}:{}",
(if update.force { "+" } else { "" }),
Expand All @@ -1296,7 +1308,7 @@ pub fn push_updates(
push_refs(
git_repo,
remote_name,
&qualified_remote_refs,
&qualified_remote_refs_expected_locations,
&refspecs,
callbacks,
)
Expand All @@ -1305,53 +1317,153 @@ pub fn push_updates(
fn push_refs(
git_repo: &git2::Repository,
remote_name: &str,
qualified_remote_refs: &[&str],
qualified_remote_refs_expected_locations: &HashMap<&str, &Option<CommitId>>,
// expected_remote_ref_locations
refspecs: &[String],
callbacks: RemoteCallbacks<'_>,
) -> Result<(), GitPushError> {
if remote_name == REMOTE_NAME_FOR_LOCAL_GIT_REPO {
return Err(GitPushError::RemoteReservedForLocalGitRepo);
}
let mut remote = git_repo.find_remote(remote_name).map_err(|err| {
if is_remote_not_found_err(&err) {
GitPushError::NoSuchRemote(remote_name.to_string())
} else {
GitPushError::InternalGitError(err)
}
})?;
let mut remaining_remote_refs: HashSet<_> = qualified_remote_refs.iter().copied().collect();
let mut push_options = git2::PushOptions::new();
let mut proxy_options = git2::ProxyOptions::new();
proxy_options.auto();
push_options.proxy_options(proxy_options);
let mut callbacks = callbacks.into_git();
callbacks.push_update_reference(|refname, status| {
// The status is Some if the ref update was rejected
if status.is_none() {
remaining_remote_refs.remove(refname);
let mut remaining_remote_refs_expected_locations =
qualified_remote_refs_expected_locations.clone();
let mut failed_push_negotiations = vec![];
let push_result = {
if remote_name == REMOTE_NAME_FOR_LOCAL_GIT_REPO {
return Err(GitPushError::RemoteReservedForLocalGitRepo);
}
Ok(())
});
push_options.remote_callbacks(callbacks);
remote
.push(refspecs, Some(&mut push_options))
.map_err(|err| match (err.class(), err.code()) {
(git2::ErrorClass::Reference, git2::ErrorCode::NotFastForward) => {
GitPushError::NotFastForward
let mut remote = git_repo.find_remote(remote_name).map_err(|err| {
if is_remote_not_found_err(&err) {
GitPushError::NoSuchRemote(remote_name.to_string())
} else {
GitPushError::InternalGitError(err)
}
_ => GitPushError::InternalGitError(err),
})?;
drop(push_options);
if remaining_remote_refs.is_empty() {
Ok(())
} else {
Err(GitPushError::RefUpdateRejected(
remaining_remote_refs
.iter()
.sorted()
.map(|name| name.to_string())
.collect(),
let mut callbacks = callbacks.into_git();
let mut push_options = git2::PushOptions::new();
let mut proxy_options = git2::ProxyOptions::new();
proxy_options.auto();
push_options.proxy_options(proxy_options);
let failed_push_negotiations_ref = &mut failed_push_negotiations;
callbacks.push_negotiation(|updates| {
for update in updates {
let dst_refname = update
.dst_refname()
.expect("Expect reference name to be valid UTF-8");
let expected_location = (*qualified_remote_refs_expected_locations
.get(dst_refname)
.expect("Push is trying to move a ref it wasn't asked to move"))
.clone();
let expected_location_oid = expected_location
.map(|id| git2::Oid::from_bytes(id.as_bytes()).unwrap())
.unwrap_or(git2::Oid::zero());
/* dbg!(
update.src_refname(), // Position to update the branch to (!)
update.dst_refname(), // Branch to update
update.dst(), // Commit id of `src_refname` (!)
update.src(), // Current position
&expected_location
); */
// See also https://github.com/git/git/blob/master/templates/hooks--pre-push.sample
// https://github.com/libgit2/libgit2/commit/efc2fec50e3bb725b2b244fdeacfb2dfad6ee78e
// src is the current position on target or 00000000000 if it's
// not on the remote. `src_refname`` seems to be the destination
// where `dst_refname` is set to.
if update.src() != expected_location_oid {
// There are two corner cases where we allow the push even
// though the branch is in an unexpected location. This
// happpens when either:

Check failure on line 1372 in lib/src/git.rs

View workflow job for this annotation

GitHub Actions / Codespell 

happpens ==> happens
//
// 1) the destination is the same as the unexpected location (so the push
// turns out to be a no-op) or
// 2) the unexpected location is an ancestor of the push destination (so the
// push is a fast-forward in Git parlance) **and** it is a descendant of
// the expected location.
//
// This is slightly different from Git's `git push` which
// would consider the situation a fast-forward as long as
// the unexpected location is an ancestor of the destination
// (or equal to it), regardless of where the expected
// location is. The difference seems like it would matter
// rarely in practice. Our rule is also different from Git's
// `git push --force-with-lease`, which would AFAIK not
// allow any corner cases at all. TODO(ilyagr): Double-check
// last statement.
//
// The reasoning for our rule is that we essentially have a
// branch conflict with 2 sides, `update.dst()` (our branch
// location) and `update.src()` (their branch location), and
// with `expected_location` as the base of the conflict. In
// fact, if the user performed a `jj git fetch` instead of
// `jj git push` they are performing, they would get exactly
// this branch conflict. We decide to allow the push **if
// and only if** `jj` would auto-resolve this conflict to
// our location, `update.dst()`. Conveniently, this also
// means that the push will be successful **if and only if**
// `jj git fetch` is a no-op for this branch (and, in
// particular, would not create a conflict).
//
// The logic from the previous paragraph allows exactly the
// two corner case described above, as can be seen in
// `test_refs.rs`. This is true even if we also consider the
// cases where some of the three members of the branch
// conflict do not exist, e.g. branch creation or deletion.
//
// TODO: Double-check this with tests, refer to them in the
// comment.
if update.src() == update.dst()
|| (git_repo
.graph_descendant_of(update.src(), expected_location_oid)
.unwrap_or(false)
&& git_repo
.graph_descendant_of(update.dst(), update.src())
.unwrap_or(false))
{
continue;
}
failed_push_negotiations_ref.push(update.dst_refname().map(|s| s.to_string()));
}
}
if failed_push_negotiations_ref.is_empty() {
Ok(())
} else {
Err(git2::Error::from_str("failed push negotiation"))
}
});
callbacks.push_update_reference(|refname, status| {
// The status is Some if the ref update was rejected
if status.is_none() {
remaining_remote_refs_expected_locations.remove(refname);
}
Ok(())
});
push_options.remote_callbacks(callbacks);
remote
.push(refspecs, Some(&mut push_options))
.map_err(|err| match (err.class(), err.code()) {
(git2::ErrorClass::Reference, git2::ErrorCode::NotFastForward) => {
GitPushError::NotFastForward
}
_ => GitPushError::InternalGitError(err),
})
};
if !failed_push_negotiations.is_empty() {
// TODO: move this into `map_err` above (if possible) once
// https://github.com/rust-lang/git2-rs/issues/1042 is fully resolved.
assert!(push_result.is_err());
Err(GitPushError::RefInUnexpectedLocation(
failed_push_negotiations,
))
} else {
push_result?;
if remaining_remote_refs_expected_locations.is_empty() {
Ok(())
} else {
Err(GitPushError::RefUpdateRejected(
remaining_remote_refs_expected_locations
.keys()
.sorted()
.map(|name| name.to_string())
.collect(),
))
}
}
}

Expand All @@ -1364,6 +1476,7 @@ pub struct RemoteCallbacks<'a> {
pub get_ssh_keys: Option<&'a mut dyn FnMut(&str) -> Vec<PathBuf>>,
pub get_password: Option<&'a mut dyn FnMut(&str, &str) -> Option<String>>,
pub get_username_password: Option<&'a mut dyn FnMut(&str) -> Option<(String, String)>>,
// pub push_negotiation: Option<&'a mut dyn FnMut(&[git2::PushUpdate<'_>]) -> bool>,
}

impl<'a> RemoteCallbacks<'a> {
Expand Down
Loading

0 comments on commit b37ee77

Please sign in to comment.