Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cli: when restoring to a previous operation, don't roll back git_refs #1541

Closed
wants to merge 2 commits into from

Conversation

martinvonz
Copy link
Member

Checklist

If applicable:

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

@martinvonz martinvonz changed the title Push wslzqkpuozxn cli: when restoring to a previous operation, don't roll back git_refs Apr 20, 2023
@ilyagr
Copy link
Contributor

ilyagr commented Apr 21, 2023

Well, that's one way to do it :).

While this looks superficially a bit suspicious, and I don't feel like I've deeply thought it over, I haven't yet thought of any reasons it could be problematic. In fact, it seems just as good as my 100x more complicated proposal in #922 (comment).

So, pretty cool! 🎉 I wonder if @yuja or anybody else has thoughts.

@yuja
Copy link
Contributor

yuja commented Apr 22, 2023

I understand this is probably what we've tried to achieve with #922 (comment) or #1487, but I don't think the end result is great.
Without this change, git fetch/import can be undone properly but git push/export can't. This PR flips the results.

Suppose the initial state is jj at S0, and git (remote or local store) at S1,

S1 <- git_actual
 |
S0 <- jj_actual, jj_git_known

git fetch/import brings us to

S1 <- git_actual, jj_actual, jj_git_known
 |
S0

If we don't roll back git_refs, undoing git fetch/import will result in the following state, and I have no idea how to recover from this.

S1 <- git_actual, jj_git_known
 |
S0 <- jj_actual

I think this is the same kind of data inconsistency we have currently for git export -> undo case.

@ilyagr
Copy link
Contributor

ilyagr commented Apr 23, 2023

I think the situation might be a little different for local branches and local git repo, and for the remote-tracking branches and the remote git repo.

For local branches, the state you described seems OK. To repeat it, the state after the undo you described is:

S1 <- git_actual, jj_git_known
 |
S0 <- jj_actual

From that, jj git export should bring us to

S1
 |
S0 <- jj_actual, git_actual, jj_git_known

In a colocated repo, the jj git export would have happened right after the undo automatically, so the intermediate state would be invisible to the user.

As far as the remote-tracking branches are concerned, they should stay at S1. I think that's what would happen as currently implemented (since jj git export does not export remote-tracking branches), but that might require a bit more thought.

For remote-tracking branches and the actual remote state, a jj git push should do the same thing as what jj git export did for local branches.

@ilyagr
Copy link
Contributor

ilyagr commented Apr 24, 2023

I'd like to surface Martin's comment from Discord:

