{"thread":{"id":"41761","subject":"git diff --exit-code does not honour textconv setting","startedAt":"2016-03-20T12:43:53Z","lastAt":"2016-04-06T06:44:21Z","messageCount":4,"participants":["Georg Pichler","Michael J Gruber","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"281273","messageId":"56EE9B09.6040700@gmail.com","threadId":"41761","inReplyTo":null,"subject":"git diff --exit-code does not honour textconv setting","fromName":"Georg Pichler","fromEmail":"georg.pichler@gmail.com","sentAt":"2016-03-20T12:43:53Z","receivedAt":"2016-03-20T12:43:53Z","isPatch":false,"sender":{"key":"georg.pichler@gmail.com","avatar":null},"body":"Hi,\n\nI realized that \"git diff --exit-code\" does not honour textconv settings.\nMaybe this behaviour is desired. It can be partially circumvented by using the \"-b\" flag if one does not care about whitespace changes.\nTo reproduce this, create an empty repository and run the following commands:\n\n(I was using git version 2.7.3)\n\n$ git config --add diff.void.textconv test\n$ echo \"foo diff=void\" >.gitattributes\n$ echo foo >foo\n$ git add . && git commit -m \"Init\"\n[master (root-commit) 70c39d9] Init\n2 files changed, 2 insertions(+)\ncreate mode 100644 .gitattributes\ncreate mode 100644 foo\n$ echo bar >foo\n$ git status\nOn branch master\nChanges not staged for commit:\n(use \"git add <file>...\" to update what will be committed)\n(use \"git checkout -- <file>...\" to discard changes in working directory)\n\nmodified: foo\n\nno changes added to commit (use \"git add\" and/or \"git commit -a\")\n$ git diff\n$ git diff --exit-code\n[exits with 1, no output]\n$ git diff --exit-code -b\n[exits with 0, no output]\n\nThe \"test\" command is used as it does not generate any output on stdout.\n\nI would expect \"git diff --exit-code\" to return with exit code 0. If this is not desired, it should be clearly stated in the man page,\nthat \"--exit-code\" does not honour the textconv setting, except if \"-b\" is given. Currently this is not clear:\n\n       --exit-code\n           Make the program exit with codes similar to diff(1). That is, it exits\n           with 1 if there were differences and 0 means no differences.\n\nBest,\nGeorg Pichler\n\n"},{"id":"282705","messageId":"5703CA0D.2090808@drmicha.warpmail.net","threadId":"41761","inReplyTo":"56EE9B09.6040700@gmail.com","subject":"Re: git diff --exit-code does not honour textconv setting","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2016-04-05T14:22:05Z","receivedAt":"2016-04-05T14:22:05Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Georg Pichler venit, vidit, dixit 20.03.2016 13:43:\n> Hi,\n> \n> I realized that \"git diff --exit-code\" does not honour textconv settings.\n> Maybe this behaviour is desired. It can be partially circumvented by using the \"-b\" flag if one does not care about whitespace changes.\n> To reproduce this, create an empty repository and run the following commands:\n> \n> (I was using git version 2.7.3)\n> \n> $ git config --add diff.void.textconv test\n> $ echo \"foo diff=void\" >.gitattributes\n> $ echo foo >foo\n> $ git add . && git commit -m \"Init\"\n> [master (root-commit) 70c39d9] Init\n> 2 files changed, 2 insertions(+)\n> create mode 100644 .gitattributes\n> create mode 100644 foo\n> $ echo bar >foo\n> $ git status\n> On branch master\n> Changes not staged for commit:\n> (use \"git add <file>...\" to update what will be committed)\n> (use \"git checkout -- <file>...\" to discard changes in working directory)\n> \n> modified: foo\n> \n> no changes added to commit (use \"git add\" and/or \"git commit -a\")\n> $ git diff\n> $ git diff --exit-code\n> [exits with 1, no output]\n> $ git diff --exit-code -b\n> [exits with 0, no output]\n> \n> The \"test\" command is used as it does not generate any output on stdout.\n\n\"test\" is a bit of a red herring here since it will receive commands.\nBut your example works even with \"true\" which ignores its commands and\nproduces no output.\n\n> I would expect \"git diff --exit-code\" to return with exit code 0. If this is not desired, it should be clearly stated in the man page,\n> that \"--exit-code\" does not honour the textconv setting, except if \"-b\" is given. Currently this is not clear:\n> \n>        --exit-code\n>            Make the program exit with codes similar to diff(1). That is, it exits\n>            with 1 if there were differences and 0 means no differences.\n> \n> Best,\n> Georg Pichler\n\nThe description doesn't make it clear whether exit-code refers to the\nactual diff (foo vs. bar) or to the diff after textconv (empty vs.\nempty). In any case, \"-b\" should not make a difference for your example.\n\n\ndiff_flush() in diff.c has this piece of code:\n\n        /*\n         * Report the content-level differences with HAS_CHANGES;\n         * diff_addremove/diff_change does not set the bit when\n         * DIFF_FROM_CONTENTS is in effect (e.g. with -w).\n         */\n        if (DIFF_OPT_TST(options, DIFF_FROM_CONTENTS)) {\n                if (options->found_changes)\n                        DIFF_OPT_SET(options, HAS_CHANGES);\n                else\n                        DIFF_OPT_CLR(options, HAS_CHANGES);\n        }\n\nSo it's clear that depending on \"-b\" (or \"-w\") or not, it's taking\nshortcuts or looking at the textconved diff but I'm not sure where's the\nproper place to fix that.\n\nMichael\n"},{"id":"282740","messageId":"xmqqvb3vzlw5.fsf@gitster.mtv.corp.google.com","threadId":"41761","inReplyTo":"5703CA0D.2090808@drmicha.warpmail.net","subject":"Re: git diff --exit-code does not honour textconv setting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-05T23:16:58Z","receivedAt":"2016-04-05T23:16:58Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber <git@drmicha.warpmail.net> writes:\n\n>> The \"test\" command is used as it does not generate any output on stdout.\n>\n> \"test\" is a bit of a red herring here since it will receive commands.\n> But your example works even with \"true\" which ignores its commands and\n> produces no output.\n\nEither sounds strange, as they exit without reading their input at\nall, so the other side of the pipe may get an write error it has to\nhandle ;-)\n\n> The description doesn't make it clear whether exit-code refers to\n> the actual diff (foo vs. bar) or to the diff after textconv (empty\n> vs. empty). In any case, \"-b\" should not make a difference for\n> your example.\n>\n> diff_flush() in diff.c has this piece of code:\n>\n\nIt is understandable that \"-b\" makes difference.  The actual\nshort-cut happens a bit before the code you quoted.\n\n\tif (output_format & DIFF_FORMAT_NO_OUTPUT &&\n\t    DIFF_OPT_TST(options, EXIT_WITH_STATUS) &&\n\t    DIFF_OPT_TST(options, DIFF_FROM_CONTENTS)) {\n\t\t/*\n\t\t * run diff_flush_patch for the exit status. setting\n\t\t * options->file to /dev/null should be safe, because we\n\t\t * aren't supposed to produce any output anyway.\n\t\t */\n\t\tif (options->close_file)\n\t\t\tfclose(options->file);\n\t\toptions->file = fopen(\"/dev/null\", \"w\");\n\t\tif (!options->file)\n\t\t\tdie_errno(\"Could not open /dev/null\");\n\t\toptions->close_file = 1;\n\t\tfor (i = 0; i < q->nr; i++) {\n\t\t\tstruct diff_filepair *p = q->queue[i];\n\t\t\tif (check_pair_status(p))\n\t\t\t\tdiff_flush_patch(p, options);\n\t\t\tif (options->found_changes)\n\t\t\t\tbreak;\n\t\t}\n\t}\n\nWhen running without producing output but we need the exit status,\nunless DIFF_FROM_CONTENTS is set (e.g. \"-b\" in use), we do not run\nthe code that would inspect the blob contents that would have\nproduced the actual patch.  When DIFF_FROM_CONTENTS is set, we need\nto dig into the blob contents and the body of the above if() gets\nrun only to trigger o->found_changes (e.g. in builtin_diff() that\nsets ecbdata.found_changesp to point at o->found_changes before\ncalling into the xdiff machinery), but the output is discarded to\n/dev/null.\n\nThe textconv filter is an unfortunate beast, in that while some\npaths use one while others don't, so it won't be as simple as \"-b\"\nthat flips DIFF_FROM_CONTENTS to say \"We need to inspect blobs for\nALL FILEPAIRS to see if there is any difference\" if we want to keep\nthe optimization as much as possible.\n\nWithout DIFF_FROM_CONTENTS set, any filepair that point at two\ndifferent blobs that might end up to be the same after applying\ntextconv will flip the o->found_changes bit on, and with\nEXIT_WITH_STATUS bit on, we have another optimization to skip\nchecking the remainder of the filepairs.  So if you want to support\ntextconv with QUICK, you would also need to disable that\noptimization by teaching diff_change() to refrain from setting\nHAS_CHANGES bit, i.e.\n\n    diff --git a/diff.h b/diff.h\n    index 125447b..f6c0c07 100644\n    --- a/diff.h\n    +++ b/diff.h\n    @@ -69,7 +69,7 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)\n     #define DIFF_OPT_FIND_COPIES_HARDER  (1 <<  6)\n     #define DIFF_OPT_FOLLOW_RENAMES      (1 <<  7)\n     #define DIFF_OPT_RENAME_EMPTY        (1 <<  8)\n    -/* (1 <<  9) unused */\n    +#define DIFF_OPT_WITH_TEXTCONV       (1 <<  9)\n     #define DIFF_OPT_HAS_CHANGES         (1 << 10)\n     #define DIFF_OPT_QUICK               (1 << 11)\n     #define DIFF_OPT_NO_INDEX            (1 << 12)\n\n    diff --git a/diff.c b/diff.c\n    index 4dfe660..0016ad2 100644\n    --- a/diff.c\n    +++ b/diff.c\n    @@ -5018,6 +5018,11 @@ void diff_change(struct diff_options *options,\n            two->dirty_submodule = new_dirty_submodule;\n            p = diff_queue(&diff_queued_diff, one, two);\n\n    +       if (either path one or path two has textconv defined) {\n    +               DIFF_OPT_SET(options, DIFF_WITH_TEXTCONV);\n    +               return;\n    +       }\n    +\n            if (DIFF_OPT_TST(options, DIFF_FROM_CONTENTS))\n                    return;\n\nAnd then in order to keep the optimization, add this to the above\ncodepath:\n\n    diff --git a/diff.c b/diff.c\n    index 4dfe660..7b318cc 100644\n    --- a/diff.c\n    +++ b/diff.c\n    @@ -4598,6 +4598,7 @@ void diff_flush(struct diff_options *options)\n            int i, output_format = options->output_format;\n            int separator = 0;\n            int dirstat_by_line = 0;\n    +       int textconv_hack;\n\n            /*\n             * Order: raw, stat, summary, patch\n    @@ -4652,9 +4653,17 @@ void diff_flush(struct diff_options *options)\n                    separator++;\n            }\n\n    +       /*\n    +        * If there is any path that needs textconv and we haven't\n    +        * found any change on paths that don't, we need to pass\n    +        * them through textconv and see the textual difference.\n    +        */\n    +       textconv_hack = (DIFF_OPT_TST(options, DIFF_WITH_TEXTCONV) &&\n    +                        !DIFF_OPT_TST(options, HAS_CHANGES));\n    +\n            if (output_format & DIFF_FORMAT_NO_OUTPUT &&\n                DIFF_OPT_TST(options, EXIT_WITH_STATUS) &&\n    -           DIFF_OPT_TST(options, DIFF_FROM_CONTENTS)) {\n    +           (textconv_hack || DIFF_OPT_TST(options, DIFF_FROM_CONTENTS))) {\n                    /*\n                     * run diff_flush_patch for the exit status. setting\n                     * options->file to /dev/null should be safe, because we\n\nso that if we find some other change (e.g. diff_addremove() noticed\na path added or removed, or diff_change() noticed a changed blob\nthat does not have textconv defined), we do not have to inspect all\nthe paths to see if there is any change at all by going into the\nbody of this if() block.\n\nAnd then finally, clean things up in a way similar to how\nDIFF_FROM_CONTENTS codepath handles the HAS_CHANGES bit.\n\n    diff --git a/diff.c b/diff.c\n    index 4dfe660..7b318cc 100644\n    --- a/diff.c\n    +++ b/diff.c\n    @@ -4709,7 +4718,7 @@ free_queue:\n             * diff_addremove/diff_change does not set the bit when\n             * DIFF_FROM_CONTENTS is in effect (e.g. with -w).\n             */\n    -       if (DIFF_OPT_TST(options, DIFF_FROM_CONTENTS)) {\n    +       if (textconv_hack || DIFF_OPT_TST(options, DIFF_FROM_CONTENTS)) {\n                    if (options->found_changes)\n                            DIFF_OPT_SET(options, HAS_CHANGES);\n                    else\n\nBy the way, I have a suspicion that the existing code immediately\nafter the context of this hunk is wrong to clear HAS_CHANGES bit\nwhen options->found_changes is clear.  HAS_CHANGES bit should be\nclear upon entry to this function, when DIFF_FROM_CONTENTS is in\nuse.\n\nI also suspect that addremove() that refrains from setting\nHAS_CHANGES when DIFF_FROM_CONTENTS is in effect is wrong.  No\nmatter what combination of -w or -b is used, an addition of a new\nfile, or a removal of an existing file, is a change.\n"},{"id":"282750","messageId":"5704B045.8000005@drmicha.warpmail.net","threadId":"41761","inReplyTo":"xmqqvb3vzlw5.fsf@gitster.mtv.corp.google.com","subject":"Re: git diff --exit-code does not honour textconv setting","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2016-04-06T06:44:21Z","receivedAt":"2016-04-06T06:44:21Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 06.04.2016 01:16:\n> Michael J Gruber <git@drmicha.warpmail.net> writes:\n> \n>>> The \"test\" command is used as it does not generate any output on stdout.\n>>\n>> \"test\" is a bit of a red herring here since it will receive commands.\n>> But your example works even with \"true\" which ignores its commands and\n>> produces no output.\n> \n> Either sounds strange, as they exit without reading their input at\n> all, so the other side of the pipe may get an write error it has to\n> handle ;-)\n> \n>> The description doesn't make it clear whether exit-code refers to\n>> the actual diff (foo vs. bar) or to the diff after textconv (empty\n>> vs. empty). In any case, \"-b\" should not make a difference for\n>> your example.\n>>\n>> diff_flush() in diff.c has this piece of code:\n>>\n> \n> It is understandable that \"-b\" makes difference.  The actual\n> short-cut happens a bit before the code you quoted.\n> \n> \tif (output_format & DIFF_FORMAT_NO_OUTPUT &&\n> \t    DIFF_OPT_TST(options, EXIT_WITH_STATUS) &&\n> \t    DIFF_OPT_TST(options, DIFF_FROM_CONTENTS)) {\n> \t\t/*\n> \t\t * run diff_flush_patch for the exit status. setting\n> \t\t * options->file to /dev/null should be safe, because we\n> \t\t * aren't supposed to produce any output anyway.\n> \t\t */\n> \t\tif (options->close_file)\n> \t\t\tfclose(options->file);\n> \t\toptions->file = fopen(\"/dev/null\", \"w\");\n> \t\tif (!options->file)\n> \t\t\tdie_errno(\"Could not open /dev/null\");\n> \t\toptions->close_file = 1;\n> \t\tfor (i = 0; i < q->nr; i++) {\n> \t\t\tstruct diff_filepair *p = q->queue[i];\n> \t\t\tif (check_pair_status(p))\n> \t\t\t\tdiff_flush_patch(p, options);\n> \t\t\tif (options->found_changes)\n> \t\t\t\tbreak;\n> \t\t}\n> \t}\n> \n> When running without producing output but we need the exit status,\n> unless DIFF_FROM_CONTENTS is set (e.g. \"-b\" in use), we do not run\n> the code that would inspect the blob contents that would have\n> produced the actual patch.  When DIFF_FROM_CONTENTS is set, we need\n> to dig into the blob contents and the body of the above if() gets\n> run only to trigger o->found_changes (e.g. in builtin_diff() that\n> sets ecbdata.found_changesp to point at o->found_changes before\n> calling into the xdiff machinery), but the output is discarded to\n> /dev/null.\n> \n> The textconv filter is an unfortunate beast, in that while some\n> paths use one while others don't, so it won't be as simple as \"-b\"\n> that flips DIFF_FROM_CONTENTS to say \"We need to inspect blobs for\n> ALL FILEPAIRS to see if there is any difference\" if we want to keep\n> the optimization as much as possible.\n> \n> Without DIFF_FROM_CONTENTS set, any filepair that point at two\n> different blobs that might end up to be the same after applying\n> textconv will flip the o->found_changes bit on, and with\n> EXIT_WITH_STATUS bit on, we have another optimization to skip\n> checking the remainder of the filepairs.  So if you want to support\n> textconv with QUICK, you would also need to disable that\n> optimization by teaching diff_change() to refrain from setting\n> HAS_CHANGES bit, i.e.\n> \n>     diff --git a/diff.h b/diff.h\n>     index 125447b..f6c0c07 100644\n>     --- a/diff.h\n>     +++ b/diff.h\n>     @@ -69,7 +69,7 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)\n>      #define DIFF_OPT_FIND_COPIES_HARDER  (1 <<  6)\n>      #define DIFF_OPT_FOLLOW_RENAMES      (1 <<  7)\n>      #define DIFF_OPT_RENAME_EMPTY        (1 <<  8)\n>     -/* (1 <<  9) unused */\n>     +#define DIFF_OPT_WITH_TEXTCONV       (1 <<  9)\n>      #define DIFF_OPT_HAS_CHANGES         (1 << 10)\n>      #define DIFF_OPT_QUICK               (1 << 11)\n>      #define DIFF_OPT_NO_INDEX            (1 << 12)\n> \n>     diff --git a/diff.c b/diff.c\n>     index 4dfe660..0016ad2 100644\n>     --- a/diff.c\n>     +++ b/diff.c\n>     @@ -5018,6 +5018,11 @@ void diff_change(struct diff_options *options,\n>             two->dirty_submodule = new_dirty_submodule;\n>             p = diff_queue(&diff_queued_diff, one, two);\n> \n>     +       if (either path one or path two has textconv defined) {\n>     +               DIFF_OPT_SET(options, DIFF_WITH_TEXTCONV);\n>     +               return;\n>     +       }\n>     +\n>             if (DIFF_OPT_TST(options, DIFF_FROM_CONTENTS))\n>                     return;\n> \n> And then in order to keep the optimization, add this to the above\n> codepath:\n> \n>     diff --git a/diff.c b/diff.c\n>     index 4dfe660..7b318cc 100644\n>     --- a/diff.c\n>     +++ b/diff.c\n>     @@ -4598,6 +4598,7 @@ void diff_flush(struct diff_options *options)\n>             int i, output_format = options->output_format;\n>             int separator = 0;\n>             int dirstat_by_line = 0;\n>     +       int textconv_hack;\n> \n>             /*\n>              * Order: raw, stat, summary, patch\n>     @@ -4652,9 +4653,17 @@ void diff_flush(struct diff_options *options)\n>                     separator++;\n>             }\n> \n>     +       /*\n>     +        * If there is any path that needs textconv and we haven't\n>     +        * found any change on paths that don't, we need to pass\n>     +        * them through textconv and see the textual difference.\n>     +        */\n>     +       textconv_hack = (DIFF_OPT_TST(options, DIFF_WITH_TEXTCONV) &&\n>     +                        !DIFF_OPT_TST(options, HAS_CHANGES));\n>     +\n>             if (output_format & DIFF_FORMAT_NO_OUTPUT &&\n>                 DIFF_OPT_TST(options, EXIT_WITH_STATUS) &&\n>     -           DIFF_OPT_TST(options, DIFF_FROM_CONTENTS)) {\n>     +           (textconv_hack || DIFF_OPT_TST(options, DIFF_FROM_CONTENTS))) {\n>                     /*\n>                      * run diff_flush_patch for the exit status. setting\n>                      * options->file to /dev/null should be safe, because we\n> \n> so that if we find some other change (e.g. diff_addremove() noticed\n> a path added or removed, or diff_change() noticed a changed blob\n> that does not have textconv defined), we do not have to inspect all\n> the paths to see if there is any change at all by going into the\n> body of this if() block.\n> \n> And then finally, clean things up in a way similar to how\n> DIFF_FROM_CONTENTS codepath handles the HAS_CHANGES bit.\n> \n>     diff --git a/diff.c b/diff.c\n>     index 4dfe660..7b318cc 100644\n>     --- a/diff.c\n>     +++ b/diff.c\n>     @@ -4709,7 +4718,7 @@ free_queue:\n>              * diff_addremove/diff_change does not set the bit when\n>              * DIFF_FROM_CONTENTS is in effect (e.g. with -w).\n>              */\n>     -       if (DIFF_OPT_TST(options, DIFF_FROM_CONTENTS)) {\n>     +       if (textconv_hack || DIFF_OPT_TST(options, DIFF_FROM_CONTENTS)) {\n>                     if (options->found_changes)\n>                             DIFF_OPT_SET(options, HAS_CHANGES);\n>                     else\n> \n> By the way, I have a suspicion that the existing code immediately\n> after the context of this hunk is wrong to clear HAS_CHANGES bit\n> when options->found_changes is clear.  HAS_CHANGES bit should be\n> clear upon entry to this function, when DIFF_FROM_CONTENTS is in\n> use.\n> \n> I also suspect that addremove() that refrains from setting\n> HAS_CHANGES when DIFF_FROM_CONTENTS is in effect is wrong.  No\n> matter what combination of -w or -b is used, an addition of a new\n> file, or a removal of an existing file, is a change.\n> \n\nWe certainly share that suspicion.\n\nEven with your clear explanations, I still can't wrap my brain\ncompletely around that flow of different cases. But in any case:\n\n--textconv is about presenting blobs (and, as a consequence diffs) to\nthe user.\n\n--name-status and the like are about differences between the actual\nobjects (irrespective of any possibly coinciding textual representation)\n\nShouldn't \"--exit-code\" rather be in the \"name-status ballpark\" than in\nthe \"textconv ballpark\", i.e. signal that there are changes irrespective\nof representation?\n\nOn the other hand, if \"--exit-code\" should be in the \"textconv ballbark\"\nthen, e.g., it should return 0 resp. 1 on a file with mere white space\nchanges when run with resp. without \"-w\", which means we can never use\noptimizations with --exit-code, or more correctly: optimization (quick)\ncan identify \"equal\" (equal blobs produce equal textual representations)\nbut not \"unequal\". textconv filters are functions, but not necessarily\ninjective. So we could keep \"quick\" in the presumably more common case\nof equal blobs.\n\nMichael\n"}]}