Difficulty with parsing colorized diff output

Hello, I have a rather elaborate diff highlighter that I have implemented as a post-processor to regular git output. I am writing to discuss some difficult aspects of git diff's color output that I am observing with version 2.19.2. This is not a regression report; I am trying to implement a new feature and am stymied by these details.

My goal is to detect SGR color sequences, e.g. '\x1b[32m', that exist in the source text, and have my highlighter print escaped representations of those. For example, I have checked in files that are expected test outputs for tools that emit color codes, and diffs of those get very confusing.

Figuring out which color codes are from the source text and which were added by git is proving very difficult. The obvious solution is to turn git diff coloring off, but as far as I can see this also turns off all coloring for logs, which is undesirable.

Then I tried to remove just the color codes that git adds to the diff. This almost works, but there are some irregularities. Most lines begin with a style/color code and end with a reset code, which would be a perfect indicator that git is using colors. However:

* Context lines do not begin with reset code, but do end with a reset code. It would be preferable in my opinion if they had both (like every other line), or none at all.

* Added lines have excess codes after the plus sign. The entire prefix is, `\x1b[32m+\x1b[m\x1b[32m` translating to GREEN PLUS RESET GREEN. Emitting codes after the plus sign makes the parsing more complex and idiosyncratic.

In summary, I would like to suggest the following improvements:

* Remove the excess codes after the plus sign.

* When git diff is adding colors, ensure that every line begins with an SGR code and ends with the RESET code.

* Add a config feature to turn on log coloring while leaving diff coloring off.

I would be willing to attempt a fix for this myself, but I'd like to hear what the maintainers think first, and would appreciate any hints as to where I should start looking in the code base.

If anyone is curious about the implementation it is called `same-same` and lives here: https://github.com/gwk/pithy/blob/master/pithy/bin/same_same.py

I configure it like this in .gitconfig:

[core]
  pager = same-same | LESSANSIENDCHARS=mK less --RAW-CONTROL-CHARS
[interactive]
  diffFilter = same-same -interactive | LESSANSIENDCHARS=mK less --RAW-CONTROL-CHARS

Thank you, George

On Fri, Dec 07, 2018 at 07:09:58PM -0500, George King wrote:

Another gotcha that I don't think you've hit yet: whitespace highlighting can color arbitrary parts of the line.

Try this, for example:

  printf 'foo\n' >old
  printf '\t \tfoo\n' >new
  git diff --no-index old new

So I'm not sure what you want to do can ever be completely robust. That said, I'm not opposed to making Git's color output more regular. It seems like a reasonable cleanup on its own.

