{"thread":{"id":"16470","subject":"[PATCH (Eek!)] git diff does not honor --no-ext-diff","startedAt":"2008-11-26T07:12:25Z","lastAt":"2008-11-26T10:01:27Z","messageCount":4,"participants":["Nazri Ramliy","Junio C Hamano","René Scharfe"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"96517","messageId":"544dda350811252312u7ef5533bwb20b37640d861487@mail.gmail.com","threadId":"16470","inReplyTo":null,"subject":"[PATCH (Eek!)] git diff does not honor --no-ext-diff","fromName":"Nazri Ramliy","fromEmail":"ayiehere@gmail.com","sentAt":"2008-11-26T07:12:25Z","receivedAt":"2008-11-26T07:12:25Z","isPatch":true,"sender":{"key":"ayiehere@gmail.com","avatar":"https://avatars.githubusercontent.com/u/164756?v=4"},"body":"Hello list,\n\ngit-diff does not honor the --no-ext-diff option in both cases when the external\ndiff program is set via diff.external and gitattributes.\n\nIs this intentional? If not the following patch seems to fix it.\n\nI think there must be a cleaner way of fixing this than doing it by\n`hand' hence the Eek!\n\nNazri.\n\n builtin-diff.c |   14 +++++++++++++-\n 1 files changed, 13 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-diff.c b/builtin-diff.c\nindex 7ceceeb..4ac7e15 100644\n--- a/builtin-diff.c\n+++ b/builtin-diff.c\n@@ -290,6 +290,19 @@ int cmd_diff(int argc, const char **argv, const\nchar *prefix)\n        /* Otherwise, we are doing the usual \"git\" diff */\n        rev.diffopt.skip_stat_unmatch = !!diff_auto_refresh_index;\n\n+       DIFF_OPT_SET(&rev.diffopt, ALLOW_EXTERNAL);\n+       /*\n+        * Do we have --no-ext-diff and have external diff setup via either\n+        * gitconfig or gitattributes, then clear ALLOW_EXTERNAL by hand.  Eek.\n+        */\n+       for (i = 1; i < argc; i++) {\n+               const char *arg = argv[i];\n+               if (!strcmp(arg, \"--\"))\n+                       break;\n+               else if (!strcmp(arg, \"--no-ext-diff\"))\n+                       DIFF_OPT_CLR(&rev.diffopt, ALLOW_EXTERNAL);\n+       }\n+\n        if (nongit)\n                die(\"Not a git repository\");\n        argc = setup_revisions(argc, argv, &rev, NULL);\n@@ -298,7 +311,6 @@ int cmd_diff(int argc, const char **argv, const\nchar *prefix)\n                if (diff_setup_done(&rev.diffopt) < 0)\n                        die(\"diff_setup_done failed\");\n        }\n-       DIFF_OPT_SET(&rev.diffopt, ALLOW_EXTERNAL);\n        DIFF_OPT_SET(&rev.diffopt, RECURSIVE);\n        DIFF_OPT_SET(&rev.diffopt, ALLOW_TEXTCONV);\n"},{"id":"96519","messageId":"7vprkihqk6.fsf@gitster.siamese.dyndns.org","threadId":"16470","inReplyTo":"544dda350811252312u7ef5533bwb20b37640d861487@mail.gmail.com","subject":"Re: [PATCH (Eek!)] git diff does not honor --no-ext-diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-11-26T07:52:41Z","receivedAt":"2008-11-26T07:52:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Nazri Ramliy\" <ayiehere@gmail.com> writes:\n\n> git-diff does not honor the --no-ext-diff option in both cases when the external\n> diff program is set via diff.external and gitattributes.\n>\n> Is this intentional?\n\nJudging from 72909be (Add diff-option --ext-diff, 2007-06-30), I think\nthis was intended in the sense that --ext-diff and --no-ext-diff were\nmeant to be no-op for \"diff\" itself when they were introduced.\n\nHaving said that, I do not know if I agree with the original intention.\nIt looks more like an oversight that came from focusing only on what a new\nbehaviour for the \"log\" family should be, than a logical design decision\nto exclude \"diff\" from this codepath.\n\nWouldn't this be a better patch?\n\n builtin-diff.c |    5 ++++-\n 1 files changed, 4 insertions(+), 1 deletions(-)\n\ndiff --git c/builtin-diff.c w/builtin-diff.c\nindex 7ceceeb..b90d8bc 100644\n--- c/builtin-diff.c\n+++ w/builtin-diff.c\n@@ -290,6 +290,9 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t/* Otherwise, we are doing the usual \"git\" diff */\n \trev.diffopt.skip_stat_unmatch = !!diff_auto_refresh_index;\n \n+\t/* Default to let external be used */\n+\tDIFF_OPT_SET(&rev.diffopt, ALLOW_EXTERNAL);\n+\n \tif (nongit)\n \t\tdie(\"Not a git repository\");\n \targc = setup_revisions(argc, argv, &rev, NULL);\n@@ -298,7 +301,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\tif (diff_setup_done(&rev.diffopt) < 0)\n \t\t\tdie(\"diff_setup_done failed\");\n \t}\n-\tDIFF_OPT_SET(&rev.diffopt, ALLOW_EXTERNAL);\n+\n \tDIFF_OPT_SET(&rev.diffopt, RECURSIVE);\n \tDIFF_OPT_SET(&rev.diffopt, ALLOW_TEXTCONV);\n \n"},{"id":"96520","messageId":"492D01D1.7020702@lsrfire.ath.cx","threadId":"16470","inReplyTo":"7vprkihqk6.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH (Eek!)] git diff does not honor --no-ext-diff","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2008-11-26T07:59:13Z","receivedAt":"2008-11-26T07:59:13Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Junio C Hamano schrieb:\n> \"Nazri Ramliy\" <ayiehere@gmail.com> writes:\n> \n>> git-diff does not honor the --no-ext-diff option in both cases when the external\n>> diff program is set via diff.external and gitattributes.\n>>\n>> Is this intentional?\n> \n> Judging from 72909be (Add diff-option --ext-diff, 2007-06-30), I think\n> this was intended in the sense that --ext-diff and --no-ext-diff were\n> meant to be no-op for \"diff\" itself when they were introduced.\n> \n> Having said that, I do not know if I agree with the original intention.\n> It looks more like an oversight that came from focusing only on what a new\n> behaviour for the \"log\" family should be, than a logical design decision\n> to exclude \"diff\" from this codepath.\n> \n> Wouldn't this be a better patch?\n\nYes.  And feel free to squash in the following. :)\n\ndiff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh\nindex dfe3fbc..ec787b4 100755\n--- a/t/t4020-diff-external.sh\n+++ b/t/t4020-diff-external.sh\n@@ -43,6 +43,13 @@ test_expect_success 'GIT_EXTERNAL_DIFF environment should apply only to diff' '\n \n '\n \n+test_expect_success 'GIT_EXTERNAL_DIFF environment and --no-ext-diff' '\n+\n+\tGIT_EXTERNAL_DIFF=echo git diff --no-ext-diff |\n+\tgrep \"^diff --git a/file b/file\"\n+\n+'\n+\n test_expect_success 'diff attribute' '\n \n \tgit config diff.parrot.command echo &&\n@@ -68,6 +75,13 @@ test_expect_success 'diff attribute should apply only to diff' '\n \n '\n \n+test_expect_success 'diff attribute and --no-ext-diff' '\n+\n+\tgit diff --no-ext-diff |\n+\tgrep \"^diff --git a/file b/file\"\n+\n+'\n+\n test_expect_success 'diff attribute' '\n \n \tgit config --unset diff.parrot.command &&\n@@ -94,6 +108,13 @@ test_expect_success 'diff attribute should apply only to diff' '\n \n '\n \n+test_expect_success 'diff attribute and --no-ext-diff' '\n+\n+\tgit diff --no-ext-diff |\n+\tgrep \"^diff --git a/file b/file\"\n+\n+'\n+\n test_expect_success 'no diff with -diff' '\n \techo >.gitattributes \"file -diff\" &&\n \tgit diff | grep Binary\n"},{"id":"96531","messageId":"544dda350811260201t66a58c9av3f390d6f314ffa05@mail.gmail.com","threadId":"16470","inReplyTo":"492D01D1.7020702@lsrfire.ath.cx","subject":"Re: [PATCH (Eek!)] git diff does not honor --no-ext-diff","fromName":"Nazri Ramliy","fromEmail":"ayiehere@gmail.com","sentAt":"2008-11-26T10:01:27Z","receivedAt":"2008-11-26T10:01:27Z","isPatch":true,"sender":{"key":"ayiehere@gmail.com","avatar":"https://avatars.githubusercontent.com/u/164756?v=4"},"body":"On Wed, Nov 26, 2008 at 3:59 PM, René Scharfe\n<rene.scharfe@lsrfire.ath.cx> wrote:\n> Junio C Hamano schrieb:\n>>\n>> Wouldn't this be a better patch?\n>\n> Yes.  And feel free to squash in the following. :)\n>\n\nGood! So the Eek! part is not really necessary.\n\nNow I can trigger the external diff whenever I say so via GIT_EXTERNAL_DIFF.\n\nThanks.\n\nNazri.\n"}]}