Parents are forgotten when moving changes out of a merge commit #2859
Comments
thoughtpolice
changed the title
moveing out of a merge commit
|
This is technically related to the problem with automatically abandoned commits discussed here: #2766 (comment) . They are still two separate problems, but the solution might lurk in similar parts of the code. For this specific issue, Martin suggested: Lines 461 to 462 in 0af1a15 |
|
A separate thought: if we fix this, This is because I'm worried about the use-case of a user doing |
Doesn't it abandon the |
|
@yuja, I'm no sure exactly what you mean since there are already so many related issues and behaviors. Here's some of my reasoning:
And a separate chain of logic:
|
I expect the resulting empty merge would be rewritten from the
Because a merge commit isn't discardable, 2 should be skipped.
If
I expect
I disagree. A merge commit isn't usually noop, so it shouldn't be discarded implicitly. Otherwise, the merged state would be lost by |
Interesting, I think I understand your point of view. It is quite different from my mental model, but seems also self-consistent. I feel slightly less comfortable with it, but probably just because it feels a bit unfamiliar for now. IIUC, in your language, I was suggesting changing the definition of "discardable" commits to include empty merge commits with no description. You are suggesting keeping the definition as-is, and merely having commands like The main practical difference is this: in my model, I feel slightly bothered by the inconsistency between OTOH, the benefit of your perspective is that we can use the same notion of "discardable" here as we do in In any case, it is a good point that there is a self-consistent model that doesn't require
If you wanted to keep the merge commit, you could just Overall, I don't see a serious problem with either of our approaches at the moment. We should document whichever approach we choose, as they are subtly different. |
Yes. A merge commit isn't garbage, so keep it. If it were a garbage, we wouldn't have to fix |
|
SGTM, let's try the approach that makes more sense to you, it seems just fine to me. I still find it fascinating that there are two possibilities where I only saw one. |
|
I use I've been arguing that jj understands merges well enough that there's no such thing as an "evil merge" and that user can use merge commits just like regular commits. So I think empty merge commits without descriptions are as discardable. I know we consider merge commits not discardable in |
Turns out there's no impact on existing tests. I'm a bit surprised by that. I guess we need a bit more test coverage. |
Actually, on second thought, this does seem like a problem with @yuja 's model. @yuja, what should the result of I think that in both of our models, as far as I understand them, the result of Another option would be to have |
|
So I will say that after I filed this I "fixed" it by doing simply giving the empty merge commit a description, then This actually allows me to move changes "past" the merge, as I would expect, without the awkward behavior of the original comment. There's another set of odd interactions here that may be the same symptom. Let's say that Now, fill the (previously empty) merge commit with a change: Now let's create a What happened? It looked like a no-op!
It's actually quite awkward to get out of this state now! Because you can't just move it out of the merge directly due to this behavior, you have to create two new working copy commits: Then move the change from the merge into This all makes perfect sense with the current behavior. But I don't know how sensible it actually is. :) EDIT: Also I find it rather hilarious that this whole process ended up with |
That's actually what I would have expected :) What did you expect? |
|
Right, that's what it currently does, and what I saw (and see) as a bug. I would expect |
Interesting. I've never considered that way. I expect In any case, I wouldn't want my empty merge commit get abandoned by |
If it has no description and no changes, is it still useful? The only information it carries seems to be the set of parents then. I can see that having some value, perhaps as a reminder to yourself that you plan to do something with the merge. Do you think it would be annoying to have to get used to adding a description for merge commits you want to keep (if they're also empty and head commits)? |
Well, I could pass That said, I have to agree that it would be confusing if |
This happens already if you do Update: On second thought, the shape of the graph presentation does change sometimes from this behavior, because the new commit has a newer date, and the way Right now, the two options I see are either what I suggested (making empty no-description commits discardable regardless of the number of parents) or to special-case I have a preference for the first option, but not a strong one. I think both of these have advantages -- the first one seems more theoretically pure and IMO easier to remember, but the second one might be less surprising in some situations. I wonder if there's a third option. |
Just a comment. IMHO, it's whether the workspace (conceptually) remembers the previous state or not. I just feel it's unusual that |
… empty `@` This demonstrates the minor bug discussed in martinvonz#2766 (comment) AKA martinvonz#2869. It's also interesting whether changing the definition of "discardable" commit would affect this test, see martinvonz#2859 (comment) (I think it won't, but still)
… empty `@` This demonstrates the minor bug discussed in martinvonz#2766 (comment) AKA martinvonz#2869. It's also interesting whether changing the definition of "discardable" commit would affect this test, see martinvonz#2859 (comment) (I think it won't, but still)
… empty `@` This demonstrates the minor bug discussed in martinvonz#2766 (comment) AKA martinvonz#2869. It's also interesting whether changing the definition of "discardable" commit would affect this test, see martinvonz#2859 (comment) (I think it won't, but still)
…est case This demonstrates the minor bug discussed in martinvonz#2766 (comment) AKA martinvonz#2869. It's also interesting whether changing the definition of "discardable" commit would affect this test, see martinvonz#2859 (comment) (I think it won't, but still)
…est case This demonstrates the minor bug discussed in #2766 (comment) AKA #2869. It's also interesting whether changing the definition of "discardable" commit would affect this test, see #2859 (comment) (I think it won't, but still)
I recently needed to test something on top of a two branches at the same time, so I created a new commit on top of both of them (i.e. a merge commit). I then ran tests and made some adjustments to the code. These adjustments belonged in one of the parent branches, so I used `jj squash --into` to squash it in there. Unfortunately, that meant that my working copy became a single-parent commit based on one of the branches only. We already had #2859 for tracking this issue. This patch changes the behavior so we create a new working-copy commit with all of the previous parents.
Merge commits are very similar to non-merge commits in jj. An empty merge commit with no description is not really different from an empty non-merge commit with no description. As we discussed on #2859, we should not treat merge commits differently when updating away from them. This patch adds test for the current behavior (which is to leave the merge commit in place).
I recently needed to test something on top of a two branches at the same time, so I created a new commit on top of both of them (i.e. a merge commit). I then ran tests and made some adjustments to the code. These adjustments belonged in one of the parent branches, so I used `jj squash --into` to squash it in there. Unfortunately, that meant that my working copy became a single-parent commit based on one of the branches only. We already had #2859 for tracking this issue. This patch changes the behavior so we create a new working-copy commit with all of the previous parents.
Merge commits are very similar to non-merge commits in jj. An empty merge commit with no description is not really different from an empty non-merge commit with no description. As we discussed on #2859, we should not treat merge commits differently when updating away from them. This patch adds test for the current behavior (which is to leave the merge commit in place).
I recently needed to test something on top of a two branches at the same time, so I created a new commit on top of both of them (i.e. a merge commit). I then ran tests and made some adjustments to the code. These adjustments belonged in one of the parent branches, so I used `jj squash --into` to squash it in there. Unfortunately, that meant that my working copy became a single-parent commit based on one of the branches only. We already had #2859 for tracking this issue. This patch changes the behavior so we create a new working-copy commit with all of the previous parents.
Merge commits are very similar to non-merge commits in jj. An empty merge commit with no description is not really different from an empty non-merge commit with no description. As we discussed on #2859, we should not treat merge commits differently when updating away from them. This patch adds test for the current behavior (which is to leave the merge commit in place).
Create a merge:
Now, modify a file, then
movethe change out:The merge commit gets undone, because a new wcc is created (
nuz) without the parents of the originalwt:As Martin noted on Discord, this also happens with
jj abandon, so after getting to the above state, you can do:And it will result in the same state as above, i.e. the parents are gone and you end up on the left-most parent.
The text was updated successfully, but these errors were encountered: