{"thread":{"id":"31041","subject":"[PATCH] diff: respect --no-ext-diff with typechange","startedAt":"2012-07-17T00:27:00Z","lastAt":"2012-07-19T11:49:38Z","messageCount":8,"participants":["Jakub Vrana","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"195181","messageId":"000301cd63b2$e39a2130$aace6390$@vrana.cz","threadId":"31041","inReplyTo":null,"subject":"[PATCH] diff: respect --no-ext-diff with typechange","fromName":"Jakub Vrana","fromEmail":"jakub@vrana.cz","sentAt":"2012-07-17T00:27:00Z","receivedAt":"2012-07-17T00:27:00Z","isPatch":true,"sender":{"key":"jakub@vrana.cz","avatar":null},"body":"If external diff is specified through diff.external then it is used even if\n`git diff --no-ext-diff` is used when there is a typechange.\n\nSigned-off-by: Jakub Vrana <jakub@vrana.cz>\n---\n diff.c |    3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/diff.c b/diff.c\nindex 208096f..898d610 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3074,6 +3074,9 @@ static void run_diff(struct diff_filepair *p, struct\ndiff_options *o)\n \tif (o->prefix_length)\n \t\tstrip_prefix(o->prefix_length, &name, &other);\n \n+\tif (!DIFF_OPT_TST(o, ALLOW_EXTERNAL))\n+\t\tpgm = NULL;\n+\n \tif (DIFF_PAIR_UNMERGED(p)) {\n \t\trun_diff_cmd(pgm, name, NULL, attr_path,\n \t\t\t     NULL, NULL, NULL, o, p);\n-- \n1.7.10.msysgit.1\n"},{"id":"195187","messageId":"20120717041603.GD20945@sigill.intra.peff.net","threadId":"31041","inReplyTo":"000301cd63b2$e39a2130$aace6390$@vrana.cz","subject":"Re: [PATCH] diff: respect --no-ext-diff with typechange","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-17T04:16:03Z","receivedAt":"2012-07-17T04:16:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 16, 2012 at 05:27:00PM -0700, Jakub Vrana wrote:\n\n> If external diff is specified through diff.external then it is used even if\n> `git diff --no-ext-diff` is used when there is a typechange.\n\nEek. That has some minor security implications, as it means that it is\ndangerous to run even plumbing inspection command in somebody else's\nrepository.\n\nHowever...\n\n>  diff.c |    3 +++\n>  1 file changed, 3 insertions(+)\n> \n> diff --git a/diff.c b/diff.c\n> index 208096f..898d610 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -3074,6 +3074,9 @@ static void run_diff(struct diff_filepair *p, struct\n> diff_options *o)\n>  \tif (o->prefix_length)\n>  \t\tstrip_prefix(o->prefix_length, &name, &other);\n>  \n> +\tif (!DIFF_OPT_TST(o, ALLOW_EXTERNAL))\n> +\t\tpgm = NULL;\n> +\n>  \tif (DIFF_PAIR_UNMERGED(p)) {\n>  \t\trun_diff_cmd(pgm, name, NULL, attr_path,\n>  \t\t\t     NULL, NULL, NULL, o, p);\n\nrun_diff_cmd already checks the ALLOW_EXTERNAL bit and sets pgm to NULL\nthere. So as far as I can tell, we are not actually running the external\ndiff. However, there is still a problem. Later in run_diff we do:\n\n        if (!pgm &&\n            DIFF_FILE_VALID(one) && DIFF_FILE_VALID(two) &&\n            (S_IFMT & one->mode) != (S_IFMT & two->mode)) {\n                /*\n                 * a filepair that changes between file and symlink\n                 * needs to be split into deletion and creation.\n                 */\n                struct diff_filespec *null = alloc_filespec(two->path);\n                run_diff_cmd(NULL, name, other, attr_path,\n                             one, null, &msg, o, p);\n                free(null);\n                strbuf_release(&msg);\n\n                null = alloc_filespec(one->path);\n                run_diff_cmd(NULL, name, other, attr_path,\n                             null, two, &msg, o, p);\n                free(null);\n        }\n        else\n                run_diff_cmd(pgm, name, other, attr_path,\n                             one, two, &msg, o, p);\n\nIOW, we split up a typechange if we are feeding it to the internal diff\ngenerator, because builtin_diff will not show diffs between different\ntypes. But the check for \"!pgm\" here is not right; we don't know yet\nwhether we will be builtin or external, because we have not checked\nALLOW_EXTERNAL yet.\n\nSo I think your fix is the right thing, but the bug it is fixing is not\n\"do not run external diff even when --no-ext-diff is specified\". It is\n\"do not accidentally feed typechange diffs to builtin_diff\".\n\nYou can see the difference in output with this script (and it works fine\nwith your patch applied):\n\n    git init -q repo && cd repo &&\n    echo content >file && git add file && git commit -q -m regular &&\n    rm file && ln -s dest file && git commit -q -a -m typechange &&\n    export GIT_PAGER=cat &&\n    export GIT_EXTERNAL_DIFF='echo doing external diff' &&\n    git show HEAD^ --format='=== %s, ext ===' --ext-diff &&\n    git show HEAD^ --format='=== %s, no-ext ===' --no-ext-diff &&\n    git show HEAD  --format='=== %s, ext ===' --ext-diff &&\n    git show HEAD  --format='=== %s, no-ext ===' --no-ext-diff\n\n-Peff\n"},{"id":"195238","messageId":"000e01cd6481$bbd63970$3382ac50$@vrana.cz","threadId":"31041","inReplyTo":"20120717041603.GD20945@sigill.intra.peff.net","subject":"RE: [PATCH] diff: respect --no-ext-diff with typechange","fromName":"Jakub Vrana","fromEmail":"jakub@vrana.cz","sentAt":"2012-07-18T01:07:40Z","receivedAt":"2012-07-18T01:07:40Z","isPatch":true,"sender":{"key":"jakub@vrana.cz","avatar":null},"body":"Yes, I was fixing the invalid (!pgm) condition, sorry for a non-precise description.\n\nDoes it mean that my patch is accepted or is there something else I need to do?\n\n-- \nJakub Vrana\n\n\n-----Original Message-----\nFrom: Jeff King [mailto:peff@peff.net] \nSent: Monday, July 16, 2012 9:16 PM\nTo: Jakub Vrana\nCc: git@vger.kernel.org; gitster@pobox.com\nSubject: Re: [PATCH] diff: respect --no-ext-diff with typechange\n\nOn Mon, Jul 16, 2012 at 05:27:00PM -0700, Jakub Vrana wrote:\n\n> If external diff is specified through diff.external then it is used \n> even if `git diff --no-ext-diff` is used when there is a typechange.\n\nEek. That has some minor security implications, as it means that it is dangerous to run even plumbing inspection command in somebody else's repository.\n\nHowever...\n\n>  diff.c |    3 +++\n>  1 file changed, 3 insertions(+)\n> \n> diff --git a/diff.c b/diff.c\n> index 208096f..898d610 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -3074,6 +3074,9 @@ static void run_diff(struct diff_filepair *p, \n> struct diff_options *o)\n>  \tif (o->prefix_length)\n>  \t\tstrip_prefix(o->prefix_length, &name, &other);\n>  \n> +\tif (!DIFF_OPT_TST(o, ALLOW_EXTERNAL))\n> +\t\tpgm = NULL;\n> +\n>  \tif (DIFF_PAIR_UNMERGED(p)) {\n>  \t\trun_diff_cmd(pgm, name, NULL, attr_path,\n>  \t\t\t     NULL, NULL, NULL, o, p);\n\nrun_diff_cmd already checks the ALLOW_EXTERNAL bit and sets pgm to NULL there. So as far as I can tell, we are not actually running the external diff. However, there is still a problem. Later in run_diff we do:\n\n        if (!pgm &&\n            DIFF_FILE_VALID(one) && DIFF_FILE_VALID(two) &&\n            (S_IFMT & one->mode) != (S_IFMT & two->mode)) {\n                /*\n                 * a filepair that changes between file and symlink\n                 * needs to be split into deletion and creation.\n                 */\n                struct diff_filespec *null = alloc_filespec(two->path);\n                run_diff_cmd(NULL, name, other, attr_path,\n                             one, null, &msg, o, p);\n                free(null);\n                strbuf_release(&msg);\n\n                null = alloc_filespec(one->path);\n                run_diff_cmd(NULL, name, other, attr_path,\n                             null, two, &msg, o, p);\n                free(null);\n        }\n        else\n                run_diff_cmd(pgm, name, other, attr_path,\n                             one, two, &msg, o, p);\n\nIOW, we split up a typechange if we are feeding it to the internal diff generator, because builtin_diff will not show diffs between different types. But the check for \"!pgm\" here is not right; we don't know yet whether we will be builtin or external, because we have not checked ALLOW_EXTERNAL yet.\n\nSo I think your fix is the right thing, but the bug it is fixing is not \"do not run external diff even when --no-ext-diff is specified\". It is \"do not accidentally feed typechange diffs to builtin_diff\".\n\nYou can see the difference in output with this script (and it works fine with your patch applied):\n\n    git init -q repo && cd repo &&\n    echo content >file && git add file && git commit -q -m regular &&\n    rm file && ln -s dest file && git commit -q -a -m typechange &&\n    export GIT_PAGER=cat &&\n    export GIT_EXTERNAL_DIFF='echo doing external diff' &&\n    git show HEAD^ --format='=== %s, ext ===' --ext-diff &&\n    git show HEAD^ --format='=== %s, no-ext ===' --no-ext-diff &&\n    git show HEAD  --format='=== %s, ext ===' --ext-diff &&\n    git show HEAD  --format='=== %s, no-ext ===' --no-ext-diff\n\n-Peff\n"},{"id":"195242","messageId":"7v7gu1y7tg.fsf@alter.siamese.dyndns.org","threadId":"31041","inReplyTo":"000e01cd6481$bbd63970$3382ac50$@vrana.cz","subject":"Re: [PATCH] diff: respect --no-ext-diff with typechange","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-18T05:08:59Z","receivedAt":"2012-07-18T05:08:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Jakub Vrana\" <jakub@vrana.cz> writes:\n\n> Yes, I was fixing the invalid (!pgm) condition, sorry for a non-precise description.\n>\n> Does it mean that my patch is accepted or is there something else I need to do?\n\nThe impression I got from Peff's review was that the problem\ndescription in the proposed commit log message did not describe the\nreality at all, and the added three lines did not do what the\nmessage implied they do.  So I do not see how it can be acceptable\nby anybody.\n\nIt also needs a test to protect this fix from being broken by other\npeople in the future.\n\nI've followed the codepath myself, and here is what I would have\nliked the submitted patch to look like.  Note that run_diff_cmd()\nno longer needs to reset pgm to NULL based on ALLOW_EXTERNAL, but\nit still needs to look at it to decide if per-path external userdiff\nneeds to be called.\n\n-- >8 --\nSubject: diff: correctly disable external_diff with --no-ext-diff\n\nUpon seeing a type-change filepair, \"diff --no-ext-diff\" does not\nshow the usual \"deletion followed by addition\" split patch and does\nnot run the external diff driver either.\n\nThis is because the logic to disable external diff was placed at a\nwrong level in the callchain.  run_diff_cmd() decides to show the\nsplit patch only when external diff driver is not configured or\nspecified via GIT_EXTERNAL_DIFF environment, but this is done before\nchecking if --no-ext-diff was given.  To make things worse,\nrun_diff_cmd() checks --no-ext-diff and disables the output for such\na filepair completely, as the callchain below it (e.g. builtin_diff)\ndoes not want to handle typechange filepairs.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n * The use of userdiff_find_by_path() in run_diff_cmd() may be iffy;\n   it is probably OK to override diff.external with a more specific\n   per-path configuration, but I think an external diff specified by\n   the GIT_EXTERNAL_DIFF environment may want to trump the\n   configured per-path one, as an environment is a stronger one-shot\n   request.\n\n   But this patch is not about changing that semantics, so I left it\n   as-is. \n\n diff.c                   |  8 +++++---\n t/t4020-diff-external.sh | 19 +++++++++++++++++++\n 2 files changed, 24 insertions(+), 3 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 208096f..62cbe14 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2992,9 +2992,8 @@ static void run_diff_cmd(const char *pgm,\n \tint complete_rewrite = (p->status == DIFF_STATUS_MODIFIED) && p->score;\n \tint must_show_header = 0;\n \n-\tif (!DIFF_OPT_TST(o, ALLOW_EXTERNAL))\n-\t\tpgm = NULL;\n-\telse {\n+\n+\tif (DIFF_OPT_TST(o, ALLOW_EXTERNAL)) {\n \t\tstruct userdiff_driver *drv = userdiff_find_by_path(attr_path);\n \t\tif (drv && drv->external)\n \t\t\tpgm = drv->external;\n@@ -3074,6 +3073,9 @@ static void run_diff(struct diff_filepair *p, struct diff_options *o)\n \tif (o->prefix_length)\n \t\tstrip_prefix(o->prefix_length, &name, &other);\n \n+\tif (!DIFF_OPT_TST(o, ALLOW_EXTERNAL))\n+\t\tpgm = NULL;\n+\n \tif (DIFF_PAIR_UNMERGED(p)) {\n \t\trun_diff_cmd(pgm, name, NULL, attr_path,\n \t\t\t     NULL, NULL, NULL, o, p);\ndiff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh\nindex 533afc1..5a5f68c 100755\n--- a/t/t4020-diff-external.sh\n+++ b/t/t4020-diff-external.sh\n@@ -48,7 +48,26 @@ test_expect_success 'GIT_EXTERNAL_DIFF environment and --no-ext-diff' '\n \n '\n \n+test_expect_success SYMLINKS 'typechange diff' '\n+\trm -f file &&\n+\tln -s elif file &&\n+\tGIT_EXTERNAL_DIFF=echo git diff  | {\n+\t\tread path oldfile oldhex oldmode newfile newhex newmode &&\n+\t\ttest \"z$path\" = zfile &&\n+\t\ttest \"z$oldmode\" = z100644 &&\n+\t\ttest \"z$newhex\" = \"z$_z40\" &&\n+\t\ttest \"z$newmode\" = z120000 &&\n+\t\toh=$(git rev-parse --verify HEAD:file) &&\n+\t\ttest \"z$oh\" = \"z$oldhex\"\n+\t} &&\n+\tGIT_EXTERNAL_DIFF=echo git diff --no-ext-diff >actual &&\n+\tgit diff >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'diff attribute' '\n+\tgit reset --hard &&\n+\techo third >file &&\n \n \tgit config diff.parrot.command echo &&\n \n"},{"id":"195249","messageId":"20120718062329.GA12942@sigill.intra.peff.net","threadId":"31041","inReplyTo":"7v7gu1y7tg.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] diff: respect --no-ext-diff with typechange","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-18T06:23:29Z","receivedAt":"2012-07-18T06:23:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 17, 2012 at 10:08:59PM -0700, Junio C Hamano wrote:\n\n> The impression I got from Peff's review was that the problem\n> description in the proposed commit log message did not describe the\n> reality at all, and the added three lines did not do what the\n> message implied they do.  So I do not see how it can be acceptable\n> by anybody.\n> \n> It also needs a test to protect this fix from being broken by other\n> people in the future.\n\nYeah, exactly.\n\n> -- >8 --\n> Subject: diff: correctly disable external_diff with --no-ext-diff\n> \n> Upon seeing a type-change filepair, \"diff --no-ext-diff\" does not\n> show the usual \"deletion followed by addition\" split patch and does\n> not run the external diff driver either.\n> \n> This is because the logic to disable external diff was placed at a\n> wrong level in the callchain.  run_diff_cmd() decides to show the\n> split patch only when external diff driver is not configured or\n> specified via GIT_EXTERNAL_DIFF environment, but this is done before\n> checking if --no-ext-diff was given.  To make things worse,\n> run_diff_cmd() checks --no-ext-diff and disables the output for such\n> a filepair completely, as the callchain below it (e.g. builtin_diff)\n> does not want to handle typechange filepairs.\n> \n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n\nYour patch looks good to me.\n\n> ---\n>  * The use of userdiff_find_by_path() in run_diff_cmd() may be iffy;\n>    it is probably OK to override diff.external with a more specific\n>    per-path configuration, but I think an external diff specified by\n>    the GIT_EXTERNAL_DIFF environment may want to trump the\n>    configured per-path one, as an environment is a stronger one-shot\n>    request.\n\nI think this date all the way back to f1af60b (Support 'diff=pgm'\nattribute, 2007-04-22). There's a tradeoff here; usually environment\nvariables trump config, but you end up using a large hammer (here is how\nto diff _all_ files externally) to hit a small nail (here is how to diff\n_just_ this file).  I suspect it isn't that big a problem in practice\nbecause people tend to use either one mechanism or the other.\n\nThe most sensible thing to me is probably $GIT_EXTERNAL_DIFF, followed\nby attributes, followed by diff.external. That uses the more specific\ndiff pulled from the on-disk config, but allows you to do one-shot overrides\nwith the environment as long as you are careful to restrict your command\n(e.g., \"GIT_EXTERNAL_DIFF=foo-differ git diff -- file.foo\").\n\n>    But this patch is not about changing that semantics, so I left it\n>    as-is.\n\nSounds sensible.\n\n-Peff\n"},{"id":"195250","messageId":"20120718070626.GA20938@sigill.intra.peff.net","threadId":"31041","inReplyTo":"20120718062329.GA12942@sigill.intra.peff.net","subject":"Re: [PATCH] diff: respect --no-ext-diff with typechange","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-18T07:06:26Z","receivedAt":"2012-07-18T07:06:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 18, 2012 at 02:23:29AM -0400, Jeff King wrote:\n\n> >  * The use of userdiff_find_by_path() in run_diff_cmd() may be iffy;\n> >    it is probably OK to override diff.external with a more specific\n> >    per-path configuration, but I think an external diff specified by\n> >    the GIT_EXTERNAL_DIFF environment may want to trump the\n> >    configured per-path one, as an environment is a stronger one-shot\n> >    request.\n> \n> I think this date all the way back to f1af60b (Support 'diff=pgm'\n> attribute, 2007-04-22). There's a tradeoff here; usually environment\n> variables trump config, but you end up using a large hammer (here is how\n> to diff _all_ files externally) to hit a small nail (here is how to diff\n> _just_ this file).  I suspect it isn't that big a problem in practice\n> because people tend to use either one mechanism or the other.\n> \n> The most sensible thing to me is probably $GIT_EXTERNAL_DIFF, followed\n> by attributes, followed by diff.external. That uses the more specific\n> diff pulled from the on-disk config, but allows you to do one-shot overrides\n> with the environment as long as you are careful to restrict your command\n> (e.g., \"GIT_EXTERNAL_DIFF=foo-differ git diff -- file.foo\").\n\nHere's a patch implementing that. This is definitely how I would have\ndone it if I were starting from scratch. My only hesitation now is that\nI don't care too deeply either way, and it is technically a behavior\nchange. So there is a chance of regression for something that nobody has\nactually complained about. To be honest, I doubt many people are using\nexternal diff at all these days; textconv is closer to what most people\nwant, and is much easier to use.\n\n-- >8 --\nSubject: [PATCH] diff: fix precedence of external diff options\n\nThere are three ways to specify an external diff command:\nGIT_EXTERNAL_DIFF in the environment, diff.external in the\nconfig, or a \"diff\" gitattribute. The current order of\nprecedence is:\n\n  1. gitattribute\n\n  2. GIT_EXTERNAL_DIFF\n\n  3. diff.external\n\nBut usually our rule is that environment variables should\ntake precedence over on-disk config (i.e., option 2 should\ncome before option 1). This situation is trickier than some,\nbecause option 1 is more specific to the individual file\nthan option 2 (which affects all files), so it might be\npreferable. So the current behavior can be seen as\nimplementing \"do the specific thing if we can, but fall back\nto this general thing\".\n\nHowever, since you can already implement that behavior (by\ncombining attributes with a diff.external setting), and\nbecause environment variables can be useful for one-shot\noverrides (e.g., \"GIT_EXTERNAL_DIFF=foo git diff -- bar\" to\noverride bar's gitattribute setting), let's switch the\nprecedence of options 1 and 2 above.\n\nWhile we're adding tests to t4020 for the precedence, let's\nalso make sure that diff.external works at all by running it\nthrough the same tests as GIT_EXTERNAL_DIFF.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n diff.c                   | 35 +++++++++++++++--------------------\n t/t4020-diff-external.sh | 41 +++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 56 insertions(+), 20 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 62cbe14..ed2de96 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -238,18 +238,20 @@ static char *quote_two(const char *one, const char *two)\n \treturn strbuf_detach(&res, NULL);\n }\n \n-static const char *external_diff(void)\n+static const char *external_diff(const char *path)\n {\n-\tstatic const char *external_diff_cmd = NULL;\n-\tstatic int done_preparing = 0;\n+\tconst char *r;\n+\tstruct userdiff_driver *drv;\n \n-\tif (done_preparing)\n-\t\treturn external_diff_cmd;\n-\texternal_diff_cmd = getenv(\"GIT_EXTERNAL_DIFF\");\n-\tif (!external_diff_cmd)\n-\t\texternal_diff_cmd = external_diff_cmd_cfg;\n-\tdone_preparing = 1;\n-\treturn external_diff_cmd;\n+\tr = getenv(\"GIT_EXTERNAL_DIFF\");\n+\tif (r)\n+\t\treturn r;\n+\n+\tdrv = userdiff_find_by_path(path);\n+\tif (drv && drv->external)\n+\t\treturn drv->external;\n+\n+\treturn external_diff_cmd_cfg;\n }\n \n static struct diff_tempfile {\n@@ -2992,13 +2994,6 @@ static void run_diff_cmd(const char *pgm,\n \tint complete_rewrite = (p->status == DIFF_STATUS_MODIFIED) && p->score;\n \tint must_show_header = 0;\n \n-\n-\tif (DIFF_OPT_TST(o, ALLOW_EXTERNAL)) {\n-\t\tstruct userdiff_driver *drv = userdiff_find_by_path(attr_path);\n-\t\tif (drv && drv->external)\n-\t\t\tpgm = drv->external;\n-\t}\n-\n \tif (msg) {\n \t\t/*\n \t\t * don't use colors when the header is intended for an\n@@ -3059,7 +3054,7 @@ static void strip_prefix(int prefix_length, const char **namep, const char **oth\n \n static void run_diff(struct diff_filepair *p, struct diff_options *o)\n {\n-\tconst char *pgm = external_diff();\n+\tconst char *pgm = NULL;\n \tstruct strbuf msg;\n \tstruct diff_filespec *one = p->one;\n \tstruct diff_filespec *two = p->two;\n@@ -3073,8 +3068,8 @@ static void run_diff(struct diff_filepair *p, struct diff_options *o)\n \tif (o->prefix_length)\n \t\tstrip_prefix(o->prefix_length, &name, &other);\n \n-\tif (!DIFF_OPT_TST(o, ALLOW_EXTERNAL))\n-\t\tpgm = NULL;\n+\tif (DIFF_OPT_TST(o, ALLOW_EXTERNAL))\n+\t\tpgm = external_diff(attr_path);\n \n \tif (DIFF_PAIR_UNMERGED(p)) {\n \t\trun_diff_cmd(pgm, name, NULL, attr_path,\ndiff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh\nindex 5a5f68c..80f5b64 100755\n--- a/t/t4020-diff-external.sh\n+++ b/t/t4020-diff-external.sh\n@@ -65,6 +65,33 @@ test_expect_success SYMLINKS 'typechange diff' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'diff.external' '\n+\tgit reset --hard &&\n+\techo third >file &&\n+\ttest_config diff.external echo &&\n+\tgit diff | {\n+\t\tread path oldfile oldhex oldmode newfile newhex newmode &&\n+\t\ttest \"z$path\" = zfile &&\n+\t\ttest \"z$oldmode\" = z100644 &&\n+\t\ttest \"z$newhex\" = \"z$_z40\" &&\n+\t\ttest \"z$newmode\" = z100644 &&\n+\t\toh=$(git rev-parse --verify HEAD:file) &&\n+\t\ttest \"z$oh\" = \"z$oldhex\"\n+\t}\n+'\n+\n+test_expect_success 'diff.external should apply only to diff' '\n+\ttest_config diff.external echo &&\n+\tgit log -p -1 HEAD |\n+\tgrep \"^diff --git a/file b/file\"\n+'\n+\n+test_expect_success 'diff.external and --no-ext-diff' '\n+\ttest_config diff.external echo &&\n+\tgit diff --no-ext-diff |\n+\tgrep \"^diff --git a/file b/file\"\n+'\n+\n test_expect_success 'diff attribute' '\n \tgit reset --hard &&\n \techo third >file &&\n@@ -132,6 +159,20 @@ test_expect_success 'diff attribute and --no-ext-diff' '\n \n '\n \n+test_expect_success 'diff attribute trumps diff.external' '\n+\ttest_config diff.foo.command \"echo ext-attribute\" &&\n+\ttest_config diff.external \"echo ext-global\" &&\n+\techo \"file diff=foo\" >.gitattributes &&\n+\tgit diff | grep ext-attribute\n+'\n+\n+test_expect_success 'GIT_EXTERNAL_DIFF trumps attributes and diff.external' '\n+\ttest_config diff.foo.command \"echo ext-attribute\" &&\n+\ttest_config diff.external \"echo ext-global\" &&\n+\techo \"file diff=foo\" >.gitattributes &&\n+\tGIT_EXTERNAL_DIFF=\"echo ext-env\" git diff | grep ext-env\n+'\n+\n test_expect_success 'no diff with -diff' '\n \techo >.gitattributes \"file -diff\" &&\n \tgit diff | grep Binary\n-- \n1.7.10.5.40.gbbc17de\n"},{"id":"195302","messageId":"7vd33swuti.fsf@alter.siamese.dyndns.org","threadId":"31041","inReplyTo":"20120718070626.GA20938@sigill.intra.peff.net","subject":"Re: [PATCH] diff: respect --no-ext-diff with typechange","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-18T22:47:21Z","receivedAt":"2012-07-18T22:47:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I don't care too deeply either way, and it is technically a behavior\n> change. So there is a chance of regression for something that nobody has\n> actually complained about.\n\nThanks. I share the same feeling, but the code after the patch looks\nmuch nicer, which is somewhat sad.\n\n> To be honest, I doubt many people are using\n> external diff at all these days; textconv is closer to what most people\n> want, and is much easier to use.\n\nIt depends on the payload and the output you want.  I wouldn't think\nit is fair to challenge anybody to come up with a solution based on\nthe textconv machinery to replicate what the compare-cooking.perl in\nthe 'todo' branch (used as an external diff driver for\nwhats-cooking.txt) does.\n"},{"id":"195327","messageId":"20120719114938.GB29774@sigill.intra.peff.net","threadId":"31041","inReplyTo":"7vd33swuti.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] diff: respect --no-ext-diff with typechange","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-19T11:49:38Z","receivedAt":"2012-07-19T11:49:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 18, 2012 at 03:47:21PM -0700, Junio C Hamano wrote:\n\n> > I don't care too deeply either way, and it is technically a behavior\n> > change. So there is a chance of regression for something that nobody has\n> > actually complained about.\n> \n> Thanks. I share the same feeling, but the code after the patch looks\n> much nicer, which is somewhat sad.\n\nIf we're not going to do it, perhaps we should at least include the\ntests I wrote (modified for the current behavior), since there was no\ncoverage in this area previously. Patch is below.\n\n> > To be honest, I doubt many people are using\n> > external diff at all these days; textconv is closer to what most people\n> > want, and is much easier to use.\n> \n> It depends on the payload and the output you want.  I wouldn't think\n> it is fair to challenge anybody to come up with a solution based on\n> the textconv machinery to replicate what the compare-cooking.perl in\n> the 'todo' branch (used as an external diff driver for\n> whats-cooking.txt) does.\n\nOh, absolutely. The compare-cooking script is a great example of what\nyou can do with the flexibility that external diff offers. But based on\nmy experience and reading the list, the much more common request is to\nproduce a meaningful diff of two binary word processor documents. :)\n\nGoogling around, the other common use seems to be to stuff the output\ninto a visual diff tool. Though I think that \"git difftool\" is a better\napproach for that.\n\n-Peff\n\n-- >8 --\nSubject: [PATCH] diff: test precedence of external diff drivers\n\nThere are three ways to specify an external diff command:\nGIT_EXTERNAL_DIFF in the environment, diff.external in the\nconfig, or a \"diff\" gitattribute. The current order of\nprecedence is:\n\n  1. gitattribute\n\n  2. GIT_EXTERNAL_DIFF\n\n  3. diff.external\n\nUsually our rule is that environment variables should take\nprecedence over on-disk config (i.e., option 2 should come\nbefore option 1). However, this situation is trickier than\nsome, because option 1 is more specific to the individual\nfile than option 2 (which affects all files), so it might be\npreferable. So the current behavior can be seen as\nimplementing \"do the specific thing if we can, but fall back\nto this general thing\".\n\nThis is probably not what we would do if we were writing git\nfrom scratch, but it has been this way for several years,\nand is not worth changing. So let's at least document that\nthis is the way it's supposed to work with a test.\n\nWhile we're there, let's also make sure that diff.external\n(which was not previously tested at all) works by running it\nthrough the same tests as GIT_EXTERNAL_DIFF.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t4020-diff-external.sh | 40 ++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 40 insertions(+)\n\ndiff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh\nindex 5a5f68c..2e7d73f 100755\n--- a/t/t4020-diff-external.sh\n+++ b/t/t4020-diff-external.sh\n@@ -65,6 +65,33 @@ test_expect_success SYMLINKS 'typechange diff' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'diff.external' '\n+\tgit reset --hard &&\n+\techo third >file &&\n+\ttest_config diff.external echo &&\n+\tgit diff | {\n+\t\tread path oldfile oldhex oldmode newfile newhex newmode &&\n+\t\ttest \"z$path\" = zfile &&\n+\t\ttest \"z$oldmode\" = z100644 &&\n+\t\ttest \"z$newhex\" = \"z$_z40\" &&\n+\t\ttest \"z$newmode\" = z100644 &&\n+\t\toh=$(git rev-parse --verify HEAD:file) &&\n+\t\ttest \"z$oh\" = \"z$oldhex\"\n+\t}\n+'\n+\n+test_expect_success 'diff.external should apply only to diff' '\n+\ttest_config diff.external echo &&\n+\tgit log -p -1 HEAD |\n+\tgrep \"^diff --git a/file b/file\"\n+'\n+\n+test_expect_success 'diff.external and --no-ext-diff' '\n+\ttest_config diff.external echo &&\n+\tgit diff --no-ext-diff |\n+\tgrep \"^diff --git a/file b/file\"\n+'\n+\n test_expect_success 'diff attribute' '\n \tgit reset --hard &&\n \techo third >file &&\n@@ -132,6 +159,19 @@ test_expect_success 'diff attribute and --no-ext-diff' '\n \n '\n \n+test_expect_success 'GIT_EXTERNAL_DIFF trumps diff.external' '\n+\t>.gitattributes &&\n+\ttest_config diff.external \"echo ext-global\" &&\n+\tGIT_EXTERNAL_DIFF=\"echo ext-env\" git diff | grep ext-env\n+'\n+\n+test_expect_success 'attributes trump GIT_EXTERNAL_DIFF and diff.external' '\n+\ttest_config diff.foo.command \"echo ext-attribute\" &&\n+\ttest_config diff.external \"echo ext-global\" &&\n+\techo \"file diff=foo\" >.gitattributes &&\n+\tGIT_EXTERNAL_DIFF=\"echo ext-env\" git diff | grep ext-attribute\n+'\n+\n test_expect_success 'no diff with -diff' '\n \techo >.gitattributes \"file -diff\" &&\n \tgit diff | grep Binary\n-- \n1.7.10.5.40.g059818d\n"}]}