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

[Common] Add git conflict marker highlighting #4047

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

deathaxe
Copy link
Collaborator

@deathaxe deathaxe commented Sep 29, 2024

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 This file gets stuck in the loading progress bar sublime_text#6498, which is caused by git merge conflict markers, but applies to all syntax definitions. It is therefore a possible alternative for [JavaScript] Fix infinite loop caused by diff conflict markers #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 [ShellScript] Add ZSH #4024. Further action depends on which PR is merged first - if at all.

This commit adds patterns to treat git conflict markers as comments
in syntax definitions.

Goals are
1. improve UX when using ST as git merge tool
2. reduce risk of badly breaking syntax highlighting due to conflict markers
@deathaxe deathaxe force-pushed the pr/common/git-conflict-markers branch from 038ae7f to 7af7f63 Compare September 29, 2024 10:04
@michaelblyons
Copy link
Collaborator

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.

@deathaxe
Copy link
Collaborator Author

deathaxe commented Sep 29, 2024

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
Copy link
Collaborator Author

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.

This commit adjusts patterns to optionally support names next to all
conflict markers in the same way to comply with diff3's manpage.
@michaelblyons
Copy link
Collaborator

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

@deathaxe
Copy link
Collaborator Author

deathaxe commented Oct 4, 2024

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

This commit...

1. applies more generic text.diff3 scope as those markers are not tight to git
2. renames and moves syntax definition to Packages/Text/Diff3.sublime-syntax
@deathaxe
Copy link
Collaborator Author

deathaxe commented Oct 5, 2024

  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 deathaxe force-pushed the pr/common/git-conflict-markers branch from 1c0e745 to b5da5ec Compare October 25, 2024 15:52
@deathaxe
Copy link
Collaborator Author

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.

@michaelblyons
Copy link
Collaborator

michaelblyons commented Oct 25, 2024

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.

@deathaxe
Copy link
Collaborator Author

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.

@michaelblyons
Copy link
Collaborator

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.

@deathaxe
Copy link
Collaborator Author

How about adding support for normal output?

@michaelblyons
Copy link
Collaborator

michaelblyons commented Oct 25, 2024

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.

As it is planned to add more than just conflicts.
@deathaxe
Copy link
Collaborator Author

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?

@michaelblyons
Copy link
Collaborator

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.

Implements https://www.gnu.org/software/diffutils/manual/diffutils.html#An-Example-of-diff3-Normal-Format

Syntax tolerates leading whitespace in case it is to be embedded
into Markdown's fenced code blocks or any other possibly indented contexts.

Patterns should be strict enough for risk of false positives
to be acceptable low.
Apply same syntax specific settings as for diff syntax.
... to align it with main scope Diff.sublime-syntax uses.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants