[PATCH (Eek!)] git diff does not honor --no-ext-diff

[PATCH (Eek!)] git diff does not honor --no-ext-diff

[Nazri Ramliy]

Hello list,

git-diff does not honor the --no-ext-diff option in both cases when the external
diff program is set via diff.external and gitattributes.

Is this intentional? If not the following patch seems to fix it.

I think there must be a cleaner way of fixing this than doing it by
`hand' hence the Eek!

Nazri.

 builtin-diff.c |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/builtin-diff.c b/builtin-diff.c
index 7ceceeb..4ac7e15 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -290,6 +290,19 @@ int cmd_diff(int argc, const char **argv, const
char *prefix)
        /* Otherwise, we are doing the usual "git" diff */
        rev.diffopt.skip_stat_unmatch = !!diff_auto_refresh_index;

+       DIFF_OPT_SET(&rev.diffopt, ALLOW_EXTERNAL);
+       /*
+        * Do we have --no-ext-diff and have external diff setup via either
+        * gitconfig or gitattributes, then clear ALLOW_EXTERNAL by hand.  Eek.
+        */
+       for (i = 1; i < argc; i++) {
+               const char *arg = argv[i];
+               if (!strcmp(arg, "--"))
+                       break;
+               else if (!strcmp(arg, "--no-ext-diff"))
+                       DIFF_OPT_CLR(&rev.diffopt, ALLOW_EXTERNAL);
+       }
+
        if (nongit)
                die("Not a git repository");
        argc = setup_revisions(argc, argv, &rev, NULL);
@@ -298,7 +311,6 @@ int cmd_diff(int argc, const char **argv, const
char *prefix)
                if (diff_setup_done(&rev.diffopt) < 0)
                        die("diff_setup_done failed");
        }
-       DIFF_OPT_SET(&rev.diffopt, ALLOW_EXTERNAL);
        DIFF_OPT_SET(&rev.diffopt, RECURSIVE);
        DIFF_OPT_SET(&rev.diffopt, ALLOW_TEXTCONV);

Re: [PATCH (Eek!)] git diff does not honor --no-ext-diff

[Junio C Hamano]

"Nazri Ramliy" <ayiehere@gmail.com> writes:

> git-diff does not honor the --no-ext-diff option in both cases when the external
> diff program is set via diff.external and gitattributes.
>
> Is this intentional?

Judging from 72909be (Add diff-option --ext-diff, 2007-06-30), I think
this was intended in the sense that --ext-diff and --no-ext-diff were
meant to be no-op for "diff" itself when they were introduced.

Having said that, I do not know if I agree with the original intention.
It looks more like an oversight that came from focusing only on what a new
behaviour for the "log" family should be, than a logical design decision
to exclude "diff" from this codepath.

Wouldn't this be a better patch?

 builtin-diff.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git c/builtin-diff.c w/builtin-diff.c
index 7ceceeb..b90d8bc 100644
--- c/builtin-diff.c
+++ w/builtin-diff.c
@@ -290,6 +290,9 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	/* Otherwise, we are doing the usual "git" diff */
 	rev.diffopt.skip_stat_unmatch = !!diff_auto_refresh_index;
 
+	/* Default to let external be used */
+	DIFF_OPT_SET(&rev.diffopt, ALLOW_EXTERNAL);
+
 	if (nongit)
 		die("Not a git repository");
 	argc = setup_revisions(argc, argv, &rev, NULL);
@@ -298,7 +301,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 		if (diff_setup_done(&rev.diffopt) < 0)
 			die("diff_setup_done failed");
 	}
-	DIFF_OPT_SET(&rev.diffopt, ALLOW_EXTERNAL);
+
 	DIFF_OPT_SET(&rev.diffopt, RECURSIVE);
 	DIFF_OPT_SET(&rev.diffopt, ALLOW_TEXTCONV);
 

Re: [PATCH (Eek!)] git diff does not honor --no-ext-diff

[René Scharfe]

Junio C Hamano schrieb:
> "Nazri Ramliy" <ayiehere@gmail.com> writes:
> 
>> git-diff does not honor the --no-ext-diff option in both cases when the external
>> diff program is set via diff.external and gitattributes.
>>
>> Is this intentional?
> 
> Judging from 72909be (Add diff-option --ext-diff, 2007-06-30), I think
> this was intended in the sense that --ext-diff and --no-ext-diff were
> meant to be no-op for "diff" itself when they were introduced.
> 
> Having said that, I do not know if I agree with the original intention.
> It looks more like an oversight that came from focusing only on what a new
> behaviour for the "log" family should be, than a logical design decision
> to exclude "diff" from this codepath.
> 
> Wouldn't this be a better patch?

Yes.  And feel free to squash in the following. :)

diff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh
index dfe3fbc..ec787b4 100755
--- a/t/t4020-diff-external.sh
+++ b/t/t4020-diff-external.sh
@@ -43,6 +43,13 @@ test_expect_success 'GIT_EXTERNAL_DIFF environment should apply only to diff' '
 
 '
 
+test_expect_success 'GIT_EXTERNAL_DIFF environment and --no-ext-diff' '
+
+	GIT_EXTERNAL_DIFF=echo git diff --no-ext-diff |
+	grep "^diff --git a/file b/file"
+
+'
+
 test_expect_success 'diff attribute' '
 
 	git config diff.parrot.command echo &&
@@ -68,6 +75,13 @@ test_expect_success 'diff attribute should apply only to diff' '
 
 '
 
+test_expect_success 'diff attribute and --no-ext-diff' '
+
+	git diff --no-ext-diff |
+	grep "^diff --git a/file b/file"
+
+'
+
 test_expect_success 'diff attribute' '
 
 	git config --unset diff.parrot.command &&
@@ -94,6 +108,13 @@ test_expect_success 'diff attribute should apply only to diff' '
 
 '
 
+test_expect_success 'diff attribute and --no-ext-diff' '
+
+	git diff --no-ext-diff |
+	grep "^diff --git a/file b/file"
+
+'
+
 test_expect_success 'no diff with -diff' '
 	echo >.gitattributes "file -diff" &&
 	git diff | grep Binary

Re: [PATCH (Eek!)] git diff does not honor --no-ext-diff

[Nazri Ramliy]

On Wed, Nov 26, 2008 at 3:59 PM, René Scharfe
<rene.scharfe@lsrfire.ath.cx> wrote:
> Junio C Hamano schrieb:
>>
>> Wouldn't this be a better patch?
>
> Yes.  And feel free to squash in the following. :)
>

Good! So the Eek! part is not really necessary.

Now I can trigger the external diff whenever I say so via GIT_EXTERNAL_DIFF.

Thanks.

Nazri.