Re: [PATCH] diff: respect --no-ext-diff with typechange

From: Jeff King <peff@peff.net>
Date: Jul 18, 2012, 06:23 UTC
Message-ID: <20120718062329.GA12942@sigill.intra.peff.net>
In-Reply-To: <7v7gu1y7tg.fsf@alter.siamese.dyndns.org>

On Tue, Jul 17, 2012 at 10:08:59PM -0700, Junio C Hamano wrote:

Show 8 quoted lines

> The impression I got from Peff's review was that the problem
> description in the proposed commit log message did not describe the
> reality at all, and the added three lines did not do what the
> message implied they do.  So I do not see how it can be acceptable
> by anybody.
> 
> It also needs a test to protect this fix from being broken by other
> people in the future.

Yeah, exactly.

Show 17 quoted lines

> -- >8 --
> Subject: diff: correctly disable external_diff with --no-ext-diff
> 
> Upon seeing a type-change filepair, "diff --no-ext-diff" does not
> show the usual "deletion followed by addition" split patch and does
> not run the external diff driver either.
> 
> This is because the logic to disable external diff was placed at a
> wrong level in the callchain.  run_diff_cmd() decides to show the
> split patch only when external diff driver is not configured or
> specified via GIT_EXTERNAL_DIFF environment, but this is done before
> checking if --no-ext-diff was given.  To make things worse,
> run_diff_cmd() checks --no-ext-diff and disables the output for such
> a filepair completely, as the callchain below it (e.g. builtin_diff)
> does not want to handle typechange filepairs.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Your patch looks good to me.

Show 7 quoted lines

> ---
>  * The use of userdiff_find_by_path() in run_diff_cmd() may be iffy;
>    it is probably OK to override diff.external with a more specific
>    per-path configuration, but I think an external diff specified by
>    the GIT_EXTERNAL_DIFF environment may want to trump the
>    configured per-path one, as an environment is a stronger one-shot
>    request.

I think this date all the way back to f1af60b (Support 'diff=pgm' attribute, 2007-04-22). There's a tradeoff here; usually environment variables trump config, but you end up using a large hammer (here is how to diff _all_ files externally) to hit a small nail (here is how to diff _just_ this file). I suspect it isn't that big a problem in practice because people tend to use either one mechanism or the other.

The most sensible thing to me is probably $GIT_EXTERNAL_DIFF, followed by attributes, followed by diff.external. That uses the more specific diff pulled from the on-disk config, but allows you to do one-shot overrides with the environment as long as you are careful to restrict your command (e.g., "GIT_EXTERNAL_DIFF=foo-differ git diff -- file.foo").

>    But this patch is not about changing that semantics, so I left it
>    as-is.

Sounds sensible.

-Peff