`jj git push`: safety checks when pushing branch sideways, similar to `git push --force-with-lease` #3522

ilyagr · 2024-04-17T14:35:37Z

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.

There are a few TODOs that may be added later (in this or separate PRs):

Document this behavior in jj help git push, possibly elsewhere.
Adjustments to user-facing UI
Additional tests

See TODOs in the code for more details. It's possible I forgot some, but it's probably time for others to look at the general form of this PR in any case.

Checklist

If applicable:

I have updated CHANGELOG.md
I have updated the documentation (README.md, docs/, demos/)
(n/a) I have updated the config schema (cli/src/config-schema.json)
I have added tests to cover my changes

lib/src/git.rs

ilyagr · 2024-05-15T15:58:13Z

Thanks, there is no rush at all.

cli/tests/test_git_push.rs

lib/src/git.rs

yuja · 2024-05-19T03:33:03Z

lib/src/git.rs

+        // information. See https://github.com/rust-lang/git2-rs/issues/1042.
+        assert!(push_result.is_err());
+        Err(GitPushError::RefInUnexpectedLocation(
+            failed_push_negotiations,


nit: sort here to stabilize error representation?

yuja · 2024-05-19T03:33:58Z

lib/tests/test_git.rs

@@ -19,6 +19,7 @@ use std::{fs, iter, thread};

 use assert_matches::assert_matches;
 use git2::Oid;
+use insta::{assert_debug_snapshot, assert_snapshot};


nit: we usually spell insta::assert_*() at call sites (I don't know why, though.)

FWIW, I wouldn't mind a PR that consistently imports the symbols.

I'm OK with either convention. Explicit insta:: might remind new people this is related to cargo insta.

I don't mind either, but I think most of the insta assertions in test_git.rs can be removed.

yuja · 2024-05-19T03:35:30Z

lib/tests/test_git.rs

+            ),
+        ],
+    )
+    "###


Suppose these assertions are sanity checks, it's probably better to just assert!(...has_conflicts()). Insta snapshots are verbose.

yuja · 2024-05-19T03:37:37Z

lib/tests/test_git.rs

+    "###
+    );
+    let result = attempt_push_expecting_sideways(None);
+    assert_snapshot!(format!("{result:?}"), @r###"Err(RefInUnexpectedLocation(["refs/heads/main"]))"###);


nit: it can be insta::assert_debug_snapshot!(), but I would probably use assert_matches!(.., Err(RefInUnexpectedLocation(_))) since the failed ref names aren't important.

lib/src/git.rs

yuja · 2024-05-25T08:40:34Z

lib/src/git.rs

+                    // 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).


Thanks. The allow_push() function looks unnecessarily verbose, but I think it'll be cleaned up. Overall, I like it.

Adds two tests where pushing should fail, but currently succeeds.

This tests `git push` attempting to create a branch when the branch already unexpectedly exists on the remote. This should (and does) fail. Also changes another test to use `jj_cmd_failure`.

The new names are a bit clearer. Soem tests already use a `sideways_commit`, `parent_of_main_commit` will be needed in an upcoming test.

Hopefully, splitting the no-op portion will make the following commits easier to review.

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.

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).

Now that we always force push, it should not occur in practice.

"Move forward" instead of "Move", "Move sideways" or "Move backward" instead of (now misleading) "Force...".

ilyagr mentioned this pull request Apr 17, 2024

lib git.rs: remove workaround for a now-fixed libgit2 bug #3516

Merged

ilyagr force-pushed the force-with-lease branch 8 times, most recently from 64c74f3 to 2d13e97 Compare April 18, 2024 21:13

necauqua reviewed Apr 18, 2024

View reviewed changes

lib/src/git.rs Outdated Show resolved Hide resolved

lib/src/git.rs Outdated Show resolved Hide resolved

ilyagr force-pushed the force-with-lease branch 6 times, most recently from 81b11aa to f7cec4f Compare April 20, 2024 06:51

ilyagr force-pushed the force-with-lease branch from f7cec4f to 61e5cd1 Compare May 2, 2024 05:15

ilyagr force-pushed the force-with-lease branch 13 times, most recently from 8b7e930 to bb9547c Compare May 12, 2024 00:06

yuja reviewed May 19, 2024

View reviewed changes

ilyagr force-pushed the force-with-lease branch 6 times, most recently from 8cd786a to bb9739c Compare May 21, 2024 00:23

ilyagr marked this pull request as draft May 24, 2024 18:00

ilyagr force-pushed the force-with-lease branch 7 times, most recently from 181bd7d to af5b193 Compare May 25, 2024 04:45

yuja reviewed May 25, 2024

View reviewed changes

ilyagr added 7 commits May 25, 2024 14:28

test_git_push: demo behavior we'd like to be safer

ea82ea3

Adds two tests where pushing should fail, but currently succeeds.

test_git_push: add a test for creating unexpectedly existing branch

51a8c4c

This tests `git push` attempting to create a branch when the branch already unexpectedly exists on the remote. This should (and does) fail. Also changes another test to use `jj_cmd_failure`.

test_git.rs: add two more commits to PushSetup, rename them

35130ce

The new names are a bit clearer. Soem tests already use a `sideways_commit`, `parent_of_main_commit` will be needed in an upcoming test.

jj git push: no-op push negotiation, plumb data to it

1ac675f

Hopefully, splitting the no-op portion will make the following commits easier to review.

jj git push: safety checks in push negotiation, "force-with-lease"

d032ced

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.

return more info from allow_push (TO AMEND)

9447467

print_debug_message (to amend?)

46663fa

ilyagr force-pushed the force-with-lease branch from af5b193 to e73cf47 Compare May 25, 2024 22:23

ilyagr added 4 commits May 25, 2024 15:23

jj git push: always force-push, all safety logic now in push_negotiation

7a119ef

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).

test_git.rs: delete now-useless test

17337fb

jj git push: remove the NotFastForward error

f39d72e

Now that we always force push, it should not occur in practice.

cli git push: clearer user-facing messages TODO: test backward

9396979

"Move forward" instead of "Move", "Move sideways" or "Move backward" instead of (now misleading) "Force...".

ilyagr force-pushed the force-with-lease branch from e73cf47 to 9396979 Compare May 25, 2024 22:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`jj git push`: safety checks when pushing branch sideways, similar to `git push --force-with-lease` #3522

`jj git push`: safety checks when pushing branch sideways, similar to `git push --force-with-lease` #3522

ilyagr commented Apr 17, 2024 •

edited

ilyagr commented May 15, 2024

yuja May 19, 2024

yuja May 19, 2024

martinvonz May 21, 2024

ilyagr May 21, 2024

yuja May 21, 2024

yuja May 19, 2024

yuja May 19, 2024

yuja May 25, 2024

jj git push: safety checks when pushing branch sideways, similar to git push --force-with-lease #3522

Are you sure you want to change the base?

jj git push: safety checks when pushing branch sideways, similar to git push --force-with-lease #3522

Conversation

ilyagr commented Apr 17, 2024 • edited

Checklist

ilyagr commented May 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

`jj git push`: safety checks when pushing branch sideways, similar to `git push --force-with-lease` #3522

`jj git push`: safety checks when pushing branch sideways, similar to `git push --force-with-lease` #3522

ilyagr commented Apr 17, 2024 •

edited