git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCHv2 0/8] Resending sb/range-diff-colors

From
Johannes Schindelin <johannes.schindelin@gmx.de>
Date
Aug 14, 2018, 15:05 UTC
Message-ID
<nycvar.QRO.7.76.6.1808141702570.71@tvgsbejvaqbjf.bet>
In-Reply-To
<20180814014122.30662-1-sbeller@google.com>
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...
Show 7 quoted lines
> 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

Show 156 quoted lines
>      +	:     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@google.com>
>          Signed-off-by: Junio C Hamano <gitster@pobox.com>
> 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@gmx.de>
>          Signed-off-by: Stefan Beller <sbeller@google.com>
>          Signed-off-by: Junio C Hamano <gitster@pobox.com>
>      
>     @@ -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@google.com>
>     -    Signed-off-by: Junio C Hamano <gitster@pobox.com>
>      
>      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 {
>
Previous: Stefan BellerNext: Stefan Beller
Message 31 of 32 in “Resending sb/range-diff-colors”
  1. 0/8 Resending sb/range-diff-colorsStefan Beller, Aug 10, 2018
  2. 1/8 test_decode_color: understand FAINT and ITALICStefan Beller, Aug 10, 2018
  3. 2/8 t3206: add color test for range-diff --dual-colorStefan Beller, Aug 10, 2018
  4. Johannes SchindelinAug 13, 2018
  5. Stefan BellerAug 13, 2018
  6. 3/8 diff.c: simplify caller of emit_line_0Stefan Beller, Aug 10, 2018
  7. Johannes SchindelinAug 13, 2018
  8. 4/8 diff.c: reorder arguments for emit_line_ws_markupStefan Beller, Aug 10, 2018
  9. Johannes SchindelinAug 13, 2018
  10. 5/8 diff.c: add set_sign to emit_line_0Stefan Beller, Aug 10, 2018
  11. Johannes SchindelinAug 13, 2018
  12. Stefan BellerAug 13, 2018
  13. 6/8 diff: use emit_line_0 once per lineStefan Beller, Aug 10, 2018
  14. Johannes SchindelinAug 13, 2018
  15. 7/8 diff.c: compute reverse locally in emit_line_0Stefan Beller, Aug 10, 2018
  16. Johannes SchindelinAug 13, 2018
  17. Stefan BellerAug 13, 2018
  18. 8/8 diff.c: rewrite emit_line_0 more understandablyStefan Beller, Aug 10, 2018
  19. Johannes SchindelinAug 13, 2018
  20. Stefan BellerAug 13, 2018
  21. Stefan BellerAug 13, 2018
  22. 0/8 Resending sb/range-diff-colorsStefan Beller, Aug 14, 2018
  23. 1/8 test_decode_color: understand FAINT and ITALICStefan Beller, Aug 14, 2018
  24. 2/8 t3206: add color test for range-diff --dual-colorStefan Beller, Aug 14, 2018
  25. 3/8 diff.c: simplify caller of emit_line_0Stefan Beller, Aug 14, 2018
  26. 4/8 diff.c: reorder arguments for emit_line_ws_markupStefan Beller, Aug 14, 2018
  27. 5/8 diff.c: add set_sign to emit_line_0Stefan Beller, Aug 14, 2018
  28. 6/8 diff: use emit_line_0 once per lineStefan Beller, Aug 14, 2018
  29. 7/8 diff.c: omit check for line prefix in emit_line_0Stefan Beller, Aug 14, 2018
  30. 8/8 diff.c: rewrite emit_line_0 more understandablyStefan Beller, Aug 14, 2018
  31. Johannes SchindelinAug 14, 2018
  32. Stefan BellerAug 14, 2018

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.