The change in [test_git_fetch.rs](http://test_git_fetch.rs/) is interesting. There we fetch from a remote, then undo, then fetch again. The second fetch has no effect because we already had those remote branches - the undo just made the commits they point to hidden. That's obviously not great, but I'm not sure it means that the approach is wrong; maybe we just need to warn the user that the undo did left some git refs and remote branch behind

This also seems related to what Yuya was talking about.

I think this is a definite problem, but I'm hoping it's solvable. The root of the issue seems to be that the branch positions of the local git repo are used as part of the logic in jj git fetch and jj git push. (I still need to more carefully understand how those work, so correct me if I'm wrong) So, I see two possible solutions:

  1. Do a jj git import after every jj git fetch. I think that would fix this particular test. We might need to do more to make sure jj's and git's idea of where the local branches match when necessary (for instance, a jj git export before a jj git push).
  2. (Update: It seems like this "cleaner solution" is pretty much how jj works now, though it does use git's remote-tracking branches). A cleaner solution would be to reword the fetch and push functionality so that it does not use the git repo's branches at all, but only the information about local/remote branches from jj repo. Relatedly, this means that jj git fetch should not change the local git repo's branch positions in any way. When fetching, libgit2 would be used to tell jj where the remote's branches point and to pull in the right commits, and that's pretty much it.

The second way also has the advantage that a large part of push and fetch functionality will stop being specific to the git backend.


A separate point of Martin's from Discord that seems worth surfacing is that if we want to use this approach, the PR would have to be modified to also preserve remote-tracking branches when undoing.

@yuja
Copy link
Contributor

yuja commented Apr 24, 2023

This also seems related to what Yuya was talking about.

Exactly. #1541 (comment) is an explanation about this test failure, and a reason why I thought the current behavior was better in that scenario. Since jj undo effectively reverted git::import_refs(), jj shouldn't know newly fetched git refs. Without this PR, the state after jj git fetch && jj undo would be close to the state after git fetch without using jj.

FWIW, I think my workaround is a middle ground. It would only apply to automated git export of colocated repo.
yuja@002962e

@ilyagr
Copy link
Contributor

ilyagr commented Apr 24, 2023

I'm thinking of trying to finish this PR (fix the fetch bug following idea 1 or 2, add holding back of remote-tracking branches). I think that, as long as that approach is feasible, it would result in a simpler mental model that seems correct (that is, consistent with itself and with a predictable user-facing behavior) to me.

I suspect that trying to just change git export for colocated repositories will leave non-colocated repositories with some concurrency or git export bugs, though there may be a way to make that work that I'm missing. This comes from a point of view where #922 is not just a bug in colocated repos, but also a (less commonly triggered) bug in non-colocated repositories. We could declare that we don't care about jj git import/export in non-colocated repos being 100% predictable, but I'm not sure we want to do that, and I suspect (but am not certain) that these bugs would still occasionally show up with fetch/push.

A bit of an aside/clarification: if we follow the idea 2 I suggested to its logical conclusion, that would noticeably change the local git repo in non-colocated repos. The git repo wouldn't have any branches anymore, only commits and jj/keep refs. This is similar to how today it doesn't have a HEAD. The user would have to explicitly jj git export to create those branches. We could also create an option to do it automatically, just like for colocated repos.

@martinvonz
Copy link
Member Author

I haven't had much time to think more about this, but some questions that came to mind are:

  • I think colocated repos currently just as if the user had done jj git import; jj <command>; jj git export, which means they would get the same behavior (and same bugs) in a non-colocated repo if . Do we think users expect the same or different behavior?

  • Similar to above, do users expect the same behavior from import/export as from pull/push? Hopefully, but I'm not sure about that either.

  • I've wanted to restructure remote branches for a while. I think the local Git repo could be modeled just like a remote. I think that's also what you said, @ilyagr. So I like that idea, but I haven't thought it through. It sounds like it could simplify a bit.

@ilyagr
Copy link
Contributor

ilyagr commented Apr 24, 2023

I think colocated repos currently just as if the user had done jj git import; jj ; jj git export, which means they would get the same behavior (and same bugs) in a non-colocated repo if . Do we think users expect the same or different behavior?

I think there are several valid possibilities for dealing with local git repository portions of jj repositories. Actually, to me there seems to be just One True Behavior to aim for in colocated repos (based on the desire to support interleaving jj and git commands as seamlessly as possible), but several possibilities in non-colocated ones:

a. Treat them just like colocated ones, with automatic jj git import/export
b. Some hybrid behavior similar to what we have now.
c. Only support accessing the branches in the local Git repositiores after an explicit jj git export.
d. Only support git branches in colocated repos.

Possibilities c. and d. go well together with "solution 2" from #1541 (comment), as I mentioned in my previous comment. We could additionally allow something like jj workspace new --colocated or jj git workspace new to create a git workspace in another dir (either colocated with a new jj workspace or as a pure git workspace with some support to jj git import/export with it).

yuja added a commit to yuja/jj that referenced this pull request Apr 25, 2023
…repo

If "jj op undo" doesn't roll back git refs (jj-vcs#1541), test_git_import_undo()
would get weird state. I think these tests are easier to follow than
test_git_fetch_undo() since no remote refs are involved.
@ilyagr

This comment was marked as duplicate.

@yuja
Copy link
Contributor

yuja commented Apr 25, 2023

  • I think colocated repos currently just as if the user had done jj git import; jj <command>; jj git export, which means they would get the same behavior (and same bugs) in a non-colocated repo if . Do we think users expect the same or different behavior?

  • Similar to above, do users expect the same behavior from import/export as from pull/push? Hopefully, but I'm not sure about that either.

I expect import/export is basically the same as pull/push. The local git repo is similar to git remotes, so I personally wouldn't mind if undoing jj git export/push doesn't do something meaningful. A weird thing about colocated repos is that jj git import/export is hidden from user, so undo has to somehow work across jj/git boundary.

  • I've wanted to restructure remote branches for a while. I think the local Git repo could be modeled just like a remote.

+1

@ilyagr
Copy link
Contributor

ilyagr commented Apr 25, 2023

Firstly, I realized that the "idea 2" I kept discussing above is pretty much how jj now works, seemingly.

Secondly, I did run into some trouble trying to fix that test. Again, my problem seems related to what Yuya is talking about. Some very vague thoughts follow.

I think the reason is this code:

https://github.com/martinvonz/jj/blob/d165e931eb04896c54d87f27e2c226c040ecdc7a/lib/src/git.rs#L167-L176

There's something about it that makes the way fetch has remote branches affect local branches in a different way than importing git's local branches affects jj's local branches. I need to think about it some more.

One issue I have with this code is that, in my mental model, jj shouldn't use git's remote-tracking branches here (at least on fetch, as opposed to an import after an undo) but, instead, it should use the actual commit positions on the remote.

@martinvonz
Copy link
Member Author

One issue I have with this code is that, in my mental model, jj shouldn't use git's remote-tracking branches here (at least on fetch, as opposed to an import after an undo) but, instead, it should use the actual commit positions on the remote.

Feel free to change that if it helps. I've also felt recently that that code is a bit weird :)

yuja added a commit that referenced this pull request Apr 25, 2023
…repo

If "jj op undo" doesn't roll back git refs (#1541), test_git_import_undo()
would get weird state. I think these tests are easier to follow than
test_git_fetch_undo() since no remote refs are involved.
@ilyagr
Copy link
Contributor

ilyagr commented Apr 25, 2023

Similar to above, do users expect the same behavior from import/export as from pull/push? Hopefully, but I'm not sure about that either.

There is one distinction: jj git export is a private operation that's mostly undoable (though not as automatically as other jj operations), while jj git push could be a public and destructively irreversible one (you could destroy somebody else's changes). I realized that I oversimplified my mental model because, for import/export and colocated repos, I cared less about bugs that show up when a jj git import is not preceded by a jj git export.

We can't really do this for push & fetch. The weird fetch-undo behavior we were discussing here wouldn't happen if every jj git fetch would be preceded by a jj git push, but there are cases where the user cannot do that since it would be dangerous or outright destructive.

@ilyagr
Copy link
Contributor

ilyagr commented Apr 27, 2023

I think I'm getting a better understanding of @yuja's point that we have to choose between jj git push/export being possible to "undo properly" and jj git fetch/import being possible to undo properly. However, I'm also coming to the opinion that the latter ("jj git fetch/import being possible to undo properly") is less important and should be sacrificed. In other words, I'm starting to come around to the opinion @martinvonz expressed on Discord in the beginning (I already quoted this above, but now I add some bolding):

The change in test_git_fetch.rs is interesting. There we fetch from a remote, then undo, then fetch again. The second fetch has no effect because we already had those remote branches - the undo just made the commits they point to hidden. That's obviously not great, but I'm not sure it means that the approach is wrong; maybe we just need to warn the user that the undo did left some git refs and remote branch behind.

Here's some reasoning. This ended up being a bit of a manifesto; hope it makes sense.

How important is jj git fetch/import being possible to undo properly, really?

Here, "undo properly" means that jj git fetch && jj undo && jj git fetch is the same as jj git fetch. This is what happens if remote-tracking branches are restored on undo.

The behavior that I'm advertising (preserving remote-tracking branches and refs on undo) would instead have the above be the same as jj git fetch && jj undo; the second jj git fetch becomes a noop.1

Stated this way, this looks like a problem, especially if the user's goal was to undo the fetch specifically. However, that can be worked around with an error message after the second fetch: "Warning: Branch blah is different from blah@origin after the fetch. To make them the same, you can jj git push --branch blah to change the remote branch or jj branch set --to-remote origin blah to change the local branch."

Behavior if jj undo is just one operation of many

I think that preserving the branches on undo starts looking advantageous if we change our philosophy of what fetch & undo are about to the following:

  • It is important that jj git fetch tells the user whether the state of the remote has changed and, if so, what needs to happen before it's safe to push to that remote.
  • The user's undo may be just one in a sequence of changes the user makes to the branch and its commits.

Following the second point, consider this scenario: say the user did a fetch, then a little later used jj restore to go to some operation before that fetch, and then makes some more edits. Now, they are thinking about doing a second fetch or a push.

Approach 1: remote-tracking branches are unaffected by undo

In this approach, a second fetch creates a conflict if and only if there were additional changes on the remote. This matches the philosophy I mentioned. This also matches the desired behavior of a jj git push performed at that moment, which should fail if and only if there were additional changes on the remote (similarly to a git push --force-with-lease).

There's an easy way to reason about what undoing the first fetch did to the repo: the undo was equivalent to jj branch set blah -r somewhere. In fact, undoing a push with this approach is also equivalent to a jj branch set (Update: actually, it seems to be a no-op, which is a particularly trivial kind of jj branch set). This also seems like a nice property.

From the perspective of the first paragraph above, jj git fetch && jj undo && jj git fetch being equivalent to jj git fetch && jj undo seems like a reasonable price to pay. From the perspective of the second paragraph, it starts to make a lot of sense.

Approach 2: remote-tracking branches are restored on undo

OTOH, if remote-tracking branches are restored on undo, the second jj git fetch would always create a conflict and a jj git push at that point would always fail.

This doesn't seem like a great experience.

One could argue that, from a certain perspective (if the user forgot that pushing their branch would lose remote commits), creating these conflicts is a safer alternative. However, I don't think so. From that perspective, wouldn't pushing after rewriting a commit be just as dangerous? In fact, I'll add this to my list of philosophical points:

  • Undoing a fetch/push should not be treated as a more dangerous operation than abandoning/rewriting a commit that exists on the remote.

I'm pretty sure we preserve the remote-tracking branch when rewriting/abandoning (don't we?) so that pushing/fetching after an abandon succeeds (unless the remote changed). We should do the same for undo.

Thus, Approach 1 seems better to me.

Footnotes

  1. Update: For completeness, here's what happens to "jj git push" from the perspective of being undone properly. If the remote-tracking branches are restored, jj git push && jj undo && jj git push will usually result in the second push failing. You'd need to do jj git fetch to recover, and all together jj git push && jj undo && jj git fetch would be a no-op. So, jj git push is not undone properly. OTOH, if remote-tracking branches are preserved, jj git push would be undone properly, but in a somewhat tautological way: the jj undo after a jj git push would be a no-op. So, jj git push && jj undo && jj git push is equivalent to a single jj git push.

@yuja
Copy link
Contributor

yuja commented Apr 27, 2023

I haven't yet read your proposal thoroughly, but a few comments.

Here, "undo properly" means that jj git fetch && jj undo && jj git fetch is the same as jj git fetch. This is what hapens if remote-tracking branches are restored on undo.

The behavior that I'm advertising (preserving remote-tracking branches and refs on undo) would instead have the above be the same as jj git fetch && jj undo.

If jj git fetch && jj undo is identical to jj git fetch (i.e. jj undo is noop), that's fine. However, I would say it's broken if the third && jj git fetch didn't move refs to the latest state. I wouldn't figure out why jj git fetch no longer works.

The current behavior (jj git export/push can't be undone properly) makes more sense to me because the exported/pushed changes are out of our control.

FWIW, if we really want to keep our known git_refs consistent, I think we can reconstruct it from operation log by applying deltas that couldn't be undone. It'll be something like cherry-picking git::export_refs() operations. For simple scenarios, maybe we can track the last export/import direction per ref, and preserve refs of direction == export.

@ilyagr
Copy link
Contributor

ilyagr commented Apr 27, 2023

However, I would say it's broken if the third && jj git fetch didn't move refs to the latest state.

It makes sense why this feels broken. In spite of this, I'm indeed trying to present a point of view from which it's actually OK for the third jj git fetch to not move the refs.

In the world I'm proposing, the undo after a jj git fetch is equivalent to a jj branch set. In jj git fetch && jj branch set && jj git fetch, we don't expect the second jj git fetch to more the refs (unless something changed on the remote in the meantime).

One vague philosophy this follows is that the result of each invocation of undo or op restore should be equivalent to a series of normal jj operations. Of those, only the commands that talk to the external world (fetch, pull, import, export) change the remote-tracking branches, so either undo shouldn't touch remote-tracking branches or it should talk to the external world (which would be a bad idea, since undo should be predictable, but the outcome of talking to the external world isn't). For more philosophies, see my previous post.

@ilyagr
Copy link
Contributor

ilyagr commented Apr 27, 2023

FWIW, if we really want to keep our known git_refs consistent, I think we can reconstruct it from operation log by applying deltas that couldn't be undone

I think you are suggesting treating jj's remote-tracking branches and jj's git_refs differently with respect to undo. While this could be done, I haven't thought of a way this could result in a behavior that would make sense. At least, I think this would create an irrevocable difference between push/fetch and export/import.

In my current mental model, git_refs for normal branches are, as far as jj's model is concerned, just the remote-tracking branches for the special remote that is the local git repo. If your mental model is different, we aren't on the same page.

Aside: I can't make any sense out of git_refs for remote-tracking branches beyond as a private implementation detail of jj git fetch. I think they shouldn't exist (similarity to how jj/keep refs are not in git_refs). If there is some way to treat them differently from actual remote-tracking branches, have that be of benefit, and make sense, I get too confused to understand it.

@yuja
Copy link
Contributor

yuja commented Apr 27, 2023

In the world I'm proposing, the undo after a jj git fetch is equivalent to a jj branch set. In jj git fetch && jj branch set && jj git fetch, we don't expect the second jj git fetch to more the refs (unless something changed on the remote in the meantime).

Ah, now I understand what you mean, thanks. However, I feel it would be as bad as the current undo behavior in colocated repo.

IMO, the problem of colocated undo is that there are implicit jj git export which the user would never think undoing. In your proposed model, jj git fetch can be considered to have an implicit jj branch set triggered conditionally, and jj undo can only revert the implicit jj branch set part.

In my current mental model, git_refs for normal branches are, as far as jj's model is concerned, just the remote-tracking branches for the special remote that is the local git repo

I also consider that way. Perhaps, the difference is that I think remote-tracking branches are jj's internal state which can be moved back on jj undo (at least for the last fetch-ed ones.)

@yuja
Copy link
Contributor

yuja commented Apr 27, 2023

remote-tracking branches are jj's internal state which can be moved back on jj undo

To be clear, my mental model of jj undo/op restore is filesystem snapshot of the repository, something similar to rsync from the backup. So if I undo jj git fetch, I expect the remote-tracking refs would be rolled back, and a subsequent jj git fetch will fetch from the remote again. It's the same for jj git push, and I would pretty much avoid undoing push.

@ilyagr
Copy link
Contributor

ilyagr commented Apr 27, 2023

In your proposed model, jj git fetch can be considered to have an implicit jj branch set triggered conditionally, and jj undo can only revert the implicit jj branch set part.

That's right.

the problem of colocated undo is that there are implicit jj git export which the user would never think undoing.

I didn't understand this part. Note that, as far as I can tell, undoing a jj git export in my model is a no-op (since jj git export does not have an implicit jj branch set part).

I feel it would be as bad as the current undo behavior in colocated repo.

I think it should be fine, or I'm missing something.

In a colocated repo, every operation is preceded by an automatic jj git import and succeeded by an automatic jj git export. So, a jj git import of operation N+1 always follows a jj git export of operation N. This means that

  • It is a no-op if and only if nothing changed in the local git repository in between operations N and N+1 (no git operations happened).
  • If something did change, it imports those external operations, and creates a conflict if necessary.

This applies to undo and restore operations as well, which in this model are equivalent to a sequence of normal operations (e.g. undoing an import/export is either a noop or equivalent to a jj branch set). The jj git export that's automatically added to them should succeed in most cases (unless the jj git import before the undo created conflicts, or if there's some concurrent operations and race conditions happening).

