Handle diff3 conflict markers by jcouv · Pull Request #62391 · dotnet/roslyn

jcouv · 2022-07-05T22:56:01Z

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

src/EditorFeatures/CSharpTest/ConflictMarkerResolution/ConflictMarkerResolutionTests.cs

CyrusNajmabadi · 2022-07-05T23:12:16Z

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

In reply to: 1175590449

src/Features/Core/Portable/ConflictMarkerResolution/AbstractConflictMarkerCodeFixProvider.cs

CyrusNajmabadi · 2022-07-05T23:17:21Z

src/Features/Core/Portable/ConflictMarkerResolution/AbstractConflictMarkerCodeFixProvider.cs

+                        firstMiddleLine = secondMiddleLine;
+                    }
+
+                    return success;


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

src/Features/Core/Portable/ConflictMarkerResolution/AbstractConflictMarkerCodeFixProvider.cs

jcouv · 2022-07-07T16:19:19Z

@CyrusNajmabadi I added VB support and some classification tests

CyrusNajmabadi · 2022-07-07T16:24:48Z

IDE lgtm :)

jasonmalinowski · 2022-07-07T18:28:19Z

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

In reply to: 1178044319

RikkiGibson · 2022-07-07T19:01:50Z

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

+
+            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());


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

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.

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

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".

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

jcouv · 2022-07-08T16:17:28Z

@dotnet/roslyn-compiler for review. Thanks

jcouv · 2022-07-08T17:53:14Z

@dotnet/roslyn-compiler for second review. Thanks

jcouv · 2022-07-11T15:48:49Z

@dotnet/roslyn-compiler for second review. Thanks

jcouv · 2022-07-12T19:54:59Z

@dotnet/roslyn-compiler for second review. Thanks

jcouv · 2022-07-14T16:06:41Z

@dotnet/roslyn-compiler for second review. Thanks

333fred

Done review pass.

333fred · 2022-07-14T23:35:05Z

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

+        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();


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

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.

333fred · 2022-07-14T23:36:01Z

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

+
+            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());


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

333fred · 2022-07-14T23:38:51Z

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

+        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();


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

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

* 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) ...

Handle diff3 conflict markers

8baece9

jcouv self-assigned this Jul 5, 2022

ghost added the Area-Compilers label Jul 5, 2022