Packages icon indicating copy to clipboard operation
Packages copied to clipboard

[Common] Add git conflict marker highlighting

Open deathaxe opened this issue 1 year ago • 18 comments

This PR ...

  1. adds ~~Git Formats/Git Merge.sublime-settings~~ Text/Diff3.sublime-syntax with simple and restrictive patterns for git conflict markers.

    They explicitly do not push contexts to avoid interfering with interpolations or any other advanced syntax features.

  2. includes those patterns as sort of comments in all syntax bundled definitions.

    Comments are included in most contexts in all syntax definitions, thus conflict markers should be matched properly with high chances as well.

  3. includes merge conflict patterns into known multi-line strings or heredocs.

grafik

  1. adds conflicts to local symbol list to support quick navigation or finding them

grafik

Goals

  1. improve UX when using ST as git merge tool
  2. reduce risk of badly breaking syntax highlighting due to conflict markers

Motivation

  1. This PR was created as possible fix/workaround for https://github.com/sublimehq/sublime_text/issues/6498, which is caused by git merge conflict markers, but applies to all syntax definitions. It is therefore a possible alternative for #4046, assuming there are no other "invalid syntax" scenarios, which can break JSX/TSX in such bad ways.
  2. Git is a rather common tool these days, and ST can be used as an editor to edit commit messages or resolve conflicts. As such, git conflict markers shouldn't break syntax highlighting or even cause crashes or deadlocks.
  3. various linters/tools, such as JSX/TSX language servers also detect git conflict markers and highlight them, most likely to avoid same kind of issues we see with regards to confusing syntax engine.

Remarks

  1. With this PR all syntax definitions depend on Git Merge.sublime-syntax and thus require Git Formats package to be enabled.

    Pro: Single file to maintain, simple re-usable context for all syntaxes

    Contra: inter-package dependency

    Alternative: duplicate conflict context into all (about 40) syntax definitions :/

  2. Initial state of this PR provides a "works in most cases" quality based on existing comments/strings contexts. There may be some situations left, in which conflict markers are not yet detected. Support can however easily be improved in future.

  3. Some syntaxes such as Markdown use === to highlight headings, which still may cause syntax highlighting issues caused by unavoidable ambiguities. So this PR can provide "the base we can" quality.

  4. This PR excludes changes to ShellScript to avoid conflicts with #4024. Further action depends on which PR is merged first - if at all.

deathaxe avatar Sep 29 '24 09:09 deathaxe

I have some thoughts, not in any order, nor necessarily exhaustive:

  1. If Git is disabled, I expect the matches just don't show up. It's not catastrophic, right?
  2. I like the concept. I have a minimal "Conflict" syntax lying around somewhere with markup.line.(inserted|deleted), which is probably mostly obsolete with this change.
  3. In case it's not obvious to everyone, this can result in mismatched context start/end, but that was already happening anyway.
  4. If this is well-received, we should also support ||||| for diff3 markers.
  5. This is a great example of how injections would be helpful as a core feature.

michaelblyons avatar Sep 29 '24 17:09 michaelblyons

If Git is disabled, I expect the matches just don't show up. It's not catastrophic, right?

The only downside would be console messages about "no such context".

In case it's not obvious to everyone, this can result in mismatched context start/end, but that was already happening anyway.

Rather rarely caused by injected conflict marker patterns as they are atomic and don't push contexts, but possibly by the way ours/theirs is interrupting normal source code. That's nothing which can be fixed.

If this is well-received, we should also support ||||| for diff3 markers.

Possible

This is a great example of how injections would be helpful as a core feature.

Certainly.

deathaxe avatar Sep 29 '24 18:09 deathaxe

An alternative location for the Merge syntax would be within Text package, which if disabled causes lots of trouble anyway and is thus less likely to be.

This would be justified by conflict markers not being limited to git, but rather being a common unix convention, with regards to diff3.

deathaxe avatar Sep 29 '24 18:09 deathaxe

by the way ours/theirs is interrupting normal source code.

Yes, that's what I mean. Agreed that it can't be helped.

