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.
> -- >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 <gits...@pobox.com>
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
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
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html