`jj git push` docs: document safety checks #3473

ilyagr · 2024-04-08T21:40:31Z

As discussed in
https://discord.com/channels/968932220549103686/1226363798483636265/1226415448615288864

bnjmnt4n

Thanks for adding this!

yuja · 2024-04-09T02:16:53Z

cli/src/commands/git.rs

+///    do not match, `jj` will refuse to push the branch. In this case, you need
+///    to `jj git fetch --remote <remote name>`. This may create some branch
+///    conflicts, which you'll need to resolve before trying `jj git push`
+///    again.


I don't remember the exact implementation, but the first statement seems inaccurate.

What we do is basically checking if local branch is diverged from the (local) remote-tracking branch, and enable force push only when needed. There's a TODO to make force push safer by using force-with-lease, but it's not implemented yet.

I'm a bit confused and unsure what precisely you mean. I might un-confuse myself after I do some research, but if you could remind me what you are worried about, it might help. My memory of what happens exactly is hazy; here are a few possibilities of what might be the problem that came to my mind.

Let's say the local branch moved sideways and the remote branch moved somewhere else. I'm pretty sure (or at least I very much hope) jj git push would fail in this case. However, if "force push" was enabled, though, because the local branch moved sideways, it would succeed. Do you think that's a possibility? (I might look at the code tomorrow, if I do this will be the first thing I'll think about).

I'm also realizing that what I wrote doesn't take into account the difference between jj's remote-tracking branches and the git repo's remote-tracking branch refs. I hope this difference is not very material, but I could be forgetting something important.

If what I wrote is correct, and the difference between git's and jj's remote tracking branches is immaterial, I'm not sure whether using Git's "force-with-lease" would gain us anything.

Let's say the local branch moved sideways and the remote branch moved somewhere else. I'm pretty sure (or at least I very much hope) jj git push would fail in this case. However, if "force push" was enabled, though, because the local branch moved sideways, it would succeed. Do you think that's a possibility? (I might look at the code tomorrow, if I do this will be the first thing I'll think about).

Yes, I'm pretty sure that's a possibility. That's why we want to switch to force-with-lease.

Let's say the local branch moved sideways and the remote branch moved somewhere else. I'm pretty sure (or at least I very much hope) jj git push would fail in this case.

jj git push will do force push in that situation, which will probably succeed. "force-with-lease" will fix the problem, but it's not implemented yet iirc.

I'm also realizing that what I wrote doesn't take into account the difference between jj's remote-tracking branches and the git repo's remote-tracking branch refs. I hope this difference is not very material, but I could be forgetting something important.

I think jj git push will use the jj's knowledge about the remote-tracking branches.

I see, thanks. I'll hopefully look into it.

I guess the problem is that we need the Git library's support for the push to both be atomic and to do force-with-lease. Grr.

This might also mean that the answer I gave in https://discord.com/channels/968932220549103686/1226363798483636265 was incorrect.

I also found libgit2/libgit2#3947 (comment) and libgit2/libgit2#4286 (comment). Not sure about gix.

This seems totally doable now 🎉 . See https://github.com/rust-lang/git2-rs/pull/926/files#diff-a9e4250104d90130c09939fc70ad76c0d3db800fc0e42a990b1d73d98009e9cb. I'll try to implement 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...".

As discussed in https://discord.com/channels/968932220549103686/1226363798483636265/1226415448615288864

ilyagr force-pushed the pushdocs branch 2 times, most recently from f9bd67a to 7525a1c Compare April 8, 2024 21:42

ilyagr marked this pull request as ready for review April 8, 2024 21:43

ilyagr force-pushed the pushdocs branch 3 times, most recently from b96f57a to 6db66d2 Compare April 8, 2024 21:54

bnjmnt4n approved these changes Apr 9, 2024

View reviewed changes

yuja reviewed Apr 9, 2024

View reviewed changes

ilyagr marked this pull request as draft April 9, 2024 17:59

ilyagr force-pushed the pushdocs branch 5 times, most recently from dcb500e to 77caf04 Compare April 18, 2024 03:04

ilyagr force-pushed the pushdocs branch 13 times, most recently from 3c98d68 to b360222 Compare April 20, 2024 06:51

ilyagr force-pushed the pushdocs branch from b360222 to 7461982 Compare May 2, 2024 05:15

ilyagr force-pushed the pushdocs branch 2 times, most recently from e43de26 to d59bf11 Compare May 13, 2024 04:14

ilyagr force-pushed the pushdocs branch 8 times, most recently from cd1c194 to ef7185e Compare May 21, 2024 00:23

ilyagr force-pushed the pushdocs branch 8 times, most recently from 440e399 to 31a83d4 Compare May 25, 2024 04:45

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 pushdocs branch from 31a83d4 to 0e0f601 Compare May 25, 2024 22:23

ilyagr added 5 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...".

jj git push docs: document safety checks

aef94bd

As discussed in https://discord.com/channels/968932220549103686/1226363798483636265/1226415448615288864

ilyagr force-pushed the pushdocs branch from 0e0f601 to aef94bd Compare May 25, 2024 22:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`jj git push` docs: document safety checks #3473

`jj git push` docs: document safety checks #3473

ilyagr commented Apr 8, 2024

bnjmnt4n left a comment

yuja Apr 9, 2024

ilyagr Apr 9, 2024 •

edited

martinvonz Apr 9, 2024

yuja Apr 9, 2024

ilyagr Apr 9, 2024

ilyagr Apr 9, 2024 •

edited

ilyagr Apr 12, 2024

jj git push docs: document safety checks #3473

Are you sure you want to change the base?

jj git push docs: document safety checks #3473

Conversation

ilyagr commented Apr 8, 2024

bnjmnt4n left a comment

Choose a reason for hiding this comment

yuja Apr 9, 2024

Choose a reason for hiding this comment

ilyagr Apr 9, 2024 • edited

Choose a reason for hiding this comment

martinvonz Apr 9, 2024

Choose a reason for hiding this comment

yuja Apr 9, 2024

Choose a reason for hiding this comment

ilyagr Apr 9, 2024

Choose a reason for hiding this comment

ilyagr Apr 9, 2024 • edited

Choose a reason for hiding this comment

ilyagr Apr 12, 2024

Choose a reason for hiding this comment

`jj git push` docs: document safety checks #3473

`jj git push` docs: document safety checks #3473

ilyagr Apr 9, 2024 •

edited

ilyagr Apr 9, 2024 •

edited