<<<<<
func foo (bar) {
=====
func foo (bar, baz) {
>>>>>
// Extra nesting

michaelblyons avatar Sep 29 '24 18:09 michaelblyons

Still UX is way better then with most syntaxes highlighting those markers as operators, tag punctuation or even illegal.

deathaxe avatar Oct 04 '24 13:10 deathaxe

  1. Renamed scopes to original unix diff3 scope as those markers are not tight to git.
  2. moved it to Text package to always be available.

Ideally Diff package seems the more suitable target, but it is not included in Sublime Merge and could also be disabled like Git Formats.

deathaxe avatar Oct 05 '24 10:10 deathaxe

Moved Diff3.sublime-syntax into Diff package as it finally feels misplaced in Text package.

@dpjohnst : As Diff3 is included by all other syntaxes, Diff package needs to be shipped with Sublime Merge, once this PR gets merged.

deathaxe avatar Oct 25 '24 15:10 deathaxe

Moved Diff3.sublime-syntax into Diff package as it finally feels misplaced in Text package.

Good. I thought about recommending this, and forgot to post.

If I were doing this, I'd make the syntax a functional one called Diff/Conflict.sublime-syntax and have markup add/remove sections in between the conflict markers. (Unsure what to use for the Diff3 common base. I personally use comment for those, but I could see markup modified as another possibility.) You can still grab just the marker contexts for your injection into the other syntaxes.

Edit: I guess reusing the same contexts wouldn't work since one has to push and the other can't, but maybe having them in that same file would still be useful.

michaelblyons avatar Oct 25 '24 16:10 michaelblyons

Name "Diff3" is inspired by the original unix tool producing output with such tags, like unix tool "diff" produces output with syntax "Diff".

Renaming it to Conflict.sublime-syntax would mean to remove reference to diff3, which I don't find ideal.

Context conflicts exists to be able to add more diff3 specific rules, which are not of interest in other syntaxes. I haven't checked if that will be required though.

We could otherwise follow Diff.sublime-syntax and just put everything in main. I currently don't see reasons to add each pattern into a dedicated context as they are always required together and chances they need to be overridden individually seem rather low.

markup scope is somewhat inspired by Markdown, which scopes most such special things that way. That scope is also rather uncommon in normal source code and thus may provide prominent enough highlighting to clearly stand out from the rest.

Scoping it comment would be as correct though.

deathaxe avatar Oct 25 '24 16:10 deathaxe

How about Diff3 Conflict.sublime-syntax, then? The reason I'm shy about calling it Diff3.sublime-syntax is that the "normal" Diff3 output looks very different from that.

michaelblyons avatar Oct 25 '24 16:10 michaelblyons

How about adding support for normal output?

deathaxe avatar Oct 25 '24 16:10 deathaxe

That might work. IIRC, one of the (three!) diff3 output modes has a conflict with one kind of normal diff output, but they might not conflict with each other. It might be possible to bundle all three diff3 output styles into a single syntax.

If that's the route you like, it doesn't need to hold up this PR. There's enough here already that might be controversial.

michaelblyons avatar Oct 25 '24 17:10 michaelblyons

Well, I'd probably start with normal output mode.

I have no strong meaning about scope names - just explained their origin.

With a look at Diff we could also use meta.separator punctuation.definition.separator for <<<<< etc.

Anything which allows them to be uniquely addressed by color schemes might be ok. The question is what defaults we prefer. Should they appear like comments in the first place or just like plain text (which Mariana/Monokai) highlight them, currently.

Another question arises. Diff uses source.diff. Should we therefore change main scope to source.diff3?

deathaxe avatar Oct 25 '24 17:10 deathaxe

With a look at Diff we could also use meta.separator punctuation.definition.separator for <<<<< etc.

I'm opposed. I think the scope you already chose is the right one, and Diff should change to match.

Anything which allows them to be uniquely addressed by color schemes might be ok. The question is what defaults we prefer. Should they appear like comments in the first place or just like plain text (which Mariana/Monokai) highlight them, currently.

Oh, the conflict markers? I would not use comment. I think your original punctuation.section is correct, as I said above.

I was only talking about if a simple syntax is made whose main highlights Diff3 conflicts, not the injection of conflict markers into complicated source code.

octopus
<<<<< a
tiger
whale
\^^^^^ markup.line.deleted
|||||
koala
\^^^^^ ???? not sure if `comment` or `markup.line.modified`
=====
elephant
\^^^^^^^^ markup.line.inserted
>>>>>

Another question arises. Diff uses source.diff. Should we therefore change main scope to source.diff3?

Seems fine. No strong opinions. I've changed my mind on this a bunch of times with no obvious right answer.

michaelblyons avatar Oct 25 '24 18:10 michaelblyons

image


Test files should start with syntax_test_, compare https://www.sublimetext.com/docs/syntax.html#testing.

⚠️ Otherwise they are not picked up.

jrappen avatar Nov 23 '24 09:11 jrappen

This may have interesting application for BracketHighlighter and something modeled after Git Conflict Resolver.

michaelblyons avatar Feb 13 '25 16:02 michaelblyons

... this may have interesting application for BracketHighlighter and something modeled after Git Conflict Resolver

FYI @facelessuser @sascha-wolf @alexocode

jrappen avatar Feb 14 '25 07:02 jrappen

I'll revisit this PR once #4111 is merged.

deathaxe avatar Feb 14 '25 07:02 deathaxe

Rebased branch and tweaked syntaxes to use Diff/Diff.sublime-syntax#conflict-markers.

Yet question remains, whether it is a good idea to create such massive cross package depenedency.

deathaxe avatar Feb 24 '25 20:02 deathaxe

I also thought I saw a Git Formats tmPreferences file

Yeah, I can always look at PRs for hours and a second after hitting push, I find something.

deathaxe avatar Feb 24 '25 20:02 deathaxe

@deathaxe Could you fix conflicts on your end? Were you going to add anything else?

jrappen avatar Apr 19 '25 07:04 jrappen

With regards to how shebang is implemented, I tend to think conflict marker contexts should also be implemented individually for each syntax to avoid cross-package dependencies, especially as Diff is not part of Sublime Merge.

deathaxe avatar Apr 19 '25 08:04 deathaxe

If someone finds unhandled multi-line strings in a syntax, feel free to report it.

Otherwise with dependencies to Diff.sublime-syntax being removed, this PR should be ready.

deathaxe avatar Apr 19 '25 09:04 deathaxe

Thanks, I'll take a look later tonight.

jrappen avatar Apr 19 '25 10:04 jrappen

I'm slightly concerned some of these changes might be lost as we'll have to double check this later again against a few major re-writes which are going on right now.

I'll make sure to add your changes from here to my PRs.

@michaelblyons Your current re-writes are affected, too.

jrappen avatar Apr 19 '25 18:04 jrappen

@deathaxe Sorry, I do have one more question.

Would it make sense to add these as folding markers as well?

jrappen avatar Apr 19 '25 19:04 jrappen

Folding would only make sense if ours/theirs/base parts could be folded individually. With base part being optional syntax, a begin/end may be missing, causing everything being folded.

It's also not quite sure how it interferes with syntaxes' own folding rules.

Hence I'd hesitate to "just" add some rules for it.

A possible ruleset, which might work may look like:

<?xml version="1.0" encoding="UTF-8"?>
<plist version="1.0">
<dict>
    <key>scope</key>
    <string>text, source</string>
    <key>settings</key>
    <dict>
        <key>foldScopes</key>
        <array>
            <dict>
                <key>begin</key>
                <string>
                    meta.block.conflict.begin - entity - punctuation,
                    meta.block.conflict.separator - entity - punctuation
                </string>
                <key>end</key>
                <string>meta.block.conflict punctuation</string>
            </dict>
        </array>
    </dict>
</dict>
</plist>

It should however be batteltested before adding.

deathaxe avatar Apr 19 '25 19:04 deathaxe

FWIW, it even makes a difference in Sublime Merge...

grafik

deathaxe avatar Apr 30 '25 07:04 deathaxe

Cool!

deathaxe avatar May 01 '25 20:05 deathaxe