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.

Sign up for GitHub

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

Jump to bottom

Handle diff3 conflict markers #62391

Merged
merged 7 commits into from
Jul 16, 2022
Merged

Handle diff3 conflict markers #62391

merged 7 commits into from
Jul 16, 2022

Conversation

jcouv
Copy link
Member

@jcouv jcouv commented Jul 5, 2022 •
edited
Loading

Extends our support for diff markers in source to diff3 format (which adds a '|||||||' conflict marker).
Recent example of diff3: 80bdf24
git config --global merge.conflictstyle diff3

@jcouv jcouv self-assigned this Jul 5, 2022
@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Jul 5, 2022 •
edited by jcouv
Loading

oops. missed this is draft. ping me when you would like a review :)

In reply to: 1175590449

CyrusNajmabadi
CyrusNajmabadi reviewed Jul 5, 2022
View reviewed changes
src/Features/Core/Portable/ConflictMarkerResolution/AbstractConflictMarkerCodeFixProvider.cs Outdated
firstMiddleLine = secondMiddleLine;
}

return success;
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Jul 5, 2022 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

nit: all these blocks end teh same way. so instead, can you break out and then handle the firstMiddleLien = secondMiddleLine stuff outside the switch? thanks! #Closed

@jcouv jcouv marked this pull request as ready for review July 7, 2022 16:18
@jcouv jcouv requested review from a team as code owners July 7, 2022 16:18
@jcouv
Copy link
Member Author

jcouv commented Jul 7, 2022

@CyrusNajmabadi I added VB support and some classification tests

@CyrusNajmabadi
Copy link
Member

IDE lgtm :)

@RikkiGibson RikkiGibson self-assigned this Jul 7, 2022
@jasonmalinowski
Copy link
Member

jasonmalinowski commented Jul 7, 2022 •
edited by jcouv
Loading

@jcouv Oh goodness this has bugged me for a long time. Thanks for fixing it!

In reply to: 1178044319

RikkiGibson
RikkiGibson reviewed Jul 7, 2022
View reviewed changes
src/Compilers/CSharp/Test/Syntax/LexicalAndXml/LexicalTests.cs Outdated

trivia = token.LeadingTrivia[2];
Assert.True(trivia.Kind() == SyntaxKind.DisabledTextTrivia);
Assert.Equal("disabled text 2\r\n======= \r\nmore disabled text\r\n", trivia.ToFullString());
Copy link
Contributor

@RikkiGibson RikkiGibson Jul 7, 2022 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

Is it fine that we don't lex this as disabled text, followed by a conflict marker, followed by more disabled text? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I think so. That's what we did prior to this PR and I can't think of any benefit to trying to parse the second ======= as a marker.

Copy link
Member

Choose a reason for hiding this comment

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

I would in fact have been concerned if we did lex this as a marker.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you help me understand why that would be a concern? It seems like if we want to represent conflict regions in structured trivia then we would want to indicate which things are markers and which things are "content".

Copy link
Member

Choose a reason for hiding this comment

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

There aren't any diff styles (that I know of) where there are two ======= sections.

@jcouv jcouv requested a review from RikkiGibson July 8, 2022 16:17
@jcouv
Copy link
Member Author

jcouv commented Jul 8, 2022

@dotnet/roslyn-compiler for review. Thanks

@jcouv
Copy link
Member Author

jcouv commented Jul 8, 2022

@dotnet/roslyn-compiler for second review. Thanks

3 similar comments
@jcouv
Copy link
Member Author

jcouv commented Jul 11, 2022

@dotnet/roslyn-compiler for second review. Thanks

@jcouv
Copy link
Member Author

jcouv commented Jul 12, 2022

@dotnet/roslyn-compiler for second review. Thanks

@jcouv
Copy link
Member Author

jcouv commented Jul 14, 2022

@dotnet/roslyn-compiler for second review. Thanks

333fred
333fred reviewed Jul 14, 2022
View reviewed changes
Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

Done review pass.

