Skip to content

Commit

Permalink
Actual push negotiation TODOs: can NotFastForward happen? Remove `for…
Browse files Browse the repository at this point in the history 
…ce`?

Thanks to rust-lang/git2-rs#926 and @karin0 for
making this possible.
  • Loading branch information
@ilyagr
ilyagr committed Apr 19, 2024
1 parent 9b3c116 commit 81b11aa
Show file tree
Hide file tree
Showing 3 changed files with 140 additions and 71 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
47 changes: 24 additions & 23 deletions cli/tests/test_git_push.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -200,12 +200,15 @@ 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
let (stdout, stderr) = test_env.jj_cmd_ok(&workspace_root, &["git", "push", "--remote=other"]);
insta::assert_snapshot!(stdout, @"");
// 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 fails.
let stderr = test_env.jj_cmd_failure(&workspace_root, &["git", "push", "--remote=other"]);
insta::assert_snapshot!(stderr, @r###"
Branch changes to push to other:
Add branch branch1 to d73041288714
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 @@ -234,15 +237,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 +283,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 @@ -342,11 +341,13 @@ fn test_git_push_deletion_unexpectedly_moved() {
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 @@ -380,7 +381,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 @@ -497,15 +498,17 @@ 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?
// BUG?
let assert = test_env
.jj_cmd(&workspace_root, &["git", "push", "-bbranch1"])
.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 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 @@ -523,10 +526,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 @@ -627,15 +626,17 @@ 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?
// BUG?
let assert = test_env
.jj_cmd(&workspace_root, &["git", "push", "-bbranch1"])
.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:
Move branch branch1 from 8144f454db0f to 9a0388ceac79
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
152 changes: 109 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,106 @@ 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 {
// TODO: What would it mean if dst_refname was empty? AH, NEED TO USE dst
// INSTEAD!
let expected_location = update.dst_refname().and_then(|refname| {
(*qualified_remote_refs_expected_locations
.get(refname)
.expect("Push is trying to move a ref it wasn't asked to move"))
.clone()
});
/* 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(), // Expected 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 Some(update.src())
.filter(|oid| !oid.is_zero())
.map(|oid| oid.as_bytes().to_vec())
!= expected_location.map(|commitid| commitid.as_bytes().to_vec())
/* TODO: Check if src and src_refname point to different things, if not this is a
* no-op */
{
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 +1429,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

0 comments on commit 81b11aa

Please sign in to comment.