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

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

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.

-- >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 <redacted>

Your patch looks good to me.

---
 * 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