So far, I continue to think that import/export can share the same behavior as fetch/push. From this perspective, colocated repos as the easy case, since we don't have to think of what happens if there are two imports without an export in between and vice-versa.

my mental model of jj undo/op restore is filesystem snapshot of the repository, something similar to rsync from the backup.

This model makes sense, but it's important to explicitly acknowledge that it is incompatible with what I am proposing. It now seems to me that the rsync model just doesn't lead to a good user experience; this was a surprising discovery to me.

The problem is that jj can't restore the remotes to a past state, and even restoring the local git repo to a past state seems highly problematic. In principle, we could have a command for jj to try to restore the whole world to a past state (a kind of undo that would include some fetches and pushes), but that can obviously fail and might just be a bad idea.

@yuja
Copy link
Contributor

yuja commented Apr 28, 2023

In your proposed model, jj git fetch can be considered to have an implicit jj branch set triggered conditionally, and jj undo can only revert the implicit jj branch set part.

That's right.

the problem of colocated undo is that there are implicit jj git export which the user would never think undoing.

I didn't understand this part. Note that, as far as I can tell, undoing a jj git export in my model is a no-op (since jj git export does not have an implicit jj branch set part).

In your proposed model, that's true. And restoring across jj git import/fetch
gets broken instead (or looks broken unless the user understand the exact model.)

