Skip to content

Handle diff3-style merge correctly#27405

Merged
chrmarti merged 4 commits into
microsoft:masterfrom
katemihalikova:diff3
Jun 13, 2017
Merged

Handle diff3-style merge correctly#27405
chrmarti merged 4 commits into
microsoft:masterfrom
katemihalikova:diff3

Conversation

@katemihalikova

Copy link
Copy Markdown
Contributor

Fixes pprice/vscode-better-merge#23.

I'm using merge.conflictStyle = diff3 git setting, but the style (Current/Common ancestors/Incoming) is not supported in VS Code. In the new merge extension (#27150), the Common ancestors block is merged together with the Current block. This PR adds that support. As diff3 is a superset of the default git conflict style, I've modified the original algorithm to support both styles. The same fix was also proposed by @spiffytech in the original issue.

@mention-bot

Copy link
Copy Markdown

@katemihalikova, thanks for your PR! By analyzing the history of the files in this pull request, we identified @pprice to be a potential reviewer.

@msftclas

Copy link
Copy Markdown

@katemihalikova,
Thanks for having already signed the Contribution License Agreement. Your agreement has not been validated yet. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

@sandy081

Copy link
Copy Markdown
Member

@kieferrm Not sure who owns the merge extension, hence assigning this to you. Please re-assign it to the right person.

Thanks.

@pprice pprice left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 Thank you @katemihalikova LGTM! @chrmarti / @kieferrm can give final approval.

I tested Kate's fork locally, and everything appears to be working great.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kieferrm, @chrmarti: We should probably add a new theme color from common ancestor blocks. I'd be in favor of coloring them at some point.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit; might as well mark commonAncestors as optional e.g. commonAncestors?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Marked as optional

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit; Could be condensed to

const tokenAfterCurrentBlock = scanned.commonAncestors || scanned.splitter; 

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Condensed

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If, for some reason, we ended up with:

<<<<<<< HEAD
Foo
||||||| common
foo
||||||| common
fooo
=======
blah
>>>>>>> incoming

We will end up extending the "current" content over the first merge ancestor block. If we encounter two blocks of ||||||| I think there are two thing we could do:

  • Favorable: Don't overwrite currentConflict.commonAncestors, just leave it with the first occurrence, this way the current content block will span the correct range. (e.g. else if (currentConflict && !currentConflict.commonAncestors && line.text.startsWith(commonAncestorsMarker))
  • Break out of scanning (like we do if we encounter a start marker before an end marker), so we don't provide any decoration if we are unsure what to do.

FWIW I think this is also true of existing ======= scanning...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Incorporated the first solution for both markers, now we are using just the first token of each type, just like TypeScript does.

@chrmarti chrmarti self-assigned this Jun 1, 2017
@chrmarti chrmarti added this to the June 2017 milestone Jun 1, 2017
@katemihalikova

Copy link
Copy Markdown
Contributor Author

I've resolved the conflict.

@chrmarti

chrmarti commented Jun 7, 2017

Copy link
Copy Markdown
Collaborator

Thanks for your contribution @katemihalikova.

I will add the workbench colors on top. One corner case is with multiple common ancestors where these have only a single header decoration. With some background colors applied this looks like:
screen shot 2017-06-07 at 9 49 33 am

Would adding one header and one content decoration for each common ancestor improve on this?

@katemihalikova

Copy link
Copy Markdown
Contributor Author

Yeah that makes sense, I'll add it.

@katemihalikova

Copy link
Copy Markdown
Contributor Author

@chrmarti The last commit added support for multiple common ancestors blocks:

image

(background colors are not included)

@chrmarti chrmarti merged commit ab52889 into microsoft:master Jun 13, 2017
@chrmarti

Copy link
Copy Markdown
Collaborator

Thanks @katemihalikova !

@katemihalikova katemihalikova deleted the diff3 branch June 15, 2017 13:59
@github-actions github-actions Bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants