Does not handle input from `git log --graph -p` properly

[blueyed]

When using git log --graph -p --color | diff-so-fancy | less the graph symbols apparently prevent diff-so-fancy from matching the input correctly.

When trying it in diff-so-fancy's repo (3dd2580), there's also some warnings:

:"\x{00e9}" does not map to utf8 at diff-so-fancy/libexec/diff-so-fancy.pl line 151, <> line 1102.
"\x{00e9}" does not map to utf8 at diff-so-fancy/libexec/diff-so-fancy.pl line 151, <> line 1102.
"\x{00e9}" does not map to utf8 at diff-so-fancy/libexec/diff-so-fancy.pl line 151, <> line 1102.
"\x{00e9}" does not map to utf8 at diff-so-fancy/libexec/diff-so-fancy.pl line 151, <> line 1102.
"\x{00e9}" does not map to utf8 at diff-so-fancy/libexec/diff-so-fancy.pl line 151, <> line 1102.

[scottchiefbaker]

Diff-so-fancy doesn't support the --graph option. Specifically it adds various characters | \ * to the beginning of each line which d-s-f doesn't understand. Moreover, it also messes up the indentation level of each line which d-s-f expects to be consistent.

D-s-f should work fine (it does for me) git log -p --color | diff-so-fancy without the --graph.

I consider this a CANTFIX. If you feel this is incorrect feel free to re-open this bug.

[blueyed]

I could imagine this to be fixable: the graph characters would be matched optionally (and/or trigger a special mode), and the indentation level would then be relative / set when a hunk starts.

I am using --graph by default often (basically git log --abbrev-commit --decorate --submodule=log --graph --stat -p).

[stevemao]

@blueyed Would it be good if we make d-s-f a noop if it sees some format that it doesn't understand?

[blueyed]

Might be fixed by 9e99f91. Need to check.

[blueyed]

Much better..

But it does colorizes the second ---/+++ section in the following (with git log --graph -p --color | diff-so-fancy | less in this repo):

| * commit fb8eba1
|/  Author: Steve Mao <maochenyan@gmail.com>
|   Date:   Mon Aug 8 21:39:25 2016 +1000
|   
|       change lib/ to libexec/
|       
|       fixes https://github.com/Homebrew/homebrew-core/pull/3663#issuecomment-23801
|   
|   diff --git a/.travis.yml b/.travis.yml
|   index ba69d93..d97717e 100644
|   --- a/.travis.yml
|   +++ b/.travis.yml
|   @@ -33,4 +33,4 @@ install:
|    script:
|      - ./bats/bin/bats test/*.bats
|      - shellcheck diff-so-fancy update-deps.sh
|   -  - perlcritic -1 -q --theme freenode lib/diff-so-fancy.pl
|   +  - perlcritic -1 -q --theme freenode libexec/diff-so-fancy.pl
|   diff --git a/diff-so-fancy b/diff-so-fancy
|   index 946d56f..93a5a62 100755
|   --- a/diff-so-fancy
|   +++ b/diff-so-fancy
|   @@ -23,7 +23,7 @@ else
|    fi

[blueyed]

@scottchiefbaker please re-open the issue.

[OJFord]

But it does colorizes the second ---/+++ section

Yep, I think it's because it's not designed to see a second diff, so the second header's --- is taken as removing a line that begins --.

Might be fairly simple to 'reset' on seeing a second diff a/ b/ line @scottchiefbaker?

[scottchiefbaker]

If there are more than two colums of "+" or "-" at the beginning of the line d-s-f will definitely not know how to handle it. This is a know "feature"... because the use case for more than two is pretty slim, and the fix is pretty gigantic.

[scottchiefbaker]

This might be fixed now that we support three-way diffs. Can you test again? Is this still something we should focus on?

[artem-nefedov]

I found that diff-so-fancy generates incorrect patch when git log -p --graph is called.
Here's how to reproduce it (latest version cf153d8):

1. Commit file with the following content:

$ cat test.txt
-1 - option 1
-2 - option 2
-4 - option 4
-5 - option 5
$ git add test.txt
$ git commit -m 'test file'

2. Add following line between options 2 and 4:

-3 - option 3

3. Commit changes

$ git commit -a -m 'option 3'

4. Test log -p without --graph ("option 3" is highlighted as green here):

-1 - option 1
-2 - option 2
-3 - option 3
-4 - option 4
-5 - option 5

5. Test log -p --graph:

|  -1 - option 1
|  -2 - option 2
|  -4 - option 4
|  -5 - option 5
| +-3 - option 3

Suddenly, it's moved to the end of file.
It would by one thing if it didn't work, but silently generating incorrect patch without any errors is pretty dangerous in my opinion.

[ferdnyc]

@artem-nefedov Well! That is rather messed up. And it doesn't happen with git's own diff-highlight, I see.

Ah! Because the git DiffHighlight.pm (on which diff-so-fancy's is originally based, though they've since diverged) received git/git@4551fbb around 15 months ago. The commit message:

commit 4551fbba141e0b2e4d16830f76f784e9c960bdf8
Author: Jeff King peff@peff.net
Date: 2018-03-21 01:59:01 -0400

diff-highlight: detect --graph by indent

This patch fixes a corner case where diff-highlight may
scramble some diffs when combined with --graph.

Commit 7e4ffb4c17 (diff-highlight: add support for --graph
output, 2016-08-29) taught diff-highlight to skip past the
graph characters at the start of each line with this regex:

  ($COLOR?\|$COLOR?\s+)*

[...]

However, when we soak up all of those spaces with the
$GRAPH regex, we may accidentally include the leading space
for a context line. That means we may consider the actual
contents of a context line as part of the diff syntax. In
other words, something like this:

   normal context line
  -old line
  +new line
   -this is a context line with a leading dash

would cause us to see that final context line as a removal
line, and we'd end up showing the hunk in the wrong order:

  normal context line
  -old line
   -this is a context line with a leading dash
  +new line

...Sounds pretty familiar, no?

Cherry-picking that commit, assuming it's still compatible with diff-so-fancy's diverged DiffHighlight.pm, might solve this in one shot.

[scottchiefbaker]

If you test d-s-f with that one commit cherry picked let me know. It's definitely something I'd take a pull-request on.