Skip to content

Commit

Permalink
jj git push: safety checks in push negotiation, "force-with-lease"
Browse files Browse the repository at this point in the history 
As explained in the commit, our logic is a bit more complicated than
that of `git push --force-with-lease`. This is to match the behavior of
`jj git fetch` and branch conflict resolution rules.
  • Loading branch information
@ilyagr
ilyagr committed May 15, 2024
1 parent 72ed0ab commit 6dc7ca2
Show file tree
Hide file tree
Showing 6 changed files with 615 additions and 80 deletions.
12 changes: 12 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Fixed bugs

* Previously, `jj git push` only made sure that the branch is in the expected
location on the remote server when pushing a branch forward (as opposed to
sideways or backwards). Now, `jj git push` makes a safety check in all cases
and fails whenever `jj git fetch` would have introduced a conflict.

In other words, previously branches that moved sideways or backward were
pushed similarly to Git's `git push --force`; now they have protections
similar to `git push --force-with-lease` (though not identical to it, to match
the behavior of `jj git fetch`). Note also that because of the way `jj git
fetch` works, `jj` does not suffer from the same problems as Git's `git push
--force-with-lease` in situations when `git fetch` is run in the background.

* When the working copy commit becomes immutable, a new one is automatically created on top of it
to avoid letting the user edit the immutable one.

Expand Down
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
28 changes: 16 additions & 12 deletions cli/tests/test_git_push.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,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 @@ -233,15 +240,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 45a3aa29e907 to c35839cb8e8c
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 @@ -283,16 +286,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 45a3aa29e907 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 @@ -338,15 +341,16 @@ fn test_git_push_deletion_unexpectedly_moved() {
Hint: Branches marked as deleted will be *deleted permanently* on the remote on the next `jj git push`. Use `jj branch forget` to prevent this.
"###);

// 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 45a3aa29e907
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 +384,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 4c595cf9ac0a
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
227 changes: 168 additions & 59 deletions lib/src/git.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1210,6 +1210,10 @@ 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. {}",
render_ref_in_unexpected_location_details(.0)
)]
RefInUnexpectedLocation(Vec<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 All @@ -1218,6 +1222,10 @@ pub enum GitPushError {
InternalGitError(#[from] git2::Error),
}

fn render_ref_in_unexpected_location_details(refs: &[String]) -> String {
format!("Affected refs: {:?}", refs.iter().sorted().collect_vec())
}

#[derive(Clone, Debug)]
pub struct GitBranchPushTargets {
pub branch_updates: Vec<(String, BranchPushUpdate)>,
Expand Down Expand Up @@ -1293,9 +1301,8 @@ pub fn push_updates(
refspecs.push(format!(":{}", update.qualified_name));
}
}
// TODO(ilyagr): this function will be reused to implement force-with-lease, so
// don't inline for now. If it's after 2024-06-01 or so, ilyagr may have
// forgotten to delete this comment.
// TODO(ilyagr): `push_refs`, or parts of it, should probably be inlined. This
// requires adjusting some tests.
push_refs(
git_repo,
remote_name,
Expand Down Expand Up @@ -1324,64 +1331,166 @@ fn push_refs(
})?;
let mut remaining_remote_refs_expected_locations =
qualified_remote_refs_expected_locations.clone();
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_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());
// `update.src()` and `update.dst()` are the current position of the branch
// `dst_refname` on the server and the position we are trying to push the branch
// to. Both can also be 0000000000, indicating branch creation or deletion.

// Short-term TODO: Replace this with actual rules of when to permit the push
tracing::info!(
"Forcing {dst_refname} to {dst}. It was at {src} at the server. We expected it to \
be at {expected_location_oid}.",
src = update.src(),
dst = update.dst()
);
}
Ok(())
});
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
let mut failed_push_negotiations = vec![];
let push_result = {
let failed_push_negotiations_ref = &mut failed_push_negotiations;
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_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());

// `update.src()` and `update.dst()` are the current position of
// the branch `dst_refname` on the server and the position we
// are trying to push the branch to (AKA our local branch
// location). Both can also be 0000000000, indicating branch
// creation or deletion.
if update.src() != expected_location_oid {
// There are a few corner cases where we allow the push even
// though the branch is in an unexpected location. All of
// these exceptions **require** that the push is a
// "fast-forward" on the server: the push destination is a
// descendant of or equal to the unexpected location. There
// are additional conditions on `expected_location` that
// need to be satisfied; see the `if`` conditions below.
//
// This is sometimes more restrictive than Git's `git push`
// which would allow *all* fast-forwards to succeed,
// 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 the last
// statement.
//
// The reasoning for our rule is that we can consider this
// as 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 equivalence of our condition to the branch conflict
// resolution rules is checked in tests (currently in
// `test_git.rs`). If we ever change the rules of how branch
// conflicts are resolved, this logic should change as well.
// Alternatively, this equivalence is helpful as a guide but
// is not set in stone; we could change the behavior in
// corner cases if necessary, especially if we come up with
// another sensible rule to follow.
if update.src() == update.dst() {
// This is the situation of what we call "A - B + A = A"
// conflicts, see also test_refs.rs and
// https://github.com/martinvonz/jj/blob/c9b44f382824301e6c0fdd6f4cbc52bb00c50995/lib/src/merge.rs#L92.
tracing::info!(
"The push of {dst_refname} is unexpectedly a no-op, the remote branch \
is already at {dst}. We expected it to be at \
{expected_location_oid}. We don't consider this an error.",
dst = update.dst()
);
continue;
};
if git_repo
.graph_descendant_of(update.dst(), update.src())
.unwrap_or(false)
&& (expected_location_oid.is_zero()
|| git_repo
.graph_descendant_of(update.src(), expected_location_oid)
.unwrap_or(false))
{
tracing::info!(
"We allow the push of {dst_refname} to {dst}, even though it is \
unexpectedly at {src} on the server rather than the expected \
{expected_location_oid}. The desired location is a descendant of the \
actual location, and the actual location is a descendant of the \
expected location.",
src = update.src(),
dst = update.dst()
);
// TODO(ilyagr): We could consider printing a user-facing message at this
// point.
continue;
}
// While we show debug info in the message with `--verbose`,
// there's probably no need to show the detailed commit
// locations to the user normally. They should do a `jj git
// fetch`, and the resulting branch conflicts should contain
// all the information they need.
tracing::info!(
"Cannot push {dst_refname} to {dst}; it is at unexpectedly at {src} on \
the server as opposed to the expected {expected_location_oid}",
src = update.src(),
dst = update.dst()
);
failed_push_negotiations_ref.push(dst_refname.to_string());
}
}
_ => GitPushError::InternalGitError(err),
})?;
drop(push_options);
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(),
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() {
// If the push negotiation returned an error, `remote.push` would not
// have pushed anything and would have returned an error, as expected.
// However, the error it returns is not necessarily the error we'd
// expect. It also depends on the exact versions of `libgit2` and
// `git2.rs`. So, we cannot rely on it containing any useful
// information. See https://github.com/rust-lang/git2-rs/issues/1042.
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 Down
Loading

0 comments on commit 6dc7ca2

Please sign in to comment.