Packages icon indicating copy to clipboard operation
Packages copied to clipboard

[Diff] Refactor for Diff3, Git, and conflicts

Open michaelblyons opened this issue 1 year ago • 18 comments

Big refactor of Diff syntax according to the GNU and SVN documentation. New Git Diff syntax, also used by Git Log.

Thank you to the reviewers!

Notable changes

  • Lots of documentation links
  • Lots of new tests, mostly pulled from relevant documentation
  • Some scope tweaking, but preserving the stacking from #2178 for backwards-compatibility
  • New syntax for Git Diff emails (git am and git format-patch)
    • When the normal Diff syntax detects the "magic" first line, it delegates out to Git Diff instead.
  • New support for
    • Diff3 formats
    • Conflict sections
    • "Combined" unified diffs, only in Git Diff

Potentially controversial

  • Conflicts somewhat with #4047, but should now be nearly compatible.
  • storage.modifier distinguishes between the two Git file modes (namely 644 and 755)
  • Markup scopes color the ++++---- and similar, even though they're not actually markup.
  • Users and emails are delegated out to Mailmap.
  • Whatever you noticed.

michaelblyons avatar Dec 07 '24 21:12 michaelblyons

Am I supposed to see output on the failing unit tests? I don't think anything's failing locally.

michaelblyons avatar Dec 07 '24 23:12 michaelblyons

Am I supposed to see output on the failing unit tests? I don't think anything's failing locally.

Presumably it is the error about Diff mixed inheritance versions at the very top of the CI console output: https://github.com/sublimehq/Packages/actions/runs/12216318274/job/34079277181#step:4:6

(Bit weird that the error comes before the packages path information etc)

error parsing lexer: Packages/Diff/Diff.sublime-syntax: syntax inheritance does not allow for mixed versions at line 5 column 1

keith-hall avatar Dec 08 '24 06:12 keith-hall

Should be fixed by updating Packages/PHP/Embeddings/Diff (for PHP Interpolated).sublime-syntax to version 2.

deathaxe avatar Dec 08 '24 11:12 deathaxe

🤦‍♂️ I think my ad blocker was preventing display of the test output. Thanks!

michaelblyons avatar Dec 08 '24 13:12 michaelblyons

a few thoughts:

  • deathaxe started moving variables to the very bottom of syntax files, you might as well
  • similarly with contexts within the prototype section, moving that section to the bottom of contexts
  • I'd appreciate a bit more use of whitespace in syntax files, makes them easier to read
  • it might make sense to replace some of those multi-level pushes with pushes to named contexts, so it's easier to override or extend

jrappen avatar Dec 09 '24 21:12 jrappen

Currently you're only matching English headers. They might not be in English as Git supports a few languages. You'd have to look that up.

jrappen avatar Dec 09 '24 22:12 jrappen

Currently you're only matching English headers. They might not be in English as Git supports a few languages. You'd have to look that up.

For this, you are referring to the Signed-off-by:, etc., yes?

michaelblyons avatar Dec 09 '24 22:12 michaelblyons

I haven't checked what exactly needs translations, didn't have the time. I just recall deathaxe adding huge walls of variables with translations to those other Git syntaxes.

jrappen avatar Dec 10 '24 00:12 jrappen

Should we merge Conflict.sublime-syntax into the Diff syntax like #4047?

michaelblyons avatar Dec 18 '24 13:12 michaelblyons

Unsure. As both don't share anything and merge conflicts appear without anything diff related, a stand-alone syntax is ok, too, I guess. Normal diffs won't ever contain conflict markers, would they?

deathaxe avatar Dec 18 '24 14:12 deathaxe

Normal diffs won't ever contain conflict markers, would they?

No, but after I filed "Conflict?" into my broad organization of diff types, I'm much more willing to concede merging it. Almost none of the other diff types share any commonality with each other. They're just simple enough that they can coexist in the same file with no issue. Conflict could do the same (though I would change the variable names).

I'm considering adding a side-by-side diff highlighter, too, although I haven't looked too hard. (I think it'd be hard to do perfect, but easy to do good-enough.)

michaelblyons avatar Dec 18 '24 15:12 michaelblyons

I am fine with both, keeping or merging.

deathaxe avatar Dec 18 '24 15:12 deathaxe

I've brought Conflict.sublime-syntax back in, ~~along with a conservative side-by-side highlighter. A more risky side-by-side syntax at https://github.com/michaelblyons/SublimeSyntax-Defaults/pull/10~~ Edit: Deleted side-by-side now that we have overrideable beginning-of-line.

michaelblyons avatar Dec 18 '24 20:12 michaelblyons

Rework of Git Log to use Git Diff contexts at https://github.com/michaelblyons/SublimeSyntax-Defaults/pull/11

michaelblyons avatar Dec 19 '24 19:12 michaelblyons

let us know when/if you need any review or feedback 😁

jrappen avatar Dec 20 '24 20:12 jrappen

let us know when/if you need any review or feedback 😁

Haha. Sorry if you're flooded in notifications. The PR is a lot better after you and DeathAxe's reviews. I think if you're looking for something, ~~check my PRs to this PR (michaelblyons#11 and michaelblyons#10) and~~ decide if you care about any of the things in the "controversial" heading in OP.

Thanks again.

michaelblyons avatar Dec 20 '24 22:12 michaelblyons

Empty lines between contexts is ok, but starting arbritary emptly lines between patterns is too much.

deathaxe avatar Dec 22 '24 12:12 deathaxe

There are places where I have considered newlines in stacked scopes, but I am not planning to add an empty line between meta and includes. PackageDev scoping is sufficient to make that distinction very obvious.

michaelblyons avatar Dec 22 '24 13:12 michaelblyons

Hooray! Thanks, everyone.

michaelblyons avatar Feb 24 '25 18:02 michaelblyons