Hi Stefan,
On Mon, 13 Aug 2018, Stefan Beller wrote:
> This applies on top of js/range-diff (a7be92acd9600), this version
> * resolves a semantic conflict in the test
> (Did range-diff change its output slightly?)
If by semantic conflict you mean...
> 23: 6a1c7698c68 ! 23: 5701745379b t3206: add color test for range-diff --dual-color
> @@ -23,7 +23,7 @@
> + : s/4/A/<RESET>
> + : <RESET>
> + : <REVERSE><GREEN>+<RESET><BOLD> Also a silly comment here!<RESET>
> -+ : <REVERSE><GREEN>+<RESET>
> ++ : <REVERSE><GREEN>+<RESET><BOLD><RESET>
... this, then I do not understand. This looks like a straight-up change
in your commit. v1 of this patch did not append <BOLD><RESET> to the end,
while v2 apparently does.
And it looks a bit funny, indeed.
In any case, from what I see you indeed addressed all my comments (the
need_reset was done differently than I suggested, but still okay).
Ciao,
Dscho
> + : diff --git a/file b/file<RESET>
> + : <RED> --- a/file<RESET>
> + : <GREEN> +++ b/file<RESET>
> 24: 7e12ece1d34 = 24: 4e56f63a5f5 diff.c: simplify caller of emit_line_0
> 25: 74dabd6d36f ! 25: cf126556940 diff.c: reorder arguments for emit_line_ws_markup
> @@ -3,7 +3,8 @@
> diff.c: reorder arguments for emit_line_ws_markup
>
> The order shall be all colors first, then the content, flags at the end.
> - The colors are in order.
> + The colors are in the order of occurrence, i.e. first the color for the
> + sign and then the color for the rest of the line.
>
> Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx>
> Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
> 26: e304e15aa6b ! 26: 69008364cb8 diff.c: add set_sign to emit_line_0
> @@ -2,14 +2,17 @@
>
> diff.c: add set_sign to emit_line_0
>
> - For now just change the signature, we'll reason about the actual
> - change in a follow up patch.
> + Split the meaning of the `set` parameter that is passed to
> + emit_line_0()` to separate between the color of the "sign" (i.e.
> + the diff marker '+', '-' or ' ' that is passed in as the `first`
> + parameter) and the color of the rest of the line.
>
> - Pass 'set_sign' (which is output before the sign) and 'set' which
> - controls the color after the first character. Hence, promote any
> - 'set's to 'set_sign' as we want to have color before the sign
> - for now.
> + This changes the meaning of the `set` parameter to no longer refer
> + to the color of the diff marker, but instead to refer to the color
> + of the rest of the line. A value of `NULL` indicates that the rest
> + of the line wants to be colored the same as the diff marker.
>
> + Helped-by: Johannes Schindelin <Johannes.Schindelin@xxxxxx>
> Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx>
> Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
>
> @@ -30,12 +33,15 @@
> if (reverse && want_color(o->use_color))
> fputs(GIT_COLOR_REVERSE, file);
> - fputs(set, file);
> -+ if (set_sign && set_sign[0])
> ++ if (set_sign)
> + fputs(set_sign, file);
> if (first && !nofirst)
> fputc(first, file);
> -+ if (set)
> ++ if (set && set != set_sign) {
> ++ if (set_sign)
> ++ fputs(reset, file);
> + fputs(set, file);
> ++ }
> fwrite(line, len, 1, file);
> fputs(reset, file);
> }
> 27: 8d935d5212c < -: ----------- diff: use emit_line_0 once per line
> 28: 2344aac787a < -: ----------- diff.c: compute reverse locally in emit_line_0
> -: ----------- > 27: 5d2629281a1 diff: use emit_line_0 once per line
> -: ----------- > 28: 5240e94a69a diff.c: omit check for line prefix in emit_line_0
> 29: 4dc97b54a35 ! 29: 058e03a1601 diff.c: rewrite emit_line_0 more understandably
> @@ -2,21 +2,15 @@
>
> diff.c: rewrite emit_line_0 more understandably
>
> - emit_line_0 has no nested conditions, but puts out all its arguments
> - (if set) in order. There is still a slight confusion with set
> - and set_sign, but let's defer that to a later patch.
> + Rewrite emit_line_0 to have fewer (nested) conditions.
>
> - 'first' used be output always no matter if it was 0, but that got lost
> - at "diff: add an internal option to dual-color diffs of diffs",
> - 2018-07-21), as there we broadened the meaning of 'first' to also
> - signal an early return.
> -
> - The change in 'emit_line' makes sure that 'first' is never content, but
> - always under our control, a sign or special character in the beginning
> - of the line (or 0, in which case we ignore it).
> + The change in 'emit_line' makes sure that 'first' is never user data,
> + but always under our control, a sign or special character in the
> + beginning of the line (or 0, in which case we ignore it).
> + So from now on, let's pass only a diff marker or 0 as the 'first'
> + character of the line.
>
> Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx>
> - Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
>
> diff --git a/diff.c b/diff.c
> --- a/diff.c
> @@ -26,9 +20,7 @@
> {
> int has_trailing_newline, has_trailing_carriage_return;
> - int nofirst;
> - int reverse = !!set && !!set_sign;
> -+ int needs_reset = 0;
> -+
> ++ int needs_reset = 0; /* at the end of the line */
> FILE *file = o->file;
>
> fputs(diff_line_prefix(o), file);
> @@ -51,15 +43,16 @@
> - if (len || !nofirst) {
> - if (reverse && want_color(o->use_color))
> - fputs(GIT_COLOR_REVERSE, file);
> -- if (set_sign || set)
> -- fputs(set_sign ? set_sign : set, file);
> +- if (set_sign)
> +- fputs(set_sign, file);
> - if (first && !nofirst)
> - fputc(first, file);
> - if (len) {
> -- if (set_sign && set && set != set_sign)
> -- fputs(reset, file);
> -- if (set)
> +- if (set && set != set_sign) {
> +- if (set_sign)
> +- fputs(reset, file);
> - fputs(set, file);
> +- }
> - fwrite(line, len, 1, file);
> - }
> - fputs(reset, file);
> @@ -79,8 +72,8 @@
> + needs_reset = 1;
> + }
> +
> -+ if (set_sign || set) {
> -+ fputs(set_sign ? set_sign : set, file);
> ++ if (set_sign) {
> ++ fputs(set_sign, file);
> + needs_reset = 1;
> }
> +
> @@ -97,7 +90,7 @@
> + needs_reset = 1;
> + }
> + fwrite(line, len, 1, file);
> -+ needs_reset |= len > 0;
> ++ needs_reset = 1; /* 'line' may contain color codes. */
> +
> +end_of_line:
> + if (needs_reset)
> @@ -109,8 +102,8 @@
> static void emit_line(struct diff_options *o, const char *set, const char *reset,
> const char *line, int len)
> {
> -- emit_line_0(o, set, NULL, reset, line[0], line+1, len-1);
> -+ emit_line_0(o, set, NULL, reset, 0, line, len);
> +- emit_line_0(o, set, NULL, 0, reset, line[0], line+1, len-1);
> ++ emit_line_0(o, set, NULL, 0, reset, 0, line, len);
> }
>
> enum diff_symbol {
>
