Re: [PATCH v3] range-diff: always pass at least minimal diff options

[Martin Ågren]

On Mon, 3 Dec 2018 at 22:21, Eric Sunshine  wrote:
> [es: retain diff coloring when going to stdout]
>
> Signed-off-by: Martin Ågren 
> Signed-off-by: Eric Sunshine 
> ---
>
> 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.file = rev->diffopt.file;
> +   opts.use_color = rev->diffopt.use_color;

Ah, s/0/rev->diffopt.use_color/, well that's obvious.

Thanks!

Martin

Re: [PATCH v3] range-diff: always pass at least minimal diff options

[Junio C Hamano]

Eric Sunshine  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:$" -* &&
> + grep "= 1: .* s/5/A" -* &&
> + grep "= 2: .* s/4/A" -* &&
> + grep "= 3: .* s/11/B" -* &&
> + grep "= 4: .* s/12/B" -*
>   '
>  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