* [PATCH 0/8] Add color test for range-diff, simplify diff.c @ 2018-07-28 3:04 Stefan Beller 2018-07-28 3:04 ` [PATCH 1/8] test_decode_color: understand FAINT and ITALIC Stefan Beller ` (8 more replies) 0 siblings, 9 replies; 30+ messages in thread From: Stefan Beller @ 2018-07-28 3:04 UTC (permalink / raw) To: git; +Cc: Johannes.Schindelin, Stefan Beller This is based on origin/js/range-diff (c255a588bcd) and is also available via git fetch https://github.com/stefanbeller/git ws_cleanup-ontop-range-diff-2 This adds some color testing to range-diff and then attempts to make the code in diff.c around emit_line_0 more readable. I think we can go further bit more (by e.g. cherry-picking "ws: do not reset and set color twice" found at [1]), but that would be feature work. This series doesn't change any existing tests! [1] https://github.com/stefanbeller/git/tree/ws_cleanup-ontop-range-diff Thanks, Stefan Stefan Beller (8): test_decode_color: understand FAINT and ITALIC t3206: add color test for range-diff --dual-color diff.c: simplify caller of emit_line_0 diff.c: reorder arguments for emit_line_ws_markup diff.c: add set_sign to emit_line_0 diff: use emit_line_0 once per line diff.c: compute reverse locally in emit_line_0 diff.c: rewrite emit_line_0 more understandably diff.c | 94 +++++++++++++++++++++++------------------ t/t3206-range-diff.sh | 39 +++++++++++++++++ t/test-lib-functions.sh | 2 + 3 files changed, 93 insertions(+), 42 deletions(-) -- 2.18.0.345.g5c9ce644c3-goog ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 1/8] test_decode_color: understand FAINT and ITALIC 2018-07-28 3:04 [PATCH 0/8] Add color test for range-diff, simplify diff.c Stefan Beller @ 2018-07-28 3:04 ` Stefan Beller 2018-07-28 3:04 ` [PATCH 2/8] t3206: add color test for range-diff --dual-color Stefan Beller ` (7 subsequent siblings) 8 siblings, 0 replies; 30+ messages in thread From: Stefan Beller @ 2018-07-28 3:04 UTC (permalink / raw) To: git; +Cc: Johannes.Schindelin, Stefan Beller Signed-off-by: Stefan Beller <sbeller@google.com> --- t/test-lib-functions.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index 2b2181dca09..be8244c47b5 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -42,6 +42,8 @@ test_decode_color () { function name(n) { if (n == 0) return "RESET"; if (n == 1) return "BOLD"; + if (n == 2) return "FAINT"; + if (n == 3) return "ITALIC"; if (n == 7) return "REVERSE"; if (n == 30) return "BLACK"; if (n == 31) return "RED"; -- 2.18.0.345.g5c9ce644c3-goog ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 2/8] t3206: add color test for range-diff --dual-color 2018-07-28 3:04 [PATCH 0/8] Add color test for range-diff, simplify diff.c Stefan Beller 2018-07-28 3:04 ` [PATCH 1/8] test_decode_color: understand FAINT and ITALIC Stefan Beller @ 2018-07-28 3:04 ` Stefan Beller 2018-07-28 6:27 ` Eric Sunshine 2018-07-28 3:04 ` [PATCH 3/8] diff.c: simplify caller of emit_line_0 Stefan Beller ` (6 subsequent siblings) 8 siblings, 1 reply; 30+ messages in thread From: Stefan Beller @ 2018-07-28 3:04 UTC (permalink / raw) To: git; +Cc: Johannes.Schindelin, Stefan Beller The 'expect'ed outcome is taken by running the 'range-diff |decode'; it is not meant as guidance, rather as a documentation of the current situation. Signed-off-by: Stefan Beller <sbeller@google.com> --- t/t3206-range-diff.sh | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh index 2237c7f4af9..ef652865cd7 100755 --- a/t/t3206-range-diff.sh +++ b/t/t3206-range-diff.sh @@ -142,4 +142,43 @@ test_expect_success 'changed message' ' test_cmp expected actual ' +test_expect_success 'simple coloring' ' + q_to_tab >expect <<-EOF && + <YELLOW>1: a4b3333 = 1: f686024 s/5/A/<RESET> + <RED>2: f51d370 <RESET><YELLOW>!<RESET><GREEN> 2: 4ab067d<RESET><YELLOW> s/4/A/<RESET> + <REVERSE><CYAN>@@ -2,6 +2,8 @@<RESET> + <RESET> + s/4/A/<RESET> + <RESET> + <REVERSE><GREEN>+<RESET> <BOLD> Also a silly comment here!<RESET> + <REVERSE><GREEN>+<RESET> + diff --git a/file b/file<RESET> + <RED> --- a/file<RESET> + <GREEN> +++ b/file<RESET> + <RED>3: 0559556 <RESET><YELLOW>!<RESET><GREEN> 3: b9cb956<RESET><YELLOW> s/11/B/<RESET> + <REVERSE><CYAN>@@ -10,7 +10,7 @@<RESET> + 9<RESET> + 10<RESET> + <RED> -11<RESET> + <REVERSE><RED>-<RESET><FAINT;GREEN>+BB<RESET> + <REVERSE><GREEN>+<RESET><BOLD;GREEN>+B<RESET> + 12<RESET> + 13<RESET> + 14<RESET> + <RED>4: d966c5c <RESET><YELLOW>!<RESET><GREEN> 4: 8add5f1<RESET><YELLOW> s/12/B/<RESET> + <REVERSE><CYAN>@@ -8,7 +8,7 @@<RESET> + <CYAN> @@<RESET> + 9<RESET> + 10<RESET> + <REVERSE><RED>-<RESET><FAINT> BB<RESET> + <REVERSE><GREEN>+<RESET> <BOLD>B<RESET> + <RED> -12<RESET> + <GREEN> +B<RESET> + 13<RESET> + EOF + git range-diff changed...changed-message --color --dual-color >actual.raw && + test_decode_color >actual <actual.raw && + test_cmp expect actual +' + test_done -- 2.18.0.345.g5c9ce644c3-goog ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH 2/8] t3206: add color test for range-diff --dual-color 2018-07-28 3:04 ` [PATCH 2/8] t3206: add color test for range-diff --dual-color Stefan Beller @ 2018-07-28 6:27 ` Eric Sunshine 2018-07-30 19:55 ` Stefan Beller 0 siblings, 1 reply; 30+ messages in thread From: Eric Sunshine @ 2018-07-28 6:27 UTC (permalink / raw) To: Stefan Beller; +Cc: Git List, Johannes Schindelin On Fri, Jul 27, 2018 at 11:05 PM Stefan Beller <sbeller@google.com> wrote: > The 'expect'ed outcome is taken by running the 'range-diff |decode'; > it is not meant as guidance, rather as a documentation of the current > situation. I'm not really sure what this is trying to say. It seems _too_ brief. Did you want a space after the vertical bar before "decode"? > Signed-off-by: Stefan Beller <sbeller@google.com> > --- > diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh > +test_expect_success 'simple coloring' ' > + q_to_tab >expect <<-EOF && Why 'q_to_tab'? I don't see any "q"'s in the body. I also don't see any variable interpolation in the body, so maybe you want -\EOF instead? ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/8] t3206: add color test for range-diff --dual-color 2018-07-28 6:27 ` Eric Sunshine @ 2018-07-30 19:55 ` Stefan Beller 0 siblings, 0 replies; 30+ messages in thread From: Stefan Beller @ 2018-07-30 19:55 UTC (permalink / raw) To: Eric Sunshine; +Cc: git, Johannes Schindelin On Fri, Jul 27, 2018 at 11:28 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > > On Fri, Jul 27, 2018 at 11:05 PM Stefan Beller <sbeller@google.com> wrote: > > The 'expect'ed outcome is taken by running the 'range-diff |decode'; > > it is not meant as guidance, rather as a documentation of the current > > situation. > > I'm not really sure what this is trying to say. It seems _too_ brief. > > Did you want a space after the vertical bar before "decode"? I am trying to say that this patch was generated by inserting a "true && test_pause" first and then inside that paused test, I just run source <path>/t/test-lib-functions.sh git range-diff changed...changed-message --color --dual-color \ | test_decode_color and saved that as the expected outcome, I was prepared to massage it gently by s/<TAB>/Q/ but that was not needed, but I forgot the q_to_tab in place. By adding the test this way, I just want to state "I observed the functionality as produced in this patch". I do not want to endorse the coloring scheme or claim it is good (it is, but I also still have nits to pick). So I tried to briefly say that the test is essentially "autogenerated" by just observing output at that point in time. > > > Signed-off-by: Stefan Beller <sbeller@google.com> > > --- > > diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh > > +test_expect_success 'simple coloring' ' > > + q_to_tab >expect <<-EOF && > > Why 'q_to_tab'? I don't see any "q"'s in the body. > > I also don't see any variable interpolation in the body, so maybe you > want -\EOF instead? All good suggestions! Thanks for the review, I'll incorporate them. Thanks, Stefan ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 3/8] diff.c: simplify caller of emit_line_0 2018-07-28 3:04 [PATCH 0/8] Add color test for range-diff, simplify diff.c Stefan Beller 2018-07-28 3:04 ` [PATCH 1/8] test_decode_color: understand FAINT and ITALIC Stefan Beller 2018-07-28 3:04 ` [PATCH 2/8] t3206: add color test for range-diff --dual-color Stefan Beller @ 2018-07-28 3:04 ` Stefan Beller 2018-07-28 3:04 ` [PATCH 4/8] diff.c: reorder arguments for emit_line_ws_markup Stefan Beller ` (5 subsequent siblings) 8 siblings, 0 replies; 30+ messages in thread From: Stefan Beller @ 2018-07-28 3:04 UTC (permalink / raw) To: git; +Cc: Johannes.Schindelin, Stefan Beller Due to the previous condition we know "set_sign != NULL" at that point. Signed-off-by: Stefan Beller <sbeller@google.com> --- diff.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/diff.c b/diff.c index 272b0b93834..f7251c89cbb 100644 --- a/diff.c +++ b/diff.c @@ -997,8 +997,7 @@ static void emit_line_ws_markup(struct diff_options *o, emit_line_0(o, set, 0, reset, sign, line, len); else if (!ws) { /* Emit just the prefix, then the rest. */ - emit_line_0(o, set_sign ? set_sign : set, !!set_sign, reset, - sign, "", 0); + emit_line_0(o, set_sign, !!set_sign, reset, sign, "", 0); emit_line_0(o, set, 0, reset, 0, line, len); } else if (blank_at_eof) /* Blank line at EOF - paint '+' as well */ -- 2.18.0.345.g5c9ce644c3-goog ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 4/8] diff.c: reorder arguments for emit_line_ws_markup 2018-07-28 3:04 [PATCH 0/8] Add color test for range-diff, simplify diff.c Stefan Beller ` (2 preceding siblings ...) 2018-07-28 3:04 ` [PATCH 3/8] diff.c: simplify caller of emit_line_0 Stefan Beller @ 2018-07-28 3:04 ` Stefan Beller 2018-07-28 3:04 ` [PATCH 5/8] diff.c: add set_sign to emit_line_0 Stefan Beller ` (4 subsequent siblings) 8 siblings, 0 replies; 30+ messages in thread From: Stefan Beller @ 2018-07-28 3:04 UTC (permalink / raw) To: git; +Cc: Johannes.Schindelin, Stefan Beller The order shall be all colors first, then the content, flags at the end. The colors are in order. Signed-off-by: Stefan Beller <sbeller@google.com> --- diff.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/diff.c b/diff.c index f7251c89cbb..8fd2171d808 100644 --- a/diff.c +++ b/diff.c @@ -980,9 +980,9 @@ static void dim_moved_lines(struct diff_options *o) } static void emit_line_ws_markup(struct diff_options *o, - const char *set, const char *reset, - const char *line, int len, - const char *set_sign, char sign, + const char *set_sign, const char *set, + const char *reset, + char sign, const char *line, int len, unsigned ws_rule, int blank_at_eof) { const char *ws = NULL; @@ -1066,7 +1066,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o, else if (c == '-') set = diff_get_color_opt(o, DIFF_FILE_OLD); } - emit_line_ws_markup(o, set, reset, line, len, set_sign, ' ', + emit_line_ws_markup(o, set_sign, set, reset, ' ', line, len, flags & (DIFF_SYMBOL_CONTENT_WS_MASK), 0); break; case DIFF_SYMBOL_PLUS: @@ -1109,7 +1109,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o, set = diff_get_color_opt(o, DIFF_CONTEXT_BOLD); flags |= WS_IGNORE_FIRST_SPACE; } - emit_line_ws_markup(o, set, reset, line, len, set_sign, '+', + emit_line_ws_markup(o, set_sign, set, reset, '+', line, len, flags & DIFF_SYMBOL_CONTENT_WS_MASK, flags & DIFF_SYMBOL_CONTENT_BLANK_LINE_EOF); break; @@ -1152,7 +1152,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o, else set = diff_get_color_opt(o, DIFF_CONTEXT_DIM); } - emit_line_ws_markup(o, set, reset, line, len, set_sign, '-', + emit_line_ws_markup(o, set_sign, set, reset, '-', line, len, flags & DIFF_SYMBOL_CONTENT_WS_MASK, 0); break; case DIFF_SYMBOL_WORDS_PORCELAIN: -- 2.18.0.345.g5c9ce644c3-goog ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 5/8] diff.c: add set_sign to emit_line_0 2018-07-28 3:04 [PATCH 0/8] Add color test for range-diff, simplify diff.c Stefan Beller ` (3 preceding siblings ...) 2018-07-28 3:04 ` [PATCH 4/8] diff.c: reorder arguments for emit_line_ws_markup Stefan Beller @ 2018-07-28 3:04 ` Stefan Beller 2018-07-28 6:30 ` Eric Sunshine 2018-07-28 3:04 ` [PATCH 6/8] diff: use emit_line_0 once per line Stefan Beller ` (3 subsequent siblings) 8 siblings, 1 reply; 30+ messages in thread From: Stefan Beller @ 2018-07-28 3:04 UTC (permalink / raw) To: git; +Cc: Johannes.Schindelin, Stefan Beller For now just change the signature, we'll reason about the actual change in a follow up patch. Pass set_sign (which is output before the sign) and set that is setting the color after the sign. Hence, promote any 'set's to set_sign as we want to have color before the sign for now. Signed-off-by: Stefan Beller <sbeller@google.com> --- diff.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/diff.c b/diff.c index 8fd2171d808..a36ed92c54c 100644 --- a/diff.c +++ b/diff.c @@ -576,7 +576,7 @@ static void check_blank_at_eof(mmfile_t *mf1, mmfile_t *mf2, } static void emit_line_0(struct diff_options *o, - const char *set, unsigned reverse, const char *reset, + const char *set_sign, const char *set, unsigned reverse, const char *reset, int first, const char *line, int len) { int has_trailing_newline, has_trailing_carriage_return; @@ -606,9 +606,12 @@ static void emit_line_0(struct diff_options *o, if (len || !nofirst) { if (reverse && want_color(o->use_color)) fputs(GIT_COLOR_REVERSE, file); - fputs(set, file); + if (set_sign && set_sign[0]) + fputs(set_sign, file); if (first && !nofirst) fputc(first, file); + if (set) + fputs(set, file); fwrite(line, len, 1, file); fputs(reset, file); } @@ -621,7 +624,7 @@ static void emit_line_0(struct diff_options *o, static void emit_line(struct diff_options *o, const char *set, const char *reset, const char *line, int len) { - emit_line_0(o, set, 0, reset, line[0], line+1, len-1); + emit_line_0(o, set, NULL, 0, reset, line[0], line+1, len-1); } enum diff_symbol { @@ -994,17 +997,17 @@ static void emit_line_ws_markup(struct diff_options *o, } if (!ws && !set_sign) - emit_line_0(o, set, 0, reset, sign, line, len); + emit_line_0(o, set, NULL, 0, reset, sign, line, len); else if (!ws) { /* Emit just the prefix, then the rest. */ - emit_line_0(o, set_sign, !!set_sign, reset, sign, "", 0); - emit_line_0(o, set, 0, reset, 0, line, len); + emit_line_0(o, set_sign, NULL, !!set_sign, reset, sign, "", 0); + emit_line_0(o, set, NULL, 0, reset, 0, line, len); } else if (blank_at_eof) /* Blank line at EOF - paint '+' as well */ - emit_line_0(o, ws, 0, reset, sign, line, len); + emit_line_0(o, ws, NULL, 0, reset, sign, line, len); else { /* Emit just the prefix, then the rest. */ - emit_line_0(o, set_sign ? set_sign : set, !!set_sign, reset, + emit_line_0(o, set_sign ? set_sign : set, NULL, !!set_sign, reset, sign, "", 0); ws_check_emit(line, len, ws_rule, o->file, set, reset, ws); @@ -1028,7 +1031,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o, context = diff_get_color_opt(o, DIFF_CONTEXT); reset = diff_get_color_opt(o, DIFF_RESET); putc('\n', o->file); - emit_line_0(o, context, 0, reset, '\\', + emit_line_0(o, context, NULL, 0, reset, '\\', nneof, strlen(nneof)); break; case DIFF_SYMBOL_SUBMODULE_HEADER: -- 2.18.0.345.g5c9ce644c3-goog ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH 5/8] diff.c: add set_sign to emit_line_0 2018-07-28 3:04 ` [PATCH 5/8] diff.c: add set_sign to emit_line_0 Stefan Beller @ 2018-07-28 6:30 ` Eric Sunshine 0 siblings, 0 replies; 30+ messages in thread From: Eric Sunshine @ 2018-07-28 6:30 UTC (permalink / raw) To: Stefan Beller; +Cc: Git List, Johannes Schindelin On Fri, Jul 27, 2018 at 11:05 PM Stefan Beller <sbeller@google.com> wrote: > For now just change the signature, we'll reason about the actual > change in a follow up patch. > > Pass set_sign (which is output before the sign) and set that is setting > the color after the sign. Hence, promote any 'set's to set_sign as > we want to have color before the sign for now. ECANTPARSE: "and set that is setting" ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 6/8] diff: use emit_line_0 once per line 2018-07-28 3:04 [PATCH 0/8] Add color test for range-diff, simplify diff.c Stefan Beller ` (4 preceding siblings ...) 2018-07-28 3:04 ` [PATCH 5/8] diff.c: add set_sign to emit_line_0 Stefan Beller @ 2018-07-28 3:04 ` Stefan Beller 2018-07-28 3:04 ` [PATCH 7/8] diff.c: compute reverse locally in emit_line_0 Stefan Beller ` (2 subsequent siblings) 8 siblings, 0 replies; 30+ messages in thread From: Stefan Beller @ 2018-07-28 3:04 UTC (permalink / raw) To: git; +Cc: Johannes.Schindelin, Stefan Beller All lines that use emit_line_0 multiple times per line, are combined into a single call to emit_line_0, making use of the 'set' argument. Signed-off-by: Stefan Beller <sbeller@google.com> --- diff.c | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/diff.c b/diff.c index a36ed92c54c..fdad7ffdd77 100644 --- a/diff.c +++ b/diff.c @@ -583,10 +583,7 @@ static void emit_line_0(struct diff_options *o, int nofirst; FILE *file = o->file; - if (first) - fputs(diff_line_prefix(o), file); - else if (!len) - return; + fputs(diff_line_prefix(o), file); if (len == 0) { has_trailing_newline = (first == '\n'); @@ -606,13 +603,17 @@ static void emit_line_0(struct diff_options *o, if (len || !nofirst) { if (reverse && want_color(o->use_color)) fputs(GIT_COLOR_REVERSE, file); - if (set_sign && set_sign[0]) - fputs(set_sign, file); + if (set_sign || set) + fputs(set_sign ? set_sign : set, file); if (first && !nofirst) fputc(first, file); - if (set) - fputs(set, file); - fwrite(line, len, 1, file); + if (len) { + if (set_sign && set && set != set_sign) + fputs(reset, file); + if (set) + fputs(set, file); + fwrite(line, len, 1, file); + } fputs(reset, file); } if (has_trailing_carriage_return) @@ -999,16 +1000,13 @@ static void emit_line_ws_markup(struct diff_options *o, if (!ws && !set_sign) emit_line_0(o, set, NULL, 0, reset, sign, line, len); else if (!ws) { - /* Emit just the prefix, then the rest. */ - emit_line_0(o, set_sign, NULL, !!set_sign, reset, sign, "", 0); - emit_line_0(o, set, NULL, 0, reset, 0, line, len); + emit_line_0(o, set_sign, set, !!set_sign, reset, sign, line, len); } else if (blank_at_eof) /* Blank line at EOF - paint '+' as well */ emit_line_0(o, ws, NULL, 0, reset, sign, line, len); else { /* Emit just the prefix, then the rest. */ - emit_line_0(o, set_sign ? set_sign : set, NULL, !!set_sign, reset, - sign, "", 0); + emit_line_0(o, set_sign, set, !!set_sign, reset, sign, "", 0); ws_check_emit(line, len, ws_rule, o->file, set, reset, ws); } -- 2.18.0.345.g5c9ce644c3-goog ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 7/8] diff.c: compute reverse locally in emit_line_0 2018-07-28 3:04 [PATCH 0/8] Add color test for range-diff, simplify diff.c Stefan Beller ` (5 preceding siblings ...) 2018-07-28 3:04 ` [PATCH 6/8] diff: use emit_line_0 once per line Stefan Beller @ 2018-07-28 3:04 ` Stefan Beller 2018-07-28 3:04 ` [PATCH 8/8] diff.c: rewrite emit_line_0 more understandably Stefan Beller 2018-07-31 0:31 ` [PATCHv2 0/8] Add color test for range-diff, simplify diff.c Stefan Beller 8 siblings, 0 replies; 30+ messages in thread From: Stefan Beller @ 2018-07-28 3:04 UTC (permalink / raw) To: git; +Cc: Johannes.Schindelin, Stefan Beller Signed-off-by: Stefan Beller <sbeller@google.com> --- diff.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/diff.c b/diff.c index fdad7ffdd77..f565a2c0c2b 100644 --- a/diff.c +++ b/diff.c @@ -576,11 +576,12 @@ static void check_blank_at_eof(mmfile_t *mf1, mmfile_t *mf2, } static void emit_line_0(struct diff_options *o, - const char *set_sign, const char *set, unsigned reverse, const char *reset, + const char *set_sign, const char *set, const char *reset, int first, const char *line, int len) { int has_trailing_newline, has_trailing_carriage_return; int nofirst; + int reverse = !!set && !!set_sign; FILE *file = o->file; fputs(diff_line_prefix(o), file); @@ -625,7 +626,7 @@ static void emit_line_0(struct diff_options *o, static void emit_line(struct diff_options *o, const char *set, const char *reset, const char *line, int len) { - emit_line_0(o, set, NULL, 0, reset, line[0], line+1, len-1); + emit_line_0(o, set, NULL, reset, line[0], line+1, len-1); } enum diff_symbol { @@ -998,15 +999,15 @@ static void emit_line_ws_markup(struct diff_options *o, } if (!ws && !set_sign) - emit_line_0(o, set, NULL, 0, reset, sign, line, len); + emit_line_0(o, set, NULL, reset, sign, line, len); else if (!ws) { - emit_line_0(o, set_sign, set, !!set_sign, reset, sign, line, len); + emit_line_0(o, set_sign, set, reset, sign, line, len); } else if (blank_at_eof) /* Blank line at EOF - paint '+' as well */ - emit_line_0(o, ws, NULL, 0, reset, sign, line, len); + emit_line_0(o, ws, NULL, reset, sign, line, len); else { /* Emit just the prefix, then the rest. */ - emit_line_0(o, set_sign, set, !!set_sign, reset, sign, "", 0); + emit_line_0(o, set_sign, set, reset, sign, "", 0); ws_check_emit(line, len, ws_rule, o->file, set, reset, ws); } @@ -1029,7 +1030,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o, context = diff_get_color_opt(o, DIFF_CONTEXT); reset = diff_get_color_opt(o, DIFF_RESET); putc('\n', o->file); - emit_line_0(o, context, NULL, 0, reset, '\\', + emit_line_0(o, context, NULL, reset, '\\', nneof, strlen(nneof)); break; case DIFF_SYMBOL_SUBMODULE_HEADER: -- 2.18.0.345.g5c9ce644c3-goog ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 8/8] diff.c: rewrite emit_line_0 more understandably 2018-07-28 3:04 [PATCH 0/8] Add color test for range-diff, simplify diff.c Stefan Beller ` (6 preceding siblings ...) 2018-07-28 3:04 ` [PATCH 7/8] diff.c: compute reverse locally in emit_line_0 Stefan Beller @ 2018-07-28 3:04 ` Stefan Beller 2018-07-28 6:33 ` Eric Sunshine 2018-07-31 0:31 ` [PATCHv2 0/8] Add color test for range-diff, simplify diff.c Stefan Beller 8 siblings, 1 reply; 30+ messages in thread From: Stefan Beller @ 2018-07-28 3:04 UTC (permalink / raw) To: git; +Cc: Johannes.Schindelin, Stefan Beller emit_line_0 has no nested conditions, but puts out all its arguments (if set) in order. There is still a slight confusion with set and set_sign, but let's defer that to a later patch. 'first' used be output always no matter if it was 0, but that got lost got lost at e8c285c4f9c (diff: add an internal option to dual-color diffs of diffs, 2018-07-21), as there we broadened the meaning of 'first' to also signal an early return. The change in 'emit_line' makes sure that 'first' is never content, but always under our control, a sign or special character in the beginning of the line (or 0, in which case we ignore it). Signed-off-by: Stefan Beller <sbeller@google.com> --- diff.c | 73 +++++++++++++++++++++++++++++++++------------------------- 1 file changed, 41 insertions(+), 32 deletions(-) diff --git a/diff.c b/diff.c index f565a2c0c2b..2bd4d3d6839 100644 --- a/diff.c +++ b/diff.c @@ -580,43 +580,52 @@ static void emit_line_0(struct diff_options *o, int first, const char *line, int len) { int has_trailing_newline, has_trailing_carriage_return; - int nofirst; int reverse = !!set && !!set_sign; + int needs_reset = 0; + FILE *file = o->file; fputs(diff_line_prefix(o), file); - if (len == 0) { - has_trailing_newline = (first == '\n'); - has_trailing_carriage_return = (!has_trailing_newline && - (first == '\r')); - nofirst = has_trailing_newline || has_trailing_carriage_return; - } else { - has_trailing_newline = (len > 0 && line[len-1] == '\n'); - if (has_trailing_newline) - len--; - has_trailing_carriage_return = (len > 0 && line[len-1] == '\r'); - if (has_trailing_carriage_return) - len--; - nofirst = 0; - } - - if (len || !nofirst) { - if (reverse && want_color(o->use_color)) - fputs(GIT_COLOR_REVERSE, file); - if (set_sign || set) - fputs(set_sign ? set_sign : set, file); - if (first && !nofirst) - fputc(first, file); - if (len) { - if (set_sign && set && set != set_sign) - fputs(reset, file); - if (set) - fputs(set, file); - fwrite(line, len, 1, file); - } - fputs(reset, file); + has_trailing_newline = (len > 0 && line[len-1] == '\n'); + if (has_trailing_newline) + len--; + + has_trailing_carriage_return = (len > 0 && line[len-1] == '\r'); + if (has_trailing_carriage_return) + len--; + + if (!len && !first) + goto end_of_line; + + if (reverse && want_color(o->use_color)) { + fputs(GIT_COLOR_REVERSE, file); + needs_reset = 1; + } + + if (set_sign || set) { + fputs(set_sign ? set_sign : set, file); + needs_reset = 1; } + + if (first) + fputc(first, file); + + if (!len) + goto end_of_line; + + if (set) { + if (set_sign && set != set_sign) + fputs(reset, file); + fputs(set, file); + needs_reset = 1; + } + fwrite(line, len, 1, file); + needs_reset |= len > 0; + +end_of_line: + if (needs_reset) + fputs(reset, file); if (has_trailing_carriage_return) fputc('\r', file); if (has_trailing_newline) @@ -626,7 +635,7 @@ static void emit_line_0(struct diff_options *o, static void emit_line(struct diff_options *o, const char *set, const char *reset, const char *line, int len) { - emit_line_0(o, set, NULL, reset, line[0], line+1, len-1); + emit_line_0(o, set, NULL, reset, 0, line, len); } enum diff_symbol { -- 2.18.0.345.g5c9ce644c3-goog ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH 8/8] diff.c: rewrite emit_line_0 more understandably 2018-07-28 3:04 ` [PATCH 8/8] diff.c: rewrite emit_line_0 more understandably Stefan Beller @ 2018-07-28 6:33 ` Eric Sunshine 0 siblings, 0 replies; 30+ messages in thread From: Eric Sunshine @ 2018-07-28 6:33 UTC (permalink / raw) To: Stefan Beller; +Cc: Git List, Johannes Schindelin On Fri, Jul 27, 2018 at 11:05 PM Stefan Beller <sbeller@google.com> wrote: > emit_line_0 has no nested conditions, but puts out all its arguments > (if set) in order. There is still a slight confusion with set > and set_sign, but let's defer that to a later patch. > > 'first' used be output always no matter if it was 0, but that got lost > got lost at e8c285c4f9c (diff: add an internal option to dual-color > diffs of diffs, 2018-07-21), as there we broadened the meaning of 'first' > to also signal an early return. s/used be/used to be/ redundant "got lost" > The change in 'emit_line' makes sure that 'first' is never content, but > always under our control, a sign or special character in the beginning > of the line (or 0, in which case we ignore it). > > Signed-off-by: Stefan Beller <sbeller@google.com> ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCHv2 0/8] Add color test for range-diff, simplify diff.c 2018-07-28 3:04 [PATCH 0/8] Add color test for range-diff, simplify diff.c Stefan Beller ` (7 preceding siblings ...) 2018-07-28 3:04 ` [PATCH 8/8] diff.c: rewrite emit_line_0 more understandably Stefan Beller @ 2018-07-31 0:31 ` Stefan Beller 2018-07-31 0:31 ` [PATCH 1/8] test_decode_color: understand FAINT and ITALIC Stefan Beller ` (8 more replies) 8 siblings, 9 replies; 30+ messages in thread From: Stefan Beller @ 2018-07-31 0:31 UTC (permalink / raw) To: sbeller; +Cc: Johannes.Schindelin, git, sunshine addressed all of Erics feedback: * reworded commit messages * dropped q_to_tab and use cat instead * use -\EOF isntead of -EOF Thanks, Stefan Stefan Beller (8): test_decode_color: understand FAINT and ITALIC t3206: add color test for range-diff --dual-color diff.c: simplify caller of emit_line_0 diff.c: reorder arguments for emit_line_ws_markup diff.c: add set_sign to emit_line_0 diff: use emit_line_0 once per line diff.c: compute reverse locally in emit_line_0 diff.c: rewrite emit_line_0 more understandably diff.c | 94 +++++++++++++++++++++++------------------ t/t3206-range-diff.sh | 39 +++++++++++++++++ t/test-lib-functions.sh | 2 + 3 files changed, 93 insertions(+), 42 deletions(-) ./git-range-diff ws_cleanup-ontop-range-diff-2@{2.hours.ago}...HEAD >>0000-cover-letter.patch [dropped changes to range-diff itself] 13: a02ea020ae7 ! 13: 16f71b43f48 t3206: add color test for range-diff --dual-color @@ -2,9 +2,7 @@ t3206: add color test for range-diff --dual-color - The 'expect'ed outcome is taken by running the 'range-diff |decode'; - it is not meant as guidance, rather as a documentation of the current - situation. + The 'expect'ed outcome has been taken by running the 'range-diff | decode'. Signed-off-by: Stefan Beller <sbeller@google.com> @@ -15,8 +13,8 @@ test_cmp expected actual ' -+test_expect_success 'simple coloring' ' -+ q_to_tab >expect <<-EOF && ++test_expect_success 'dual-coloring' ' ++ cat >expect <<-\EOF && + <YELLOW>1: a4b3333 = 1: f686024 s/5/A/<RESET> + <RED>2: f51d370 <RESET><YELLOW>!<RESET><GREEN> 2: 4ab067d<RESET><YELLOW> s/4/A/<RESET> + <REVERSE><CYAN>@@ -2,6 +2,8 @@<RESET> 14: c8734075229 = 14: abd1ec80608 diff.c: simplify caller of emit_line_0 15: ba98acffcda = 15: bc29037f4f0 diff.c: reorder arguments for emit_line_ws_markup 16: 5a576baeb49 ! 16: 8f6ee340f1e diff.c: add set_sign to emit_line_0 @@ -5,9 +5,10 @@ For now just change the signature, we'll reason about the actual change in a follow up patch. - Pass set_sign (which is output before the sign) and set that is setting - the color after the sign. Hence, promote any 'set's to set_sign as - we want to have color before the sign for now. + Pass 'set_sign' (which is output before the sign) and 'set' which + controls the color after the first character. Hence, promote any + 'set's to 'set_sign' as we want to have color before the sign + for now. Signed-off-by: Stefan Beller <sbeller@google.com> 17: 4e2d5a4c7f3 = 17: 0ab5920a9ab diff: use emit_line_0 once per line 18: 460713e1c3c = 18: 2d05ebdd280 diff.c: compute reverse locally in emit_line_0 19: e442d722b7f ! 19: 001e6042d81 diff.c: rewrite emit_line_0 more understandably @@ -7,9 +7,9 @@ and set_sign, but let's defer that to a later patch. 'first' used be output always no matter if it was 0, but that got lost - got lost at e8c285c4f9c (diff: add an internal option to dual-color - diffs of diffs, 2018-07-21), as there we broadened the meaning of 'first' - to also signal an early return. + at "diff: add an internal option to dual-color diffs of diffs", + 2018-07-21), as there we broadened the meaning of 'first' to also + signal an early return. The change in 'emit_line' makes sure that 'first' is never content, but always under our control, a sign or special character in the beginning ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 1/8] test_decode_color: understand FAINT and ITALIC 2018-07-31 0:31 ` [PATCHv2 0/8] Add color test for range-diff, simplify diff.c Stefan Beller @ 2018-07-31 0:31 ` Stefan Beller 2018-07-31 0:31 ` [PATCH 2/8] t3206: add color test for range-diff --dual-color Stefan Beller ` (7 subsequent siblings) 8 siblings, 0 replies; 30+ messages in thread From: Stefan Beller @ 2018-07-31 0:31 UTC (permalink / raw) To: sbeller; +Cc: Johannes.Schindelin, git, sunshine Signed-off-by: Stefan Beller <sbeller@google.com> --- t/test-lib-functions.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index 2b2181dca09..be8244c47b5 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -42,6 +42,8 @@ test_decode_color () { function name(n) { if (n == 0) return "RESET"; if (n == 1) return "BOLD"; + if (n == 2) return "FAINT"; + if (n == 3) return "ITALIC"; if (n == 7) return "REVERSE"; if (n == 30) return "BLACK"; if (n == 31) return "RED"; -- 2.18.0.132.g195c49a2227 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 2/8] t3206: add color test for range-diff --dual-color 2018-07-31 0:31 ` [PATCHv2 0/8] Add color test for range-diff, simplify diff.c Stefan Beller 2018-07-31 0:31 ` [PATCH 1/8] test_decode_color: understand FAINT and ITALIC Stefan Beller @ 2018-07-31 0:31 ` Stefan Beller 2018-07-31 20:51 ` Junio C Hamano 2018-07-31 0:31 ` [PATCH 3/8] diff.c: simplify caller of emit_line_0 Stefan Beller ` (6 subsequent siblings) 8 siblings, 1 reply; 30+ messages in thread From: Stefan Beller @ 2018-07-31 0:31 UTC (permalink / raw) To: sbeller; +Cc: Johannes.Schindelin, git, sunshine The 'expect'ed outcome has been taken by running the 'range-diff | decode'. Signed-off-by: Stefan Beller <sbeller@google.com> --- t/t3206-range-diff.sh | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh index 2237c7f4af9..019724e61a0 100755 --- a/t/t3206-range-diff.sh +++ b/t/t3206-range-diff.sh @@ -142,4 +142,43 @@ test_expect_success 'changed message' ' test_cmp expected actual ' +test_expect_success 'dual-coloring' ' + cat >expect <<-\EOF && + <YELLOW>1: a4b3333 = 1: f686024 s/5/A/<RESET> + <RED>2: f51d370 <RESET><YELLOW>!<RESET><GREEN> 2: 4ab067d<RESET><YELLOW> s/4/A/<RESET> + <REVERSE><CYAN>@@ -2,6 +2,8 @@<RESET> + <RESET> + s/4/A/<RESET> + <RESET> + <REVERSE><GREEN>+<RESET> <BOLD> Also a silly comment here!<RESET> + <REVERSE><GREEN>+<RESET> + diff --git a/file b/file<RESET> + <RED> --- a/file<RESET> + <GREEN> +++ b/file<RESET> + <RED>3: 0559556 <RESET><YELLOW>!<RESET><GREEN> 3: b9cb956<RESET><YELLOW> s/11/B/<RESET> + <REVERSE><CYAN>@@ -10,7 +10,7 @@<RESET> + 9<RESET> + 10<RESET> + <RED> -11<RESET> + <REVERSE><RED>-<RESET><FAINT;GREEN>+BB<RESET> + <REVERSE><GREEN>+<RESET><BOLD;GREEN>+B<RESET> + 12<RESET> + 13<RESET> + 14<RESET> + <RED>4: d966c5c <RESET><YELLOW>!<RESET><GREEN> 4: 8add5f1<RESET><YELLOW> s/12/B/<RESET> + <REVERSE><CYAN>@@ -8,7 +8,7 @@<RESET> + <CYAN> @@<RESET> + 9<RESET> + 10<RESET> + <REVERSE><RED>-<RESET><FAINT> BB<RESET> + <REVERSE><GREEN>+<RESET> <BOLD>B<RESET> + <RED> -12<RESET> + <GREEN> +B<RESET> + 13<RESET> + EOF + git range-diff changed...changed-message --color --dual-color >actual.raw && + test_decode_color >actual <actual.raw && + test_cmp expect actual +' + test_done -- 2.18.0.132.g195c49a2227 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH 2/8] t3206: add color test for range-diff --dual-color 2018-07-31 0:31 ` [PATCH 2/8] t3206: add color test for range-diff --dual-color Stefan Beller @ 2018-07-31 20:51 ` Junio C Hamano 0 siblings, 0 replies; 30+ messages in thread From: Junio C Hamano @ 2018-07-31 20:51 UTC (permalink / raw) To: Stefan Beller; +Cc: Johannes.Schindelin, git, sunshine Stefan Beller <sbeller@google.com> writes: > The 'expect'ed outcome has been taken by running the 'range-diff | decode'. > > Signed-off-by: Stefan Beller <sbeller@google.com> > --- > t/t3206-range-diff.sh | 39 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh > index 2237c7f4af9..019724e61a0 100755 > --- a/t/t3206-range-diff.sh > +++ b/t/t3206-range-diff.sh > @@ -142,4 +142,43 @@ test_expect_success 'changed message' ' > test_cmp expected actual > ' > > +test_expect_success 'dual-coloring' ' > + cat >expect <<-\EOF && It is a good idea to use something like "sed -e 's/^|//'" instead of "cat" here; that way allows you to mark the left-edge of the data with "|", for a test vector like this one that has a line that would otherwise violate the whitespace style rules. An inferiour alternative would be to add .gitaddtibute entry to make this file exempt from indent-with-tab rule, but even in this 40-line block there only is one line that requires such a workaround, and it won't help the initial application of this patch to get modified when applied with "am --whitespace=fix". > + <YELLOW>1: a4b3333 = 1: f686024 s/5/A/<RESET> > + <RED>2: f51d370 <RESET><YELLOW>!<RESET><GREEN> 2: 4ab067d<RESET><YELLOW> s/4/A/<RESET> > + <REVERSE><CYAN>@@ -2,6 +2,8 @@<RESET> > + <RESET> > + s/4/A/<RESET> > + <RESET> > + <REVERSE><GREEN>+<RESET> <BOLD> Also a silly comment here!<RESET> > + <REVERSE><GREEN>+<RESET> > + diff --git a/file b/file<RESET> > + <RED> --- a/file<RESET> > + <GREEN> +++ b/file<RESET> > + <RED>3: 0559556 <RESET><YELLOW>!<RESET><GREEN> 3: b9cb956<RESET><YELLOW> s/11/B/<RESET> > + <REVERSE><CYAN>@@ -10,7 +10,7 @@<RESET> > + 9<RESET> > + 10<RESET> > + <RED> -11<RESET> > + <REVERSE><RED>-<RESET><FAINT;GREEN>+BB<RESET> > + <REVERSE><GREEN>+<RESET><BOLD;GREEN>+B<RESET> > + 12<RESET> > + 13<RESET> > + 14<RESET> > + <RED>4: d966c5c <RESET><YELLOW>!<RESET><GREEN> 4: 8add5f1<RESET><YELLOW> s/12/B/<RESET> > + <REVERSE><CYAN>@@ -8,7 +8,7 @@<RESET> > + <CYAN> @@<RESET> > + 9<RESET> > + 10<RESET> > + <REVERSE><RED>-<RESET><FAINT> BB<RESET> > + <REVERSE><GREEN>+<RESET> <BOLD>B<RESET> > + <RED> -12<RESET> > + <GREEN> +B<RESET> > + 13<RESET> > + EOF > + git range-diff changed...changed-message --color --dual-color >actual.raw && > + test_decode_color >actual <actual.raw && > + test_cmp expect actual > +' > + > test_done ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 3/8] diff.c: simplify caller of emit_line_0 2018-07-31 0:31 ` [PATCHv2 0/8] Add color test for range-diff, simplify diff.c Stefan Beller 2018-07-31 0:31 ` [PATCH 1/8] test_decode_color: understand FAINT and ITALIC Stefan Beller 2018-07-31 0:31 ` [PATCH 2/8] t3206: add color test for range-diff --dual-color Stefan Beller @ 2018-07-31 0:31 ` Stefan Beller 2018-07-31 0:31 ` [PATCH 4/8] diff.c: reorder arguments for emit_line_ws_markup Stefan Beller ` (5 subsequent siblings) 8 siblings, 0 replies; 30+ messages in thread From: Stefan Beller @ 2018-07-31 0:31 UTC (permalink / raw) To: sbeller; +Cc: Johannes.Schindelin, git, sunshine Due to the previous condition we know "set_sign != NULL" at that point. Signed-off-by: Stefan Beller <sbeller@google.com> --- diff.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/diff.c b/diff.c index 272b0b93834..f7251c89cbb 100644 --- a/diff.c +++ b/diff.c @@ -997,8 +997,7 @@ static void emit_line_ws_markup(struct diff_options *o, emit_line_0(o, set, 0, reset, sign, line, len); else if (!ws) { /* Emit just the prefix, then the rest. */ - emit_line_0(o, set_sign ? set_sign : set, !!set_sign, reset, - sign, "", 0); + emit_line_0(o, set_sign, !!set_sign, reset, sign, "", 0); emit_line_0(o, set, 0, reset, 0, line, len); } else if (blank_at_eof) /* Blank line at EOF - paint '+' as well */ -- 2.18.0.132.g195c49a2227 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 4/8] diff.c: reorder arguments for emit_line_ws_markup 2018-07-31 0:31 ` [PATCHv2 0/8] Add color test for range-diff, simplify diff.c Stefan Beller ` (2 preceding siblings ...) 2018-07-31 0:31 ` [PATCH 3/8] diff.c: simplify caller of emit_line_0 Stefan Beller @ 2018-07-31 0:31 ` Stefan Beller 2018-07-31 0:31 ` [PATCH 5/8] diff.c: add set_sign to emit_line_0 Stefan Beller ` (4 subsequent siblings) 8 siblings, 0 replies; 30+ messages in thread From: Stefan Beller @ 2018-07-31 0:31 UTC (permalink / raw) To: sbeller; +Cc: Johannes.Schindelin, git, sunshine The order shall be all colors first, then the content, flags at the end. The colors are in order. Signed-off-by: Stefan Beller <sbeller@google.com> --- diff.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/diff.c b/diff.c index f7251c89cbb..8fd2171d808 100644 --- a/diff.c +++ b/diff.c @@ -980,9 +980,9 @@ static void dim_moved_lines(struct diff_options *o) } static void emit_line_ws_markup(struct diff_options *o, - const char *set, const char *reset, - const char *line, int len, - const char *set_sign, char sign, + const char *set_sign, const char *set, + const char *reset, + char sign, const char *line, int len, unsigned ws_rule, int blank_at_eof) { const char *ws = NULL; @@ -1066,7 +1066,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o, else if (c == '-') set = diff_get_color_opt(o, DIFF_FILE_OLD); } - emit_line_ws_markup(o, set, reset, line, len, set_sign, ' ', + emit_line_ws_markup(o, set_sign, set, reset, ' ', line, len, flags & (DIFF_SYMBOL_CONTENT_WS_MASK), 0); break; case DIFF_SYMBOL_PLUS: @@ -1109,7 +1109,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o, set = diff_get_color_opt(o, DIFF_CONTEXT_BOLD); flags |= WS_IGNORE_FIRST_SPACE; } - emit_line_ws_markup(o, set, reset, line, len, set_sign, '+', + emit_line_ws_markup(o, set_sign, set, reset, '+', line, len, flags & DIFF_SYMBOL_CONTENT_WS_MASK, flags & DIFF_SYMBOL_CONTENT_BLANK_LINE_EOF); break; @@ -1152,7 +1152,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o, else set = diff_get_color_opt(o, DIFF_CONTEXT_DIM); } - emit_line_ws_markup(o, set, reset, line, len, set_sign, '-', + emit_line_ws_markup(o, set_sign, set, reset, '-', line, len, flags & DIFF_SYMBOL_CONTENT_WS_MASK, 0); break; case DIFF_SYMBOL_WORDS_PORCELAIN: -- 2.18.0.132.g195c49a2227 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 5/8] diff.c: add set_sign to emit_line_0 2018-07-31 0:31 ` [PATCHv2 0/8] Add color test for range-diff, simplify diff.c Stefan Beller ` (3 preceding siblings ...) 2018-07-31 0:31 ` [PATCH 4/8] diff.c: reorder arguments for emit_line_ws_markup Stefan Beller @ 2018-07-31 0:31 ` Stefan Beller 2018-07-31 0:31 ` [PATCH 6/8] diff: use emit_line_0 once per line Stefan Beller ` (3 subsequent siblings) 8 siblings, 0 replies; 30+ messages in thread From: Stefan Beller @ 2018-07-31 0:31 UTC (permalink / raw) To: sbeller; +Cc: Johannes.Schindelin, git, sunshine For now just change the signature, we'll reason about the actual change in a follow up patch. Pass 'set_sign' (which is output before the sign) and 'set' which controls the color after the first character. Hence, promote any 'set's to 'set_sign' as we want to have color before the sign for now. Signed-off-by: Stefan Beller <sbeller@google.com> --- diff.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/diff.c b/diff.c index 8fd2171d808..a36ed92c54c 100644 --- a/diff.c +++ b/diff.c @@ -576,7 +576,7 @@ static void check_blank_at_eof(mmfile_t *mf1, mmfile_t *mf2, } static void emit_line_0(struct diff_options *o, - const char *set, unsigned reverse, const char *reset, + const char *set_sign, const char *set, unsigned reverse, const char *reset, int first, const char *line, int len) { int has_trailing_newline, has_trailing_carriage_return; @@ -606,9 +606,12 @@ static void emit_line_0(struct diff_options *o, if (len || !nofirst) { if (reverse && want_color(o->use_color)) fputs(GIT_COLOR_REVERSE, file); - fputs(set, file); + if (set_sign && set_sign[0]) + fputs(set_sign, file); if (first && !nofirst) fputc(first, file); + if (set) + fputs(set, file); fwrite(line, len, 1, file); fputs(reset, file); } @@ -621,7 +624,7 @@ static void emit_line_0(struct diff_options *o, static void emit_line(struct diff_options *o, const char *set, const char *reset, const char *line, int len) { - emit_line_0(o, set, 0, reset, line[0], line+1, len-1); + emit_line_0(o, set, NULL, 0, reset, line[0], line+1, len-1); } enum diff_symbol { @@ -994,17 +997,17 @@ static void emit_line_ws_markup(struct diff_options *o, } if (!ws && !set_sign) - emit_line_0(o, set, 0, reset, sign, line, len); + emit_line_0(o, set, NULL, 0, reset, sign, line, len); else if (!ws) { /* Emit just the prefix, then the rest. */ - emit_line_0(o, set_sign, !!set_sign, reset, sign, "", 0); - emit_line_0(o, set, 0, reset, 0, line, len); + emit_line_0(o, set_sign, NULL, !!set_sign, reset, sign, "", 0); + emit_line_0(o, set, NULL, 0, reset, 0, line, len); } else if (blank_at_eof) /* Blank line at EOF - paint '+' as well */ - emit_line_0(o, ws, 0, reset, sign, line, len); + emit_line_0(o, ws, NULL, 0, reset, sign, line, len); else { /* Emit just the prefix, then the rest. */ - emit_line_0(o, set_sign ? set_sign : set, !!set_sign, reset, + emit_line_0(o, set_sign ? set_sign : set, NULL, !!set_sign, reset, sign, "", 0); ws_check_emit(line, len, ws_rule, o->file, set, reset, ws); @@ -1028,7 +1031,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o, context = diff_get_color_opt(o, DIFF_CONTEXT); reset = diff_get_color_opt(o, DIFF_RESET); putc('\n', o->file); - emit_line_0(o, context, 0, reset, '\\', + emit_line_0(o, context, NULL, 0, reset, '\\', nneof, strlen(nneof)); break; case DIFF_SYMBOL_SUBMODULE_HEADER: -- 2.18.0.132.g195c49a2227 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 6/8] diff: use emit_line_0 once per line 2018-07-31 0:31 ` [PATCHv2 0/8] Add color test for range-diff, simplify diff.c Stefan Beller ` (4 preceding siblings ...) 2018-07-31 0:31 ` [PATCH 5/8] diff.c: add set_sign to emit_line_0 Stefan Beller @ 2018-07-31 0:31 ` Stefan Beller 2018-07-31 0:31 ` [PATCH 7/8] diff.c: compute reverse locally in emit_line_0 Stefan Beller ` (2 subsequent siblings) 8 siblings, 0 replies; 30+ messages in thread From: Stefan Beller @ 2018-07-31 0:31 UTC (permalink / raw) To: sbeller; +Cc: Johannes.Schindelin, git, sunshine All lines that use emit_line_0 multiple times per line, are combined into a single call to emit_line_0, making use of the 'set' argument. Signed-off-by: Stefan Beller <sbeller@google.com> --- diff.c | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/diff.c b/diff.c index a36ed92c54c..fdad7ffdd77 100644 --- a/diff.c +++ b/diff.c @@ -583,10 +583,7 @@ static void emit_line_0(struct diff_options *o, int nofirst; FILE *file = o->file; - if (first) - fputs(diff_line_prefix(o), file); - else if (!len) - return; + fputs(diff_line_prefix(o), file); if (len == 0) { has_trailing_newline = (first == '\n'); @@ -606,13 +603,17 @@ static void emit_line_0(struct diff_options *o, if (len || !nofirst) { if (reverse && want_color(o->use_color)) fputs(GIT_COLOR_REVERSE, file); - if (set_sign && set_sign[0]) - fputs(set_sign, file); + if (set_sign || set) + fputs(set_sign ? set_sign : set, file); if (first && !nofirst) fputc(first, file); - if (set) - fputs(set, file); - fwrite(line, len, 1, file); + if (len) { + if (set_sign && set && set != set_sign) + fputs(reset, file); + if (set) + fputs(set, file); + fwrite(line, len, 1, file); + } fputs(reset, file); } if (has_trailing_carriage_return) @@ -999,16 +1000,13 @@ static void emit_line_ws_markup(struct diff_options *o, if (!ws && !set_sign) emit_line_0(o, set, NULL, 0, reset, sign, line, len); else if (!ws) { - /* Emit just the prefix, then the rest. */ - emit_line_0(o, set_sign, NULL, !!set_sign, reset, sign, "", 0); - emit_line_0(o, set, NULL, 0, reset, 0, line, len); + emit_line_0(o, set_sign, set, !!set_sign, reset, sign, line, len); } else if (blank_at_eof) /* Blank line at EOF - paint '+' as well */ emit_line_0(o, ws, NULL, 0, reset, sign, line, len); else { /* Emit just the prefix, then the rest. */ - emit_line_0(o, set_sign ? set_sign : set, NULL, !!set_sign, reset, - sign, "", 0); + emit_line_0(o, set_sign, set, !!set_sign, reset, sign, "", 0); ws_check_emit(line, len, ws_rule, o->file, set, reset, ws); } -- 2.18.0.132.g195c49a2227 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 7/8] diff.c: compute reverse locally in emit_line_0 2018-07-31 0:31 ` [PATCHv2 0/8] Add color test for range-diff, simplify diff.c Stefan Beller ` (5 preceding siblings ...) 2018-07-31 0:31 ` [PATCH 6/8] diff: use emit_line_0 once per line Stefan Beller @ 2018-07-31 0:31 ` Stefan Beller 2018-07-31 0:31 ` [PATCH 8/8] diff.c: rewrite emit_line_0 more understandably Stefan Beller 2018-08-01 19:13 ` [PATCHv2 0/8] Add color test for range-diff, simplify diff.c Junio C Hamano 8 siblings, 0 replies; 30+ messages in thread From: Stefan Beller @ 2018-07-31 0:31 UTC (permalink / raw) To: sbeller; +Cc: Johannes.Schindelin, git, sunshine Signed-off-by: Stefan Beller <sbeller@google.com> --- diff.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/diff.c b/diff.c index fdad7ffdd77..f565a2c0c2b 100644 --- a/diff.c +++ b/diff.c @@ -576,11 +576,12 @@ static void check_blank_at_eof(mmfile_t *mf1, mmfile_t *mf2, } static void emit_line_0(struct diff_options *o, - const char *set_sign, const char *set, unsigned reverse, const char *reset, + const char *set_sign, const char *set, const char *reset, int first, const char *line, int len) { int has_trailing_newline, has_trailing_carriage_return; int nofirst; + int reverse = !!set && !!set_sign; FILE *file = o->file; fputs(diff_line_prefix(o), file); @@ -625,7 +626,7 @@ static void emit_line_0(struct diff_options *o, static void emit_line(struct diff_options *o, const char *set, const char *reset, const char *line, int len) { - emit_line_0(o, set, NULL, 0, reset, line[0], line+1, len-1); + emit_line_0(o, set, NULL, reset, line[0], line+1, len-1); } enum diff_symbol { @@ -998,15 +999,15 @@ static void emit_line_ws_markup(struct diff_options *o, } if (!ws && !set_sign) - emit_line_0(o, set, NULL, 0, reset, sign, line, len); + emit_line_0(o, set, NULL, reset, sign, line, len); else if (!ws) { - emit_line_0(o, set_sign, set, !!set_sign, reset, sign, line, len); + emit_line_0(o, set_sign, set, reset, sign, line, len); } else if (blank_at_eof) /* Blank line at EOF - paint '+' as well */ - emit_line_0(o, ws, NULL, 0, reset, sign, line, len); + emit_line_0(o, ws, NULL, reset, sign, line, len); else { /* Emit just the prefix, then the rest. */ - emit_line_0(o, set_sign, set, !!set_sign, reset, sign, "", 0); + emit_line_0(o, set_sign, set, reset, sign, "", 0); ws_check_emit(line, len, ws_rule, o->file, set, reset, ws); } @@ -1029,7 +1030,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o, context = diff_get_color_opt(o, DIFF_CONTEXT); reset = diff_get_color_opt(o, DIFF_RESET); putc('\n', o->file); - emit_line_0(o, context, NULL, 0, reset, '\\', + emit_line_0(o, context, NULL, reset, '\\', nneof, strlen(nneof)); break; case DIFF_SYMBOL_SUBMODULE_HEADER: -- 2.18.0.132.g195c49a2227 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 8/8] diff.c: rewrite emit_line_0 more understandably 2018-07-31 0:31 ` [PATCHv2 0/8] Add color test for range-diff, simplify diff.c Stefan Beller ` (6 preceding siblings ...) 2018-07-31 0:31 ` [PATCH 7/8] diff.c: compute reverse locally in emit_line_0 Stefan Beller @ 2018-07-31 0:31 ` Stefan Beller 2018-08-01 19:13 ` [PATCHv2 0/8] Add color test for range-diff, simplify diff.c Junio C Hamano 8 siblings, 0 replies; 30+ messages in thread From: Stefan Beller @ 2018-07-31 0:31 UTC (permalink / raw) To: sbeller; +Cc: Johannes.Schindelin, git, sunshine emit_line_0 has no nested conditions, but puts out all its arguments (if set) in order. There is still a slight confusion with set and set_sign, but let's defer that to a later patch. 'first' used be output always no matter if it was 0, but that got lost at "diff: add an internal option to dual-color diffs of diffs", 2018-07-21), as there we broadened the meaning of 'first' to also signal an early return. The change in 'emit_line' makes sure that 'first' is never content, but always under our control, a sign or special character in the beginning of the line (or 0, in which case we ignore it). Signed-off-by: Stefan Beller <sbeller@google.com> --- diff.c | 73 +++++++++++++++++++++++++++++++++------------------------- 1 file changed, 41 insertions(+), 32 deletions(-) diff --git a/diff.c b/diff.c index f565a2c0c2b..2bd4d3d6839 100644 --- a/diff.c +++ b/diff.c @@ -580,43 +580,52 @@ static void emit_line_0(struct diff_options *o, int first, const char *line, int len) { int has_trailing_newline, has_trailing_carriage_return; - int nofirst; int reverse = !!set && !!set_sign; + int needs_reset = 0; + FILE *file = o->file; fputs(diff_line_prefix(o), file); - if (len == 0) { - has_trailing_newline = (first == '\n'); - has_trailing_carriage_return = (!has_trailing_newline && - (first == '\r')); - nofirst = has_trailing_newline || has_trailing_carriage_return; - } else { - has_trailing_newline = (len > 0 && line[len-1] == '\n'); - if (has_trailing_newline) - len--; - has_trailing_carriage_return = (len > 0 && line[len-1] == '\r'); - if (has_trailing_carriage_return) - len--; - nofirst = 0; - } - - if (len || !nofirst) { - if (reverse && want_color(o->use_color)) - fputs(GIT_COLOR_REVERSE, file); - if (set_sign || set) - fputs(set_sign ? set_sign : set, file); - if (first && !nofirst) - fputc(first, file); - if (len) { - if (set_sign && set && set != set_sign) - fputs(reset, file); - if (set) - fputs(set, file); - fwrite(line, len, 1, file); - } - fputs(reset, file); + has_trailing_newline = (len > 0 && line[len-1] == '\n'); + if (has_trailing_newline) + len--; + + has_trailing_carriage_return = (len > 0 && line[len-1] == '\r'); + if (has_trailing_carriage_return) + len--; + + if (!len && !first) + goto end_of_line; + + if (reverse && want_color(o->use_color)) { + fputs(GIT_COLOR_REVERSE, file); + needs_reset = 1; + } + + if (set_sign || set) { + fputs(set_sign ? set_sign : set, file); + needs_reset = 1; } + + if (first) + fputc(first, file); + + if (!len) + goto end_of_line; + + if (set) { + if (set_sign && set != set_sign) + fputs(reset, file); + fputs(set, file); + needs_reset = 1; + } + fwrite(line, len, 1, file); + needs_reset |= len > 0; + +end_of_line: + if (needs_reset) + fputs(reset, file); if (has_trailing_carriage_return) fputc('\r', file); if (has_trailing_newline) @@ -626,7 +635,7 @@ static void emit_line_0(struct diff_options *o, static void emit_line(struct diff_options *o, const char *set, const char *reset, const char *line, int len) { - emit_line_0(o, set, NULL, reset, line[0], line+1, len-1); + emit_line_0(o, set, NULL, reset, 0, line, len); } enum diff_symbol { -- 2.18.0.132.g195c49a2227 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCHv2 0/8] Add color test for range-diff, simplify diff.c 2018-07-31 0:31 ` [PATCHv2 0/8] Add color test for range-diff, simplify diff.c Stefan Beller ` (7 preceding siblings ...) 2018-07-31 0:31 ` [PATCH 8/8] diff.c: rewrite emit_line_0 more understandably Stefan Beller @ 2018-08-01 19:13 ` Junio C Hamano 2018-08-01 19:46 ` Stefan Beller 8 siblings, 1 reply; 30+ messages in thread From: Junio C Hamano @ 2018-08-01 19:13 UTC (permalink / raw) To: Stefan Beller; +Cc: Johannes.Schindelin, git, sunshine Stefan Beller <sbeller@google.com> writes: > Stefan Beller (8): > test_decode_color: understand FAINT and ITALIC > t3206: add color test for range-diff --dual-color > diff.c: simplify caller of emit_line_0 > diff.c: reorder arguments for emit_line_ws_markup > diff.c: add set_sign to emit_line_0 > diff: use emit_line_0 once per line > diff.c: compute reverse locally in emit_line_0 > diff.c: rewrite emit_line_0 more understandably > > diff.c | 94 +++++++++++++++++++++++------------------ > t/t3206-range-diff.sh | 39 +++++++++++++++++ > t/test-lib-functions.sh | 2 + > 3 files changed, 93 insertions(+), 42 deletions(-) As I cannot merge this as is to 'pu' and keep going, I'll queue the following to the tip. I think it can be squashed to "t3206: add color test" but for now they will remain separate patches. Subject: [PATCH] fixup! t3206: add color test for range-diff --dual-color --- t/t3206-range-diff.sh | 64 +++++++++++++++++++++---------------------- 1 file changed, 32 insertions(+), 32 deletions(-) diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh index 019724e61a..e3b0764b43 100755 --- a/t/t3206-range-diff.sh +++ b/t/t3206-range-diff.sh @@ -143,38 +143,38 @@ test_expect_success 'changed message' ' ' test_expect_success 'dual-coloring' ' - cat >expect <<-\EOF && - <YELLOW>1: a4b3333 = 1: f686024 s/5/A/<RESET> - <RED>2: f51d370 <RESET><YELLOW>!<RESET><GREEN> 2: 4ab067d<RESET><YELLOW> s/4/A/<RESET> - <REVERSE><CYAN>@@ -2,6 +2,8 @@<RESET> - <RESET> - s/4/A/<RESET> - <RESET> - <REVERSE><GREEN>+<RESET> <BOLD> Also a silly comment here!<RESET> - <REVERSE><GREEN>+<RESET> - diff --git a/file b/file<RESET> - <RED> --- a/file<RESET> - <GREEN> +++ b/file<RESET> - <RED>3: 0559556 <RESET><YELLOW>!<RESET><GREEN> 3: b9cb956<RESET><YELLOW> s/11/B/<RESET> - <REVERSE><CYAN>@@ -10,7 +10,7 @@<RESET> - 9<RESET> - 10<RESET> - <RED> -11<RESET> - <REVERSE><RED>-<RESET><FAINT;GREEN>+BB<RESET> - <REVERSE><GREEN>+<RESET><BOLD;GREEN>+B<RESET> - 12<RESET> - 13<RESET> - 14<RESET> - <RED>4: d966c5c <RESET><YELLOW>!<RESET><GREEN> 4: 8add5f1<RESET><YELLOW> s/12/B/<RESET> - <REVERSE><CYAN>@@ -8,7 +8,7 @@<RESET> - <CYAN> @@<RESET> - 9<RESET> - 10<RESET> - <REVERSE><RED>-<RESET><FAINT> BB<RESET> - <REVERSE><GREEN>+<RESET> <BOLD>B<RESET> - <RED> -12<RESET> - <GREEN> +B<RESET> - 13<RESET> + sed -e "s|^:||" >expect <<-\EOF && + :<YELLOW>1: a4b3333 = 1: f686024 s/5/A/<RESET> + :<RED>2: f51d370 <RESET><YELLOW>!<RESET><GREEN> 2: 4ab067d<RESET><YELLOW> s/4/A/<RESET> + : <REVERSE><CYAN>@@ -2,6 +2,8 @@<RESET> + : <RESET> + : s/4/A/<RESET> + : <RESET> + : <REVERSE><GREEN>+<RESET> <BOLD> Also a silly comment here!<RESET> + : <REVERSE><GREEN>+<RESET> + : diff --git a/file b/file<RESET> + : <RED> --- a/file<RESET> + : <GREEN> +++ b/file<RESET> + :<RED>3: 0559556 <RESET><YELLOW>!<RESET><GREEN> 3: b9cb956<RESET><YELLOW> s/11/B/<RESET> + : <REVERSE><CYAN>@@ -10,7 +10,7 @@<RESET> + : 9<RESET> + : 10<RESET> + : <RED> -11<RESET> + : <REVERSE><RED>-<RESET><FAINT;GREEN>+BB<RESET> + : <REVERSE><GREEN>+<RESET><BOLD;GREEN>+B<RESET> + : 12<RESET> + : 13<RESET> + : 14<RESET> + :<RED>4: d966c5c <RESET><YELLOW>!<RESET><GREEN> 4: 8add5f1<RESET><YELLOW> s/12/B/<RESET> + : <REVERSE><CYAN>@@ -8,7 +8,7 @@<RESET> + : <CYAN> @@<RESET> + : 9<RESET> + : 10<RESET> + : <REVERSE><RED>-<RESET><FAINT> BB<RESET> + : <REVERSE><GREEN>+<RESET> <BOLD>B<RESET> + : <RED> -12<RESET> + : <GREEN> +B<RESET> + : 13<RESET> EOF git range-diff changed...changed-message --color --dual-color >actual.raw && test_decode_color >actual <actual.raw && -- 2.18.0-321-gffc6fa0e39 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCHv2 0/8] Add color test for range-diff, simplify diff.c 2018-08-01 19:13 ` [PATCHv2 0/8] Add color test for range-diff, simplify diff.c Junio C Hamano @ 2018-08-01 19:46 ` Stefan Beller 2018-08-02 15:48 ` Junio C Hamano 0 siblings, 1 reply; 30+ messages in thread From: Stefan Beller @ 2018-08-01 19:46 UTC (permalink / raw) To: Junio C Hamano; +Cc: Johannes Schindelin, git, Eric Sunshine On Wed, Aug 1, 2018 at 12:13 PM Junio C Hamano <gitster@pobox.com> wrote: > > Stefan Beller <sbeller@google.com> writes: > > > Stefan Beller (8): > > test_decode_color: understand FAINT and ITALIC > > t3206: add color test for range-diff --dual-color > > diff.c: simplify caller of emit_line_0 > > diff.c: reorder arguments for emit_line_ws_markup > > diff.c: add set_sign to emit_line_0 > > diff: use emit_line_0 once per line > > diff.c: compute reverse locally in emit_line_0 > > diff.c: rewrite emit_line_0 more understandably > > > > diff.c | 94 +++++++++++++++++++++++------------------ > > t/t3206-range-diff.sh | 39 +++++++++++++++++ > > t/test-lib-functions.sh | 2 + > > 3 files changed, 93 insertions(+), 42 deletions(-) > > As I cannot merge this as is to 'pu' and keep going, I'll queue the > following to the tip. I think it can be squashed to "t3206: add > color test" but for now they will remain separate patches. > > Subject: [PATCH] fixup! t3206: add color test for range-diff --dual-color Thanks for dealing with my stubbornness here. I assumed that the contribution would be a one time hassle during git-am, not an ongoing problem during the whole time the patch flows through pu/next/master, which is why I punted on doing this change and resending in a timely manner. Further I assumed the sed trick as below is harder to read, but it turns out it is not. It is still very understandable. https://public-inbox.org/git/nycvar.QRO.7.76.6.1808011800570.71@tvgsbejvaqbjf.bet/ sounds like DScho wants to incorporate some white space related stuff that might collide with the later parts of this series, so I am not sure how easy it will be to carry this through pu, so feel free to drop it as well. Thanks! Stefan ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCHv2 0/8] Add color test for range-diff, simplify diff.c 2018-08-01 19:46 ` Stefan Beller @ 2018-08-02 15:48 ` Junio C Hamano 0 siblings, 0 replies; 30+ messages in thread From: Junio C Hamano @ 2018-08-02 15:48 UTC (permalink / raw) To: Stefan Beller; +Cc: Johannes Schindelin, git, Eric Sunshine Stefan Beller <sbeller@google.com> writes: > On Wed, Aug 1, 2018 at 12:13 PM Junio C Hamano <gitster@pobox.com> wrote: >> >> Stefan Beller <sbeller@google.com> writes: >> >> > Stefan Beller (8): >> > test_decode_color: understand FAINT and ITALIC >> > t3206: add color test for range-diff --dual-color >> > diff.c: simplify caller of emit_line_0 >> > diff.c: reorder arguments for emit_line_ws_markup >> > diff.c: add set_sign to emit_line_0 >> > diff: use emit_line_0 once per line >> > diff.c: compute reverse locally in emit_line_0 >> > diff.c: rewrite emit_line_0 more understandably >> > >> > diff.c | 94 +++++++++++++++++++++++------------------ >> > t/t3206-range-diff.sh | 39 +++++++++++++++++ >> > t/test-lib-functions.sh | 2 + >> > 3 files changed, 93 insertions(+), 42 deletions(-) >> >> As I cannot merge this as is to 'pu' and keep going, I'll queue the >> following to the tip. I think it can be squashed to "t3206: add >> color test" but for now they will remain separate patches. >> >> Subject: [PATCH] fixup! t3206: add color test for range-diff --dual-color > > Thanks for dealing with my stubbornness here. I didn't know you were stubborn---I just thought you were busy in other things. > I assumed that the contribution would be a one time hassle > during git-am, not an ongoing problem during the whole time > the patch flows through pu/next/master, which is why I punted > on doing this change and resending in a timely manner. It is a bit worse, in that it won't be at pu/next/master boundary but each and every time the integration branches are rebuilt, which happens a few times a day. Whitespace breakage after a merge is detected and my automation stops to ask for manual inspection. ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 0/8] Resending sb/range-diff-colors @ 2018-08-10 22:34 Stefan Beller 2018-08-10 22:34 ` [PATCH 5/8] diff.c: add set_sign to emit_line_0 Stefan Beller 2018-08-14 1:41 ` [PATCHv2 0/8] Resending sb/range-diff-colors Stefan Beller 0 siblings, 2 replies; 30+ messages in thread From: Stefan Beller @ 2018-08-10 22:34 UTC (permalink / raw) To: gitster, Johannes.Schindelin; +Cc: git, Stefan Beller This is also avaliable as git fetch https://github.com/stefanbeller/git sb/range-diff-colors and is a resend of sb/range-diff-colors. It is rebased on the version of range-diff that Johannes just sent out (pr-1/dscho/branch-diff-v5 in GGG repo), and squashes the fisup commit (which had to be adapted slightly in one line, too) range-diff below. Stefan Beller (8): test_decode_color: understand FAINT and ITALIC t3206: add color test for range-diff --dual-color diff.c: simplify caller of emit_line_0 diff.c: reorder arguments for emit_line_ws_markup diff.c: add set_sign to emit_line_0 diff: use emit_line_0 once per line diff.c: compute reverse locally in emit_line_0 diff.c: rewrite emit_line_0 more understandably diff.c | 94 +++++++++++++++++++++++------------------ t/t3206-range-diff.sh | 39 +++++++++++++++++ t/test-lib-functions.sh | 2 + 3 files changed, 93 insertions(+), 42 deletions(-) ./git-range-diff \ 5bf616af71afe1c4c36da7f21077662febf28cbe..c1b144ea695514cfe185fe70089198621c38d01c \ ccf8c1bb2459d33c7dc97098c08c47ca7d77ed3e..4dc97b54a35c60c25ab7634441d60711ead0e84e \ >>0000-cover-letter.patch 1: 7f88339e03e = 1: 0fedd4c0a20 test_decode_color: understand FAINT and ITALIC 2: 13e8528be69 < -: ----------- t3206: add color test for range-diff --dual-color -: ----------- > 2: 6a1c7698c68 t3206: add color test for range-diff --dual-color 3: 2f80811b319 = 3: 7e12ece1d34 diff.c: simplify caller of emit_line_0 4: 15af0d378c8 ! 4: 74dabd6d36f diff.c: reorder arguments for emit_line_ws_markup @@ -35,7 +35,7 @@ case DIFF_SYMBOL_PLUS: @@ set = diff_get_color_opt(o, DIFF_CONTEXT_BOLD); - flags |= WS_IGNORE_FIRST_SPACE; + flags &= ~DIFF_SYMBOL_CONTENT_WS_MASK; } - emit_line_ws_markup(o, set, reset, line, len, set_sign, '+', + emit_line_ws_markup(o, set_sign, set, reset, '+', line, len, 5: dce49bb58fd = 5: e304e15aa6b diff.c: add set_sign to emit_line_0 6: 7581e16d63f = 6: 8d935d5212c diff: use emit_line_0 once per line 7: d070d393f73 = 7: 2344aac787a diff.c: compute reverse locally in emit_line_0 8: 669e7516e03 = 8: 4dc97b54a35 diff.c: rewrite emit_line_0 more understandably 9: c1b144ea695 < -: ----------- fixup! t3206: add color test for range-diff --dual-color ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 5/8] diff.c: add set_sign to emit_line_0 2018-08-10 22:34 [PATCH 0/8] Resending sb/range-diff-colors Stefan Beller @ 2018-08-10 22:34 ` Stefan Beller 2018-08-13 12:16 ` Johannes Schindelin 2018-08-14 1:41 ` [PATCHv2 0/8] Resending sb/range-diff-colors Stefan Beller 1 sibling, 1 reply; 30+ messages in thread From: Stefan Beller @ 2018-08-10 22:34 UTC (permalink / raw) To: gitster, Johannes.Schindelin; +Cc: git, Stefan Beller For now just change the signature, we'll reason about the actual change in a follow up patch. Pass 'set_sign' (which is output before the sign) and 'set' which controls the color after the first character. Hence, promote any 'set's to 'set_sign' as we want to have color before the sign for now. Signed-off-by: Stefan Beller <sbeller@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- diff.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/diff.c b/diff.c index ab6e6a88a56..5eea5edca50 100644 --- a/diff.c +++ b/diff.c @@ -622,7 +622,7 @@ static void check_blank_at_eof(mmfile_t *mf1, mmfile_t *mf2, } static void emit_line_0(struct diff_options *o, - const char *set, unsigned reverse, const char *reset, + const char *set_sign, const char *set, unsigned reverse, const char *reset, int first, const char *line, int len) { int has_trailing_newline, has_trailing_carriage_return; @@ -652,9 +652,12 @@ static void emit_line_0(struct diff_options *o, if (len || !nofirst) { if (reverse && want_color(o->use_color)) fputs(GIT_COLOR_REVERSE, file); - fputs(set, file); + if (set_sign && set_sign[0]) + fputs(set_sign, file); if (first && !nofirst) fputc(first, file); + if (set) + fputs(set, file); fwrite(line, len, 1, file); fputs(reset, file); } @@ -667,7 +670,7 @@ static void emit_line_0(struct diff_options *o, static void emit_line(struct diff_options *o, const char *set, const char *reset, const char *line, int len) { - emit_line_0(o, set, 0, reset, line[0], line+1, len-1); + emit_line_0(o, set, NULL, 0, reset, line[0], line+1, len-1); } enum diff_symbol { @@ -1199,17 +1202,17 @@ static void emit_line_ws_markup(struct diff_options *o, } if (!ws && !set_sign) - emit_line_0(o, set, 0, reset, sign, line, len); + emit_line_0(o, set, NULL, 0, reset, sign, line, len); else if (!ws) { /* Emit just the prefix, then the rest. */ - emit_line_0(o, set_sign, !!set_sign, reset, sign, "", 0); - emit_line_0(o, set, 0, reset, 0, line, len); + emit_line_0(o, set_sign, NULL, !!set_sign, reset, sign, "", 0); + emit_line_0(o, set, NULL, 0, reset, 0, line, len); } else if (blank_at_eof) /* Blank line at EOF - paint '+' as well */ - emit_line_0(o, ws, 0, reset, sign, line, len); + emit_line_0(o, ws, NULL, 0, reset, sign, line, len); else { /* Emit just the prefix, then the rest. */ - emit_line_0(o, set_sign ? set_sign : set, !!set_sign, reset, + emit_line_0(o, set_sign ? set_sign : set, NULL, !!set_sign, reset, sign, "", 0); ws_check_emit(line, len, ws_rule, o->file, set, reset, ws); @@ -1233,7 +1236,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o, context = diff_get_color_opt(o, DIFF_CONTEXT); reset = diff_get_color_opt(o, DIFF_RESET); putc('\n', o->file); - emit_line_0(o, context, 0, reset, '\\', + emit_line_0(o, context, NULL, 0, reset, '\\', nneof, strlen(nneof)); break; case DIFF_SYMBOL_SUBMODULE_HEADER: -- 2.18.0.865.gffc8e1a3cd6-goog ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH 5/8] diff.c: add set_sign to emit_line_0 2018-08-10 22:34 ` [PATCH 5/8] diff.c: add set_sign to emit_line_0 Stefan Beller @ 2018-08-13 12:16 ` Johannes Schindelin 2018-08-13 23:42 ` Stefan Beller 0 siblings, 1 reply; 30+ messages in thread From: Johannes Schindelin @ 2018-08-13 12:16 UTC (permalink / raw) To: Stefan Beller; +Cc: gitster, git Hi Stefan, On Fri, 10 Aug 2018, Stefan Beller wrote: > For now just change the signature, we'll reason about the actual > change in a follow up patch. > > Pass 'set_sign' (which is output before the sign) and 'set' which > controls the color after the first character. Hence, promote any > 'set's to 'set_sign' as we want to have color before the sign > for now. I'll freely admit that I had to study the diff in order to understand this paragraph. My I suggest something along those lines instead? Split the meaning of the `set` parameter that is passed to `emit_line_0()` to separate between the color of the "sign" (i.e. the diff marker '+', '-' or ' ' that is passed in as the `first` parameter) and the color of the rest of the line. This changes the meaning of the `set` parameter to no longer refer to the color of the diff marker, but instead to refer to the color of the rest of the line. A value of `NULL` indicates that the rest of the line wants to be colored the same as the diff marker. Ciao, Dscho > > Signed-off-by: Stefan Beller <sbeller@google.com> > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > diff.c | 21 ++++++++++++--------- > 1 file changed, 12 insertions(+), 9 deletions(-) > > diff --git a/diff.c b/diff.c > index ab6e6a88a56..5eea5edca50 100644 > --- a/diff.c > +++ b/diff.c > @@ -622,7 +622,7 @@ static void check_blank_at_eof(mmfile_t *mf1, mmfile_t *mf2, > } > > static void emit_line_0(struct diff_options *o, > - const char *set, unsigned reverse, const char *reset, > + const char *set_sign, const char *set, unsigned reverse, const char *reset, > int first, const char *line, int len) > { > int has_trailing_newline, has_trailing_carriage_return; > @@ -652,9 +652,12 @@ static void emit_line_0(struct diff_options *o, > if (len || !nofirst) { > if (reverse && want_color(o->use_color)) > fputs(GIT_COLOR_REVERSE, file); > - fputs(set, file); > + if (set_sign && set_sign[0]) > + fputs(set_sign, file); > if (first && !nofirst) > fputc(first, file); > + if (set) > + fputs(set, file); > fwrite(line, len, 1, file); > fputs(reset, file); > } > @@ -667,7 +670,7 @@ static void emit_line_0(struct diff_options *o, > static void emit_line(struct diff_options *o, const char *set, const char *reset, > const char *line, int len) > { > - emit_line_0(o, set, 0, reset, line[0], line+1, len-1); > + emit_line_0(o, set, NULL, 0, reset, line[0], line+1, len-1); > } > > enum diff_symbol { > @@ -1199,17 +1202,17 @@ static void emit_line_ws_markup(struct diff_options *o, > } > > if (!ws && !set_sign) > - emit_line_0(o, set, 0, reset, sign, line, len); > + emit_line_0(o, set, NULL, 0, reset, sign, line, len); > else if (!ws) { > /* Emit just the prefix, then the rest. */ > - emit_line_0(o, set_sign, !!set_sign, reset, sign, "", 0); > - emit_line_0(o, set, 0, reset, 0, line, len); > + emit_line_0(o, set_sign, NULL, !!set_sign, reset, sign, "", 0); > + emit_line_0(o, set, NULL, 0, reset, 0, line, len); > } else if (blank_at_eof) > /* Blank line at EOF - paint '+' as well */ > - emit_line_0(o, ws, 0, reset, sign, line, len); > + emit_line_0(o, ws, NULL, 0, reset, sign, line, len); > else { > /* Emit just the prefix, then the rest. */ > - emit_line_0(o, set_sign ? set_sign : set, !!set_sign, reset, > + emit_line_0(o, set_sign ? set_sign : set, NULL, !!set_sign, reset, > sign, "", 0); > ws_check_emit(line, len, ws_rule, > o->file, set, reset, ws); > @@ -1233,7 +1236,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o, > context = diff_get_color_opt(o, DIFF_CONTEXT); > reset = diff_get_color_opt(o, DIFF_RESET); > putc('\n', o->file); > - emit_line_0(o, context, 0, reset, '\\', > + emit_line_0(o, context, NULL, 0, reset, '\\', > nneof, strlen(nneof)); > break; > case DIFF_SYMBOL_SUBMODULE_HEADER: > -- > 2.18.0.865.gffc8e1a3cd6-goog > > ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 5/8] diff.c: add set_sign to emit_line_0 2018-08-13 12:16 ` Johannes Schindelin @ 2018-08-13 23:42 ` Stefan Beller 0 siblings, 0 replies; 30+ messages in thread From: Stefan Beller @ 2018-08-13 23:42 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Junio C Hamano, git On Mon, Aug 13, 2018 at 5:15 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote: > > Hi Stefan, > > On Fri, 10 Aug 2018, Stefan Beller wrote: > > > For now just change the signature, we'll reason about the actual > > change in a follow up patch. > > > > Pass 'set_sign' (which is output before the sign) and 'set' which > > controls the color after the first character. Hence, promote any > > 'set's to 'set_sign' as we want to have color before the sign > > for now. > > I'll freely admit that I had to study the diff in order to understand this > paragraph. > > My I suggest something along those lines instead? > > Split the meaning of the `set` parameter that is passed to > `emit_line_0()` to separate between the color of the "sign" (i.e. > the diff marker '+', '-' or ' ' that is passed in as the `first` > parameter) and the color of the rest of the line. > > This changes the meaning of the `set` parameter to no longer refer > to the color of the diff marker, but instead to refer to the color > of the rest of the line. A value of `NULL` indicates that the rest > of the line wants to be colored the same as the diff marker. > That is a wonderful commit message, I'll just use it as-is. Thanks, Stefan ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCHv2 0/8] Resending sb/range-diff-colors 2018-08-10 22:34 [PATCH 0/8] Resending sb/range-diff-colors Stefan Beller 2018-08-10 22:34 ` [PATCH 5/8] diff.c: add set_sign to emit_line_0 Stefan Beller @ 2018-08-14 1:41 ` Stefan Beller 2018-08-14 1:41 ` [PATCH 5/8] diff.c: add set_sign to emit_line_0 Stefan Beller 1 sibling, 1 reply; 30+ messages in thread From: Stefan Beller @ 2018-08-14 1:41 UTC (permalink / raw) To: sbeller; +Cc: Johannes.Schindelin, git, gitster This is also avaliable as git fetch https://github.com/stefanbeller/git sb/range-diff-colors This applies on top of js/range-diff (a7be92acd9600), this version * resolves a semantic conflict in the test (Did range-diff change its output slightly?) * addressed Johannes feedback, such as -> keeping the reverse computation out of emit_line_0 -> better commit messages. -> Split "use emit_line_0 once per line" to have an additional "diff.c: omit check for line prefix in emit_line_0" as having these two things in the same patch is confusing The interdiff seems more useful than the range-diff here, both attached below. Thanks, Stefan Stefan Beller (8): test_decode_color: understand FAINT and ITALIC t3206: add color test for range-diff --dual-color diff.c: simplify caller of emit_line_0 diff.c: reorder arguments for emit_line_ws_markup diff.c: add set_sign to emit_line_0 diff: use emit_line_0 once per line diff.c: omit check for line prefix in emit_line_0 diff.c: rewrite emit_line_0 more understandably diff.c | 91 ++++++++++++++++++++++------------------- t/t3206-range-diff.sh | 39 ++++++++++++++++++ t/test-lib-functions.sh | 2 + 3 files changed, 91 insertions(+), 41 deletions(-) (interdiff seems to be more useful) git diff 4dc97b54a35..058e03a1601 diff --git a/builtin/range-diff.c b/builtin/range-diff.c index ef3ba22e297..f52d45d9d61 100644 --- a/builtin/range-diff.c +++ b/builtin/range-diff.c @@ -53,6 +53,8 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix) else i += c; } + while (i < argc) + argv[j++] = argv[i++]; argc = j; diff_setup_done(&diffopt); diff --git a/diff.c b/diff.c index af9316c8f91..c5c7739ce34 100644 --- a/diff.c +++ b/diff.c @@ -622,13 +622,11 @@ static void check_blank_at_eof(mmfile_t *mf1, mmfile_t *mf2, } static void emit_line_0(struct diff_options *o, - const char *set_sign, const char *set, const char *reset, + const char *set_sign, const char *set, unsigned reverse, const char *reset, int first, const char *line, int len) { int has_trailing_newline, has_trailing_carriage_return; - int reverse = !!set && !!set_sign; - int needs_reset = 0; - + int needs_reset = 0; /* at the end of the line */ FILE *file = o->file; fputs(diff_line_prefix(o), file); @@ -649,8 +647,8 @@ static void emit_line_0(struct diff_options *o, needs_reset = 1; } - if (set_sign || set) { - fputs(set_sign ? set_sign : set, file); + if (set_sign) { + fputs(set_sign, file); needs_reset = 1; } @@ -667,7 +665,7 @@ static void emit_line_0(struct diff_options *o, needs_reset = 1; } fwrite(line, len, 1, file); - needs_reset |= len > 0; + needs_reset = 1; /* 'line' may contain color codes. */ end_of_line: if (needs_reset) @@ -681,7 +679,7 @@ static void emit_line_0(struct diff_options *o, static void emit_line(struct diff_options *o, const char *set, const char *reset, const char *line, int len) { - emit_line_0(o, set, NULL, reset, 0, line, len); + emit_line_0(o, set, NULL, 0, reset, 0, line, len); } enum diff_symbol { @@ -1213,15 +1211,16 @@ static void emit_line_ws_markup(struct diff_options *o, } if (!ws && !set_sign) - emit_line_0(o, set, NULL, reset, sign, line, len); + emit_line_0(o, set, NULL, 0, reset, sign, line, len); else if (!ws) { - emit_line_0(o, set_sign, set, reset, sign, line, len); + emit_line_0(o, set_sign, set, !!set_sign, reset, sign, line, len); } else if (blank_at_eof) /* Blank line at EOF - paint '+' as well */ - emit_line_0(o, ws, NULL, reset, sign, line, len); + emit_line_0(o, ws, NULL, 0, reset, sign, line, len); else { /* Emit just the prefix, then the rest. */ - emit_line_0(o, set_sign, set, reset, sign, "", 0); + emit_line_0(o, set_sign ? set_sign : set, NULL, !!set_sign, reset, + sign, "", 0); ws_check_emit(line, len, ws_rule, o->file, set, reset, ws); } @@ -1244,7 +1243,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o, context = diff_get_color_opt(o, DIFF_CONTEXT); reset = diff_get_color_opt(o, DIFF_RESET); putc('\n', o->file); - emit_line_0(o, context, NULL, reset, '\\', + emit_line_0(o, context, NULL, 0, reset, '\\', nneof, strlen(nneof)); break; case DIFF_SYMBOL_SUBMODULE_HEADER: ./git-range-diff github/sb/range-diff-colors... below: 22: 0fedd4c0a20 = 22: dd772035ac9 test_decode_color: understand FAINT and ITALIC 23: 6a1c7698c68 ! 23: 5701745379b t3206: add color test for range-diff --dual-color @@ -23,7 +23,7 @@ + : s/4/A/<RESET> + : <RESET> + : <REVERSE><GREEN>+<RESET><BOLD> Also a silly comment here!<RESET> -+ : <REVERSE><GREEN>+<RESET> ++ : <REVERSE><GREEN>+<RESET><BOLD><RESET> + : diff --git a/file b/file<RESET> + : <RED> --- a/file<RESET> + : <GREEN> +++ b/file<RESET> 24: 7e12ece1d34 = 24: 4e56f63a5f5 diff.c: simplify caller of emit_line_0 25: 74dabd6d36f ! 25: cf126556940 diff.c: reorder arguments for emit_line_ws_markup @@ -3,7 +3,8 @@ diff.c: reorder arguments for emit_line_ws_markup The order shall be all colors first, then the content, flags at the end. - The colors are in order. + The colors are in the order of occurrence, i.e. first the color for the + sign and then the color for the rest of the line. Signed-off-by: Stefan Beller <sbeller@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 26: e304e15aa6b ! 26: 69008364cb8 diff.c: add set_sign to emit_line_0 @@ -2,14 +2,17 @@ diff.c: add set_sign to emit_line_0 - For now just change the signature, we'll reason about the actual - change in a follow up patch. + Split the meaning of the `set` parameter that is passed to + emit_line_0()` to separate between the color of the "sign" (i.e. + the diff marker '+', '-' or ' ' that is passed in as the `first` + parameter) and the color of the rest of the line. - Pass 'set_sign' (which is output before the sign) and 'set' which - controls the color after the first character. Hence, promote any - 'set's to 'set_sign' as we want to have color before the sign - for now. + This changes the meaning of the `set` parameter to no longer refer + to the color of the diff marker, but instead to refer to the color + of the rest of the line. A value of `NULL` indicates that the rest + of the line wants to be colored the same as the diff marker. + Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Stefan Beller <sbeller@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> @@ -30,12 +33,15 @@ if (reverse && want_color(o->use_color)) fputs(GIT_COLOR_REVERSE, file); - fputs(set, file); -+ if (set_sign && set_sign[0]) ++ if (set_sign) + fputs(set_sign, file); if (first && !nofirst) fputc(first, file); -+ if (set) ++ if (set && set != set_sign) { ++ if (set_sign) ++ fputs(reset, file); + fputs(set, file); ++ } fwrite(line, len, 1, file); fputs(reset, file); } 27: 8d935d5212c < -: ----------- diff: use emit_line_0 once per line 28: 2344aac787a < -: ----------- diff.c: compute reverse locally in emit_line_0 -: ----------- > 27: 5d2629281a1 diff: use emit_line_0 once per line -: ----------- > 28: 5240e94a69a diff.c: omit check for line prefix in emit_line_0 29: 4dc97b54a35 ! 29: 058e03a1601 diff.c: rewrite emit_line_0 more understandably @@ -2,21 +2,15 @@ diff.c: rewrite emit_line_0 more understandably - emit_line_0 has no nested conditions, but puts out all its arguments - (if set) in order. There is still a slight confusion with set - and set_sign, but let's defer that to a later patch. + Rewrite emit_line_0 to have fewer (nested) conditions. - 'first' used be output always no matter if it was 0, but that got lost - at "diff: add an internal option to dual-color diffs of diffs", - 2018-07-21), as there we broadened the meaning of 'first' to also - signal an early return. - - The change in 'emit_line' makes sure that 'first' is never content, but - always under our control, a sign or special character in the beginning - of the line (or 0, in which case we ignore it). + The change in 'emit_line' makes sure that 'first' is never user data, + but always under our control, a sign or special character in the + beginning of the line (or 0, in which case we ignore it). + So from now on, let's pass only a diff marker or 0 as the 'first' + character of the line. Signed-off-by: Stefan Beller <sbeller@google.com> - Signed-off-by: Junio C Hamano <gitster@pobox.com> diff --git a/diff.c b/diff.c --- a/diff.c @@ -26,9 +20,7 @@ { int has_trailing_newline, has_trailing_carriage_return; - int nofirst; - int reverse = !!set && !!set_sign; -+ int needs_reset = 0; -+ ++ int needs_reset = 0; /* at the end of the line */ FILE *file = o->file; fputs(diff_line_prefix(o), file); @@ -51,15 +43,16 @@ - if (len || !nofirst) { - if (reverse && want_color(o->use_color)) - fputs(GIT_COLOR_REVERSE, file); -- if (set_sign || set) -- fputs(set_sign ? set_sign : set, file); +- if (set_sign) +- fputs(set_sign, file); - if (first && !nofirst) - fputc(first, file); - if (len) { -- if (set_sign && set && set != set_sign) -- fputs(reset, file); -- if (set) +- if (set && set != set_sign) { +- if (set_sign) +- fputs(reset, file); - fputs(set, file); +- } - fwrite(line, len, 1, file); - } - fputs(reset, file); @@ -79,8 +72,8 @@ + needs_reset = 1; + } + -+ if (set_sign || set) { -+ fputs(set_sign ? set_sign : set, file); ++ if (set_sign) { ++ fputs(set_sign, file); + needs_reset = 1; } + @@ -97,7 +90,7 @@ + needs_reset = 1; + } + fwrite(line, len, 1, file); -+ needs_reset |= len > 0; ++ needs_reset = 1; /* 'line' may contain color codes. */ + +end_of_line: + if (needs_reset) @@ -109,8 +102,8 @@ static void emit_line(struct diff_options *o, const char *set, const char *reset, const char *line, int len) { -- emit_line_0(o, set, NULL, reset, line[0], line+1, len-1); -+ emit_line_0(o, set, NULL, reset, 0, line, len); +- emit_line_0(o, set, NULL, 0, reset, line[0], line+1, len-1); ++ emit_line_0(o, set, NULL, 0, reset, 0, line, len); } enum diff_symbol { ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 5/8] diff.c: add set_sign to emit_line_0 2018-08-14 1:41 ` [PATCHv2 0/8] Resending sb/range-diff-colors Stefan Beller @ 2018-08-14 1:41 ` Stefan Beller 0 siblings, 0 replies; 30+ messages in thread From: Stefan Beller @ 2018-08-14 1:41 UTC (permalink / raw) To: sbeller; +Cc: Johannes.Schindelin, git, gitster Split the meaning of the `set` parameter that is passed to emit_line_0()` to separate between the color of the "sign" (i.e. the diff marker '+', '-' or ' ' that is passed in as the `first` parameter) and the color of the rest of the line. This changes the meaning of the `set` parameter to no longer refer to the color of the diff marker, but instead to refer to the color of the rest of the line. A value of `NULL` indicates that the rest of the line wants to be colored the same as the diff marker. Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Stefan Beller <sbeller@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- diff.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/diff.c b/diff.c index ab6e6a88a56..4ef66389282 100644 --- a/diff.c +++ b/diff.c @@ -622,7 +622,7 @@ static void check_blank_at_eof(mmfile_t *mf1, mmfile_t *mf2, } static void emit_line_0(struct diff_options *o, - const char *set, unsigned reverse, const char *reset, + const char *set_sign, const char *set, unsigned reverse, const char *reset, int first, const char *line, int len) { int has_trailing_newline, has_trailing_carriage_return; @@ -652,9 +652,15 @@ static void emit_line_0(struct diff_options *o, if (len || !nofirst) { if (reverse && want_color(o->use_color)) fputs(GIT_COLOR_REVERSE, file); - fputs(set, file); + if (set_sign) + fputs(set_sign, file); if (first && !nofirst) fputc(first, file); + if (set && set != set_sign) { + if (set_sign) + fputs(reset, file); + fputs(set, file); + } fwrite(line, len, 1, file); fputs(reset, file); } @@ -667,7 +673,7 @@ static void emit_line_0(struct diff_options *o, static void emit_line(struct diff_options *o, const char *set, const char *reset, const char *line, int len) { - emit_line_0(o, set, 0, reset, line[0], line+1, len-1); + emit_line_0(o, set, NULL, 0, reset, line[0], line+1, len-1); } enum diff_symbol { @@ -1199,17 +1205,17 @@ static void emit_line_ws_markup(struct diff_options *o, } if (!ws && !set_sign) - emit_line_0(o, set, 0, reset, sign, line, len); + emit_line_0(o, set, NULL, 0, reset, sign, line, len); else if (!ws) { /* Emit just the prefix, then the rest. */ - emit_line_0(o, set_sign, !!set_sign, reset, sign, "", 0); - emit_line_0(o, set, 0, reset, 0, line, len); + emit_line_0(o, set_sign, NULL, !!set_sign, reset, sign, "", 0); + emit_line_0(o, set, NULL, 0, reset, 0, line, len); } else if (blank_at_eof) /* Blank line at EOF - paint '+' as well */ - emit_line_0(o, ws, 0, reset, sign, line, len); + emit_line_0(o, ws, NULL, 0, reset, sign, line, len); else { /* Emit just the prefix, then the rest. */ - emit_line_0(o, set_sign ? set_sign : set, !!set_sign, reset, + emit_line_0(o, set_sign ? set_sign : set, NULL, !!set_sign, reset, sign, "", 0); ws_check_emit(line, len, ws_rule, o->file, set, reset, ws); @@ -1233,7 +1239,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o, context = diff_get_color_opt(o, DIFF_CONTEXT); reset = diff_get_color_opt(o, DIFF_RESET); putc('\n', o->file); - emit_line_0(o, context, 0, reset, '\\', + emit_line_0(o, context, NULL, 0, reset, '\\', nneof, strlen(nneof)); break; case DIFF_SYMBOL_SUBMODULE_HEADER: -- 2.18.0.865.gffc8e1a3cd6-goog ^ permalink raw reply related [flat|nested] 30+ messages in thread
end of thread, other threads:[~2018-08-14 1:41 UTC | newest] Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-07-28 3:04 [PATCH 0/8] Add color test for range-diff, simplify diff.c Stefan Beller 2018-07-28 3:04 ` [PATCH 1/8] test_decode_color: understand FAINT and ITALIC Stefan Beller 2018-07-28 3:04 ` [PATCH 2/8] t3206: add color test for range-diff --dual-color Stefan Beller 2018-07-28 6:27 ` Eric Sunshine 2018-07-30 19:55 ` Stefan Beller 2018-07-28 3:04 ` [PATCH 3/8] diff.c: simplify caller of emit_line_0 Stefan Beller 2018-07-28 3:04 ` [PATCH 4/8] diff.c: reorder arguments for emit_line_ws_markup Stefan Beller 2018-07-28 3:04 ` [PATCH 5/8] diff.c: add set_sign to emit_line_0 Stefan Beller 2018-07-28 6:30 ` Eric Sunshine 2018-07-28 3:04 ` [PATCH 6/8] diff: use emit_line_0 once per line Stefan Beller 2018-07-28 3:04 ` [PATCH 7/8] diff.c: compute reverse locally in emit_line_0 Stefan Beller 2018-07-28 3:04 ` [PATCH 8/8] diff.c: rewrite emit_line_0 more understandably Stefan Beller 2018-07-28 6:33 ` Eric Sunshine 2018-07-31 0:31 ` [PATCHv2 0/8] Add color test for range-diff, simplify diff.c Stefan Beller 2018-07-31 0:31 ` [PATCH 1/8] test_decode_color: understand FAINT and ITALIC Stefan Beller 2018-07-31 0:31 ` [PATCH 2/8] t3206: add color test for range-diff --dual-color Stefan Beller 2018-07-31 20:51 ` Junio C Hamano 2018-07-31 0:31 ` [PATCH 3/8] diff.c: simplify caller of emit_line_0 Stefan Beller 2018-07-31 0:31 ` [PATCH 4/8] diff.c: reorder arguments for emit_line_ws_markup Stefan Beller 2018-07-31 0:31 ` [PATCH 5/8] diff.c: add set_sign to emit_line_0 Stefan Beller 2018-07-31 0:31 ` [PATCH 6/8] diff: use emit_line_0 once per line Stefan Beller 2018-07-31 0:31 ` [PATCH 7/8] diff.c: compute reverse locally in emit_line_0 Stefan Beller 2018-07-31 0:31 ` [PATCH 8/8] diff.c: rewrite emit_line_0 more understandably Stefan Beller 2018-08-01 19:13 ` [PATCHv2 0/8] Add color test for range-diff, simplify diff.c Junio C Hamano 2018-08-01 19:46 ` Stefan Beller 2018-08-02 15:48 ` Junio C Hamano 2018-08-10 22:34 [PATCH 0/8] Resending sb/range-diff-colors Stefan Beller 2018-08-10 22:34 ` [PATCH 5/8] diff.c: add set_sign to emit_line_0 Stefan Beller 2018-08-13 12:16 ` Johannes Schindelin 2018-08-13 23:42 ` Stefan Beller 2018-08-14 1:41 ` [PATCHv2 0/8] Resending sb/range-diff-colors Stefan Beller 2018-08-14 1:41 ` [PATCH 5/8] diff.c: add set_sign to emit_line_0 Stefan Beller
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.