| Message ID | 20181203212131.11299-1-sunshine@sunshineco.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] range-diff: always pass at least minimal diff options | expand |
Eric Sunshine <sunshine@sunshineco.com> writes: > This is a re-roll of Martin's v2[1]. The only difference from v2 is that > it retains coloring when emitting to the terminal (plus an in-code > comment was simplified). > > The regression introduced by d8981c3f88, in which the range-diff only > ever gets emitted to the terminal, and never to the cover letter or > commentary section of a standalone patch, makes the --range-diff option > rather useless, so this fix probably ought to be fast-tracked. Yup. Thanks. The only thing that makes me wonder is why any of the existing tests (among which I think I saw range-diff driven from format-patch) did not catch this rather obvious glitch. > diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh > index e497c1358f..048feaf6dd 100755 > --- a/t/t3206-range-diff.sh > +++ b/t/t3206-range-diff.sh > @@ -248,18 +248,24 @@ test_expect_success 'dual-coloring' ' > for prev in topic master..topic > do > test_expect_success "format-patch --range-diff=$prev" ' > - git format-patch --stdout --cover-letter --range-diff=$prev \ > + git format-patch --cover-letter --range-diff=$prev \ > master..unmodified >actual && Ah, of course. Then the "actual" file gets the names of the output files; we expect to see 5 of them. But now we have lost all the range-diff tests run under "--stdout", so next time some other change regresses only that codepath, we will not notice (which is fine for now but would want to be addressed before the end of the year around which time we certainly will all forget). Thanks. > - grep "= 1: .* s/5/A" actual && > - grep "= 2: .* s/4/A" actual && > - grep "= 3: .* s/11/B" actual && > - grep "= 4: .* s/12/B" actual > + test_when_finished "rm 000?-*" && > + test_line_count = 5 actual && > + test_i18ngrep "^Range-diff:$" 0000-* && > + grep "= 1: .* s/5/A" 0000-* && > + grep "= 2: .* s/4/A" 0000-* && > + grep "= 3: .* s/11/B" 0000-* && > + grep "= 4: .* s/12/B" 0000-* > ' > done > > test_expect_success 'format-patch --range-diff as commentary' ' > - git format-patch --stdout --range-diff=HEAD~1 HEAD~1 >actual && > - test_i18ngrep "^Range-diff:$" actual > + git format-patch --range-diff=HEAD~1 HEAD~1 >actual && > + test_when_finished "rm 0001-*" && > + test_line_count = 1 actual && > + test_i18ngrep "^Range-diff:$" 0001-* && > + grep "> 1: .* new message" 0001-* > ' > > test_done
On Mon, 3 Dec 2018 at 22:21, Eric Sunshine <sunshine@sunshineco.com> wrote: > [es: retain diff coloring when going to stdout] > > Signed-off-by: Martin Ågren <martin.agren@gmail.com> > Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> > --- > > This is a re-roll of Martin's v2[1]. The only difference from v2 is that > it retains coloring when emitting to the terminal (plus an in-code > comment was simplified). Thank you so much for this. > if (rev->rdiff1) { > + /* > + * Pass minimum required diff-options to range-diff; others > + * can be added later if deemed desirable. > + */ Agreed. > + struct diff_options opts; > + diff_setup(&opts); > + opts.file = rev->diffopt.file; > + opts.use_color = rev->diffopt.use_color; Ah, s/0/rev->diffopt.use_color/, well that's obvious. Thanks! Martin
diff --git a/builtin/log.c b/builtin/log.c index 5ac18e2848..e8e51068bd 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -1094,9 +1094,18 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout, } if (rev->rdiff1) { + /* + * Pass minimum required diff-options to range-diff; others + * can be added later if deemed desirable. + */ + struct diff_options opts; + diff_setup(&opts); + opts.file = rev->diffopt.file; + opts.use_color = rev->diffopt.use_color; + diff_setup_done(&opts); fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title); show_range_diff(rev->rdiff1, rev->rdiff2, - rev->creation_factor, 1, NULL); + rev->creation_factor, 1, &opts); } } diff --git a/log-tree.c b/log-tree.c index b243779a0b..10680c139e 100644 --- a/log-tree.c +++ b/log-tree.c @@ -755,14 +755,23 @@ void show_log(struct rev_info *opt) if (cmit_fmt_is_mail(ctx.fmt) && opt->rdiff1) { struct diff_queue_struct dq; + struct diff_options opts; memcpy(&dq, &diff_queued_diff, sizeof(diff_queued_diff)); DIFF_QUEUE_CLEAR(&diff_queued_diff); next_commentary_block(opt, NULL); fprintf_ln(opt->diffopt.file, "%s", opt->rdiff_title); + /* + * Pass minimum required diff-options to range-diff; others + * can be added later if deemed desirable. + */ + diff_setup(&opts); + opts.file = opt->diffopt.file; + opts.use_color = opt->diffopt.use_color; + diff_setup_done(&opts); show_range_diff(opt->rdiff1, opt->rdiff2, - opt->creation_factor, 1, NULL); + opt->creation_factor, 1, &opts); memcpy(&diff_queued_diff, &dq, sizeof(diff_queued_diff)); } diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh index e497c1358f..048feaf6dd 100755 --- a/t/t3206-range-diff.sh +++ b/t/t3206-range-diff.sh @@ -248,18 +248,24 @@ test_expect_success 'dual-coloring' ' for prev in topic master..topic do test_expect_success "format-patch --range-diff=$prev" ' - git format-patch --stdout --cover-letter --range-diff=$prev \ + git format-patch --cover-letter --range-diff=$prev \ master..unmodified >actual && - grep "= 1: .* s/5/A" actual && - grep "= 2: .* s/4/A" actual && - grep "= 3: .* s/11/B" actual && - grep "= 4: .* s/12/B" actual + test_when_finished "rm 000?-*" && + test_line_count = 5 actual && + test_i18ngrep "^Range-diff:$" 0000-* && + grep "= 1: .* s/5/A" 0000-* && + grep "= 2: .* s/4/A" 0000-* && + grep "= 3: .* s/11/B" 0000-* && + grep "= 4: .* s/12/B" 0000-* ' done test_expect_success 'format-patch --range-diff as commentary' ' - git format-patch --stdout --range-diff=HEAD~1 HEAD~1 >actual && - test_i18ngrep "^Range-diff:$" actual + git format-patch --range-diff=HEAD~1 HEAD~1 >actual && + test_when_finished "rm 0001-*" && + test_line_count = 1 actual && + test_i18ngrep "^Range-diff:$" 0001-* && + grep "> 1: .* new message" 0001-* ' test_done