I feel it would be as bad as the current undo behavior in colocated repo.

I think it should be fine, or I'm missing something.

My point is that reverting a part of an operation is bad because a user need
a deeper understanding of the underlying implementation.

jj <command> in colocated repo is a sequence of
jj git import && jj <command> && jj git export, and jj git export can't
be undone (under the current model.) For me, that's the source of the problem.
If I ran these three commands by myself, I would easily notice that undoing across
jj git export would cause problems.

Under your model, jj git fetch is jj git fetch-remote-refs && jj branch set,
and the first part can't be undone. IMHO, this will cause the same UX problem.

my mental model of jj undo/op restore is filesystem snapshot of the repository, something similar to rsync from the backup.

This model makes sense, but it's important to explicitly acknowledge that it is incompatible with what I am proposing.

Understood.

It now seems to me that the rsync model just doesn't lead to a good user experience; this was a surprising discovery to me.

I personally don't think the rsync model provides a bad user experience.
The only problem I have in practice is jj undo in colocated repo, which I think
can be worked around differently.

The problem is that jj can't restore the remotes to a past state, and even restoring the local git repo to a past state seems highly problematic. In principle, we could have a command for jj to try to restore the whole world to a past state (a kind of undo that would include some fetches and pushes), but that can obviously fail and might just be a bad idea.