src/Compilers/CSharp/Test/Syntax/LexicalAndXml/LexicalTests.cs Outdated
public void TestEqualsConflictMarker3()
{
// second ======= is part of disabled text
var token = Lex("======= Trailing\r\ndisabled text 2\r\n======= \r\nmore disabled text\r\n>>>>>>> Actually the end").First();
Copy link
Member

@333fred 333fred Jul 14, 2022 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

I know the rest of these tests are using the single-line syntax, but can we use raw strings here for readability? I find it very hard to understand what this is testing. #Resolved

Copy link
Member

Choose a reason for hiding this comment

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

a main problem with the compiler test codebase is that its git rules transforms the content to have unix vs windows newlines when going between systems. So it's hard to actually test/validate things like this with raw strings since the test input literally changes depending on the system you run it on. You'd need to have the test validation do the same to make up for this.

My preference would be that files are data and for many reasons (consistency, determinism, etc.) the contents are the same no matter what system you are on.

src/Compilers/CSharp/Test/Syntax/LexicalAndXml/LexicalTests.cs Outdated

trivia = token.LeadingTrivia[2];
Assert.True(trivia.Kind() == SyntaxKind.DisabledTextTrivia);
Assert.Equal("disabled text 2\r\n======= \r\nmore disabled text\r\n", trivia.ToFullString());
Copy link
Member

Choose a reason for hiding this comment

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

I would in fact have been concerned if we did lex this as a marker.

src/Compilers/CSharp/Test/Syntax/LexicalAndXml/LexicalTests.cs Outdated
public void TestPipeConflictMarker5()
{
// second ||||||| is pat of disabled text
var token = Lex("||||||| Mid\r\ndisabled text 1\r\n||||||| \r\nmore disabled text\r\n======= Trailing\r\ndisabled text 2\r\n>>>>>>> Actually the end").First();
Copy link
Member

@333fred 333fred Jul 14, 2022 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

Test with ======= first as well? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

Added TestEqualsConflictMarker4 to test ======= followed by |||||||

@jcouv jcouv requested a review from 333fred July 15, 2022 19:53
@jcouv jcouv merged commit 7693a6f into dotnet:main Jul 16, 2022
@jcouv jcouv deleted the diff3-markers branch July 16, 2022 04:56
@ghost ghost added this to the Next milestone Jul 16, 2022
333fred added a commit to 333fred/roslyn that referenced this pull request Jul 18, 2022
@333fred
Merge remote-tracking branch 'upstream/main' into forbid-inumber
cc0fc20 
* upstream/main: (40 commits)
  Revert "Generalize RoamingProfileStorageLocation to ClientSettingsStorageLocation (dotnet#62684)"
  Revert "Update SymReader to the latest version (dotnet#62695)"
  Apply changes directly to text buffer - take 2 (dotnet#62617)
  Allow diagnostic tagging to use either push or pull diagnostics
  Tests test and more tests
  Fix new rename showing overloads when not applicable (dotnet#62559)
  Add stronger type safety
  Add support for resolving contextual error types
  Pass LineFormatting options into OmniSharpCodeAction options
  Initial approach to supporting contextual types
  Update dependencies from https://github.com/dotnet/arcade build 20220715.4 (dotnet#62704)
  Handle diff3 conflict markers (dotnet#62391)
  Update SymReader to the latest version (dotnet#62695)
  Generalize RoamingProfileStorageLocation to ClientSettingsStorageLocation (dotnet#62684)
  Add IWorkspaceProjectContextFactory F# wappers (dotnet#62646)
  Use OmniSharp LineFormatting fallback options in more places.
  Remove dependency on Microsoft.VisualStudio.RemoteControl assembly from Features (dotnet#62655)
  Add array initialization optimization (dotnet#62392)
  [main] Update dependencies from dotnet/arcade (dotnet#62597)
  Keep selection on rename invocation (dotnet#62654)
  ...
adamperlin pushed a commit to adamperlin/roslyn that referenced this pull request Jul 19, 2022
@jcouv @adamperlin
Handle diff3 conflict markers (dotnet#62391)
6c363f1
@allisonchou allisonchou modified the milestones: Next, 17.4 P1 Jul 26, 2022
@ryzngard ryzngard mentioned this pull request Jul 27, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi approved these changes

@RikkiGibson RikkiGibson RikkiGibson approved these changes

@333fred 333fred 333fred approved these changes

Assignees

@RikkiGibson RikkiGibson

@jcouv jcouv

Labels
Area-Compilers
Projects
None yet
Milestone
17.4 P1
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@jcouv @CyrusNajmabadi @jasonmalinowski @333fred @RikkiGibson @allisonchou