"Jakub Vrana" <jakub@xxxxxxxx> writes:
> Yes, I was fixing the invalid (!pgm) condition, sorry for a non-precise description.
>
> Does it mean that my patch is accepted or is there something else I need to do?
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.
I've followed the codepath myself, and here is what I would have
liked the submitted patch to look like. Note that run_diff_cmd()
no longer needs to reset pgm to NULL based on ALLOW_EXTERNAL, but
it still needs to look at it to decide if per-path external userdiff
needs to be called.
-- >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>
---
* 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.
But this patch is not about changing that semantics, so I left it
as-is.
diff.c | 8 +++++---
t/t4020-diff-external.sh | 19 +++++++++++++++++++
2 files changed, 24 insertions(+), 3 deletions(-)
diff --git a/diff.c b/diff.c
index 208096f..62cbe14 100644
--- a/diff.c
+++ b/diff.c
@@ -2992,9 +2992,8 @@ static void run_diff_cmd(const char *pgm,
int complete_rewrite = (p->status == DIFF_STATUS_MODIFIED) && p->score;
int must_show_header = 0;
- if (!DIFF_OPT_TST(o, ALLOW_EXTERNAL))
- pgm = NULL;
- else {
+
+ if (DIFF_OPT_TST(o, ALLOW_EXTERNAL)) {
struct userdiff_driver *drv = userdiff_find_by_path(attr_path);
if (drv && drv->external)
pgm = drv->external;
@@ -3074,6 +3073,9 @@ static void run_diff(struct diff_filepair *p, struct diff_options *o)
if (o->prefix_length)
strip_prefix(o->prefix_length, &name, &other);
+ if (!DIFF_OPT_TST(o, ALLOW_EXTERNAL))
+ pgm = NULL;
+
if (DIFF_PAIR_UNMERGED(p)) {
run_diff_cmd(pgm, name, NULL, attr_path,
NULL, NULL, NULL, o, p);
diff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh
index 533afc1..5a5f68c 100755
--- a/t/t4020-diff-external.sh
+++ b/t/t4020-diff-external.sh
@@ -48,7 +48,26 @@ test_expect_success 'GIT_EXTERNAL_DIFF environment and --no-ext-diff' '
'
+test_expect_success SYMLINKS 'typechange diff' '
+ rm -f file &&
+ ln -s elif file &&
+ GIT_EXTERNAL_DIFF=echo git diff | {
+ read path oldfile oldhex oldmode newfile newhex newmode &&
+ test "z$path" = zfile &&
+ test "z$oldmode" = z100644 &&
+ test "z$newhex" = "z$_z40" &&
+ test "z$newmode" = z120000 &&
+ oh=$(git rev-parse --verify HEAD:file) &&
+ test "z$oh" = "z$oldhex"
+ } &&
+ GIT_EXTERNAL_DIFF=echo git diff --no-ext-diff >actual &&
+ git diff >expect &&
+ test_cmp expect actual
+'
+
test_expect_success 'diff attribute' '
+ git reset --hard &&
+ echo third >file &&
git config diff.parrot.command echo &&
--
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