True, but I feel it's natural that jj can only fix up the local state.

@ilyagr
Copy link
Contributor

ilyagr commented Apr 28, 2023

I personally don't think the rsync model provides a bad user experience.

That's what I tried to explain in my long #1541 (comment).

Briefly, I think that one of the primary functions of jj fetch is to tell me whether it's safe to push to a remote (in the sense of git push --force-with-lease) or whether something changed over there since you last looked. I think that gets broken in your model whenever you undo a fetch.

Overall, I can't say I find my argument perfectly clear; that's part of the reason the post I linked is long. I'll keep thinking if there's a better explanation. I also wouldn't mind having a clearer argument for why supporting something like --force-with-lease is important enough that we'd want to make it work nicely after jj op restore.

Another point that may or may not feel relevant: there's already one case where we take extra care to preserve remote-tracking branches so that something like --force-with-lease could work nicely. When we rewrite a commit, we move all the local branches to the new version, but we keep the remote-tracking branches at the original commit. The model that remote-tracking branches belong to the remote trumps the model of how normal branches behave when rewriting a commit. I think that undo is, ultimately, analogous.

@yuja
Copy link
Contributor

yuja commented Apr 28, 2023

Briefly, I think that one of the primary functions of jj fetch is to tell me whether it's safe to push to a remote (in the sense of git push --force-with-lease) or whether something changed over there since you last looked.

