{"thread":{"id":"11253","subject":"[PATCH 2/2] \"diff --check\" should affect exit status","startedAt":"2007-12-12T08:12:00Z","lastAt":"2007-12-12T09:56:34Z","messageCount":5,"participants":["Wincent Colaiuta","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"62871","messageId":"1197447122-93416-1-git-send-email-win@wincent.com","threadId":"11253","inReplyTo":null,"subject":"[RFC PATCH 0/2] diff whitespace tweaks","fromName":"Wincent Colaiuta","fromEmail":"win@wincent.com","sentAt":"2007-12-12T08:12:00Z","receivedAt":"2007-12-12T08:12:00Z","isPatch":true,"sender":{"key":"greg@hurrell.net","avatar":"https://avatars.githubusercontent.com/u/7074?v=4"},"body":"[1/2] Use \"whitespace\" consistently\n\nThis one is a simple consistency fix that should go on top of \"master\".\n\n[2/2] \"diff --check\" should affect exit status\n\nThis one is why I put \"RFC\" in the subject line. Basically, when I\ndiscovered that \"diff --check\" didn't affect the exit status of the\ncommand I thought it might be a bug, in which case this fix or one like\nit should go in for 1.5.4. This patch also applies on top of \"master\".\n\nBut if you don't consider that to be a bug then I guess it's \"next\"\nor even \"pu\" material and I can bring it back up after 1.5.4 is out.\n\nAnother reason why I label this as \"RFC\" is that the diff machinery\nis fairly bulky and I was not familiar with it. So this patch is the\nresult of someone who has looked at the source for half an hour and\ndeduced enough to implement the desired behaviour; but somone who\ntruly groks the diff code may be able to think of a nicer\nimplementation.\n"},{"id":"62870","messageId":"1197447122-93416-2-git-send-email-win@wincent.com","threadId":"11253","inReplyTo":"1197447122-93416-1-git-send-email-win@wincent.com","subject":"[PATCH 1/2] Use \"whitespace\" consistently","fromName":"Wincent Colaiuta","fromEmail":"win@wincent.com","sentAt":"2007-12-12T08:12:01Z","receivedAt":"2007-12-12T08:12:01Z","isPatch":true,"sender":{"key":"greg@hurrell.net","avatar":"https://avatars.githubusercontent.com/u/7074?v=4"},"body":"For consistency, change \"white space\" and \"whitespaces\" to\n\"whitespace\", fixing a couple of adjacent grammar problems in the\ndocs.\n\nSigned-off-by: Wincent Colaiuta <win@wincent.com>\n---\n Documentation/diff-options.txt |   12 ++++++------\n Documentation/glossary.txt     |    2 +-\n diff.c                         |    4 ++--\n 3 files changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex d0154bb..5d22b7b 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -175,19 +175,19 @@ endif::git-format-patch[]\n \tShorthand for \"--text\".\n \n --ignore-space-at-eol::\n-\tIgnore changes in white spaces at EOL.\n+\tIgnore changes in whitespace at EOL.\n \n --ignore-space-change::\n-\tIgnore changes in amount of white space.  This ignores white\n-\tspace at line end, and consider all other sequences of one or\n-\tmore white space characters to be equivalent.\n+\tIgnore changes in amount of whitespace.  This ignores whitespace\n+\tat line end, and considers all other sequences of one or\n+\tmore whitespace characters to be equivalent.\n \n -b::\n \tShorthand for \"--ignore-space-change\".\n \n --ignore-all-space::\n-\tIgnore white space when comparing lines.  This ignores\n-\tdifference even if one line has white space where the other\n+\tIgnore whitespace when comparing lines.  This ignores\n+\tdifferences even if one line has whitespace where the other\n \tline has none.\n \n -w::\ndiff --git a/Documentation/glossary.txt b/Documentation/glossary.txt\nindex fc18744..f9f5a32 100644\n--- a/Documentation/glossary.txt\n+++ b/Documentation/glossary.txt\n@@ -245,7 +245,7 @@ This commit is referred to as a \"merge commit\", or sometimes just a\n \tof the object's contents using the Secure Hash Algorithm\n \t1 and usually represented by the 40 character hexadecimal encoding of\n \tthe <<def_hash,hash>> of the object (possibly followed by\n-\ta white space).\n+\twhitespace).\n \n [[def_object_type]]object type::\n \tOne of the identifiers\ndiff --git a/diff.c b/diff.c\nindex f780e3e..e74a303 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1015,7 +1015,7 @@ static void checkdiff_consume(void *priv, char *line, unsigned long len)\n \t\tif (line[i - 1] == '\\t' && spaces)\n \t\t\tspace_before_tab = 1;\n \n-\t\t/* check white space at line end */\n+\t\t/* check whitespace at line end */\n \t\tif (line[len - 1] == '\\n')\n \t\t\tlen--;\n \t\tif (isspace(line[len - 1]))\n@@ -1029,7 +1029,7 @@ static void checkdiff_consume(void *priv, char *line, unsigned long len)\n \t\t\t\t\tputchar(',');\n \t\t\t}\n \t\t\tif (white_space_at_end)\n-\t\t\t\tprintf(\"white space at end\");\n+\t\t\t\tprintf(\"whitespace at end\");\n \t\t\tprintf(\":%s \", reset);\n \t\t\temit_line_with_ws(1, set, reset, ws, line, len,\n \t\t\t\t\t  data->ws_rule);\n-- \n1.5.3.7.1116.gf11de\n"},{"id":"62869","messageId":"1197447122-93416-3-git-send-email-win@wincent.com","threadId":"11253","inReplyTo":"1197447122-93416-2-git-send-email-win@wincent.com","subject":"[PATCH 2/2] \"diff --check\" should affect exit status","fromName":"Wincent Colaiuta","fromEmail":"win@wincent.com","sentAt":"2007-12-12T08:12:02Z","receivedAt":"2007-12-12T08:12:02Z","isPatch":true,"sender":{"key":"greg@hurrell.net","avatar":"https://avatars.githubusercontent.com/u/7074?v=4"},"body":"\"git diff\" has a --check option that can be used to check for whitespace\nproblems but it only reported by printing warnings to the\nconsole.\n\nNow when the --check option is used we give a non-zero exit status,\nmaking \"git diff --check\" nicer to use in scripts and hooks.\n\nSigned-off-by: Wincent Colaiuta <win@wincent.com>\n---\n Documentation/diff-options.txt |    4 +++-\n builtin-diff-files.c           |    2 ++\n builtin-diff-index.c           |    2 ++\n builtin-diff-tree.c            |    2 ++\n builtin-diff.c                 |    3 ++-\n diff.c                         |   37 ++++++++++++++++++++++++-------------\n diff.h                         |    1 +\n t/t4015-diff-whitespace.sh     |   34 ++++++++++++++++++++++++++++++++++\n 8 files changed, 70 insertions(+), 15 deletions(-)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 5d22b7b..9ecc1d7 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -93,7 +93,9 @@ endif::git-format-patch[]\n \n --check::\n \tWarn if changes introduce trailing whitespace\n-\tor an indent that uses a space before a tab.\n+\tor an indent that uses a space before a tab. Exits with\n+\tnon-zero status if problems are found. Not compatible with\n+\t--exit-code.\n \n --full-index::\n \tInstead of the first handful characters, show full\ndiff --git a/builtin-diff-files.c b/builtin-diff-files.c\nindex 046b7e3..4afc872 100644\n--- a/builtin-diff-files.c\n+++ b/builtin-diff-files.c\n@@ -33,5 +33,7 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)\n \tresult = run_diff_files_cmd(&rev, argc, argv);\n \tif (DIFF_OPT_TST(&rev.diffopt, EXIT_WITH_STATUS))\n \t\treturn DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES) != 0;\n+\tif (rev.diffopt.output_format & DIFF_FORMAT_CHECKDIFF)\n+\t\treturn DIFF_OPT_TST(&rev.diffopt, CHECK_FAILED) != 0;\n \treturn result;\n }\ndiff --git a/builtin-diff-index.c b/builtin-diff-index.c\nindex 556c506..532b284 100644\n--- a/builtin-diff-index.c\n+++ b/builtin-diff-index.c\n@@ -46,5 +46,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)\n \tresult = run_diff_index(&rev, cached);\n \tif (DIFF_OPT_TST(&rev.diffopt, EXIT_WITH_STATUS))\n \t\treturn DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES) != 0;\n+\tif (rev.diffopt.output_format & DIFF_FORMAT_CHECKDIFF)\n+\t\treturn DIFF_OPT_TST(&rev.diffopt, CHECK_FAILED) != 0;\n \treturn result;\n }\ndiff --git a/builtin-diff-tree.c b/builtin-diff-tree.c\nindex 2e13716..556c31d 100644\n--- a/builtin-diff-tree.c\n+++ b/builtin-diff-tree.c\n@@ -134,6 +134,8 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n \t\telse\n \t\t\tdiff_tree_stdin(line);\n \t}\n+\tif (opt->diffopt.output_format & DIFF_FORMAT_CHECKDIFF)\n+\t\treturn DIFF_OPT_TST(&opt->diffopt, CHECK_FAILED) != 0;\n \treturn DIFF_OPT_TST(&opt->diffopt, EXIT_WITH_STATUS)\n \t\t&& DIFF_OPT_TST(&opt->diffopt, HAS_CHANGES);\n }\ndiff --git a/builtin-diff.c b/builtin-diff.c\nindex 1b61599..6aeea93 100644\n--- a/builtin-diff.c\n+++ b/builtin-diff.c\n@@ -365,7 +365,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t     ent, ents);\n \tif (DIFF_OPT_TST(&rev.diffopt, EXIT_WITH_STATUS))\n \t\tresult = DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES) != 0;\n-\n+\tif (rev.diffopt.output_format & DIFF_FORMAT_CHECKDIFF)\n+\t\treturn DIFF_OPT_TST(&rev.diffopt, CHECK_FAILED) != 0;\n \tif (1 < rev.diffopt.skip_stat_unmatch)\n \t\trefresh_index_quietly();\n \treturn result;\ndiff --git a/diff.c b/diff.c\nindex e74a303..e89c7ce 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -996,6 +996,7 @@ struct checkdiff_t {\n \tconst char *filename;\n \tint lineno, color_diff;\n \tunsigned ws_rule;\n+\tint status;\n };\n \n static void checkdiff_consume(void *priv, char *line, unsigned long len)\n@@ -1022,6 +1023,7 @@ static void checkdiff_consume(void *priv, char *line, unsigned long len)\n \t\t\twhite_space_at_end = 1;\n \n \t\tif (space_before_tab || white_space_at_end) {\n+\t\t\tdata->status = 1;\n \t\t\tprintf(\"%s:%d: %s\", data->filename, data->lineno, ws);\n \t\t\tif (space_before_tab) {\n \t\t\t\tprintf(\"space before tab\");\n@@ -1412,7 +1414,7 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n \tdiff_free_filespec_data(two);\n }\n \n-static void builtin_checkdiff(const char *name_a, const char *name_b,\n+static int builtin_checkdiff(const char *name_a, const char *name_b,\n \t\t\t     struct diff_filespec *one,\n \t\t\t     struct diff_filespec *two, struct diff_options *o)\n {\n@@ -1420,7 +1422,7 @@ static void builtin_checkdiff(const char *name_a, const char *name_b,\n \tstruct checkdiff_t data;\n \n \tif (!two)\n-\t\treturn;\n+\t\treturn 0;\n \n \tmemset(&data, 0, sizeof(data));\n \tdata.xm.consume = checkdiff_consume;\n@@ -1449,6 +1451,7 @@ static void builtin_checkdiff(const char *name_a, const char *name_b,\n  free_and_return:\n \tdiff_free_filespec_data(one);\n \tdiff_free_filespec_data(two);\n+\treturn data.status;\n }\n \n struct diff_filespec *alloc_filespec(const char *path)\n@@ -2033,14 +2036,14 @@ static void run_diffstat(struct diff_filepair *p, struct diff_options *o,\n \tbuiltin_diffstat(name, other, p->one, p->two, diffstat, o, complete_rewrite);\n }\n \n-static void run_checkdiff(struct diff_filepair *p, struct diff_options *o)\n+static int run_checkdiff(struct diff_filepair *p, struct diff_options *o)\n {\n \tconst char *name;\n \tconst char *other;\n \n \tif (DIFF_PAIR_UNMERGED(p)) {\n \t\t/* unmerged */\n-\t\treturn;\n+\t\treturn 0;\n \t}\n \n \tname = p->one->path;\n@@ -2049,7 +2052,7 @@ static void run_checkdiff(struct diff_filepair *p, struct diff_options *o)\n \tdiff_fill_sha1_info(p->one);\n \tdiff_fill_sha1_info(p->two);\n \n-\tbuiltin_checkdiff(name, other, p->one, p->two, o);\n+\treturn builtin_checkdiff(name, other, p->one, p->two, o);\n }\n \n void diff_setup(struct diff_options *options)\n@@ -2079,7 +2082,12 @@ int diff_setup_done(struct diff_options *options)\n \tif (options->output_format & DIFF_FORMAT_NAME_STATUS)\n \t\tcount++;\n \tif (options->output_format & DIFF_FORMAT_CHECKDIFF)\n+\t{\n \t\tcount++;\n+\t\tif (DIFF_OPT_TST(options, QUIET) ||\n+\t\t    DIFF_OPT_TST(options, EXIT_WITH_STATUS))\n+\t\t\tdie(\"--check may not be used with --quiet or --exit-code\");\n+\t}\n \tif (options->output_format & DIFF_FORMAT_NO_OUTPUT)\n \t\tcount++;\n \tif (count > 1)\n@@ -2546,17 +2554,17 @@ static void diff_flush_stat(struct diff_filepair *p, struct diff_options *o,\n \trun_diffstat(p, o, diffstat);\n }\n \n-static void diff_flush_checkdiff(struct diff_filepair *p,\n+static int diff_flush_checkdiff(struct diff_filepair *p,\n \t\tstruct diff_options *o)\n {\n \tif (diff_unmodified_pair(p))\n-\t\treturn;\n+\t\treturn 0;\n \n \tif ((DIFF_FILE_VALID(p->one) && S_ISDIR(p->one->mode)) ||\n \t    (DIFF_FILE_VALID(p->two) && S_ISDIR(p->two->mode)))\n-\t\treturn; /* no tree diffs in patch format */\n+\t\treturn 0; /* no tree diffs in patch format */\n \n-\trun_checkdiff(p, o);\n+\treturn run_checkdiff(p, o);\n }\n \n int diff_queue_is_empty(void)\n@@ -2675,16 +2683,19 @@ static int check_pair_status(struct diff_filepair *p)\n \t}\n }\n \n-static void flush_one_pair(struct diff_filepair *p, struct diff_options *opt)\n+static int flush_one_pair(struct diff_filepair *p, struct diff_options *opt)\n {\n \tint fmt = opt->output_format;\n \n \tif (fmt & DIFF_FORMAT_CHECKDIFF)\n-\t\tdiff_flush_checkdiff(p, opt);\n+\t\treturn diff_flush_checkdiff(p, opt);\n \telse if (fmt & (DIFF_FORMAT_RAW | DIFF_FORMAT_NAME_STATUS))\n \t\tdiff_flush_raw(p, opt);\n \telse if (fmt & DIFF_FORMAT_NAME)\n \t\twrite_name_quoted(p->two->path, stdout, opt->line_termination);\n+\n+\t/* return value only matters with DIFF_FORMAT_CHECKDIFF */\n+\treturn 0;\n }\n \n static void show_file_mode_name(const char *newdelete, struct diff_filespec *fs)\n@@ -2923,8 +2934,8 @@ void diff_flush(struct diff_options *options)\n \t\t\t     DIFF_FORMAT_CHECKDIFF)) {\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\tflush_one_pair(p, options);\n+\t\t\tif (check_pair_status(p) && flush_one_pair(p, options))\n+\t\t\t\tDIFF_OPT_SET(options, CHECK_FAILED);\n \t\t}\n \t\tseparator++;\n \t}\ndiff --git a/diff.h b/diff.h\nindex a52496a..5d50d93 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -59,6 +59,7 @@ typedef void (*diff_format_fn_t)(struct diff_queue_struct *q,\n #define DIFF_OPT_ALLOW_EXTERNAL      (1 << 13)\n #define DIFF_OPT_EXIT_WITH_STATUS    (1 << 14)\n #define DIFF_OPT_REVERSE_DIFF        (1 << 15)\n+#define DIFF_OPT_CHECK_FAILED        (1 << 16)\n #define DIFF_OPT_TST(opts, flag)    ((opts)->flags & DIFF_OPT_##flag)\n #define DIFF_OPT_SET(opts, flag)    ((opts)->flags |= DIFF_OPT_##flag)\n #define DIFF_OPT_CLR(opts, flag)    ((opts)->flags &= ~DIFF_OPT_##flag)\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex 79fdff3..c110201 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -117,4 +117,38 @@ EOF\n git diff -b > out\n test_expect_success 'another test, with -b' 'git diff expect out'\n \n+test_expect_success 'check with no whitespace errors' '\n+\n+\tgit commit -m \"snapshot\" &&\n+\techo \"foo();\" > x &&\n+\tgit diff --check\n+\n+'\n+\n+test_expect_failure 'check with trailing whitespace' '\n+\n+\techo \"foo(); \" > x &&\n+\tgit diff --check\n+\n+'\n+\n+test_expect_failure 'check with space before tab in indent' '\n+\n+\techo \" \tfoo();\" > x &&\n+\tgit diff --check\n+\n+'\n+\n+test_expect_failure '--check and --exit-code are exclusive' '\n+\n+\tgit diff --check --exit-code\n+\n+'\n+\n+test_expect_failure '--check and --quiet are exclusive' '\n+\n+\tgit diff --check --quiet\n+\n+'\n+\n test_done\n-- \n1.5.3.7.1116.gf11de\n"},{"id":"62879","messageId":"7vmysg8e8r.fsf@gitster.siamese.dyndns.org","threadId":"11253","inReplyTo":"1197447122-93416-3-git-send-email-win@wincent.com","subject":"Re: [PATCH 2/2] \"diff --check\" should affect exit status","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-12T09:16:20Z","receivedAt":"2007-12-12T09:16:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Wincent Colaiuta <win@wincent.com> writes:\n\n> \"git diff\" has a --check option that can be used to check for whitespace\n> problems but it only reported by printing warnings to the\n> console.\n\nThe primary use of this option used to be to see where the breakages are\nin order to fix them up (IOW, the option was not about seeing if\nbreakage exists or not), but that is much easier with --color output\nthese days.\n\nIt is probably a good idea to do this, although I do not care much about\nit either way.  It probably could replace the sample pre-commit hook\ncurrently implemented in Perl.  As I happen to be fairly familiar with\nthe area, I'll take a look at the patch if/when I find time.\n\nThanks.\n"},{"id":"62887","messageId":"71D6ABDB-568E-4D5F-B901-D57C15314FF7@wincent.com","threadId":"11253","inReplyTo":"7vmysg8e8r.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/2] \"diff --check\" should affect exit status","fromName":"Wincent Colaiuta","fromEmail":"win@wincent.com","sentAt":"2007-12-12T09:56:34Z","receivedAt":"2007-12-12T09:56:34Z","isPatch":true,"sender":{"key":"greg@hurrell.net","avatar":"https://avatars.githubusercontent.com/u/7074?v=4"},"body":"El 12/12/2007, a las 10:16, Junio C Hamano escribió:\n\n> Wincent Colaiuta <win@wincent.com> writes:\n>\n>> \"git diff\" has a --check option that can be used to check for  \n>> whitespace\n>> problems but it only reported by printing warnings to the\n>> console.\n>\n> The primary use of this option used to be to see where the breakages  \n> are\n> in order to fix them up (IOW, the option was not about seeing if\n> breakage exists or not), but that is much easier with --color output\n> these days.\n>\n> It is probably a good idea to do this, although I do not care much  \n> about\n> it either way.  It probably could replace the sample pre-commit hook\n> currently implemented in Perl.\n\nActually, that's exactly why I prepared this patch; I was looking at  \nthe pre-commit hook and though, surely there must be a simpler way  \nthan this... And was surprised when I saw that \"git diff --check\"  \ndidn't alter the exit code...\n\n> As I happen to be fairly familiar with\n> the area, I'll take a look at the patch if/when I find time.\n\nThanks. I'm about to resend this patch with more tests, and one fix  \ndiscovered while adding the tests (\"git diff-tree\" was only setting  \nthe exit status with --stdin set), as well as a new version of the pre- \ncommit hook.\n\nOne nice thing about this change is that when \"diff --check\" learns  \nabout the new core.whitespace stuff, the hook will automatically  \ninherit the new behaviour.\n\nCheers,\nWincent\n"}]}