gitextensions / gitextensions

Git Extensions is a standalone UI tool for managing git repositories. It also integrates with Windows Explorer and Microsoft Visual Studio (2015/2017/2019).
https://gitextensions.github.io/
Other
7.7k stars 2.08k forks source link

[vNext] Inconsistent diff colouring #11820

Closed RussKie closed 1 month ago

RussKie commented 1 month ago

Environment

Issue description

The diff colouring for C# namespaces is inconsistent: image

Steps to reproduce

  1. Install GitExtensions-x64-4.3.0.17872-70184a4c7.msi from https://ci.appveyor.com/project/gitextensions/gitextensions/builds/50328130/artifacts
  2. Open a diff

Did this work in previous version of GitExtensions?

Yes, this is how the diff is coloured in 4.2: image

Diagnostics

No response

RussKie commented 1 month ago

@gerhardol you worked on the colouring, right?

gerhardol commented 1 month ago

The git-diff color engine is used by default. You can get the simpler GE engine back in settings.

The blue/violetish is for moved lines, a useful feature, the grey is git detection of syntax.

Unless the interpretation from git command line is incorrect, report to git.

RussKie commented 1 month ago

Here's another example:

  1. Click on a diff 1cd999e35bd4c7c0eb4cb28671927ddfcf7facad for tests/app/UnitTests/GitCommands.Tests/UserRepositoryHistory/RecentRepoSplitterTests.cs

It kind of feels to me it's a bug in our rendering - sut.SortRecentRepos = true; is moved, so at the top it's rendered "as moved" but then at the bottom in the new code it's rendered as "moved" again. image

gerhardol commented 1 month ago

@RussKie it is git that colors, it is not perfect but in general much more useful. You can override the git config to disable moved code or set ge coloring in ge settings. I do not want to change the defaults.

gerhardol commented 1 month ago

Git coloring is described in the PR,: https://github.com/gitextensions/gitextensions/pull/11590, slightly changed in #11721 The PR has an example with how to view the colors for the eight terminal colors in normal/bold/dimmed variations: https://github.com/gitextensions/gitextensions/blob/master/src/app/GitUI/Editor/Diff/AnsiEscapeUtilities.cs#L411 The default colors (as explained in the code) is mostly derived from mintty/putty https://github.com/mintty/mintty/blob/master/themes/helmholtz The slots that are colored in the PR: https://git-scm.com/docs/git-config#Documentation/git-config.txt-colordiffltslotgt Git default color names in the Git code https://github.com/git/git/blob/master/diff.c#L105 (so old moved is magenta/blue and new moved is cyan/yellow)

RussKie commented 1 month ago

Here's another example:

  1. Click on a diff 1cd999e for tests/app/UnitTests/GitCommands.Tests/UserRepositoryHistory/RecentRepoSplitterTests.cs

It kind of feels to me it's a bug in our rendering - sut.SortRecentRepos = true; is moved, so at the top it's rendered "as moved" but then at the bottom in the new code it's rendered as "moved" again. image

I still currently of an opinion the issue is in our implementation.

Running the diff in the console produces the expected results:

git diff --find-renames --find-copies --color=always --ignore-all-space --unified=3 "f347022939caf2e140cb5020c195db0b8bd71a37" "1cd999e35bd4c7c0eb4cb28671927ddfcf7facad" -- "tests/app/UnitTests/GitCommands.Tests/UserRepositoryHistory/RecentRepoSplitterTests.cs"

image

Running the diff with the full command Git Extensions is generating produces the incorrect result:

git -c color.ui=never -c diff.submodule=short -c diff.noprefix=false -c diff.mnemonicprefix=false -c diff.ignoreSubmodules=none -c core.safecrlf=false -c diff.colorMovedWS=no -c diff.wordRegex=. -c diff.colorMoved=dimmed-zebra -c color.diff.oldMoved=magenta -c color.diff.newMoved=blue -c color.diff.oldMovedAlternative=cyan -c color.diff.newMovedAlternative=yellow -c color.diff.oldMovedDimmed="magenta dim" -c color.diff.newMovedDimmed="blue dim" -c color.diff.oldMovedAlternativeDimmed="cyan dim" -c color.diff.newMovedAlternativeDimmed="yellow dim" -c color.diff.old="#000000 #ffc8c8" -c color.diff.new="#000000 #c8ffc8" diff --find-renames --find-copies --color=always --ignore-all-space --unified=3 "f347022939caf2e140cb5020c195db0b8bd71a37" "1cd999e35bd4c7c0eb4cb28671927ddfcf7facad" -- "tests/app/UnitTests/GitCommands.Tests/UserRepositoryHistory/RecentRepoSplitterTests.cs"

image

RussKie commented 1 month ago

On a second thought, you're probably correct, @gerhardol - it could be a git bug. Where would I report it?

gerhardol commented 1 month ago

Note that you can view the raw git output in the command cache if there are doubts about the GE processing of the ouputs, it is colored blue there. image

The first diff seem correct as configured. Mostly useful. Sidenote: When reviewing I constantly toggle normal, word-diff and difftastic to see diffs safe and quick. There are issues with all views.

So this should be only about the second diff. The added line is incorrectly detected as moved. So regardless of how moved is configured, I see the issue. You may use GE coloring or configure this in Git: git config --global diff.colorMoved no (default dimmed-zebra) If a user has set git-config, GE does not override.

But the diplay is an issue in Git, the move algorithm is not perfect. Me and mstv had some discussions about this both for word-diff and difftastic, another reason you need many views of a diff. (and I wasted time getting word-diff in patch format) I have not reported that. Report issues as described in: https://git-scm.com/community