Yes, and getting the latest changes is the other important function of jj fetch, I think.

I think that gets broken in your model whenever you undo a fetch.

Maybe I don't follow, sorry. Does that get broken because jj fetch && jj undo leaves the remote refs in the local git repo out of sync with jj? I think it can be mitigated if we trust jj's remote refs instead of refs in the git repo (though I don't know if that's doable with the git2 API.)

To be clear, under the situation where jj git push fails, I expect jj git fetch && jj undo && jj git push should also fail because I rolled back the fetch operation.

@yuja
Copy link
Contributor

yuja commented Apr 28, 2023

To be clear, under the situation where jj git push fails, I expect jj git fetch && jj undo && jj git push should also fail because I rolled back the fetch operation.

Maybe I got what you mean, sorry for taking so long. In your proposed model, jj git fetch && jj undo basically abandons the remote changes, and a later jj git push will overwrite the remote branch with the local changes (so long as the remote is at the known state)?

It might be useful, but I'm pretty sure I wouldn't expect such behavior from the command name undo/op restore.

@ilyagr
Copy link
Contributor

ilyagr commented Apr 28, 2023

In your proposed model, jj git fetch && jj undo basically abandons the remote changes, and a later jj git push will overwrite the remote branch with the local changes (so long as the remote is at the known state)?

