{"thread":{"id":"49095","subject":"[PATCH 0/8] Resending sb/range-diff-colors","startedAt":"2018-08-10T22:34:47Z","lastAt":"2018-08-14T16:45:56Z","messageCount":32,"participants":["Stefan Beller","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"355194","messageId":"20180810223441.30428-1-sbeller@google.com","threadId":"49095","inReplyTo":null,"subject":"[PATCH 0/8] Resending sb/range-diff-colors","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-10T22:34:33Z","receivedAt":"2018-08-10T22:34:47Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This is also avaliable as\ngit fetch https://github.com/stefanbeller/git sb/range-diff-colors\n\nand is a resend of sb/range-diff-colors.\n\nIt is rebased on the version of range-diff that Johannes just sent out\n(pr-1/dscho/branch-diff-v5 in GGG repo), and squashes the fisup commit\n(which had to be adapted slightly in one line, too)\n\nrange-diff below.\n\nStefan Beller (8):\n test_decode_color: understand FAINT and ITALIC\n t3206: add color test for range-diff --dual-color\n diff.c: simplify caller of emit_line_0\n diff.c: reorder arguments for emit_line_ws_markup\n diff.c: add set_sign to emit_line_0\n diff: use emit_line_0 once per line\n diff.c: compute reverse locally in emit_line_0\n diff.c: rewrite emit_line_0 more understandably\n\n diff.c | 94 +++++++++++++++++++++++------------------\n t/t3206-range-diff.sh | 39 +++++++++++++++++\n t/test-lib-functions.sh | 2 +\n 3 files changed, 93 insertions(+), 42 deletions(-)\n \n./git-range-diff \\\n 5bf616af71afe1c4c36da7f21077662febf28cbe..c1b144ea695514cfe185fe70089198621c38d01c \\\n ccf8c1bb2459d33c7dc97098c08c47ca7d77ed3e..4dc97b54a35c60c25ab7634441d60711ead0e84e \\\n >>0000-cover-letter.patch\n\n 1: 7f88339e03e = 1: 0fedd4c0a20 test_decode_color: understand FAINT and ITALIC\n 2: 13e8528be69 < -: ----------- t3206: add color test for range-diff --dual-color\n -: ----------- > 2: 6a1c7698c68 t3206: add color test for range-diff --dual-color\n 3: 2f80811b319 = 3: 7e12ece1d34 diff.c: simplify caller of emit_line_0\n 4: 15af0d378c8 ! 4: 74dabd6d36f diff.c: reorder arguments for emit_line_ws_markup\n @@ -35,7 +35,7 @@\n \tcase DIFF_SYMBOL_PLUS:\n @@\n \t\t\t\tset = diff_get_color_opt(o, DIFF_CONTEXT_BOLD);\n - \t\t\tflags |= WS_IGNORE_FIRST_SPACE;\n + \t\t\tflags &= ~DIFF_SYMBOL_CONTENT_WS_MASK;\n \t\t}\n -\t\temit_line_ws_markup(o, set, reset, line, len, set_sign, '+',\n +\t\temit_line_ws_markup(o, set_sign, set, reset, '+', line, len,\n 5: dce49bb58fd = 5: e304e15aa6b diff.c: add set_sign to emit_line_0\n 6: 7581e16d63f = 6: 8d935d5212c diff: use emit_line_0 once per line\n 7: d070d393f73 = 7: 2344aac787a diff.c: compute reverse locally in emit_line_0\n 8: 669e7516e03 = 8: 4dc97b54a35 diff.c: rewrite emit_line_0 more understandably\n 9: c1b144ea695 < -: ----------- fixup! t3206: add color test for range-diff --dual-color\n"},{"id":"355195","messageId":"20180810223441.30428-2-sbeller@google.com","threadId":"49095","inReplyTo":"20180810223441.30428-1-sbeller@google.com","subject":"[PATCH 1/8] test_decode_color: understand FAINT and ITALIC","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-10T22:34:34Z","receivedAt":"2018-08-10T22:34:50Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n t/test-lib-functions.sh | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 2b2181dca09..be8244c47b5 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -42,6 +42,8 @@ test_decode_color () {\n \t\tfunction name(n) {\n \t\t\tif (n == 0) return \"RESET\";\n \t\t\tif (n == 1) return \"BOLD\";\n+\t\t\tif (n == 2) return \"FAINT\";\n+\t\t\tif (n == 3) return \"ITALIC\";\n \t\t\tif (n == 7) return \"REVERSE\";\n \t\t\tif (n == 30) return \"BLACK\";\n \t\t\tif (n == 31) return \"RED\";\n-- \n2.18.0.865.gffc8e1a3cd6-goog\n\n"},{"id":"355196","messageId":"20180810223441.30428-3-sbeller@google.com","threadId":"49095","inReplyTo":"20180810223441.30428-1-sbeller@google.com","subject":"[PATCH 2/8] t3206: add color test for range-diff --dual-color","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-10T22:34:35Z","receivedAt":"2018-08-10T22:34:57Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The 'expect'ed outcome has been taken by running the 'range-diff | decode'.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n t/t3206-range-diff.sh | 39 +++++++++++++++++++++++++++++++++++++++\n 1 file changed, 39 insertions(+)\n\ndiff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\nindex 2237c7f4af9..7dc7c80a1de 100755\n--- a/t/t3206-range-diff.sh\n+++ b/t/t3206-range-diff.sh\n@@ -142,4 +142,43 @@ test_expect_success 'changed message' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'dual-coloring' '\n+\tsed -e \"s|^:||\" >expect <<-\\EOF &&\n+\t:1: a4b3333 = 1: f686024 s/5/A/\n+\t:2: f51d370 ! 2: 4ab067d s/4/A/\n+\t: @@ -2,6 +2,8 @@\n+\t: \n+\t: s/4/A/\n+\t: \n+\t: + Also a silly comment here!\n+\t: +\n+\t: diff --git a/file b/file\n+\t: --- a/file\n+\t: +++ b/file\n+\t:3: 0559556 ! 3: b9cb956 s/11/B/\n+\t: @@ -10,7 +10,7 @@\n+\t: 9\n+\t: 10\n+\t: -11\n+\t: -+BB\n+\t: ++B\n+\t: 12\n+\t: 13\n+\t: 14\n+\t:4: d966c5c ! 4: 8add5f1 s/12/B/\n+\t: @@ -8,7 +8,7 @@\n+\t: @@\n+\t: 9\n+\t: 10\n+\t: - BB\n+\t: + B\n+\t: -12\n+\t: +B\n+\t: 13\n+\tEOF\n+\tgit range-diff changed...changed-message --color --dual-color >actual.raw &&\n+\ttest_decode_color >actual \nSigned-off-by: Junio C Hamano \n---\n diff.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex ae131495216..f6df18af913 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1202,8 +1202,7 @@ static void emit_line_ws_markup(struct diff_options *o,\n \t\temit_line_0(o, set, 0, reset, sign, line, len);\n \telse if (!ws) {\n \t\t/* Emit just the prefix, then the rest. */\n-\t\temit_line_0(o, set_sign ? set_sign : set, !!set_sign, reset,\n-\t\t\t sign, \"\", 0);\n+\t\temit_line_0(o, set_sign, !!set_sign, reset, sign, \"\", 0);\n \t\temit_line_0(o, set, 0, reset, 0, line, len);\n \t} else if (blank_at_eof)\n \t\t/* Blank line at EOF - paint '+' as well */\n-- \n2.18.0.865.gffc8e1a3cd6-goog\n\n"},{"id":"355198","messageId":"20180810223441.30428-5-sbeller@google.com","threadId":"49095","inReplyTo":"20180810223441.30428-1-sbeller@google.com","subject":"[PATCH 4/8] diff.c: reorder arguments for emit_line_ws_markup","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-10T22:34:37Z","receivedAt":"2018-08-10T22:34:57Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The order shall be all colors first, then the content, flags at the end.\nThe colors are in order.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n diff.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex f6df18af913..ab6e6a88a56 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1185,9 +1185,9 @@ static void dim_moved_lines(struct diff_options *o)\n }\n \n static void emit_line_ws_markup(struct diff_options *o,\n-\t\t\t\tconst char *set, const char *reset,\n-\t\t\t\tconst char *line, int len,\n-\t\t\t\tconst char *set_sign, char sign,\n+\t\t\t\tconst char *set_sign, const char *set,\n+\t\t\t\tconst char *reset,\n+\t\t\t\tchar sign, const char *line, int len,\n \t\t\t\tunsigned ws_rule, int blank_at_eof)\n {\n \tconst char *ws = NULL;\n@@ -1271,7 +1271,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,\n \t\t\telse if (c == '-')\n \t\t\t\tset = diff_get_color_opt(o, DIFF_FILE_OLD);\n \t\t}\n-\t\temit_line_ws_markup(o, set, reset, line, len, set_sign, ' ',\n+\t\temit_line_ws_markup(o, set_sign, set, reset, ' ', line, len,\n \t\t\t\t flags & (DIFF_SYMBOL_CONTENT_WS_MASK), 0);\n \t\tbreak;\n \tcase DIFF_SYMBOL_PLUS:\n@@ -1314,7 +1314,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,\n \t\t\t\tset = diff_get_color_opt(o, DIFF_CONTEXT_BOLD);\n \t\t\tflags &= ~DIFF_SYMBOL_CONTENT_WS_MASK;\n \t\t}\n-\t\temit_line_ws_markup(o, set, reset, line, len, set_sign, '+',\n+\t\temit_line_ws_markup(o, set_sign, set, reset, '+', line, len,\n \t\t\t\t flags & DIFF_SYMBOL_CONTENT_WS_MASK,\n \t\t\t\t flags & DIFF_SYMBOL_CONTENT_BLANK_LINE_EOF);\n \t\tbreak;\n@@ -1357,7 +1357,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,\n \t\t\telse\n \t\t\t\tset = diff_get_color_opt(o, DIFF_CONTEXT_DIM);\n \t\t}\n-\t\temit_line_ws_markup(o, set, reset, line, len, set_sign, '-',\n+\t\temit_line_ws_markup(o, set_sign, set, reset, '-', line, len,\n \t\t\t\t flags & DIFF_SYMBOL_CONTENT_WS_MASK, 0);\n \t\tbreak;\n \tcase DIFF_SYMBOL_WORDS_PORCELAIN:\n-- \n2.18.0.865.gffc8e1a3cd6-goog\n\n"},{"id":"355199","messageId":"20180810223441.30428-6-sbeller@google.com","threadId":"49095","inReplyTo":"20180810223441.30428-1-sbeller@google.com","subject":"[PATCH 5/8] diff.c: add set_sign to emit_line_0","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-10T22:34:38Z","receivedAt":"2018-08-10T22:34:59Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"For now just change the signature, we'll reason about the actual\nchange in a follow up patch.\n\nPass 'set_sign' (which is output before the sign) and 'set' which\ncontrols the color after the first character. Hence, promote any\n'set's to 'set_sign' as we want to have color before the sign\nfor now.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n diff.c | 21 ++++++++++++---------\n 1 file changed, 12 insertions(+), 9 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex ab6e6a88a56..5eea5edca50 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -622,7 +622,7 @@ static void check_blank_at_eof(mmfile_t *mf1, mmfile_t *mf2,\n }\n \n static void emit_line_0(struct diff_options *o,\n-\t\t\tconst char *set, unsigned reverse, const char *reset,\n+\t\t\tconst char *set_sign, const char *set, unsigned reverse, const char *reset,\n \t\t\tint first, const char *line, int len)\n {\n \tint has_trailing_newline, has_trailing_carriage_return;\n@@ -652,9 +652,12 @@ static void emit_line_0(struct diff_options *o,\n \tif (len || !nofirst) {\n \t\tif (reverse && want_color(o->use_color))\n \t\t\tfputs(GIT_COLOR_REVERSE, file);\n-\t\tfputs(set, file);\n+\t\tif (set_sign && set_sign[0])\n+\t\t\tfputs(set_sign, file);\n \t\tif (first && !nofirst)\n \t\t\tfputc(first, file);\n+\t\tif (set)\n+\t\t\tfputs(set, file);\n \t\tfwrite(line, len, 1, file);\n \t\tfputs(reset, file);\n \t}\n@@ -667,7 +670,7 @@ static void emit_line_0(struct diff_options *o,\n static void emit_line(struct diff_options *o, const char *set, const char *reset,\n \t\t const char *line, int len)\n {\n-\temit_line_0(o, set, 0, reset, line[0], line+1, len-1);\n+\temit_line_0(o, set, NULL, 0, reset, line[0], line+1, len-1);\n }\n \n enum diff_symbol {\n@@ -1199,17 +1202,17 @@ static void emit_line_ws_markup(struct diff_options *o,\n \t}\n \n \tif (!ws && !set_sign)\n-\t\temit_line_0(o, set, 0, reset, sign, line, len);\n+\t\temit_line_0(o, set, NULL, 0, reset, sign, line, len);\n \telse if (!ws) {\n \t\t/* Emit just the prefix, then the rest. */\n-\t\temit_line_0(o, set_sign, !!set_sign, reset, sign, \"\", 0);\n-\t\temit_line_0(o, set, 0, reset, 0, line, len);\n+\t\temit_line_0(o, set_sign, NULL, !!set_sign, reset, sign, \"\", 0);\n+\t\temit_line_0(o, set, NULL, 0, reset, 0, line, len);\n \t} else if (blank_at_eof)\n \t\t/* Blank line at EOF - paint '+' as well */\n-\t\temit_line_0(o, ws, 0, reset, sign, line, len);\n+\t\temit_line_0(o, ws, NULL, 0, reset, sign, line, len);\n \telse {\n \t\t/* Emit just the prefix, then the rest. */\n-\t\temit_line_0(o, set_sign ? set_sign : set, !!set_sign, reset,\n+\t\temit_line_0(o, set_sign ? set_sign : set, NULL, !!set_sign, reset,\n \t\t\t sign, \"\", 0);\n \t\tws_check_emit(line, len, ws_rule,\n \t\t\t o->file, set, reset, ws);\n@@ -1233,7 +1236,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,\n \t\tcontext = diff_get_color_opt(o, DIFF_CONTEXT);\n \t\treset = diff_get_color_opt(o, DIFF_RESET);\n \t\tputc('\\n', o->file);\n-\t\temit_line_0(o, context, 0, reset, '\\\\',\n+\t\temit_line_0(o, context, NULL, 0, reset, '\\\\',\n \t\t\t nneof, strlen(nneof));\n \t\tbreak;\n \tcase DIFF_SYMBOL_SUBMODULE_HEADER:\n-- \n2.18.0.865.gffc8e1a3cd6-goog\n\n"},{"id":"355200","messageId":"20180810223441.30428-7-sbeller@google.com","threadId":"49095","inReplyTo":"20180810223441.30428-1-sbeller@google.com","subject":"[PATCH 6/8] diff: use emit_line_0 once per line","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-10T22:34:39Z","receivedAt":"2018-08-10T22:35:01Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"All lines that use emit_line_0 multiple times per line, are combined\ninto a single call to emit_line_0, making use of the 'set' argument.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n diff.c | 26 ++++++++++++--------------\n 1 file changed, 12 insertions(+), 14 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 5eea5edca50..01095a59d3d 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -629,10 +629,7 @@ static void emit_line_0(struct diff_options *o,\n \tint nofirst;\n \tFILE *file = o->file;\n \n-\tif (first)\n-\t\tfputs(diff_line_prefix(o), file);\n-\telse if (!len)\n-\t\treturn;\n+\tfputs(diff_line_prefix(o), file);\n \n \tif (len == 0) {\n \t\thas_trailing_newline = (first == '\\n');\n@@ -652,13 +649,17 @@ static void emit_line_0(struct diff_options *o,\n \tif (len || !nofirst) {\n \t\tif (reverse && want_color(o->use_color))\n \t\t\tfputs(GIT_COLOR_REVERSE, file);\n-\t\tif (set_sign && set_sign[0])\n-\t\t\tfputs(set_sign, file);\n+\t\tif (set_sign || set)\n+\t\t\tfputs(set_sign ? set_sign : set, file);\n \t\tif (first && !nofirst)\n \t\t\tfputc(first, file);\n-\t\tif (set)\n-\t\t\tfputs(set, file);\n-\t\tfwrite(line, len, 1, file);\n+\t\tif (len) {\n+\t\t\tif (set_sign && set && set != set_sign)\n+\t\t\t\tfputs(reset, file);\n+\t\t\tif (set)\n+\t\t\t\tfputs(set, file);\n+\t\t\tfwrite(line, len, 1, file);\n+\t\t}\n \t\tfputs(reset, file);\n \t}\n \tif (has_trailing_carriage_return)\n@@ -1204,16 +1205,13 @@ static void emit_line_ws_markup(struct diff_options *o,\n \tif (!ws && !set_sign)\n \t\temit_line_0(o, set, NULL, 0, reset, sign, line, len);\n \telse if (!ws) {\n-\t\t/* Emit just the prefix, then the rest. */\n-\t\temit_line_0(o, set_sign, NULL, !!set_sign, reset, sign, \"\", 0);\n-\t\temit_line_0(o, set, NULL, 0, reset, 0, line, len);\n+\t\temit_line_0(o, set_sign, set, !!set_sign, reset, sign, line, len);\n \t} else if (blank_at_eof)\n \t\t/* Blank line at EOF - paint '+' as well */\n \t\temit_line_0(o, ws, NULL, 0, reset, sign, line, len);\n \telse {\n \t\t/* Emit just the prefix, then the rest. */\n-\t\temit_line_0(o, set_sign ? set_sign : set, NULL, !!set_sign, reset,\n-\t\t\t sign, \"\", 0);\n+\t\temit_line_0(o, set_sign, set, !!set_sign, reset, sign, \"\", 0);\n \t\tws_check_emit(line, len, ws_rule,\n \t\t\t o->file, set, reset, ws);\n \t}\n-- \n2.18.0.865.gffc8e1a3cd6-goog\n\n"},{"id":"355201","messageId":"20180810223441.30428-8-sbeller@google.com","threadId":"49095","inReplyTo":"20180810223441.30428-1-sbeller@google.com","subject":"[PATCH 7/8] diff.c: compute reverse locally in emit_line_0","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-10T22:34:40Z","receivedAt":"2018-08-10T22:35:05Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n diff.c | 15 ++++++++-------\n 1 file changed, 8 insertions(+), 7 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 01095a59d3d..e50cd312755 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -622,11 +622,12 @@ static void check_blank_at_eof(mmfile_t *mf1, mmfile_t *mf2,\n }\n \n static void emit_line_0(struct diff_options *o,\n-\t\t\tconst char *set_sign, const char *set, unsigned reverse, const char *reset,\n+\t\t\tconst char *set_sign, const char *set, const char *reset,\n \t\t\tint first, const char *line, int len)\n {\n \tint has_trailing_newline, has_trailing_carriage_return;\n \tint nofirst;\n+\tint reverse = !!set && !!set_sign;\n \tFILE *file = o->file;\n \n \tfputs(diff_line_prefix(o), file);\n@@ -671,7 +672,7 @@ static void emit_line_0(struct diff_options *o,\n static void emit_line(struct diff_options *o, const char *set, const char *reset,\n \t\t const char *line, int len)\n {\n-\temit_line_0(o, set, NULL, 0, reset, line[0], line+1, len-1);\n+\temit_line_0(o, set, NULL, reset, line[0], line+1, len-1);\n }\n \n enum diff_symbol {\n@@ -1203,15 +1204,15 @@ static void emit_line_ws_markup(struct diff_options *o,\n \t}\n \n \tif (!ws && !set_sign)\n-\t\temit_line_0(o, set, NULL, 0, reset, sign, line, len);\n+\t\temit_line_0(o, set, NULL, reset, sign, line, len);\n \telse if (!ws) {\n-\t\temit_line_0(o, set_sign, set, !!set_sign, reset, sign, line, len);\n+\t\temit_line_0(o, set_sign, set, reset, sign, line, len);\n \t} else if (blank_at_eof)\n \t\t/* Blank line at EOF - paint '+' as well */\n-\t\temit_line_0(o, ws, NULL, 0, reset, sign, line, len);\n+\t\temit_line_0(o, ws, NULL, reset, sign, line, len);\n \telse {\n \t\t/* Emit just the prefix, then the rest. */\n-\t\temit_line_0(o, set_sign, set, !!set_sign, reset, sign, \"\", 0);\n+\t\temit_line_0(o, set_sign, set, reset, sign, \"\", 0);\n \t\tws_check_emit(line, len, ws_rule,\n \t\t\t o->file, set, reset, ws);\n \t}\n@@ -1234,7 +1235,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,\n \t\tcontext = diff_get_color_opt(o, DIFF_CONTEXT);\n \t\treset = diff_get_color_opt(o, DIFF_RESET);\n \t\tputc('\\n', o->file);\n-\t\temit_line_0(o, context, NULL, 0, reset, '\\\\',\n+\t\temit_line_0(o, context, NULL, reset, '\\\\',\n \t\t\t nneof, strlen(nneof));\n \t\tbreak;\n \tcase DIFF_SYMBOL_SUBMODULE_HEADER:\n-- \n2.18.0.865.gffc8e1a3cd6-goog\n\n"},{"id":"355202","messageId":"20180810223441.30428-9-sbeller@google.com","threadId":"49095","inReplyTo":"20180810223441.30428-1-sbeller@google.com","subject":"[PATCH 8/8] diff.c: rewrite emit_line_0 more understandably","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-10T22:34:41Z","receivedAt":"2018-08-10T22:35:06Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"emit_line_0 has no nested conditions, but puts out all its arguments\n(if set) in order. There is still a slight confusion with set\nand set_sign, but let's defer that to a later patch.\n\n'first' used be output always no matter if it was 0, but that got lost\nat \"diff: add an internal option to dual-color diffs of diffs\",\n2018-07-21), as there we broadened the meaning of 'first' to also\nsignal an early return.\n\nThe change in 'emit_line' makes sure that 'first' is never content, but\nalways under our control, a sign or special character in the beginning\nof the line (or 0, in which case we ignore it).\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n diff.c | 73 +++++++++++++++++++++++++++++++++-------------------------\n 1 file changed, 41 insertions(+), 32 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex e50cd312755..af9316c8f91 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -626,43 +626,52 @@ static void emit_line_0(struct diff_options *o,\n \t\t\tint first, const char *line, int len)\n {\n \tint has_trailing_newline, has_trailing_carriage_return;\n-\tint nofirst;\n \tint reverse = !!set && !!set_sign;\n+\tint needs_reset = 0;\n+\n \tFILE *file = o->file;\n \n \tfputs(diff_line_prefix(o), file);\n \n-\tif (len == 0) {\n-\t\thas_trailing_newline = (first == '\\n');\n-\t\thas_trailing_carriage_return = (!has_trailing_newline &&\n-\t\t\t\t\t\t(first == '\\r'));\n-\t\tnofirst = has_trailing_newline || has_trailing_carriage_return;\n-\t} else {\n-\t\thas_trailing_newline = (len > 0 && line[len-1] == '\\n');\n-\t\tif (has_trailing_newline)\n-\t\t\tlen--;\n-\t\thas_trailing_carriage_return = (len > 0 && line[len-1] == '\\r');\n-\t\tif (has_trailing_carriage_return)\n-\t\t\tlen--;\n-\t\tnofirst = 0;\n-\t}\n-\n-\tif (len || !nofirst) {\n-\t\tif (reverse && want_color(o->use_color))\n-\t\t\tfputs(GIT_COLOR_REVERSE, file);\n-\t\tif (set_sign || set)\n-\t\t\tfputs(set_sign ? set_sign : set, file);\n-\t\tif (first && !nofirst)\n-\t\t\tfputc(first, file);\n-\t\tif (len) {\n-\t\t\tif (set_sign && set && set != set_sign)\n-\t\t\t\tfputs(reset, file);\n-\t\t\tif (set)\n-\t\t\t\tfputs(set, file);\n-\t\t\tfwrite(line, len, 1, file);\n-\t\t}\n-\t\tfputs(reset, file);\n+\thas_trailing_newline = (len > 0 && line[len-1] == '\\n');\n+\tif (has_trailing_newline)\n+\t\tlen--;\n+\n+\thas_trailing_carriage_return = (len > 0 && line[len-1] == '\\r');\n+\tif (has_trailing_carriage_return)\n+\t\tlen--;\n+\n+\tif (!len && !first)\n+\t\tgoto end_of_line;\n+\n+\tif (reverse && want_color(o->use_color)) {\n+\t\tfputs(GIT_COLOR_REVERSE, file);\n+\t\tneeds_reset = 1;\n+\t}\n+\n+\tif (set_sign || set) {\n+\t\tfputs(set_sign ? set_sign : set, file);\n+\t\tneeds_reset = 1;\n \t}\n+\n+\tif (first)\n+\t\tfputc(first, file);\n+\n+\tif (!len)\n+\t\tgoto end_of_line;\n+\n+\tif (set) {\n+\t\tif (set_sign && set != set_sign)\n+\t\t\tfputs(reset, file);\n+\t\tfputs(set, file);\n+\t\tneeds_reset = 1;\n+\t}\n+\tfwrite(line, len, 1, file);\n+\tneeds_reset |= len > 0;\n+\n+end_of_line:\n+\tif (needs_reset)\n+\t\tfputs(reset, file);\n \tif (has_trailing_carriage_return)\n \t\tfputc('\\r', file);\n \tif (has_trailing_newline)\n@@ -672,7 +681,7 @@ static void emit_line_0(struct diff_options *o,\n static void emit_line(struct diff_options *o, const char *set, const char *reset,\n \t\t const char *line, int len)\n {\n-\temit_line_0(o, set, NULL, reset, line[0], line+1, len-1);\n+\temit_line_0(o, set, NULL, reset, 0, line, len);\n }\n \n enum diff_symbol {\n-- \n2.18.0.865.gffc8e1a3cd6-goog\n\n"},{"id":"355352","messageId":"nycvar.QRO.7.76.6.1808131404370.71@tvgsbejvaqbjf.bet","threadId":"49095","inReplyTo":"20180810223441.30428-3-sbeller@google.com","subject":"Re: [PATCH 2/8] t3206: add color test for range-diff --dual-color","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-08-13T12:05:39Z","receivedAt":"2018-08-13T12:05:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Stefan,\n\nOn Fri, 10 Aug 2018, Stefan Beller wrote:\n\n> +test_expect_success 'dual-coloring' '\n> +\tsed -e \"s|^:||\" >expect <<-\\EOF &&\n> +\t:1: a4b3333 = 1: f686024 s/5/A/\n> +\t:2: f51d370 ! 2: 4ab067d s/4/A/\n> +\t: @@ -2,6 +2,8 @@\n\nThat's a neat trick to have an indented here-doc that contains\nindentation. I should use this myself.\n\nThe rest of the diff looks good, too, thanks!\nDscho\n\n> +\t: \n> +\t: s/4/A/\n> +\t: \n> +\t: + Also a silly comment here!\n> +\t: +\n> +\t: diff --git a/file b/file\n> +\t: --- a/file\n> +\t: +++ b/file\n> +\t:3: 0559556 ! 3: b9cb956 s/11/B/\n> +\t: @@ -10,7 +10,7 @@\n> +\t: 9\n> +\t: 10\n> +\t: -11\n> +\t: -+BB\n> +\t: ++B\n> +\t: 12\n> +\t: 13\n> +\t: 14\n> +\t:4: d966c5c ! 4: 8add5f1 s/12/B/\n> +\t: @@ -8,7 +8,7 @@\n> +\t: @@\n> +\t: 9\n> +\t: 10\n> +\t: - BB\n> +\t: + B\n> +\t: -12\n> +\t: +B\n> +\t: 13\n> +\tEOF\n> +\tgit range-diff changed...changed-message --color --dual-color >actual.raw &&\n> +\ttest_decode_color >actual +\ttest_cmp expect actual\n> +'\n> +\n> test_done\n> -- \n> 2.18.0.865.gffc8e1a3cd6-goog\n> \n> \n"},{"id":"355353","messageId":"nycvar.QRO.7.76.6.1808131406371.71@tvgsbejvaqbjf.bet","threadId":"49095","inReplyTo":"20180810223441.30428-4-sbeller@google.com","subject":"Re: [PATCH 3/8] diff.c: simplify caller of emit_line_0","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-08-13T12:07:17Z","receivedAt":"2018-08-13T12:07:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Stefan,\n\nOn Fri, 10 Aug 2018, Stefan Beller wrote:\n\n> Due to the previous condition we know \"set_sign != NULL\" at that point.\n\nI trust your judgement on that, also on how likely this previous condition\nis to keep guaranteeing that assumption.\n\nThank you,\nDscho\n\n> Signed-off-by: Stefan Beller \n> Signed-off-by: Junio C Hamano \n> ---\n> diff.c | 3 +--\n> 1 file changed, 1 insertion(+), 2 deletions(-)\n> \n> diff --git a/diff.c b/diff.c\n> index ae131495216..f6df18af913 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -1202,8 +1202,7 @@ static void emit_line_ws_markup(struct diff_options *o,\n> \t\temit_line_0(o, set, 0, reset, sign, line, len);\n> \telse if (!ws) {\n> \t\t/* Emit just the prefix, then the rest. */\n> -\t\temit_line_0(o, set_sign ? set_sign : set, !!set_sign, reset,\n> -\t\t\t sign, \"\", 0);\n> +\t\temit_line_0(o, set_sign, !!set_sign, reset, sign, \"\", 0);\n> \t\temit_line_0(o, set, 0, reset, 0, line, len);\n> \t} else if (blank_at_eof)\n> \t\t/* Blank line at EOF - paint '+' as well */\n> -- \n> 2.18.0.865.gffc8e1a3cd6-goog\n> \n> \n"},{"id":"355354","messageId":"nycvar.QRO.7.76.6.1808131407580.71@tvgsbejvaqbjf.bet","threadId":"49095","inReplyTo":"20180810223441.30428-5-sbeller@google.com","subject":"Re: [PATCH 4/8] diff.c: reorder arguments for emit_line_ws_markup","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-08-13T12:09:34Z","receivedAt":"2018-08-13T12:09:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Stefan,\n\nOn Fri, 10 Aug 2018, Stefan Beller wrote:\n\n> The order shall be all colors first, then the content, flags at the end.\n\nOkay.\n\n> The colors are in order.\n\nIn order of what? Of the wavelength?\n\n(I agree that the order now makes more sense, and that the diff is\ncorrect.)\n\nCiao,\nDscho\n\n> \n> Signed-off-by: Stefan Beller \n> Signed-off-by: Junio C Hamano \n> ---\n> diff.c | 12 ++++++------\n> 1 file changed, 6 insertions(+), 6 deletions(-)\n> \n> diff --git a/diff.c b/diff.c\n> index f6df18af913..ab6e6a88a56 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -1185,9 +1185,9 @@ static void dim_moved_lines(struct diff_options *o)\n> }\n> \n> static void emit_line_ws_markup(struct diff_options *o,\n> -\t\t\t\tconst char *set, const char *reset,\n> -\t\t\t\tconst char *line, int len,\n> -\t\t\t\tconst char *set_sign, char sign,\n> +\t\t\t\tconst char *set_sign, const char *set,\n> +\t\t\t\tconst char *reset,\n> +\t\t\t\tchar sign, const char *line, int len,\n> \t\t\t\tunsigned ws_rule, int blank_at_eof)\n> {\n> \tconst char *ws = NULL;\n> @@ -1271,7 +1271,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,\n> \t\t\telse if (c == '-')\n> \t\t\t\tset = diff_get_color_opt(o, DIFF_FILE_OLD);\n> \t\t}\n> -\t\temit_line_ws_markup(o, set, reset, line, len, set_sign, ' ',\n> +\t\temit_line_ws_markup(o, set_sign, set, reset, ' ', line, len,\n> \t\t\t\t flags & (DIFF_SYMBOL_CONTENT_WS_MASK), 0);\n> \t\tbreak;\n> \tcase DIFF_SYMBOL_PLUS:\n> @@ -1314,7 +1314,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,\n> \t\t\t\tset = diff_get_color_opt(o, DIFF_CONTEXT_BOLD);\n> \t\t\tflags &= ~DIFF_SYMBOL_CONTENT_WS_MASK;\n> \t\t}\n> -\t\temit_line_ws_markup(o, set, reset, line, len, set_sign, '+',\n> +\t\temit_line_ws_markup(o, set_sign, set, reset, '+', line, len,\n> \t\t\t\t flags & DIFF_SYMBOL_CONTENT_WS_MASK,\n> \t\t\t\t flags & DIFF_SYMBOL_CONTENT_BLANK_LINE_EOF);\n> \t\tbreak;\n> @@ -1357,7 +1357,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,\n> \t\t\telse\n> \t\t\t\tset = diff_get_color_opt(o, DIFF_CONTEXT_DIM);\n> \t\t}\n> -\t\temit_line_ws_markup(o, set, reset, line, len, set_sign, '-',\n> +\t\temit_line_ws_markup(o, set_sign, set, reset, '-', line, len,\n> \t\t\t\t flags & DIFF_SYMBOL_CONTENT_WS_MASK, 0);\n> \t\tbreak;\n> \tcase DIFF_SYMBOL_WORDS_PORCELAIN:\n> -- \n> 2.18.0.865.gffc8e1a3cd6-goog\n> \n> \n"},{"id":"355355","messageId":"nycvar.QRO.7.76.6.1808131412100.71@tvgsbejvaqbjf.bet","threadId":"49095","inReplyTo":"20180810223441.30428-6-sbeller@google.com","subject":"Re: [PATCH 5/8] diff.c: add set_sign to emit_line_0","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-08-13T12:16:01Z","receivedAt":"2018-08-13T12:16:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Stefan,\n\nOn Fri, 10 Aug 2018, Stefan Beller wrote:\n\n> For now just change the signature, we'll reason about the actual\n> change in a follow up patch.\n> \n> Pass 'set_sign' (which is output before the sign) and 'set' which\n> controls the color after the first character. Hence, promote any\n> 'set's to 'set_sign' as we want to have color before the sign\n> for now.\n\nI'll freely admit that I had to study the diff in order to understand this\nparagraph.\n\nMy I suggest something along those lines instead?\n\n\tSplit the meaning of the `set` parameter that is passed to\n\t`emit_line_0()` to separate between the color of the \"sign\" (i.e.\n\tthe diff marker '+', '-' or ' ' that is passed in as the `first`\n\tparameter) and the color of the rest of the line.\n\n\tThis changes the meaning of the `set` parameter to no longer refer\n\tto the color of the diff marker, but instead to refer to the color\n\tof the rest of the line. A value of `NULL` indicates that the rest\n\tof the line wants to be colored the same as the diff marker.\n\nCiao,\nDscho\n\n> \n> Signed-off-by: Stefan Beller \n> Signed-off-by: Junio C Hamano \n> ---\n> diff.c | 21 ++++++++++++---------\n> 1 file changed, 12 insertions(+), 9 deletions(-)\n> \n> diff --git a/diff.c b/diff.c\n> index ab6e6a88a56..5eea5edca50 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -622,7 +622,7 @@ static void check_blank_at_eof(mmfile_t *mf1, mmfile_t *mf2,\n> }\n> \n> static void emit_line_0(struct diff_options *o,\n> -\t\t\tconst char *set, unsigned reverse, const char *reset,\n> +\t\t\tconst char *set_sign, const char *set, unsigned reverse, const char *reset,\n> \t\t\tint first, const char *line, int len)\n> {\n> \tint has_trailing_newline, has_trailing_carriage_return;\n> @@ -652,9 +652,12 @@ static void emit_line_0(struct diff_options *o,\n> \tif (len || !nofirst) {\n> \t\tif (reverse && want_color(o->use_color))\n> \t\t\tfputs(GIT_COLOR_REVERSE, file);\n> -\t\tfputs(set, file);\n> +\t\tif (set_sign && set_sign[0])\n> +\t\t\tfputs(set_sign, file);\n> \t\tif (first && !nofirst)\n> \t\t\tfputc(first, file);\n> +\t\tif (set)\n> +\t\t\tfputs(set, file);\n> \t\tfwrite(line, len, 1, file);\n> \t\tfputs(reset, file);\n> \t}\n> @@ -667,7 +670,7 @@ static void emit_line_0(struct diff_options *o,\n> static void emit_line(struct diff_options *o, const char *set, const char *reset,\n> \t\t const char *line, int len)\n> {\n> -\temit_line_0(o, set, 0, reset, line[0], line+1, len-1);\n> +\temit_line_0(o, set, NULL, 0, reset, line[0], line+1, len-1);\n> }\n> \n> enum diff_symbol {\n> @@ -1199,17 +1202,17 @@ static void emit_line_ws_markup(struct diff_options *o,\n> \t}\n> \n> \tif (!ws && !set_sign)\n> -\t\temit_line_0(o, set, 0, reset, sign, line, len);\n> +\t\temit_line_0(o, set, NULL, 0, reset, sign, line, len);\n> \telse if (!ws) {\n> \t\t/* Emit just the prefix, then the rest. */\n> -\t\temit_line_0(o, set_sign, !!set_sign, reset, sign, \"\", 0);\n> -\t\temit_line_0(o, set, 0, reset, 0, line, len);\n> +\t\temit_line_0(o, set_sign, NULL, !!set_sign, reset, sign, \"\", 0);\n> +\t\temit_line_0(o, set, NULL, 0, reset, 0, line, len);\n> \t} else if (blank_at_eof)\n> \t\t/* Blank line at EOF - paint '+' as well */\n> -\t\temit_line_0(o, ws, 0, reset, sign, line, len);\n> +\t\temit_line_0(o, ws, NULL, 0, reset, sign, line, len);\n> \telse {\n> \t\t/* Emit just the prefix, then the rest. */\n> -\t\temit_line_0(o, set_sign ? set_sign : set, !!set_sign, reset,\n> +\t\temit_line_0(o, set_sign ? set_sign : set, NULL, !!set_sign, reset,\n> \t\t\t sign, \"\", 0);\n> \t\tws_check_emit(line, len, ws_rule,\n> \t\t\t o->file, set, reset, ws);\n> @@ -1233,7 +1236,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,\n> \t\tcontext = diff_get_color_opt(o, DIFF_CONTEXT);\n> \t\treset = diff_get_color_opt(o, DIFF_RESET);\n> \t\tputc('\\n', o->file);\n> -\t\temit_line_0(o, context, 0, reset, '\\\\',\n> +\t\temit_line_0(o, context, NULL, 0, reset, '\\\\',\n> \t\t\t nneof, strlen(nneof));\n> \t\tbreak;\n> \tcase DIFF_SYMBOL_SUBMODULE_HEADER:\n> -- \n> 2.18.0.865.gffc8e1a3cd6-goog\n> \n> \n"},{"id":"355357","messageId":"nycvar.QRO.7.76.6.1808131418060.71@tvgsbejvaqbjf.bet","threadId":"49095","inReplyTo":"20180810223441.30428-7-sbeller@google.com","subject":"Re: [PATCH 6/8] diff: use emit_line_0 once per line","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-08-13T12:22:25Z","receivedAt":"2018-08-13T12:22:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Stefan,\n\nOn Fri, 10 Aug 2018, Stefan Beller wrote:\n\n> All lines that use emit_line_0 multiple times per line, are combined\n> into a single call to emit_line_0, making use of the 'set' argument.\n> \n> Signed-off-by: Stefan Beller \n> Signed-off-by: Junio C Hamano \n> ---\n> diff.c | 26 ++++++++++++--------------\n> 1 file changed, 12 insertions(+), 14 deletions(-)\n> \n> diff --git a/diff.c b/diff.c\n> index 5eea5edca50..01095a59d3d 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -629,10 +629,7 @@ static void emit_line_0(struct diff_options *o,\n> \tint nofirst;\n> \tFILE *file = o->file;\n> \n> -\tif (first)\n> -\t\tfputs(diff_line_prefix(o), file);\n> -\telse if (!len)\n> -\t\treturn;\n> +\tfputs(diff_line_prefix(o), file);\n\nHow about separating this into its own \"now that `first` is no longer\n`NULL`, skip this\" patch?\n\nI found it a bit hard to review this diff, primarily because this hunk\nwould logically make more sense after the other hunks.\n\n\n> \tif (len == 0) {\n> \t\thas_trailing_newline = (first == '\\n');\n> @@ -652,13 +649,17 @@ static void emit_line_0(struct diff_options *o,\n> \tif (len || !nofirst) {\n> \t\tif (reverse && want_color(o->use_color))\n> \t\t\tfputs(GIT_COLOR_REVERSE, file);\n> -\t\tif (set_sign && set_sign[0])\n> -\t\t\tfputs(set_sign, file);\n> +\t\tif (set_sign || set)\n> +\t\t\tfputs(set_sign ? set_sign : set, file);\n\nWait, what? Why is `set` all of a sudden also applying to `first`?\n\nI would have expected `set_sign` to apply to `first`, and `set` to the\nrest of the line.\n\n> \t\tif (first && !nofirst)\n> \t\t\tfputc(first, file);\n> -\t\tif (set)\n> -\t\t\tfputs(set, file);\n> -\t\tfwrite(line, len, 1, file);\n> +\t\tif (len) {\n> +\t\t\tif (set_sign && set && set != set_sign)\n> +\t\t\t\tfputs(reset, file);\n> +\t\t\tif (set)\n> +\t\t\t\tfputs(set, file);\n\nThat sounds as if `set == set_sign` would duplicate the `set`. How about\nthis instead?\n\n\t\t\tif (set && set != set_sign) {\n\t\t\t\tif (set_sign)\n\t\t\t\t\tfputs(reset, file);\n\t\t\t\tfputs(set, file);\n\t\t\t}\n\nThe rest looks good to me.\n\nThank you,\nDscho\n\n> +\t\t\tfwrite(line, len, 1, file);\n> +\t\t}\n> \t\tfputs(reset, file);\n> \t}\n> \tif (has_trailing_carriage_return)\n> @@ -1204,16 +1205,13 @@ static void emit_line_ws_markup(struct diff_options *o,\n> \tif (!ws && !set_sign)\n> \t\temit_line_0(o, set, NULL, 0, reset, sign, line, len);\n> \telse if (!ws) {\n> -\t\t/* Emit just the prefix, then the rest. */\n> -\t\temit_line_0(o, set_sign, NULL, !!set_sign, reset, sign, \"\", 0);\n> -\t\temit_line_0(o, set, NULL, 0, reset, 0, line, len);\n> +\t\temit_line_0(o, set_sign, set, !!set_sign, reset, sign, line, len);\n> \t} else if (blank_at_eof)\n> \t\t/* Blank line at EOF - paint '+' as well */\n> \t\temit_line_0(o, ws, NULL, 0, reset, sign, line, len);\n> \telse {\n> \t\t/* Emit just the prefix, then the rest. */\n> -\t\temit_line_0(o, set_sign ? set_sign : set, NULL, !!set_sign, reset,\n> -\t\t\t sign, \"\", 0);\n> +\t\temit_line_0(o, set_sign, set, !!set_sign, reset, sign, \"\", 0);\n> \t\tws_check_emit(line, len, ws_rule,\n> \t\t\t o->file, set, reset, ws);\n> \t}\n> -- \n> 2.18.0.865.gffc8e1a3cd6-goog\n> \n> \n"},{"id":"355358","messageId":"nycvar.QRO.7.76.6.1808131422580.71@tvgsbejvaqbjf.bet","threadId":"49095","inReplyTo":"20180810223441.30428-8-sbeller@google.com","subject":"Re: [PATCH 7/8] diff.c: compute reverse locally in emit_line_0","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-08-13T12:26:36Z","receivedAt":"2018-08-13T12:26:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\n\nOn Fri, 10 Aug 2018, Stefan Beller wrote:\n\n> Signed-off-by: Stefan Beller \n> Signed-off-by: Junio C Hamano \n\nWell, my rationale for having the explicit `reverse` parameter is: this\ncode is complex enough, introducing some magic \"this and that implies\nthis\" makes it much harder to understand.\n\nSo I am not at all sure that this is a good thing.\n\n> ---\n> diff.c | 15 ++++++++-------\n> 1 file changed, 8 insertions(+), 7 deletions(-)\n> \n> diff --git a/diff.c b/diff.c\n> index 01095a59d3d..e50cd312755 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -622,11 +622,12 @@ static void check_blank_at_eof(mmfile_t *mf1, mmfile_t *mf2,\n> }\n> \n> static void emit_line_0(struct diff_options *o,\n> -\t\t\tconst char *set_sign, const char *set, unsigned reverse, const char *reset,\n> +\t\t\tconst char *set_sign, const char *set, const char *reset,\n> \t\t\tint first, const char *line, int len)\n> {\n> \tint has_trailing_newline, has_trailing_carriage_return;\n> \tint nofirst;\n> +\tint reverse = !!set && !!set_sign;\n\nIn contrast to, say, Javascript which has this nice feature that you can\nwrite `set || set_sign` to mean `set ? set : set_sign`, I am fairly\ncertain that `set && set_sign` already evaluates to `0` or `1`. No need\nfor all those exclamation marks!!!!\n\n:-)\n\nBut as I said: I think it is a bit too magic for my liking to say \"if\nthe diff marker color is specified as well as the color for the rest of\nthe line, then the diff marker will be reversed\". That's just making the\ncode hard to understand, i.e. less readable rather than more.\n\nCiao,\nDscho\n\n> \tFILE *file = o->file;\n> \n> \tfputs(diff_line_prefix(o), file);\n> @@ -671,7 +672,7 @@ static void emit_line_0(struct diff_options *o,\n> static void emit_line(struct diff_options *o, const char *set, const char *reset,\n> \t\t const char *line, int len)\n> {\n> -\temit_line_0(o, set, NULL, 0, reset, line[0], line+1, len-1);\n> +\temit_line_0(o, set, NULL, reset, line[0], line+1, len-1);\n> }\n> \n> enum diff_symbol {\n> @@ -1203,15 +1204,15 @@ static void emit_line_ws_markup(struct diff_options *o,\n> \t}\n> \n> \tif (!ws && !set_sign)\n> -\t\temit_line_0(o, set, NULL, 0, reset, sign, line, len);\n> +\t\temit_line_0(o, set, NULL, reset, sign, line, len);\n> \telse if (!ws) {\n> -\t\temit_line_0(o, set_sign, set, !!set_sign, reset, sign, line, len);\n> +\t\temit_line_0(o, set_sign, set, reset, sign, line, len);\n> \t} else if (blank_at_eof)\n> \t\t/* Blank line at EOF - paint '+' as well */\n> -\t\temit_line_0(o, ws, NULL, 0, reset, sign, line, len);\n> +\t\temit_line_0(o, ws, NULL, reset, sign, line, len);\n> \telse {\n> \t\t/* Emit just the prefix, then the rest. */\n> -\t\temit_line_0(o, set_sign, set, !!set_sign, reset, sign, \"\", 0);\n> +\t\temit_line_0(o, set_sign, set, reset, sign, \"\", 0);\n> \t\tws_check_emit(line, len, ws_rule,\n> \t\t\t o->file, set, reset, ws);\n> \t}\n> @@ -1234,7 +1235,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,\n> \t\tcontext = diff_get_color_opt(o, DIFF_CONTEXT);\n> \t\treset = diff_get_color_opt(o, DIFF_RESET);\n> \t\tputc('\\n', o->file);\n> -\t\temit_line_0(o, context, NULL, 0, reset, '\\\\',\n> +\t\temit_line_0(o, context, NULL, reset, '\\\\',\n> \t\t\t nneof, strlen(nneof));\n> \t\tbreak;\n> \tcase DIFF_SYMBOL_SUBMODULE_HEADER:\n> -- \n> 2.18.0.865.gffc8e1a3cd6-goog\n> \n> \n"},{"id":"355359","messageId":"nycvar.QRO.7.76.6.1808131429550.71@tvgsbejvaqbjf.bet","threadId":"49095","inReplyTo":"20180810223441.30428-9-sbeller@google.com","subject":"Re: [PATCH 8/8] diff.c: rewrite emit_line_0 more understandably","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-08-13T12:42:39Z","receivedAt":"2018-08-13T12:42:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Stefan,\n\nOn Fri, 10 Aug 2018, Stefan Beller wrote:\n\n> emit_line_0 has no nested conditions, but puts out all its arguments\n> (if set) in order.\n\nWell, currently `emit_line_0()` *has* nested conditions: `first == '\\n'`\ninside `len == 0`.\n\nAnd these nested conditions make things hard to read, so resolving that\nto a simpler workflow makes a ton of sense. You can sell that better by\nstarting the commit message with something along the lines that you are\nmaking the overly complex logic easier to follow.\n\n> There is still a slight confusion with set and set_sign, but let's defer\n> that to a later patch.\n\nThere is no later patch in this here patch series. I would therefore\nappreciate it if you could spend the paragraph or two on explaining\nyourself here.\n\n> 'first' used be output always no matter if it was 0, but that got lost\n\ns/used be/used to be/\n\n> at \"diff: add an internal option to dual-color diffs of diffs\",\n> 2018-07-21), as there we broadened the meaning of 'first' to also\n> signal an early return.\n\nDid we? I thought we introduced the possibility of passing *just* a first\ncharacter or *just* a \"rest of the line\". I might misremember, though.\n\n> The change in 'emit_line' makes sure that 'first' is never content, but\n\nAwwww, you want to make 'first' sad all the time? That's\nnot nice of you...\n\nSeriously again, the adjective \"content\" has a different meaning than the\nnoun and this ambiguity made this sentence very hard for me to parse.\n\n> always under our control, a sign or special character in the beginning\n> of the line (or 0, in which case we ignore it).\n\nIt would be good to reword this paragraph to say that from now on, we will\nonly pass a diff marker as `first`, or 0.\n\n> Signed-off-by: Stefan Beller \n> Signed-off-by: Junio C Hamano \n> ---\n> diff.c | 73 +++++++++++++++++++++++++++++++++-------------------------\n> 1 file changed, 41 insertions(+), 32 deletions(-)\n> \n> diff --git a/diff.c b/diff.c\n> index e50cd312755..af9316c8f91 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -626,43 +626,52 @@ static void emit_line_0(struct diff_options *o,\n> \t\t\tint first, const char *line, int len)\n> {\n> \tint has_trailing_newline, has_trailing_carriage_return;\n> -\tint nofirst;\n> \tint reverse = !!set && !!set_sign;\n> +\tint needs_reset = 0;\n> +\n> \tFILE *file = o->file;\n> \n> \tfputs(diff_line_prefix(o), file);\n> \n> -\tif (len == 0) {\n> -\t\thas_trailing_newline = (first == '\\n');\n> -\t\thas_trailing_carriage_return = (!has_trailing_newline &&\n> -\t\t\t\t\t\t(first == '\\r'));\n> -\t\tnofirst = has_trailing_newline || has_trailing_carriage_return;\n> -\t} else {\n> -\t\thas_trailing_newline = (len > 0 && line[len-1] == '\\n');\n> -\t\tif (has_trailing_newline)\n> -\t\t\tlen--;\n> -\t\thas_trailing_carriage_return = (len > 0 && line[len-1] == '\\r');\n> -\t\tif (has_trailing_carriage_return)\n> -\t\t\tlen--;\n> -\t\tnofirst = 0;\n> -\t}\n> -\n> -\tif (len || !nofirst) {\n> -\t\tif (reverse && want_color(o->use_color))\n> -\t\t\tfputs(GIT_COLOR_REVERSE, file);\n> -\t\tif (set_sign || set)\n> -\t\t\tfputs(set_sign ? set_sign : set, file);\n> -\t\tif (first && !nofirst)\n> -\t\t\tfputc(first, file);\n> -\t\tif (len) {\n> -\t\t\tif (set_sign && set && set != set_sign)\n> -\t\t\t\tfputs(reset, file);\n> -\t\t\tif (set)\n> -\t\t\t\tfputs(set, file);\n> -\t\t\tfwrite(line, len, 1, file);\n> -\t\t}\n> -\t\tfputs(reset, file);\n> +\thas_trailing_newline = (len > 0 && line[len-1] == '\\n');\n> +\tif (has_trailing_newline)\n> +\t\tlen--;\n> +\n> +\thas_trailing_carriage_return = (len > 0 && line[len-1] == '\\r');\n> +\tif (has_trailing_carriage_return)\n> +\t\tlen--;\n> +\n> +\tif (!len && !first)\n> +\t\tgoto end_of_line;\n> +\n> +\tif (reverse && want_color(o->use_color)) {\n\nSince you implied `reverse` to mean that a non-`NULL` `set` *as well as*\n`set_sign` were passed in, and since a non-`NULL` `set` *implies* that we\nwant color, you could drop that `want_color(o->use_color)` here.\n\nBut as I stated above, I am not a fan of having such unintuitive\nimplications in the code.\n\n> +\t\tfputs(GIT_COLOR_REVERSE, file);\n> +\t\tneeds_reset = 1;\n> +\t}\n> +\n> +\tif (set_sign || set) {\n> +\t\tfputs(set_sign ? set_sign : set, file);\n> +\t\tneeds_reset = 1;\n> \t}\n> +\n> +\tif (first)\n> +\t\tfputc(first, file);\n> +\n> +\tif (!len)\n> +\t\tgoto end_of_line;\n> +\n> +\tif (set) {\n> +\t\tif (set_sign && set != set_sign)\n> +\t\t\tfputs(reset, file);\n> +\t\tfputs(set, file);\n> +\t\tneeds_reset = 1;\n> +\t}\n> +\tfwrite(line, len, 1, file);\n> +\tneeds_reset |= len > 0;\n\nFirst of all, this should be a `||=`, not a `|=`.\n\nAnd then, this code is skipped by the `if (!len) goto end_of_line;` part\nabove, so `len > 0` is *always* 1 at this point.\n\nBut then, I wonder why we bother all that much. After all, we want to\nreset whenever we used color. So why not simply initialize\n\n\tint need_reset = reverse || set_sign || set;\n\nand be done with it?\n\nThanks,\nDscho\n\n> +\n> +end_of_line:\n> +\tif (needs_reset)\n> +\t\tfputs(reset, file);\n> \tif (has_trailing_carriage_return)\n> \t\tfputc('\\r', file);\n> \tif (has_trailing_newline)\n> @@ -672,7 +681,7 @@ static void emit_line_0(struct diff_options *o,\n> static void emit_line(struct diff_options *o, const char *set, const char *reset,\n> \t\t const char *line, int len)\n> {\n> -\temit_line_0(o, set, NULL, reset, line[0], line+1, len-1);\n> +\temit_line_0(o, set, NULL, reset, 0, line, len);\n> }\n> \n> enum diff_symbol {\n> -- \n> 2.18.0.865.gffc8e1a3cd6-goog\n> \n> \n"},{"id":"355436","messageId":"CAGZ79kbnSD5oiHLMy8b7o_SHFzquqttm1anak-4cnVzy10ns_w@mail.gmail.com","threadId":"49095","inReplyTo":"nycvar.QRO.7.76.6.1808131404370.71@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 2/8] t3206: add color test for range-diff --dual-color","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-13T18:30:42Z","receivedAt":"2018-08-13T18:30:56Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Aug 13, 2018 at 5:05 AM Johannes Schindelin\n wrote:\n>\n> Hi Stefan,\n>\n> On Fri, 10 Aug 2018, Stefan Beller wrote:\n>\n> > +test_expect_success 'dual-coloring' '\n> > + sed -e \"s|^:||\" >expect <<-\\EOF &&\n> > + :1: a4b3333 = 1: f686024 s/5/A/\n> > + :2: f51d370 ! 2: 4ab067d s/4/A/\n> > + : @@ -2,6 +2,8 @@\n>\n> That's a neat trick to have an indented here-doc that contains\n> indentation. I should use this myself.\n\nThanks to Junio for applying this trick!\n"},{"id":"355446","messageId":"CAGZ79kZ=bs84WGeA5TbVUTbQL2ECUSwEJanMKmeWw+xKp-rH5Q@mail.gmail.com","threadId":"49095","inReplyTo":"nycvar.QRO.7.76.6.1808131429550.71@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 8/8] diff.c: rewrite emit_line_0 more understandably","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-13T18:57:20Z","receivedAt":"2018-08-13T18:57:35Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Aug 13, 2018 at 5:42 AM Johannes Schindelin\n wrote:\n>\n> Hi Stefan,\n>\n> On Fri, 10 Aug 2018, Stefan Beller wrote:\n>\n> > emit_line_0 has no nested conditions, but puts out all its arguments\n> > (if set) in order.\n>\n> Well, currently `emit_line_0()` *has* nested conditions: `first == '\\n'`\n> inside `len == 0`.\n>\n> And these nested conditions make things hard to read, so resolving that\n> to a simpler workflow makes a ton of sense. You can sell that better by\n> starting the commit message with something along the lines that you are\n> making the overly complex logic easier to follow.\n>\n> > There is still a slight confusion with set and set_sign, but let's defer\n> > that to a later patch.\n>\n> There is no later patch in this here patch series.\n\nyup, I got overly excited when writing this commit message, as for\nwhat we could try next.\n\n> I would therefore\n> appreciate it if you could spend the paragraph or two on explaining\n> yourself here.\n\nnoted.\n\n>\n> > at \"diff: add an internal option to dual-color diffs of diffs\",\n> > 2018-07-21), as there we broadened the meaning of 'first' to also\n> > signal an early return.\n>\n> Did we? I thought we introduced the possibility of passing *just* a first\n> character or *just* a \"rest of the line\". I might misremember, though.\n>\n> > The change in 'emit_line' makes sure that 'first' is never content, but\n>\n> Awwww, you want to make 'first' sad all the time? That's\n> not nice of you...\n>\n> Seriously again, the adjective \"content\" has a different meaning than the\n> noun and this ambiguity made this sentence very hard for me to parse.\n\nSo what is a good noun for the stuff that Git stores? (\"stuff\" is not a\ngood one)\n\n> > always under our control, a sign or special character in the beginning\n> > of the line (or 0, in which case we ignore it).\n>\n> It would be good to reword this paragraph to say that from now on, we will\n> only pass a diff marker as `first`, or 0.\n\nMakes sense.\n\n\n\n> > + if (!len && !first)\n> > + goto end_of_line;\n> > +\n> > + if (reverse && want_color(o->use_color)) {\n>\n> Since you implied `reverse` to mean that a non-`NULL` `set` *as well as*\n> `set_sign` were passed in, and since a non-`NULL` `set` *implies* that we\n> want color, you could drop that `want_color(o->use_color)` here.\n>\n> But as I stated above, I am not a fan of having such unintuitive\n> implications in the code.\n\nokay. So we'd want to be explicit about reverse again?\n\n> > + fwrite(line, len, 1, file);\n> > + needs_reset |= len > 0;\n>\n> First of all, this should be a `||=`, not a `|=`.\n>\n> And then, this code is skipped by the `if (!len) goto end_of_line;` part\n> above, so `len > 0` is *always* 1 at this point.\n>\n> But then, I wonder why we bother all that much. After all, we want to\n> reset whenever we used color. So why not simply initialize\n>\n> int need_reset = reverse || set_sign || set;\n>\n> and be done with it?\n\nI just removed that line and all tests still pass. :/\nI could have sworn of a failing test when writing the code\nthat ensures that content (noun), that contains color codes,\nwould still look okay by having a reset at the end of the line,\nso really we'd need to have\n\n need_reset = reverse || set_sign || set || (len > 0);\n\nI'll dig into this again.\n\nThanks,\nStefan\n"},{"id":"355449","messageId":"CAGZ79kYFkYqNADUVu-QCQBr5F5wFsFd8sUPfRsL5CqjnE4fDVw@mail.gmail.com","threadId":"49095","inReplyTo":"nycvar.QRO.7.76.6.1808131422580.71@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 7/8] diff.c: compute reverse locally in emit_line_0","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-13T19:00:08Z","receivedAt":"2018-08-13T19:00:21Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Aug 13, 2018 at 5:26 AM Johannes Schindelin\n wrote:\n>\n> Hi,\n>\n>\n> On Fri, 10 Aug 2018, Stefan Beller wrote:\n>\n> > Signed-off-by: Stefan Beller \n> > Signed-off-by: Junio C Hamano \n>\n> Well, my rationale for having the explicit `reverse` parameter is: this\n> code is complex enough, introducing some magic \"this and that implies\n> this\" makes it much harder to understand.\n>\n> So I am not at all sure that this is a good thing.\n\nYeah I am unsure, too. On the other hand the higher level functions\nlook so much nicer a the complexity is shoved downwards,\nsuch that each function is solving problems in their own domain.\n\nI'll think of an alternative.\n"},{"id":"355504","messageId":"CAGZ79kZvdNNF-TnT5=a2HrMZCpTTZXRuiAoKQ3L5cKjAk9UruA@mail.gmail.com","threadId":"49095","inReplyTo":"20180810223441.30428-1-sbeller@google.com","subject":"Re: [PATCH 0/8] Resending sb/range-diff-colors","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-13T23:36:38Z","receivedAt":"2018-08-13T23:36:52Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Aug 10, 2018 at 3:34 PM Stefan Beller wrote:\n>\n> This is also avaliable as\n> git fetch https://github.com/stefanbeller/git sb/range-diff-colors\n>\n> and is a resend of sb/range-diff-colors.\n\nI thought about this series a bit, and I think we would want to break\nit up into 2:\n\n * the actual sb/range-diff-colors consisting of the first two patches\n I can resend them or you can just rebase them without conflicts.\n These two patches (test_decode_color: FAINT/ITALIC + t3206: color)\n are essentially just adding the tests and making sure the colored\n range-diff will work correctly in the future\n\n * The refactoring, which might be titled sb/diff-refactor instead.\n Given Johannes review comments, I will rework all patches\n that are \"diff: something\" to have less confusion for Johannes\n reviewing them.\n\nAnd once we have these two we can have a resend of\nhttps://public-inbox.org/git/20180810224923.143625-1-sbeller@google.com/\nwhich might be titled sb/range-diff-adjust-colors.\n\n\nThanks,\nStefan\n"},{"id":"355505","messageId":"CAGZ79kbypjBQ6gnULg6ZgMVDD8h8BMzkLK=SYKZo9PHzH7z5BA@mail.gmail.com","threadId":"49095","inReplyTo":"nycvar.QRO.7.76.6.1808131412100.71@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 5/8] diff.c: add set_sign to emit_line_0","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-13T23:42:20Z","receivedAt":"2018-08-13T23:42:34Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Aug 13, 2018 at 5:15 AM Johannes Schindelin\n wrote:\n>\n> Hi Stefan,\n>\n> On Fri, 10 Aug 2018, Stefan Beller wrote:\n>\n> > For now just change the signature, we'll reason about the actual\n> > change in a follow up patch.\n> >\n> > Pass 'set_sign' (which is output before the sign) and 'set' which\n> > controls the color after the first character. Hence, promote any\n> > 'set's to 'set_sign' as we want to have color before the sign\n> > for now.\n>\n> I'll freely admit that I had to study the diff in order to understand this\n> paragraph.\n>\n> My I suggest something along those lines instead?\n>\n> Split the meaning of the `set` parameter that is passed to\n> `emit_line_0()` to separate between the color of the \"sign\" (i.e.\n> the diff marker '+', '-' or ' ' that is passed in as the `first`\n> parameter) and the color of the rest of the line.\n>\n> This changes the meaning of the `set` parameter to no longer refer\n> to the color of the diff marker, but instead to refer to the color\n> of the rest of the line. A value of `NULL` indicates that the rest\n> of the line wants to be colored the same as the diff marker.\n>\n\nThat is a wonderful commit message,\nI'll just use it as-is.\n\nThanks,\nStefan\n"},{"id":"355510","messageId":"20180814014122.30662-1-sbeller@google.com","threadId":"49095","inReplyTo":"20180810223441.30428-1-sbeller@google.com","subject":"[PATCHv2 0/8] Resending sb/range-diff-colors","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-14T01:41:14Z","receivedAt":"2018-08-14T01:41:32Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This is also avaliable as\ngit fetch https://github.com/stefanbeller/git sb/range-diff-colors\n\nThis applies on top of js/range-diff (a7be92acd9600), this version \n* resolves a semantic conflict in the test\n (Did range-diff change its output slightly?)\n* addressed Johannes feedback, such as\n -> keeping the reverse computation out of emit_line_0\n -> better commit messages.\n -> Split \"use emit_line_0 once per line\" to have an additional\n \"diff.c: omit check for line prefix in emit_line_0\" as having\n these two things in the same patch is confusing\n\nThe interdiff seems more useful than the range-diff here,\nboth attached below.\n\nThanks,\nStefan\n\nStefan Beller (8):\n test_decode_color: understand FAINT and ITALIC\n t3206: add color test for range-diff --dual-color\n diff.c: simplify caller of emit_line_0\n diff.c: reorder arguments for emit_line_ws_markup\n diff.c: add set_sign to emit_line_0\n diff: use emit_line_0 once per line\n diff.c: omit check for line prefix in emit_line_0\n diff.c: rewrite emit_line_0 more understandably\n\n diff.c | 91 ++++++++++++++++++++++-------------------\n t/t3206-range-diff.sh | 39 ++++++++++++++++++\n t/test-lib-functions.sh | 2 +\n 3 files changed, 91 insertions(+), 41 deletions(-)\n\n(interdiff seems to be more useful)\ngit diff 4dc97b54a35..058e03a1601\n\ndiff --git a/builtin/range-diff.c b/builtin/range-diff.c\nindex ef3ba22e297..f52d45d9d61 100644\n--- a/builtin/range-diff.c\n+++ b/builtin/range-diff.c\n@@ -53,6 +53,8 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)\n \t\telse\n \t\t\ti += c;\n \t}\n+\twhile (i < argc)\n+\t\targv[j++] = argv[i++];\n \targc = j;\n \tdiff_setup_done(&diffopt);\n \ndiff --git a/diff.c b/diff.c\nindex af9316c8f91..c5c7739ce34 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -622,13 +622,11 @@ static void check_blank_at_eof(mmfile_t *mf1, mmfile_t *mf2,\n }\n \n static void emit_line_0(struct diff_options *o,\n-\t\t\tconst char *set_sign, const char *set, const char *reset,\n+\t\t\tconst char *set_sign, const char *set, unsigned reverse, const char *reset,\n \t\t\tint first, const char *line, int len)\n {\n \tint has_trailing_newline, has_trailing_carriage_return;\n-\tint reverse = !!set && !!set_sign;\n-\tint needs_reset = 0;\n-\n+\tint needs_reset = 0; /* at the end of the line */\n \tFILE *file = o->file;\n \n \tfputs(diff_line_prefix(o), file);\n@@ -649,8 +647,8 @@ static void emit_line_0(struct diff_options *o,\n \t\tneeds_reset = 1;\n \t}\n \n-\tif (set_sign || set) {\n-\t\tfputs(set_sign ? set_sign : set, file);\n+\tif (set_sign) {\n+\t\tfputs(set_sign, file);\n \t\tneeds_reset = 1;\n \t}\n \n@@ -667,7 +665,7 @@ static void emit_line_0(struct diff_options *o,\n \t\tneeds_reset = 1;\n \t}\n \tfwrite(line, len, 1, file);\n-\tneeds_reset |= len > 0;\n+\tneeds_reset = 1; /* 'line' may contain color codes. */\n \n end_of_line:\n \tif (needs_reset)\n@@ -681,7 +679,7 @@ static void emit_line_0(struct diff_options *o,\n static void emit_line(struct diff_options *o, const char *set, const char *reset,\n \t\t const char *line, int len)\n {\n-\temit_line_0(o, set, NULL, reset, 0, line, len);\n+\temit_line_0(o, set, NULL, 0, reset, 0, line, len);\n }\n \n enum diff_symbol {\n@@ -1213,15 +1211,16 @@ static void emit_line_ws_markup(struct diff_options *o,\n \t}\n \n \tif (!ws && !set_sign)\n-\t\temit_line_0(o, set, NULL, reset, sign, line, len);\n+\t\temit_line_0(o, set, NULL, 0, reset, sign, line, len);\n \telse if (!ws) {\n-\t\temit_line_0(o, set_sign, set, reset, sign, line, len);\n+\t\temit_line_0(o, set_sign, set, !!set_sign, reset, sign, line, len);\n \t} else if (blank_at_eof)\n \t\t/* Blank line at EOF - paint '+' as well */\n-\t\temit_line_0(o, ws, NULL, reset, sign, line, len);\n+\t\temit_line_0(o, ws, NULL, 0, reset, sign, line, len);\n \telse {\n \t\t/* Emit just the prefix, then the rest. */\n-\t\temit_line_0(o, set_sign, set, reset, sign, \"\", 0);\n+\t\temit_line_0(o, set_sign ? set_sign : set, NULL, !!set_sign, reset,\n+\t\t\t sign, \"\", 0);\n \t\tws_check_emit(line, len, ws_rule,\n \t\t\t o->file, set, reset, ws);\n \t}\n@@ -1244,7 +1243,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,\n \t\tcontext = diff_get_color_opt(o, DIFF_CONTEXT);\n \t\treset = diff_get_color_opt(o, DIFF_RESET);\n \t\tputc('\\n', o->file);\n-\t\temit_line_0(o, context, NULL, reset, '\\\\',\n+\t\temit_line_0(o, context, NULL, 0, reset, '\\\\',\n \t\t\t nneof, strlen(nneof));\n \t\tbreak;\n \tcase DIFF_SYMBOL_SUBMODULE_HEADER:\n\n\n\n\n./git-range-diff github/sb/range-diff-colors... below:\n\n22: 0fedd4c0a20 = 22: dd772035ac9 test_decode_color: understand FAINT and ITALIC\n23: 6a1c7698c68 ! 23: 5701745379b t3206: add color test for range-diff --dual-color\n @@ -23,7 +23,7 @@\n +\t: s/4/A/\n +\t: \n +\t: + Also a silly comment here!\n -+\t: +\n ++\t: +\n +\t: diff --git a/file b/file\n +\t: --- a/file\n +\t: +++ b/file\n24: 7e12ece1d34 = 24: 4e56f63a5f5 diff.c: simplify caller of emit_line_0\n25: 74dabd6d36f ! 25: cf126556940 diff.c: reorder arguments for emit_line_ws_markup\n @@ -3,7 +3,8 @@\n diff.c: reorder arguments for emit_line_ws_markup\n \n The order shall be all colors first, then the content, flags at the end.\n - The colors are in order.\n + The colors are in the order of occurrence, i.e. first the color for the\n + sign and then the color for the rest of the line.\n \n Signed-off-by: Stefan Beller \n Signed-off-by: Junio C Hamano \n26: e304e15aa6b ! 26: 69008364cb8 diff.c: add set_sign to emit_line_0\n @@ -2,14 +2,17 @@\n \n diff.c: add set_sign to emit_line_0\n \n - For now just change the signature, we'll reason about the actual\n - change in a follow up patch.\n + Split the meaning of the `set` parameter that is passed to\n + emit_line_0()` to separate between the color of the \"sign\" (i.e.\n + the diff marker '+', '-' or ' ' that is passed in as the `first`\n + parameter) and the color of the rest of the line.\n \n - Pass 'set_sign' (which is output before the sign) and 'set' which\n - controls the color after the first character. Hence, promote any\n - 'set's to 'set_sign' as we want to have color before the sign\n - for now.\n + This changes the meaning of the `set` parameter to no longer refer\n + to the color of the diff marker, but instead to refer to the color\n + of the rest of the line. A value of `NULL` indicates that the rest\n + of the line wants to be colored the same as the diff marker.\n \n + Helped-by: Johannes Schindelin \n Signed-off-by: Stefan Beller \n Signed-off-by: Junio C Hamano \n \n @@ -30,12 +33,15 @@\n \t\tif (reverse && want_color(o->use_color))\n \t\t\tfputs(GIT_COLOR_REVERSE, file);\n -\t\tfputs(set, file);\n -+\t\tif (set_sign && set_sign[0])\n ++\t\tif (set_sign)\n +\t\t\tfputs(set_sign, file);\n \t\tif (first && !nofirst)\n \t\t\tfputc(first, file);\n -+\t\tif (set)\n ++\t\tif (set && set != set_sign) {\n ++\t\t\tif (set_sign)\n ++\t\t\t\tfputs(reset, file);\n +\t\t\tfputs(set, file);\n ++\t\t}\n \t\tfwrite(line, len, 1, file);\n \t\tfputs(reset, file);\n \t}\n27: 8d935d5212c < -: ----------- diff: use emit_line_0 once per line\n28: 2344aac787a < -: ----------- diff.c: compute reverse locally in emit_line_0\n -: ----------- > 27: 5d2629281a1 diff: use emit_line_0 once per line\n -: ----------- > 28: 5240e94a69a diff.c: omit check for line prefix in emit_line_0\n29: 4dc97b54a35 ! 29: 058e03a1601 diff.c: rewrite emit_line_0 more understandably\n @@ -2,21 +2,15 @@\n \n diff.c: rewrite emit_line_0 more understandably\n \n - emit_line_0 has no nested conditions, but puts out all its arguments\n - (if set) in order. There is still a slight confusion with set\n - and set_sign, but let's defer that to a later patch.\n + Rewrite emit_line_0 to have fewer (nested) conditions.\n \n - 'first' used be output always no matter if it was 0, but that got lost\n - at \"diff: add an internal option to dual-color diffs of diffs\",\n - 2018-07-21), as there we broadened the meaning of 'first' to also\n - signal an early return.\n -\n - The change in 'emit_line' makes sure that 'first' is never content, but\n - always under our control, a sign or special character in the beginning\n - of the line (or 0, in which case we ignore it).\n + The change in 'emit_line' makes sure that 'first' is never user data,\n + but always under our control, a sign or special character in the\n + beginning of the line (or 0, in which case we ignore it).\n + So from now on, let's pass only a diff marker or 0 as the 'first'\n + character of the line.\n \n Signed-off-by: Stefan Beller \n - Signed-off-by: Junio C Hamano \n \n diff --git a/diff.c b/diff.c\n --- a/diff.c\n @@ -26,9 +20,7 @@\n {\n \tint has_trailing_newline, has_trailing_carriage_return;\n -\tint nofirst;\n - \tint reverse = !!set && !!set_sign;\n -+\tint needs_reset = 0;\n -+\n ++\tint needs_reset = 0; /* at the end of the line */\n \tFILE *file = o->file;\n \n \tfputs(diff_line_prefix(o), file);\n @@ -51,15 +43,16 @@\n -\tif (len || !nofirst) {\n -\t\tif (reverse && want_color(o->use_color))\n -\t\t\tfputs(GIT_COLOR_REVERSE, file);\n --\t\tif (set_sign || set)\n --\t\t\tfputs(set_sign ? set_sign : set, file);\n +-\t\tif (set_sign)\n +-\t\t\tfputs(set_sign, file);\n -\t\tif (first && !nofirst)\n -\t\t\tfputc(first, file);\n -\t\tif (len) {\n --\t\t\tif (set_sign && set && set != set_sign)\n --\t\t\t\tfputs(reset, file);\n --\t\t\tif (set)\n +-\t\t\tif (set && set != set_sign) {\n +-\t\t\t\tif (set_sign)\n +-\t\t\t\t\tfputs(reset, file);\n -\t\t\t\tfputs(set, file);\n +-\t\t\t}\n -\t\t\tfwrite(line, len, 1, file);\n -\t\t}\n -\t\tfputs(reset, file);\n @@ -79,8 +72,8 @@\n +\t\tneeds_reset = 1;\n +\t}\n +\n -+\tif (set_sign || set) {\n -+\t\tfputs(set_sign ? set_sign : set, file);\n ++\tif (set_sign) {\n ++\t\tfputs(set_sign, file);\n +\t\tneeds_reset = 1;\n \t}\n +\n @@ -97,7 +90,7 @@\n +\t\tneeds_reset = 1;\n +\t}\n +\tfwrite(line, len, 1, file);\n -+\tneeds_reset |= len > 0;\n ++\tneeds_reset = 1; /* 'line' may contain color codes. */\n +\n +end_of_line:\n +\tif (needs_reset)\n @@ -109,8 +102,8 @@\n static void emit_line(struct diff_options *o, const char *set, const char *reset,\n \t\t const char *line, int len)\n {\n --\temit_line_0(o, set, NULL, reset, line[0], line+1, len-1);\n -+\temit_line_0(o, set, NULL, reset, 0, line, len);\n +-\temit_line_0(o, set, NULL, 0, reset, line[0], line+1, len-1);\n ++\temit_line_0(o, set, NULL, 0, reset, 0, line, len);\n }\n \n enum diff_symbol {\n"},{"id":"355511","messageId":"20180814014122.30662-2-sbeller@google.com","threadId":"49095","inReplyTo":"20180814014122.30662-1-sbeller@google.com","subject":"[PATCH 1/8] test_decode_color: understand FAINT and ITALIC","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-14T01:41:15Z","receivedAt":"2018-08-14T01:41:34Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n t/test-lib-functions.sh | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 2b2181dca09..be8244c47b5 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -42,6 +42,8 @@ test_decode_color () {\n \t\tfunction name(n) {\n \t\t\tif (n == 0) return \"RESET\";\n \t\t\tif (n == 1) return \"BOLD\";\n+\t\t\tif (n == 2) return \"FAINT\";\n+\t\t\tif (n == 3) return \"ITALIC\";\n \t\t\tif (n == 7) return \"REVERSE\";\n \t\t\tif (n == 30) return \"BLACK\";\n \t\t\tif (n == 31) return \"RED\";\n-- \n2.18.0.865.gffc8e1a3cd6-goog\n\n"},{"id":"355512","messageId":"20180814014122.30662-3-sbeller@google.com","threadId":"49095","inReplyTo":"20180814014122.30662-1-sbeller@google.com","subject":"[PATCH 2/8] t3206: add color test for range-diff --dual-color","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-14T01:41:16Z","receivedAt":"2018-08-14T01:41:37Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The 'expect'ed outcome has been taken by running the 'range-diff | decode'.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n t/t3206-range-diff.sh | 39 +++++++++++++++++++++++++++++++++++++++\n 1 file changed, 39 insertions(+)\n\ndiff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\nindex 2237c7f4af9..31f6458f961 100755\n--- a/t/t3206-range-diff.sh\n+++ b/t/t3206-range-diff.sh\n@@ -142,4 +142,43 @@ test_expect_success 'changed message' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'dual-coloring' '\n+\tsed -e \"s|^:||\" >expect <<-\\EOF &&\n+\t:1: a4b3333 = 1: f686024 s/5/A/\n+\t:2: f51d370 ! 2: 4ab067d s/4/A/\n+\t: @@ -2,6 +2,8 @@\n+\t: \n+\t: s/4/A/\n+\t: \n+\t: + Also a silly comment here!\n+\t: +\n+\t: diff --git a/file b/file\n+\t: --- a/file\n+\t: +++ b/file\n+\t:3: 0559556 ! 3: b9cb956 s/11/B/\n+\t: @@ -10,7 +10,7 @@\n+\t: 9\n+\t: 10\n+\t: -11\n+\t: -+BB\n+\t: ++B\n+\t: 12\n+\t: 13\n+\t: 14\n+\t:4: d966c5c ! 4: 8add5f1 s/12/B/\n+\t: @@ -8,7 +8,7 @@\n+\t: @@\n+\t: 9\n+\t: 10\n+\t: - BB\n+\t: + B\n+\t: -12\n+\t: +B\n+\t: 13\n+\tEOF\n+\tgit range-diff changed...changed-message --color --dual-color >actual.raw &&\n+\ttest_decode_color >actual \nSigned-off-by: Junio C Hamano \n---\n diff.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex ae131495216..f6df18af913 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1202,8 +1202,7 @@ static void emit_line_ws_markup(struct diff_options *o,\n \t\temit_line_0(o, set, 0, reset, sign, line, len);\n \telse if (!ws) {\n \t\t/* Emit just the prefix, then the rest. */\n-\t\temit_line_0(o, set_sign ? set_sign : set, !!set_sign, reset,\n-\t\t\t sign, \"\", 0);\n+\t\temit_line_0(o, set_sign, !!set_sign, reset, sign, \"\", 0);\n \t\temit_line_0(o, set, 0, reset, 0, line, len);\n \t} else if (blank_at_eof)\n \t\t/* Blank line at EOF - paint '+' as well */\n-- \n2.18.0.865.gffc8e1a3cd6-goog\n\n"},{"id":"355514","messageId":"20180814014122.30662-5-sbeller@google.com","threadId":"49095","inReplyTo":"20180814014122.30662-1-sbeller@google.com","subject":"[PATCH 4/8] diff.c: reorder arguments for emit_line_ws_markup","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-14T01:41:18Z","receivedAt":"2018-08-14T01:41:42Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The order shall be all colors first, then the content, flags at the end.\nThe colors are in the order of occurrence, i.e. first the color for the\nsign and then the color for the rest of the line.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n diff.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex f6df18af913..ab6e6a88a56 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1185,9 +1185,9 @@ static void dim_moved_lines(struct diff_options *o)\n }\n \n static void emit_line_ws_markup(struct diff_options *o,\n-\t\t\t\tconst char *set, const char *reset,\n-\t\t\t\tconst char *line, int len,\n-\t\t\t\tconst char *set_sign, char sign,\n+\t\t\t\tconst char *set_sign, const char *set,\n+\t\t\t\tconst char *reset,\n+\t\t\t\tchar sign, const char *line, int len,\n \t\t\t\tunsigned ws_rule, int blank_at_eof)\n {\n \tconst char *ws = NULL;\n@@ -1271,7 +1271,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,\n \t\t\telse if (c == '-')\n \t\t\t\tset = diff_get_color_opt(o, DIFF_FILE_OLD);\n \t\t}\n-\t\temit_line_ws_markup(o, set, reset, line, len, set_sign, ' ',\n+\t\temit_line_ws_markup(o, set_sign, set, reset, ' ', line, len,\n \t\t\t\t flags & (DIFF_SYMBOL_CONTENT_WS_MASK), 0);\n \t\tbreak;\n \tcase DIFF_SYMBOL_PLUS:\n@@ -1314,7 +1314,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,\n \t\t\t\tset = diff_get_color_opt(o, DIFF_CONTEXT_BOLD);\n \t\t\tflags &= ~DIFF_SYMBOL_CONTENT_WS_MASK;\n \t\t}\n-\t\temit_line_ws_markup(o, set, reset, line, len, set_sign, '+',\n+\t\temit_line_ws_markup(o, set_sign, set, reset, '+', line, len,\n \t\t\t\t flags & DIFF_SYMBOL_CONTENT_WS_MASK,\n \t\t\t\t flags & DIFF_SYMBOL_CONTENT_BLANK_LINE_EOF);\n \t\tbreak;\n@@ -1357,7 +1357,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,\n \t\t\telse\n \t\t\t\tset = diff_get_color_opt(o, DIFF_CONTEXT_DIM);\n \t\t}\n-\t\temit_line_ws_markup(o, set, reset, line, len, set_sign, '-',\n+\t\temit_line_ws_markup(o, set_sign, set, reset, '-', line, len,\n \t\t\t\t flags & DIFF_SYMBOL_CONTENT_WS_MASK, 0);\n \t\tbreak;\n \tcase DIFF_SYMBOL_WORDS_PORCELAIN:\n-- \n2.18.0.865.gffc8e1a3cd6-goog\n\n"},{"id":"355515","messageId":"20180814014122.30662-6-sbeller@google.com","threadId":"49095","inReplyTo":"20180814014122.30662-1-sbeller@google.com","subject":"[PATCH 5/8] diff.c: add set_sign to emit_line_0","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-14T01:41:19Z","receivedAt":"2018-08-14T01:41:45Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Split the meaning of the `set` parameter that is passed to\nemit_line_0()` to separate between the color of the \"sign\" (i.e.\nthe diff marker '+', '-' or ' ' that is passed in as the `first`\nparameter) and the color of the rest of the line.\n\nThis changes the meaning of the `set` parameter to no longer refer\nto the color of the diff marker, but instead to refer to the color\nof the rest of the line. A value of `NULL` indicates that the rest\nof the line wants to be colored the same as the diff marker.\n\nHelped-by: Johannes Schindelin \nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n diff.c | 24 +++++++++++++++---------\n 1 file changed, 15 insertions(+), 9 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex ab6e6a88a56..4ef66389282 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -622,7 +622,7 @@ static void check_blank_at_eof(mmfile_t *mf1, mmfile_t *mf2,\n }\n \n static void emit_line_0(struct diff_options *o,\n-\t\t\tconst char *set, unsigned reverse, const char *reset,\n+\t\t\tconst char *set_sign, const char *set, unsigned reverse, const char *reset,\n \t\t\tint first, const char *line, int len)\n {\n \tint has_trailing_newline, has_trailing_carriage_return;\n@@ -652,9 +652,15 @@ static void emit_line_0(struct diff_options *o,\n \tif (len || !nofirst) {\n \t\tif (reverse && want_color(o->use_color))\n \t\t\tfputs(GIT_COLOR_REVERSE, file);\n-\t\tfputs(set, file);\n+\t\tif (set_sign)\n+\t\t\tfputs(set_sign, file);\n \t\tif (first && !nofirst)\n \t\t\tfputc(first, file);\n+\t\tif (set && set != set_sign) {\n+\t\t\tif (set_sign)\n+\t\t\t\tfputs(reset, file);\n+\t\t\tfputs(set, file);\n+\t\t}\n \t\tfwrite(line, len, 1, file);\n \t\tfputs(reset, file);\n \t}\n@@ -667,7 +673,7 @@ static void emit_line_0(struct diff_options *o,\n static void emit_line(struct diff_options *o, const char *set, const char *reset,\n \t\t const char *line, int len)\n {\n-\temit_line_0(o, set, 0, reset, line[0], line+1, len-1);\n+\temit_line_0(o, set, NULL, 0, reset, line[0], line+1, len-1);\n }\n \n enum diff_symbol {\n@@ -1199,17 +1205,17 @@ static void emit_line_ws_markup(struct diff_options *o,\n \t}\n \n \tif (!ws && !set_sign)\n-\t\temit_line_0(o, set, 0, reset, sign, line, len);\n+\t\temit_line_0(o, set, NULL, 0, reset, sign, line, len);\n \telse if (!ws) {\n \t\t/* Emit just the prefix, then the rest. */\n-\t\temit_line_0(o, set_sign, !!set_sign, reset, sign, \"\", 0);\n-\t\temit_line_0(o, set, 0, reset, 0, line, len);\n+\t\temit_line_0(o, set_sign, NULL, !!set_sign, reset, sign, \"\", 0);\n+\t\temit_line_0(o, set, NULL, 0, reset, 0, line, len);\n \t} else if (blank_at_eof)\n \t\t/* Blank line at EOF - paint '+' as well */\n-\t\temit_line_0(o, ws, 0, reset, sign, line, len);\n+\t\temit_line_0(o, ws, NULL, 0, reset, sign, line, len);\n \telse {\n \t\t/* Emit just the prefix, then the rest. */\n-\t\temit_line_0(o, set_sign ? set_sign : set, !!set_sign, reset,\n+\t\temit_line_0(o, set_sign ? set_sign : set, NULL, !!set_sign, reset,\n \t\t\t sign, \"\", 0);\n \t\tws_check_emit(line, len, ws_rule,\n \t\t\t o->file, set, reset, ws);\n@@ -1233,7 +1239,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,\n \t\tcontext = diff_get_color_opt(o, DIFF_CONTEXT);\n \t\treset = diff_get_color_opt(o, DIFF_RESET);\n \t\tputc('\\n', o->file);\n-\t\temit_line_0(o, context, 0, reset, '\\\\',\n+\t\temit_line_0(o, context, NULL, 0, reset, '\\\\',\n \t\t\t nneof, strlen(nneof));\n \t\tbreak;\n \tcase DIFF_SYMBOL_SUBMODULE_HEADER:\n-- \n2.18.0.865.gffc8e1a3cd6-goog\n\n"},{"id":"355516","messageId":"20180814014122.30662-7-sbeller@google.com","threadId":"49095","inReplyTo":"20180814014122.30662-1-sbeller@google.com","subject":"[PATCH 6/8] diff: use emit_line_0 once per line","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-14T01:41:20Z","receivedAt":"2018-08-14T01:41:48Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"All lines that use emit_line_0 multiple times per line, are combined\ninto a single call to emit_line_0, making use of the 'set' argument.\n\nWe gain a little efficiency here, as we can omit emission of color and\naccompanying reset if 'len == 0'.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n diff.c | 16 ++++++++--------\n t/t3206-range-diff.sh | 2 +-\n 2 files changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 4ef66389282..4f430f44e64 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -656,12 +656,14 @@ static void emit_line_0(struct diff_options *o,\n \t\t\tfputs(set_sign, file);\n \t\tif (first && !nofirst)\n \t\t\tfputc(first, file);\n-\t\tif (set && set != set_sign) {\n-\t\t\tif (set_sign)\n-\t\t\t\tfputs(reset, file);\n-\t\t\tfputs(set, file);\n+\t\tif (len) {\n+\t\t\tif (set && set != set_sign) {\n+\t\t\t\tif (set_sign)\n+\t\t\t\t\tfputs(reset, file);\n+\t\t\t\tfputs(set, file);\n+\t\t\t}\n+\t\t\tfwrite(line, len, 1, file);\n \t\t}\n-\t\tfwrite(line, len, 1, file);\n \t\tfputs(reset, file);\n \t}\n \tif (has_trailing_carriage_return)\n@@ -1207,9 +1209,7 @@ static void emit_line_ws_markup(struct diff_options *o,\n \tif (!ws && !set_sign)\n \t\temit_line_0(o, set, NULL, 0, reset, sign, line, len);\n \telse if (!ws) {\n-\t\t/* Emit just the prefix, then the rest. */\n-\t\temit_line_0(o, set_sign, NULL, !!set_sign, reset, sign, \"\", 0);\n-\t\temit_line_0(o, set, NULL, 0, reset, 0, line, len);\n+\t\temit_line_0(o, set_sign, set, !!set_sign, reset, sign, line, len);\n \t} else if (blank_at_eof)\n \t\t/* Blank line at EOF - paint '+' as well */\n \t\temit_line_0(o, ws, NULL, 0, reset, sign, line, len);\ndiff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\nindex 31f6458f961..7dc7c80a1de 100755\n--- a/t/t3206-range-diff.sh\n+++ b/t/t3206-range-diff.sh\n@@ -151,7 +151,7 @@ test_expect_success 'dual-coloring' '\n \t: s/4/A/\n \t: \n \t: + Also a silly comment here!\n-\t: +\n+\t: +\n \t: diff --git a/file b/file\n \t: --- a/file\n \t: +++ b/file\n-- \n2.18.0.865.gffc8e1a3cd6-goog\n\n"},{"id":"355517","messageId":"20180814014122.30662-8-sbeller@google.com","threadId":"49095","inReplyTo":"20180814014122.30662-1-sbeller@google.com","subject":"[PATCH 7/8] diff.c: omit check for line prefix in emit_line_0","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-14T01:41:21Z","receivedAt":"2018-08-14T01:41:50Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"As the previous patch made sure we only call emit_line_0 once per line,\nwe do not need the work around introduced in f7c3b4e2d8c (diff: add an\ninternal option to dual-color diffs of diffs, 2018-08-13) that would ensure\nwe'd emit 'diff_line_prefix(o)' just once per line.\n\nBy having just one call of emit_line_0 per line, the checks are dead code.\n\nSigned-off-by: Stefan Beller \n---\n diff.c | 5 +----\n 1 file changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 4f430f44e64..7a23adf254d 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -629,10 +629,7 @@ static void emit_line_0(struct diff_options *o,\n \tint nofirst;\n \tFILE *file = o->file;\n \n-\tif (first)\n-\t\tfputs(diff_line_prefix(o), file);\n-\telse if (!len)\n-\t\treturn;\n+\tfputs(diff_line_prefix(o), file);\n \n \tif (len == 0) {\n \t\thas_trailing_newline = (first == '\\n');\n-- \n2.18.0.865.gffc8e1a3cd6-goog\n\n"},{"id":"355518","messageId":"20180814014122.30662-9-sbeller@google.com","threadId":"49095","inReplyTo":"20180814014122.30662-1-sbeller@google.com","subject":"[PATCH 8/8] diff.c: rewrite emit_line_0 more understandably","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-14T01:41:22Z","receivedAt":"2018-08-14T01:41:53Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Rewrite emit_line_0 to have fewer (nested) conditions.\n\nThe change in 'emit_line' makes sure that 'first' is never user data,\nbut always under our control, a sign or special character in the\nbeginning of the line (or 0, in which case we ignore it).\nSo from now on, let's pass only a diff marker or 0 as the 'first'\ncharacter of the line.\n\nSigned-off-by: Stefan Beller \n---\n diff.c | 73 ++++++++++++++++++++++++++++++++--------------------------\n 1 file changed, 40 insertions(+), 33 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 7a23adf254d..c5c7739ce34 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -626,43 +626,50 @@ static void emit_line_0(struct diff_options *o,\n \t\t\tint first, const char *line, int len)\n {\n \tint has_trailing_newline, has_trailing_carriage_return;\n-\tint nofirst;\n+\tint needs_reset = 0; /* at the end of the line */\n \tFILE *file = o->file;\n \n \tfputs(diff_line_prefix(o), file);\n \n-\tif (len == 0) {\n-\t\thas_trailing_newline = (first == '\\n');\n-\t\thas_trailing_carriage_return = (!has_trailing_newline &&\n-\t\t\t\t\t\t(first == '\\r'));\n-\t\tnofirst = has_trailing_newline || has_trailing_carriage_return;\n-\t} else {\n-\t\thas_trailing_newline = (len > 0 && line[len-1] == '\\n');\n-\t\tif (has_trailing_newline)\n-\t\t\tlen--;\n-\t\thas_trailing_carriage_return = (len > 0 && line[len-1] == '\\r');\n-\t\tif (has_trailing_carriage_return)\n-\t\t\tlen--;\n-\t\tnofirst = 0;\n-\t}\n-\n-\tif (len || !nofirst) {\n-\t\tif (reverse && want_color(o->use_color))\n-\t\t\tfputs(GIT_COLOR_REVERSE, file);\n-\t\tif (set_sign)\n-\t\t\tfputs(set_sign, file);\n-\t\tif (first && !nofirst)\n-\t\t\tfputc(first, file);\n-\t\tif (len) {\n-\t\t\tif (set && set != set_sign) {\n-\t\t\t\tif (set_sign)\n-\t\t\t\t\tfputs(reset, file);\n-\t\t\t\tfputs(set, file);\n-\t\t\t}\n-\t\t\tfwrite(line, len, 1, file);\n-\t\t}\n-\t\tfputs(reset, file);\n+\thas_trailing_newline = (len > 0 && line[len-1] == '\\n');\n+\tif (has_trailing_newline)\n+\t\tlen--;\n+\n+\thas_trailing_carriage_return = (len > 0 && line[len-1] == '\\r');\n+\tif (has_trailing_carriage_return)\n+\t\tlen--;\n+\n+\tif (!len && !first)\n+\t\tgoto end_of_line;\n+\n+\tif (reverse && want_color(o->use_color)) {\n+\t\tfputs(GIT_COLOR_REVERSE, file);\n+\t\tneeds_reset = 1;\n+\t}\n+\n+\tif (set_sign) {\n+\t\tfputs(set_sign, file);\n+\t\tneeds_reset = 1;\n \t}\n+\n+\tif (first)\n+\t\tfputc(first, file);\n+\n+\tif (!len)\n+\t\tgoto end_of_line;\n+\n+\tif (set) {\n+\t\tif (set_sign && set != set_sign)\n+\t\t\tfputs(reset, file);\n+\t\tfputs(set, file);\n+\t\tneeds_reset = 1;\n+\t}\n+\tfwrite(line, len, 1, file);\n+\tneeds_reset = 1; /* 'line' may contain color codes. */\n+\n+end_of_line:\n+\tif (needs_reset)\n+\t\tfputs(reset, file);\n \tif (has_trailing_carriage_return)\n \t\tfputc('\\r', file);\n \tif (has_trailing_newline)\n@@ -672,7 +679,7 @@ static void emit_line_0(struct diff_options *o,\n static void emit_line(struct diff_options *o, const char *set, const char *reset,\n \t\t const char *line, int len)\n {\n-\temit_line_0(o, set, NULL, 0, reset, line[0], line+1, len-1);\n+\temit_line_0(o, set, NULL, 0, reset, 0, line, len);\n }\n \n enum diff_symbol {\n-- \n2.18.0.865.gffc8e1a3cd6-goog\n\n"},{"id":"355550","messageId":"nycvar.QRO.7.76.6.1808141702570.71@tvgsbejvaqbjf.bet","threadId":"49095","inReplyTo":"20180814014122.30662-1-sbeller@google.com","subject":"Re: [PATCHv2 0/8] Resending sb/range-diff-colors","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-08-14T15:05:53Z","receivedAt":"2018-08-14T15:05:56Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Stefan,\n\nOn Mon, 13 Aug 2018, Stefan Beller wrote:\n\n> This applies on top of js/range-diff (a7be92acd9600), this version \n> * resolves a semantic conflict in the test\n> (Did range-diff change its output slightly?)\n\nIf by semantic conflict you mean...\n\n> 23: 6a1c7698c68 ! 23: 5701745379b t3206: add color test for range-diff --dual-color\n> @@ -23,7 +23,7 @@\n> +\t: s/4/A/\n> +\t: \n> +\t: + Also a silly comment here!\n> -+\t: +\n> ++\t: +\n\n... this, then I do not understand. This looks like a straight-up change\nin your commit. v1 of this patch did not append to the end,\nwhile v2 apparently does.\n\nAnd it looks a bit funny, indeed.\n\nIn any case, from what I see you indeed addressed all my comments (the\nneed_reset was done differently than I suggested, but still okay).\n\nCiao,\nDscho\n\n> +\t: diff --git a/file b/file\n> +\t: --- a/file\n> +\t: +++ b/file\n> 24: 7e12ece1d34 = 24: 4e56f63a5f5 diff.c: simplify caller of emit_line_0\n> 25: 74dabd6d36f ! 25: cf126556940 diff.c: reorder arguments for emit_line_ws_markup\n> @@ -3,7 +3,8 @@\n> diff.c: reorder arguments for emit_line_ws_markup\n> \n> The order shall be all colors first, then the content, flags at the end.\n> - The colors are in order.\n> + The colors are in the order of occurrence, i.e. first the color for the\n> + sign and then the color for the rest of the line.\n> \n> Signed-off-by: Stefan Beller \n> Signed-off-by: Junio C Hamano \n> 26: e304e15aa6b ! 26: 69008364cb8 diff.c: add set_sign to emit_line_0\n> @@ -2,14 +2,17 @@\n> \n> diff.c: add set_sign to emit_line_0\n> \n> - For now just change the signature, we'll reason about the actual\n> - change in a follow up patch.\n> + Split the meaning of the `set` parameter that is passed to\n> + emit_line_0()` to separate between the color of the \"sign\" (i.e.\n> + the diff marker '+', '-' or ' ' that is passed in as the `first`\n> + parameter) and the color of the rest of the line.\n> \n> - Pass 'set_sign' (which is output before the sign) and 'set' which\n> - controls the color after the first character. Hence, promote any\n> - 'set's to 'set_sign' as we want to have color before the sign\n> - for now.\n> + This changes the meaning of the `set` parameter to no longer refer\n> + to the color of the diff marker, but instead to refer to the color\n> + of the rest of the line. A value of `NULL` indicates that the rest\n> + of the line wants to be colored the same as the diff marker.\n> \n> + Helped-by: Johannes Schindelin \n> Signed-off-by: Stefan Beller \n> Signed-off-by: Junio C Hamano \n> \n> @@ -30,12 +33,15 @@\n> \t\tif (reverse && want_color(o->use_color))\n> \t\t\tfputs(GIT_COLOR_REVERSE, file);\n> -\t\tfputs(set, file);\n> -+\t\tif (set_sign && set_sign[0])\n> ++\t\tif (set_sign)\n> +\t\t\tfputs(set_sign, file);\n> \t\tif (first && !nofirst)\n> \t\t\tfputc(first, file);\n> -+\t\tif (set)\n> ++\t\tif (set && set != set_sign) {\n> ++\t\t\tif (set_sign)\n> ++\t\t\t\tfputs(reset, file);\n> +\t\t\tfputs(set, file);\n> ++\t\t}\n> \t\tfwrite(line, len, 1, file);\n> \t\tfputs(reset, file);\n> \t}\n> 27: 8d935d5212c < -: ----------- diff: use emit_line_0 once per line\n> 28: 2344aac787a < -: ----------- diff.c: compute reverse locally in emit_line_0\n> -: ----------- > 27: 5d2629281a1 diff: use emit_line_0 once per line\n> -: ----------- > 28: 5240e94a69a diff.c: omit check for line prefix in emit_line_0\n> 29: 4dc97b54a35 ! 29: 058e03a1601 diff.c: rewrite emit_line_0 more understandably\n> @@ -2,21 +2,15 @@\n> \n> diff.c: rewrite emit_line_0 more understandably\n> \n> - emit_line_0 has no nested conditions, but puts out all its arguments\n> - (if set) in order. There is still a slight confusion with set\n> - and set_sign, but let's defer that to a later patch.\n> + Rewrite emit_line_0 to have fewer (nested) conditions.\n> \n> - 'first' used be output always no matter if it was 0, but that got lost\n> - at \"diff: add an internal option to dual-color diffs of diffs\",\n> - 2018-07-21), as there we broadened the meaning of 'first' to also\n> - signal an early return.\n> -\n> - The change in 'emit_line' makes sure that 'first' is never content, but\n> - always under our control, a sign or special character in the beginning\n> - of the line (or 0, in which case we ignore it).\n> + The change in 'emit_line' makes sure that 'first' is never user data,\n> + but always under our control, a sign or special character in the\n> + beginning of the line (or 0, in which case we ignore it).\n> + So from now on, let's pass only a diff marker or 0 as the 'first'\n> + character of the line.\n> \n> Signed-off-by: Stefan Beller \n> - Signed-off-by: Junio C Hamano \n> \n> diff --git a/diff.c b/diff.c\n> --- a/diff.c\n> @@ -26,9 +20,7 @@\n> {\n> \tint has_trailing_newline, has_trailing_carriage_return;\n> -\tint nofirst;\n> - \tint reverse = !!set && !!set_sign;\n> -+\tint needs_reset = 0;\n> -+\n> ++\tint needs_reset = 0; /* at the end of the line */\n> \tFILE *file = o->file;\n> \n> \tfputs(diff_line_prefix(o), file);\n> @@ -51,15 +43,16 @@\n> -\tif (len || !nofirst) {\n> -\t\tif (reverse && want_color(o->use_color))\n> -\t\t\tfputs(GIT_COLOR_REVERSE, file);\n> --\t\tif (set_sign || set)\n> --\t\t\tfputs(set_sign ? set_sign : set, file);\n> +-\t\tif (set_sign)\n> +-\t\t\tfputs(set_sign, file);\n> -\t\tif (first && !nofirst)\n> -\t\t\tfputc(first, file);\n> -\t\tif (len) {\n> --\t\t\tif (set_sign && set && set != set_sign)\n> --\t\t\t\tfputs(reset, file);\n> --\t\t\tif (set)\n> +-\t\t\tif (set && set != set_sign) {\n> +-\t\t\t\tif (set_sign)\n> +-\t\t\t\t\tfputs(reset, file);\n> -\t\t\t\tfputs(set, file);\n> +-\t\t\t}\n> -\t\t\tfwrite(line, len, 1, file);\n> -\t\t}\n> -\t\tfputs(reset, file);\n> @@ -79,8 +72,8 @@\n> +\t\tneeds_reset = 1;\n> +\t}\n> +\n> -+\tif (set_sign || set) {\n> -+\t\tfputs(set_sign ? set_sign : set, file);\n> ++\tif (set_sign) {\n> ++\t\tfputs(set_sign, file);\n> +\t\tneeds_reset = 1;\n> \t}\n> +\n> @@ -97,7 +90,7 @@\n> +\t\tneeds_reset = 1;\n> +\t}\n> +\tfwrite(line, len, 1, file);\n> -+\tneeds_reset |= len > 0;\n> ++\tneeds_reset = 1; /* 'line' may contain color codes. */\n> +\n> +end_of_line:\n> +\tif (needs_reset)\n> @@ -109,8 +102,8 @@\n> static void emit_line(struct diff_options *o, const char *set, const char *reset,\n> \t\t const char *line, int len)\n> {\n> --\temit_line_0(o, set, NULL, reset, line[0], line+1, len-1);\n> -+\temit_line_0(o, set, NULL, reset, 0, line, len);\n> +-\temit_line_0(o, set, NULL, 0, reset, line[0], line+1, len-1);\n> ++\temit_line_0(o, set, NULL, 0, reset, 0, line, len);\n> }\n> \n> enum diff_symbol {\n> \n"},{"id":"355557","messageId":"CAGZ79kYQtKEprxVkmRW0B5Dd6FXLqVh2DQbGf_8=QDLhu=tYhA@mail.gmail.com","threadId":"49095","inReplyTo":"nycvar.QRO.7.76.6.1808141702570.71@tvgsbejvaqbjf.bet","subject":"Re: [PATCHv2 0/8] Resending sb/range-diff-colors","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-14T16:45:42Z","receivedAt":"2018-08-14T16:45:56Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Aug 14, 2018 at 8:05 AM Johannes Schindelin\n wrote:\n>\n> Hi Stefan,\n>\n> On Mon, 13 Aug 2018, Stefan Beller wrote:\n>\n> > This applies on top of js/range-diff (a7be92acd9600), this version\n> > * resolves a semantic conflict in the test\n> > (Did range-diff change its output slightly?)\n>\n> If by semantic conflict you mean...\n>\n> > 23: 6a1c7698c68 ! 23: 5701745379b t3206: add color test for range-diff --dual-color\n> > @@ -23,7 +23,7 @@\n> > + : s/4/A/\n> > + : \n> > + : + Also a silly comment here!\n> > -+ : +\n> > ++ : +\n>\n> ... this, then I do not understand. This looks like a straight-up change\n> in your commit. v1 of this patch did not append to the end,\n> while v2 apparently does.\n\nYes, I did mean this. Both before and now the test passes on top of\n js/range-diff (which was resend), so I concluded that the logic in\nrange-diff itself\nhas changed. However this is changed in a later patch to not appear any more.\n\n>\n> In any case, from what I see you indeed addressed all my comments (the\n> need_reset was done differently than I suggested, but still okay).\n\nThanks.\n\nSo unless I hear from other people I consider this series done.\n(just like range-diff is not going to be resend but built on top now)\n\nThanks,\nStefan\n"}]}