(For the Git project itself, we long ago realized that putting raw color codes into the source is a big pain when working with diffs, and we instead use tools like t/test-lib-functions.sh's test_decode_color).

Context lines do have both. It's just that the default color for context lines is empty. ;)

But yes, I think it might be reasonable to recognize when an opening color is empty, and turn the closing reset into a noop in that case (or I guess you are also advocating the opposite: turn an empty color into a noop \x1b[m or similar).

I think most of the coloring, including context lines, is coming from diff.c:emit_diff_symbol_from_struct(). Instead of unconditionally calling:

  context = diff_get_color_opt(o, DIFF_CONTEXT);
  reset = diff_get_color_opt(o, DIFF_RESET);

I suspect we could have a helper like this:

  static const char *diff_get_reset(const char *color)
  {
	if (!color || !*color)
		return "";
	return diff_colors[DIFF_RESET];
  }
  ...
  context = diff_get_color_opt(o, DIFF_CONTEXT);
  reset = diff_get_reset(context);

Yeah, I've run into this one, too (when working on diff-highlight, which similarly tries to pass-through Git's existing colors).

It's unfortunately not quite trivial, due to some interactions with whitespace coloring. See this old patch:

  https://public-inbox.org/git/20101109220136.GA5617@sigill.intra.peff.net/

and the followup:

  https://public-inbox.org/git/20101118064050.GA12825@sigill.intra.peff.net/

Yes, the fact that --pretty log coloring is tied to diffs is mostly a historical artifact. I think it would be OK to introduce a color.log config option that defaults to the value of color.diff if unset. That would be backwards compatible, but allow you to set them independently if you wanted to.

-Peff

And also we hid the colors behind #defines and such.

The content itself can contain color codes.

Instead of unconditionally resetting each line, we could parse each content line to determine if we actually have to reset the colors.

Another easier way to do so would be to drop the line

    needs_reset = 1; /* 'line' may contain color codes. */

in diff.c::emit_line_0 I run the test suite and it passes (I thought we had a test enforcing we'd reset any user provided coloring).

Then we have broken code in diff.c::emit_line_ws_markup
in the last case ("else {") which first emits the sign via
emit_line_0 and then the rest via ws_check_emit.
It sounds like we'd want to replace `reset` in the call
of emit_line_0 with
  set == set_sign ? NULL : reset
and set in the call to ws_check_emit with
  set == set_sign ? NULL : set

Manually looking at some diff output  of said diff we'd get
  <RED>- emit_line_0(o, set_sign ? set_sign : set, NULL, !!set_sign,
reset,<RESET>
  <GREEN>+<RESET> <GREEN>emit_line_0(o, set_sign ? set_sign : set,
NULL, !!set_sign, set == set_sign ? NULL : reset,<RESET>

and the issue is that we do not color the beginning white space of the emission from ws_check_emit but maybe we should.

Another idea would be to allow Git to output its output as if it was run through test_decode_color, slightly related: https://public-inbox.org/git/20180804015317.182683-8-sbeller@google.com/ i.e. we'd markup the output instead of coloring it.

On Mon, Dec 10, 2018 at 07:26:46PM -0800, Stefan Beller wrote:

Good point. I don't recall that being the motivation back when this behavior started, but it's a nice side effect (and the more recent line you mentioned in emit_line_0 certainly is doing it intentionally).

That doesn't cover _other_ terminal codes, which could also make for confusing output, but I do think color codes are somewhat special. We generally send patches through "less -R", which will pass through the colors but show escaped versions of other codes.

Yeah, I think in the most general form, the problem is that colorizing (including whitespace highlighting) loses information within a single line. It would be nice to have a machine-readable format that represents all the various annotations (like whitespace and coloring moved bits) that Git computes.

-Peff

Peff & Stefan, thank you for the feedback. For my purposes, I am content to rely on gitconfig to reduce the colors to something that I can parse without losing information. Since my first email I have found that `wsErrorHighlight = none` gets rid of the problematic extra green highlights in the `+` lines.

I still think that a config to differentiate log coloring from diff coloring would be worthwhile, as it would guarantee that highlighters are getting unadulterated lines from git.

I also think that bracketing every colored line with a color code before the space/plus/minus and a reset just before the newline would be smart, because then a parser can just parse line by line: if there is an SGR sequence before the space/plus/minus, then it would know to strip off the final reset. This is in contrast to how it stands now, where a context line (with no leading color) is ambiguous by itself; I have to remember from previous lines in the hunk that we have been seeing colors in order to know wether I should strip off the reset.

I agree that a machine-readable format would be nice, but regardless it would be useful to make the regular output more parser-friendly.

On Tue, Dec 11 2018, Jeff King wrote:

I wonder if optimizing this one way or the other matters for some terminals. I.e. if we print out some huge diff of thousands of consecutive "green" added lines is it faster/slower on some of them to do one "begin green" and "reset" at the end, or is one line at a time better, or doesn't it matter at all?

I first started playing around with terminal colors about 5 years ago, and I recall learning the hard way that Apple Terminal at least behaves very strangely when you have background colors cross line boundaries: background colors disappeared when I scrolled lines back into view. I filed a bug thinking it couldn't be right and Apple closed it as behaving according to compatibility expectations. I never figured out whether they had misunderstood my report or if old terminals were just that crazy. Instead I decided that the safe thing to do was reset after every line. Perhaps some git author reached the same conclusion.

From the perspective of parsing this output, it is really much easier if each line can be understood without considering state of previous lines. If anything, I think it is a safe approach to ensuring that it renders correctly on various terminals as well.

I just noticed that while `wsErrorHighlight = none` fixes the problem of extra green codes for regular diff, it fails to have any effect during interactive `git add -p`.

On Tue, Dec 11, 2018 at 01:55:09PM -0500, George King wrote:

This is due to the way add--interactive invokes diff. It uses the plumbing commands (diff-tree, diff-files, etc), which do not respect config which changes the output (since their purpose is to provide stable machine-readable output).

But add--interactive does a slightly funny thing: it runs each diff twice. Once to get the machine-readable version, and once to get a colorized version which it shows to the user. When it does the latter, it has to manually enable other options (e.g., passing --color as appropriate, or passing along diff.algorithm).

So the matching behavior here would be for it to look as wsErrorHighlight and pass it along as a command line option.

-Peff

On Tue, Dec 11, 2018 at 11:41:18AM -0500, George King wrote:

Yes, that's exactly it. Certain codes do not do well as they cross lines. It's been long enough that I don't remember the details, and a quick grep of the archive found too many results for me to bother wading through.

-Peff