Yes, that's right; again this is similar to how jj git fetch && jj describe (or some other way of rewriting a commit you just fetched) would work.

My point about it not being quite as helpful your model is that, in your model, jj git fetch && jj undo && jj git push fails in exactly the same way regardless of whether the remote is in a known state or not. So, that useful piece of information is lost.

@yuja
Copy link
Contributor

yuja commented May 28, 2023

Can't we simply undo the wrong kinds of undo, and rerun with the correct
options (or a sequence of commands)?

In theory yes, but it's a headache since you can make things worse. For example, let's say you did jj undo --preserve-remotes and that becomes operation abc. If you mistakenly jj undo --restore-remotes abc, things become worse. Recovering from those two mistakes is even trickier. (This specific user error is more likely to happen if --restore-remotes is the default, but you can flip the example for the other case).

IMHO, it's complicated because undo/op restore has many options. If jj undo --preserve-remotes were jj reset --op OP_ID (though reset isn't obviously a good name), I wouldn't be confused.

There is no need for a jj git unpush command. From the perspective of future pushes, the correct way to undo a push is to do nothing.

I'm not suggesting jj git unpush, but if I made things published too early, I might want to undo the push without moving local refs.

@ilyagr
Copy link
Contributor

ilyagr commented May 29, 2023

IMHO, it's complicated because undo/op restore has many options. If jj undo --preserve-remotes were jj reset --op OP_ID (though reset isn't obviously a good name), I wouldn't be confused.

It's not clear to me whether splitting undo and restore into separate commands (something like op undo-local and op restore-local?) makes things easier to understand or not.

I was hoping to postpone this decision by first fixing #922 and creating options that can do anything for someone who understands the model (for myself, at least). We can label them as "experimental" or something. Then, we could later create a friendlier UI based on ours and others' experience with these.

