-
Notifications
You must be signed in to change notification settings - Fork 319
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
Conversation
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. |
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. Suppose the initial state is
If we don't roll back
I think this is the same kind of data inconsistency we have currently for |
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:
From that,
In a colocated repo, the As far as the remote-tracking branches are concerned, they should stay at For remote-tracking branches and the actual remote state, a |
I'd like to surface Martin's comment from Discord:
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
The second way also has the advantage that a large part of 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. |
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 FWIW, I think my workaround is a middle ground. It would only apply to automated |
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 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 |
I haven't had much time to think more about this, but some questions that came to mind are:
|
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 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 |
…repo If "jj op undo" doesn't roll back git refs (martinvonz#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.
This comment was marked as duplicate.
This comment was marked as duplicate.
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
+1 |
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: Lines 167 to 176 in d165e93
There's something about it that makes the way 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 :) |
…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.
There is one distinction: We can't really do this for push & fetch. The weird |
I think I'm getting a better understanding of @yuja's point that we have to choose between
Here's some reasoning. This ended up being a bit of a manifesto; hope it makes sense. How important is
|
I haven't yet read your proposal thoroughly, but a few comments.
If The current behavior ( FWIW, if we really want to keep our known |
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 In the world I'm proposing, the One vague philosophy this follows is that the result of each invocation of |
I think you are suggesting treating jj's remote-tracking branches and jj's In my current mental model, Aside: I can't make any sense out of |
Ah, now I understand what you mean, thanks. However, I feel it would be as bad as the current IMO, the problem of colocated
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 |
To be clear, my mental model of |
That's right.
I didn't understand this part. Note that, as far as I can tell, undoing a
I think it should be fine, or I'm missing something. In a colocated repo, every operation is preceded by an automatic
This applies to 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.
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 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 |
In your proposed model, that's true. And restoring across
My point is that reverting a part of an operation is bad because a user need
Under your model,
Understood.
I personally don't think the
True, but I feel it's natural that |
That's what I tried to explain in my long #1541 (comment). Briefly, I think that one of the primary functions of 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 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 |
Yes, and getting the latest changes is the other important function of
Maybe I don't follow, sorry. Does that get broken because To be clear, under the situation where |
Maybe I got what you mean, sorry for taking so long. In your proposed model, It might be useful, but I'm pretty sure I wouldn't expect such behavior from the command name |
Yes, that's right; again this is similar to how My point about it not being quite as helpful your model is that, in your model, |
IMHO, it's complicated because
I'm not suggesting |
It's not clear to me whether splitting undo and restore into separate commands (something like 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 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. |
The result is equivalent to martinvonz#1541 but with many more tests
The result is equivalent to martinvonz#1541 but with many more tests
The result is equivalent to martinvonz#1541 but with many more tests
The result is equivalent to martinvonz#1541 but with many more tests
The result is equivalent to martinvonz#1541 but with many more tests
The result is equivalent to martinvonz#1541 but with many more tests
The result is equivalent to martinvonz#1541 but with many more tests
The result is equivalent to martinvonz#1541 but with many more tests
The result is equivalent to martinvonz#1541 but with many more tests
The result is equivalent to martinvonz#1541 but with many more tests
The result is equivalent to martinvonz#1541 but with many more tests
The result is equivalent to martinvonz#1541 but with many more tests
The result is equivalent to martinvonz#1541 but with many more tests
The result is equivalent to martinvonz#1541 but with many more tests
The result is equivalent to martinvonz#1541 but with many more tests
The result is equivalent to martinvonz#1541 but with many more tests
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
The result is equivalent to martinvonz#1541 but with many more tests
The result is equivalent to martinvonz#1541 but with many more tests
Should we close this PR? It seems obsolete, though its spirit certainly lives on. |
Yes, makes sense. Thanks for the reminder. |
Checklist
If applicable:
CHANGELOG.md