Re: Difficulty with parsing colorized diff output

From: Stefan Beller <sbeller@google.com>
Date: Dec 11, 2018, 03:26 UTC
Message-ID: <CAGZ79kbd=2_eHdbVYwmNoAYupwnP3YDn6nT0m=v1CL0AkWXk=Q@mail.gmail.com>
In-Reply-To: <20181208071634.GA18272@sigill.intra.peff.net>

> (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).

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

Show 6 quoted lines

> > * 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.
>
> Context lines do have both. It's just that the default color for context
> lines is empty. ;)

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.

Show 5 quoted lines

>
> 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).

Show 19 quoted lines

>
> 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);

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).

> > * 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.

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.