In fact, it's even possible there won't be that much need for it. Perhaps after we change the default for colocated repos to fix #922, people won't notice the problems I described in this thread and will be perfectly happy to just use plain undo and restore while keeping the "rsync" model of undoing in their head. They'll just need to resolve more conflicts manually, but perhaps they'll be fine with that.

But if there is a need for it, we'll have some suggestions we can give to people immediately, and will be gathering some experience ourselves with what is helpful.

ilyagr added a commit to ilyagr/jj that referenced this pull request Jun 26, 2023
The result is equivalent to jj-vcs#1541
but with many more tests
ilyagr added a commit to ilyagr/jj that referenced this pull request Jun 26, 2023
The result is equivalent to jj-vcs#1541
but with many more tests
ilyagr added a commit to ilyagr/jj that referenced this pull request Jun 26, 2023
The result is equivalent to jj-vcs#1541
but with many more tests
ilyagr added a commit to ilyagr/jj that referenced this pull request Jun 26, 2023
The result is equivalent to jj-vcs#1541
but with many more tests
ilyagr added a commit to ilyagr/jj that referenced this pull request Jun 26, 2023
The result is equivalent to jj-vcs#1541
but with many more tests
ilyagr added a commit to ilyagr/jj that referenced this pull request Jun 30, 2023
The result is equivalent to jj-vcs#1541
but with many more tests
ilyagr added a commit to ilyagr/jj that referenced this pull request Jun 30, 2023
The result is equivalent to jj-vcs#1541
but with many more tests
ilyagr added a commit to ilyagr/jj that referenced this pull request Jul 1, 2023
The result is equivalent to jj-vcs#1541
but with many more tests
ilyagr added a commit to ilyagr/jj that referenced this pull request Jul 2, 2023
The result is equivalent to jj-vcs#1541
but with many more tests
ilyagr added a commit to ilyagr/jj that referenced this pull request Jul 2, 2023
The result is equivalent to jj-vcs#1541
but with many more tests
ilyagr added a commit to ilyagr/jj that referenced this pull request Jul 3, 2023
The result is equivalent to jj-vcs#1541
but with many more tests
ilyagr added a commit to ilyagr/jj that referenced this pull request Jul 3, 2023
The result is equivalent to jj-vcs#1541
but with many more tests
ilyagr added a commit to ilyagr/jj that referenced this pull request Jul 3, 2023
The result is equivalent to jj-vcs#1541
but with many more tests
ilyagr added a commit to ilyagr/jj that referenced this pull request Jul 3, 2023
The result is equivalent to jj-vcs#1541
but with many more tests
ilyagr added a commit to ilyagr/jj that referenced this pull request Jul 3, 2023
The result is equivalent to jj-vcs#1541
but with many more tests
ilyagr added a commit to ilyagr/jj that referenced this pull request Jul 3, 2023
The result is equivalent to jj-vcs#1541
but with many more tests
ilyagr added a commit that referenced this pull request Jul 3, 2023
The original test is copied from @martinvonz 's [PR draft] (thanks!).

The three versions show differences in behavior due to import/export
of remote-tracking branches, and due to repo being colocated.

The former is relevant for [the discussion] of whether `jj git export` should
export remote-tracking branches. The latter will change in a follow-up commit.

Outstanding TODO: check if we have similar tests for undoing `fetch`

[PR draft]: #1541
[the discussion]: #1739
ilyagr added a commit to ilyagr/jj that referenced this pull request Jul 3, 2023
The result is equivalent to jj-vcs#1541
but with many more tests
ilyagr added a commit to ilyagr/jj that referenced this pull request Jul 10, 2023
The result is equivalent to jj-vcs#1541
but with many more tests
@ilyagr
Copy link
Contributor

ilyagr commented Aug 9, 2023

Should we close this PR? It seems obsolete, though its spirit certainly lives on.

@martinvonz
Copy link
Member Author

Should we close this PR?

Yes, makes sense. Thanks for the reminder.

@martinvonz martinvonz closed this Aug 9, 2023
@martinvonz martinvonz deleted the push-wslzqkpuozxn branch September 8, 2023 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants