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