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

merge-conflict: Add compare {current,incoming} to original #133088

Open
goldfirere opened this issue Sep 14, 2021 · 9 comments · May be fixed by #133845
Open

merge-conflict: Add compare {current,incoming} to original #133088

goldfirere opened this issue Sep 14, 2021 · 9 comments · May be fixed by #133845
Assignees
Labels
feature-request Request for new features or functionality merge-conflict Merge conflict decorations and actions
Milestone

Comments

@goldfirere
Copy link

I use 3-way merge conflict checking, where I can see all of the current changes, the original code, and the incoming changes. My workflow for resolving conflicts is to compare the original code to the current and then, separately, to the incoming changes. I can then figure out which changes are easier to port (say, reflect the original-to-incoming changes in the current code), and then actually port the changes. A critical step here is to be able to compare the original version against the updated versions.

VSCode's merge-conflict support allows me to easily compare the current code against the incoming code, but that's not nearly as actionable as comparing against the original code. Is it possible to add a feature where, in a 3-way conflict, I can compare current and incoming against original?

Useful prior art: emacs's smerge-mode allows the smerge-refine command, which cycles through the 3 possible comparisons (original/current, original/incoming, current/incoming).

@IllusionMH
Copy link
Contributor

Looks like duplicate of #37350

@goldfirere
Copy link
Author

Thanks, @IllusionMH, but I don't think so. I feel like I must be missing something, but it looks like #37350 is essentially a side-by-side representation of the diff3 conflict style that git provides. This side-by-side representation is really useful, but I'm looking for something more: word-by-word (or char-by-char) highlights of what actually changed. Let me demonstrate.

This is the rendering of a 3-way merge in VSCode today.

Screen Shot 2021-09-14 at 11 17 08 AM

With merge-conflict.compare, I can get this:

Screen Shot 2021-09-14 at 11 19 16 AM

Note that orig is highlighted, along with the fact that EQ1 and EQ2 are different. These highlights are so, so useful. But this comparison is between the current and incoming versions. I want the same display, but comparing the current and original or the incoming and original. That's it! No three-way fanciness required (beyond what git already does).

For comparison, here is emacs:

Screen Shot 2021-09-14 at 11 22 12 AM

I have cycled through the three possible head-to-head comparisons to see that original-vs-incoming involves only a 1-character change. I can then confidently change EQ2 to EQ1 in the current code, accept the current version, and move on. Without manually doing the word-by-word or char-by-char comparison, I don't see how I can as easily resolve this conflict in VSCode.

As an added bonus, I'm hoping that implementing what I want is far easier than #37350. It's exactly the same as the existing merge-conflict.compare, but just changing the files that are compared.

goldfirere added a commit to goldfirere/vscode that referenced this issue Sep 14, 2021
This commit is written by someone who has no idea what
he is doing, but it hopefully communicates the idea that
this is easy to do.
@goldfirere
Copy link
Author

I made a vain attempt to fix this myself in goldfirere@9f8c9eb, but it doesn't work (and fails the pre-commit lint, due to my constructed string). I've never worked in TypeScript before, so there could be all manner of other silliness here, but the commit should get the idea across. Someone more knowledgeable with VSCode internals could hopefully take what I have and make it actually work. (It doesn't: somehow, my new commands aren't recognized. This baffles me.)

@chrmarti chrmarti added feature-request Request for new features or functionality merge-conflict Merge conflict decorations and actions labels Sep 15, 2021
@chrmarti chrmarti added this to the Backlog milestone Sep 15, 2021
@chrmarti
Copy link
Contributor

Nothing stands out on first look. Try making some trivial change to verify it gets compiled and picked up after reloading the window.

@goldfirere
Copy link
Author

It's definitely recompiling and loading my changes. If I just changed this line in the original code to use conflict.commonAncestors[0].content, I was able to observe the result I wanted.

In the version I've built, my new command e.g. merge-conflict.compare-current-original appears in the command palette (when I Cmd+Shift+P), but then executing it produces an error that the command doesn't exist. It smells to me like the registerTextEditorCommand isn't working somehow -- or that I've mistyped the name of the command in different places. But I don't spot my mistake.

@chrmarti
Copy link
Contributor

If nothing triggers the activation of the extension, this would be expected. Try adding onCommand activation events for each command in the package.json's activationEvents. I guess you would not run them from the command palette normally which is why onStartupFinished as the only activation event works for the existing commands.

@goldfirere
Copy link
Author

Thanks for this advice. I couldn't get anything with activationEvents working, but your message inspired me to clone my local repo into a new folder (yarn clean isn't a thing... and I only thought of git clean when writing this comment), where my new stuff works. Hooray!

What are good next steps here? I already noticed a bug in my code, in that if I click on a code lens to do the comparison, the cursor still needs to be within the conflict region for the comparison to work. This is because I didn't, at first, understand how/where the conflict ever gets passed to the action. I'm pretty sure I can fix this. And I suppose I could avoid the constructed string by just enumerating the three possibilities separately. (Luckily, three is a small number.)

But the tasks beyond this are beyond me: creating tests, any documentation, etc. I could put in this time, but is it a reasonable expectation that this patch would get merged? I don't want to spend time cleaning the code and learning more about how to contribute without a reward at the end. Or, is it a better strategy to wait for the VSCode team to address this, essentially using my patch as a tighter specification of what I'd like?

What do you suggest?

Thanks for your help!

@chrmarti
Copy link
Contributor

Could you open a pull request with your latest code? I wonder if just adding more code lenses will overload the UI a bit.

@goldfirere
Copy link
Author

I agree about the code lenses -- it was actually just a small experiment to see if the strange behavior I was witnessing was related to the command palette somehow and I wanted a new way to trigger my new code path. I will remove it -- happy for this feature to be available only for someone who looks for it.

OK. I will polish a bit and post a PR. Thanks.

goldfirere added a commit to goldfirere/vscode that referenced this issue Sep 27, 2021
This allows for comparison between current/original and
original/incoming if the underlying file has 3-way merge
information in it.

Fixes microsoft#133088
@goldfirere goldfirere linked a pull request Sep 27, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Request for new features or functionality merge-conflict Merge conflict decorations and actions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants