* [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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ messages in thread
* [PATCH 0/8] Resending sb/range-diff-colors @ 2018-08-10 22:34 Stefan Beller 2018-08-10 22:34 ` [PATCH 7/8] diff.c: compute reverse locally in emit_line_0 Stefan Beller 0 siblings, 1 reply; 29+ 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] 29+ messages in thread
* [PATCH 7/8] diff.c: compute reverse locally in 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:26 ` Johannes Schindelin 0 siblings, 1 reply; 29+ messages in thread From: Stefan Beller @ 2018-08-10 22:34 UTC (permalink / raw) To: gitster, Johannes.Schindelin; +Cc: git, Stefan Beller Signed-off-by: Stefan Beller <sbeller@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- diff.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/diff.c b/diff.c index 01095a59d3d..e50cd312755 100644 --- a/diff.c +++ b/diff.c @@ -622,11 +622,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); @@ -671,7 +672,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 { @@ -1203,15 +1204,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); } @@ -1234,7 +1235,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.865.gffc8e1a3cd6-goog ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH 7/8] diff.c: compute reverse locally in emit_line_0 2018-08-10 22:34 ` [PATCH 7/8] diff.c: compute reverse locally in emit_line_0 Stefan Beller @ 2018-08-13 12:26 ` Johannes Schindelin 2018-08-13 19:00 ` Stefan Beller 0 siblings, 1 reply; 29+ messages in thread From: Johannes Schindelin @ 2018-08-13 12:26 UTC (permalink / raw) To: Stefan Beller; +Cc: gitster, git Hi, On Fri, 10 Aug 2018, Stefan Beller wrote: > Signed-off-by: Stefan Beller <sbeller@google.com> > Signed-off-by: Junio C Hamano <gitster@pobox.com> Well, my rationale for having the explicit `reverse` parameter is: this code is complex enough, introducing some magic "this and that implies this" makes it much harder to understand. So I am not at all sure that this is a good thing. > --- > diff.c | 15 ++++++++------- > 1 file changed, 8 insertions(+), 7 deletions(-) > > diff --git a/diff.c b/diff.c > index 01095a59d3d..e50cd312755 100644 > --- a/diff.c > +++ b/diff.c > @@ -622,11 +622,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; In contrast to, say, Javascript which has this nice feature that you can write `set || set_sign` to mean `set ? set : set_sign`, I am fairly certain that `set && set_sign` already evaluates to `0` or `1`. No need for all those exclamation marks!!!! :-) But as I said: I think it is a bit too magic for my liking to say "if the diff marker color is specified as well as the color for the rest of the line, then the diff marker will be reversed". That's just making the code hard to understand, i.e. less readable rather than more. Ciao, Dscho > FILE *file = o->file; > > fputs(diff_line_prefix(o), file); > @@ -671,7 +672,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 { > @@ -1203,15 +1204,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); > } > @@ -1234,7 +1235,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.865.gffc8e1a3cd6-goog > > ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 7/8] diff.c: compute reverse locally in emit_line_0 2018-08-13 12:26 ` Johannes Schindelin @ 2018-08-13 19:00 ` Stefan Beller 0 siblings, 0 replies; 29+ messages in thread From: Stefan Beller @ 2018-08-13 19:00 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Junio C Hamano, git On Mon, Aug 13, 2018 at 5:26 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote: > > Hi, > > > On Fri, 10 Aug 2018, Stefan Beller wrote: > > > Signed-off-by: Stefan Beller <sbeller@google.com> > > Signed-off-by: Junio C Hamano <gitster@pobox.com> > > Well, my rationale for having the explicit `reverse` parameter is: this > code is complex enough, introducing some magic "this and that implies > this" makes it much harder to understand. > > So I am not at all sure that this is a good thing. Yeah I am unsure, too. On the other hand the higher level functions look so much nicer a the complexity is shoved downwards, such that each function is solving problems in their own domain. I'll think of an alternative. ^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2018-08-13 19:00 UTC | newest] Thread overview: 29+ 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 7/8] diff.c: compute reverse locally in emit_line_0 Stefan Beller 2018-08-13 12:26 ` Johannes Schindelin 2018-08-13 19:00 ` 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.