> (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.
> > * 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.
>
> 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);
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@xxxxxxxxxx/
i.e. we'd markup the output instead of coloring it.
