Re: [ANNOUNCE] Git v2.20.0-rc1

From: Eric Sunshine <sunshine@sunshineco.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: Junio C Hamano <gitster@pobox.com>,
	Git List <git@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	git-packagers@googlegroups.com
Subject: Re: [ANNOUNCE] Git v2.20.0-rc1
Date: Thu, 22 Nov 2018 14:27:04 -0500	[thread overview]
Message-ID: <CAPig+cSzyT5N5=YeX+VgRq1t0VbWqXLHSB=g=V=O-nLdCWrE9g@mail.gmail.com> (raw)
In-Reply-To: <87ftvt14uw.fsf@evledraar.gmail.com>

On Thu, Nov 22, 2018 at 10:58 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> There's a regression related to this that I wanted to send a headsup
> for, but don't have time to fix today. Now range-diff in format-patch
> includes --stat output. See e.g. my
> https://public-inbox.org/git/20181122132823.9883-1-avarab@gmail.com/

Umf. Unfortunate fallout from [1].

> diff --git a/builtin/log.c b/builtin/log.c
> @@ -1094,9 +1094,12 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
>         if (rev->rdiff1) {
> +               const int oldfmt = rev->diffopt.output_format;
>                 fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title);
> +               rev->diffopt.output_format &= ~(DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY);
>                 show_range_diff(rev->rdiff1, rev->rdiff2,
>                                 rev->creation_factor, 1, &rev->diffopt);
> +               rev->diffopt.output_format = oldfmt;
>         }
>  }

A few questions/observations:

Does this same "fix" need to be applied to the --interdiff case just
above this --range-diff block?

Does the same "fix" need to be applied to the --interdiff and
--range-diff cases in log-tree.c:show_log()?

Aside from fixing the broken --no-patches option[2], a goal of the
series was to some day make --stat do something useful. Doesn't this
"fix" go against that goal?

The way this change needs to be spread around at various locations is
making it feel like a BandAid "fix" rather than a proper solution. It
seems like it should be fixed at a different level, though I'm not
sure yet if that level is higher (where the options get set) or lower
(where they actually affect the operation).

Until we figure out the answers to these questions, I wonder if a more
sensible short-term solution would be to back out [1] and just keep
[2], which fixed the --no-patches regression.

> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> @@ -248,8 +248,10 @@ test_expect_success 'dual-coloring' '
>  for prev in topic master..topic
>  do
>         test_expect_success "format-patch --range-diff=$prev" '
> +               cat actual &&

Debugging session gunk?

>                 git format-patch --stdout --cover-letter --range-diff=$prev \
>                         master..unmodified >actual &&
> +               ! grep "a => b" actual &&
>                 grep "= 1: .* s/5/A" actual &&

[1]: https://public-inbox.org/git/20181113185558.23438-4-avarab@gmail.com/
[2]: https://public-inbox.org/git/20181113185558.23438-3-avarab@gmail.com/