-
Notifications
You must be signed in to change notification settings - Fork 779
conflicts: prefer emitting snapshot for first side #7873
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
Conversation
|
Did you mistype something?
Edit: reading it again, I guess you are saying diff instead of snapshot, when before all I saw was first instead of first. |
a66e374 to
f06b500
Compare
Yes, that is what I intended. I updated the description to be hopefully a bit clearer. |
|
FWIW, I'm not attached to the idea of even trying to pick the smaller diff. Would it be even better in your mind if we always picked the first side as the snapshot? It would at least remove the confusion that I sometimes hear about why the markers are/appear inconsistent. |
I personally do like that it at least sometimes picks the second side as the snapshot, but it only would matter to me in extreme cases. Like if I made a large change to a file, but then I rebased it onto trunk where someone made a small change (e.g. fixing a typo), it's nice for me to be able to see the diff of the typo instead of having to re-apply the whole diff for my large change. But maybe I'm just used to the current implementation so the inconsistency doesn't really bother me :) If we do decide to go with a similar approach to what I proposed in this PR, I would also be open to other factors instead of 10% larger. For instance, preferring the first side unless it would make the diff more than twice as large (which is presumably more rare). I was initially thinking it's safer to make a small adjustment, since a larger change might annoy more people, but now I'm thinking maybe 10% is too small of a change that it wouldn't even be worth making. For some context, I'm mainly thinking about this in relation to what I was experimenting with in #7692, because some conflict labels (e.g. for squashed commits) are easier to understand with the first side as the snapshot. I was trying to come up with readable examples, but it was always rendering the diff first since I was using the labels "left", "base", and "right" for the sides, so I was thinking it would be good to have a bias towards snapshot first, instead of the old implementation which actually has a bias towards diffs first. |
Often, the diffs for the first side and second side are approximately the same size, so we have the choice of whether we want to emit a snapshot for the first side or for the second side. Currently, we prefer to emit a diff for the first side when possible. I think it would make more sense to prefer emitting snapshots instead for the first side since the first side of the merge usually makes more sense semantically as the snapshot. For instance, in merges, the first side is the first parent, and in rebases, the first side is the destination to which the diff is being applied. Therefore, I think we should prefer to emit a snapshot for the first term unless it would create a diff that's more than 10% larger.
f06b500 to
2d82549
Compare
|
I think #7917 would probably be a better option than this so I'm closing this PR in favor of that issue |
Often, the diffs for the first side and second side are approximately the same size, so we have the choice of whether we want to emit a snapshot for the first side or for the second side. Currently, we prefer to emit a diff for the first side when possible.
I think it would make more sense to prefer emitting snapshots instead for the first side since the first side of the merge usually makes more sense semantically as the snapshot. For instance, in merges, the first side is the first parent, and in rebases, the first side is the destination to which the diff is being applied. Therefore, I think we should prefer to emit a snapshot for the first term unless it would create a diff that's more than 10% larger.
Stacked on #7872
Checklist
If applicable:
CHANGELOG.mdREADME.md,docs/,demos/)cli/src/config-schema.json)