{"thread":{"id":"11272","subject":"[PATCH 0/5] \"diff --check\" and whitespace enhancements","startedAt":"2007-12-13T13:32:26Z","lastAt":"2007-12-14T20:54:30Z","messageCount":24,"participants":["Wincent Colaiuta","Andreas Ericsson","Junio C Hamano","Jakub Narebski","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"63006","messageId":"1197552751-53480-1-git-send-email-win@wincent.com","threadId":"11272","inReplyTo":null,"subject":"[PATCH 0/5] \"diff --check\" and whitespace enhancements","fromName":"Wincent Colaiuta","fromEmail":"win@wincent.com","sentAt":"2007-12-13T13:32:26Z","receivedAt":"2007-12-13T13:32:26Z","isPatch":true,"sender":{"key":"greg@hurrell.net","avatar":"https://avatars.githubusercontent.com/u/7074?v=4"},"body":"This is a repost of the topic I posted yesterday, rebased on top\nof master and omitting the changes already applied to it.\n\nOf the series the most interesting is [3/5], where I implement\nJunio's suggestion of further refactoring to get rid of\nemit_line_with_ws().\n\n[1/5] \"diff --check\" should affect exit status\n[2/5] New version of pre-commit hook\n[3/5] Unify whitespace checking\n[4/5] Make \"diff --check\" output match \"git apply\"\n[5/5] Add tests for \"git diff --check\" with core.whitespace options\n\nI think there are still some opportunities for further refactoring\nand clean-up, but this is a good start.\n\nOverall the refactoring saves duplication and reduces the line\ncount (apart from the tests, of course, where line count goes\nunequivocally up).\n\n Documentation/diff-options.txt |    4 +-\n builtin-apply.c                |   56 +++----------\n builtin-diff-files.c           |    2 +\n builtin-diff-index.c           |    2 +\n builtin-diff-tree.c            |   28 +++---\n builtin-diff.c                 |    3 +-\n cache.h                        |    4 +\n diff.c                         |  172 +++++++++---------------------------\n diff.h                         |    1 +\n t/t4015-diff-whitespace.sh     |  189 +++++++++++++++++++++++++++++++++++++++-\n templates/hooks--pre-commit    |   67 ++-------------\n ws.c                           |  105 ++++++++++++++++++++++\n 12 files changed, 379 insertions(+), 254 deletions(-)\n\nCheers,\nWincent\n"},{"id":"63009","messageId":"1197552751-53480-2-git-send-email-win@wincent.com","threadId":"11272","inReplyTo":"1197552751-53480-1-git-send-email-win@wincent.com","subject":"[PATCH 1/5] \"diff --check\" should affect exit status","fromName":"Wincent Colaiuta","fromEmail":"win@wincent.com","sentAt":"2007-12-13T13:32:27Z","receivedAt":"2007-12-13T13:32:27Z","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            |   28 ++++----\n builtin-diff.c                 |    3 +-\n diff.c                         |   37 +++++++----\n diff.h                         |    1 +\n t/t4015-diff-whitespace.sh     |  137 +++++++++++++++++++++++++++++++++++++++-\n 8 files changed, 184 insertions(+), 30 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..9ab90cb 100644\n--- a/builtin-diff-tree.c\n+++ b/builtin-diff-tree.c\n@@ -117,23 +117,23 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n \t\tbreak;\n \t}\n \n-\tif (!read_stdin)\n-\t\treturn DIFF_OPT_TST(&opt->diffopt, EXIT_WITH_STATUS)\n-\t\t\t&& DIFF_OPT_TST(&opt->diffopt, HAS_CHANGES);\n+\tif (read_stdin) {\n+\t\tif (opt->diffopt.detect_rename)\n+\t\t\topt->diffopt.setup |= (DIFF_SETUP_USE_SIZE_CACHE |\n+\t\t\t\t\t       DIFF_SETUP_USE_CACHE);\n+\t\twhile (fgets(line, sizeof(line), stdin)) {\n+\t\t\tunsigned char sha1[20];\n \n-\tif (opt->diffopt.detect_rename)\n-\t\topt->diffopt.setup |= (DIFF_SETUP_USE_SIZE_CACHE |\n-\t\t\t\t       DIFF_SETUP_USE_CACHE);\n-\twhile (fgets(line, sizeof(line), stdin)) {\n-\t\tunsigned char sha1[20];\n-\n-\t\tif (get_sha1_hex(line, sha1)) {\n-\t\t\tfputs(line, stdout);\n-\t\t\tfflush(stdout);\n+\t\t\tif (get_sha1_hex(line, sha1)) {\n+\t\t\t\tfputs(line, stdout);\n+\t\t\t\tfflush(stdout);\n+\t\t\t}\n+\t\t\telse\n+\t\t\t\tdiff_tree_stdin(line);\n \t\t}\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 55fb84c..86d01a3 100644\n--- a/builtin-diff.c\n+++ b/builtin-diff.c\n@@ -353,7 +353,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 9c79ee2..39109a6 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1031,6 +1031,7 @@ struct checkdiff_t {\n \tconst char *filename;\n \tint lineno, color_diff;\n \tunsigned ws_rule;\n+\tunsigned status;\n };\n \n static void checkdiff_consume(void *priv, char *line, unsigned long len)\n@@ -1064,6 +1065,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@@ -1454,7 +1456,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@@ -1462,7 +1464,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@@ -1491,6 +1493,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@@ -2075,14 +2078,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@@ -2091,7 +2094,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@@ -2121,7 +2124,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@@ -2588,17 +2596,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@@ -2717,16 +2725,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@@ -2965,8 +2976,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 6adf9d1..dc538b3 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -117,7 +117,6 @@ EOF\n git diff -b > out\n test_expect_success 'another test, with -b' 'git diff expect out'\n \n-\n test_expect_success 'check mixed spaces and tabs in indent' '\n \n \t# This is indented with SP HT SP.\n@@ -126,4 +125,140 @@ test_expect_success 'check mixed spaces and tabs in indent' '\n \n '\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+\t# indent has space followed by hard tab\n+\techo \" \tfoo();\" > x &&\n+\tgit diff --check\n+\n+'\n+\n+test_expect_failure '--check and --exit-code are exclusive' '\n+\n+\tgit checkout x &&\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_expect_success 'check staged with no whitespace errors' '\n+\n+\techo \"foo();\" > x &&\n+\tgit add x &&\n+\tgit diff --cached --check\n+\n+'\n+\n+test_expect_failure 'check staged with trailing whitespace' '\n+\n+\techo \"foo(); \" > x &&\n+\tgit add x &&\n+\tgit diff --cached --check\n+\n+'\n+\n+test_expect_failure 'check staged with space before tab in indent' '\n+\n+\t# indent has space followed by hard tab\n+\techo \" \tfoo();\" > x &&\n+\tgit add x &&\n+\tgit diff --cached --check\n+\n+'\n+\n+test_expect_success 'check with no whitespace errors (diff-index)' '\n+\n+\techo \"foo();\" > x &&\n+\tgit add x &&\n+\tgit diff-index --check HEAD\n+\n+'\n+\n+test_expect_failure 'check with trailing whitespace (diff-index)' '\n+\n+\techo \"foo(); \" > x &&\n+\tgit add x &&\n+\tgit diff-index --check HEAD\n+\n+'\n+\n+test_expect_failure 'check with space before tab in indent (diff-index)' '\n+\n+\t# indent has space followed by hard tab\n+\techo \" \tfoo();\" > x &&\n+\tgit add x &&\n+\tgit diff-index --check HEAD\n+\n+'\n+\n+test_expect_success 'check staged with no whitespace errors (diff-index)' '\n+\n+\techo \"foo();\" > x &&\n+\tgit add x &&\n+\tgit diff-index --cached --check HEAD\n+\n+'\n+\n+test_expect_failure 'check staged with trailing whitespace (diff-index)' '\n+\n+\techo \"foo(); \" > x &&\n+\tgit add x &&\n+\tgit diff-index --cached --check HEAD\n+\n+'\n+\n+test_expect_failure 'check staged with space before tab in indent (diff-index)' '\n+\n+\t# indent has space followed by hard tab\n+\techo \" \tfoo();\" > x &&\n+\tgit add x &&\n+\tgit diff-index --cached --check HEAD\n+\n+'\n+\n+test_expect_success 'check with no whitespace errors (diff-tree)' '\n+\n+\techo \"foo();\" > x &&\n+\tgit commit -m \"new commit\" x &&\n+\tgit diff-tree --check HEAD^ HEAD\n+\n+'\n+\n+test_expect_failure 'check with trailing whitespace (diff-tree)' '\n+\n+\techo \"foo(); \" > x &&\n+\tgit commit -m \"another commit\" x &&\n+\tgit diff-tree --check HEAD^ HEAD\n+\n+'\n+\n+test_expect_failure 'check with space before tab in indent (diff-tree)' '\n+\n+\t# indent has space followed by hard tab\n+\techo \" \tfoo();\" > x &&\n+\tgit commit -m \"yet another\" x &&\n+\tgit diff-tree --check HEAD^ HEAD\n+\n+'\n+\n test_done\n-- \n1.5.4.rc0.4.g50348\n"},{"id":"63014","messageId":"1197552751-53480-3-git-send-email-win@wincent.com","threadId":"11272","inReplyTo":"1197552751-53480-2-git-send-email-win@wincent.com","subject":"[PATCH 2/5] New version of pre-commit hook","fromName":"Wincent Colaiuta","fromEmail":"win@wincent.com","sentAt":"2007-12-13T13:32:28Z","receivedAt":"2007-12-13T13:32:28Z","isPatch":true,"sender":{"key":"greg@hurrell.net","avatar":"https://avatars.githubusercontent.com/u/7074?v=4"},"body":"Now that \"git diff --check\" indicates problems with its exit code the\npre-commit hook becomes a trivial one-liner.\n\nA side effect of this is that when \"git diff --check\" learns to detect\nproblems according to core.whitespace in the future, the hook's\nbehaviour will evolve to match without any changes required.\n\nSigned-off-by: Wincent Colaiuta <win@wincent.com>\n---\n templates/hooks--pre-commit |   67 ++++--------------------------------------\n 1 files changed, 7 insertions(+), 60 deletions(-)\n\ndiff --git a/templates/hooks--pre-commit b/templates/hooks--pre-commit\nindex 7092bae..f8c7be7 100644\n--- a/templates/hooks--pre-commit\n+++ b/templates/hooks--pre-commit\n@@ -7,64 +7,11 @@\n #\n # To enable this hook, make this file executable.\n \n-# This is slightly modified from Andrew Morton's Perfect Patch.\n-# Lines you introduce should not have trailing whitespace.\n-# Also check for an indentation that has SP before a TAB.\n+git-diff-index -M --cached --check HEAD -- && exit 0\n \n-if git-rev-parse --verify HEAD 2>/dev/null\n-then\n-\tgit-diff-index -p -M --cached HEAD --\n-else\n-\t# NEEDSWORK: we should produce a diff with an empty tree here\n-\t# if we want to do the same verification for the initial import.\n-\t:\n-fi |\n-perl -e '\n-    my $found_bad = 0;\n-    my $filename;\n-    my $reported_filename = \"\";\n-    my $lineno;\n-    sub bad_line {\n-\tmy ($why, $line) = @_;\n-\tif (!$found_bad) {\n-\t    print STDERR \"*\\n\";\n-\t    print STDERR \"* You have some suspicious patch lines:\\n\";\n-\t    print STDERR \"*\\n\";\n-\t    $found_bad = 1;\n-\t}\n-\tif ($reported_filename ne $filename) {\n-\t    print STDERR \"* In $filename\\n\";\n-\t    $reported_filename = $filename;\n-\t}\n-\tprint STDERR \"* $why (line $lineno)\\n\";\n-\tprint STDERR \"$filename:$lineno:$line\\n\";\n-    }\n-    while (<>) {\n-\tif (m|^diff --git a/(.*) b/\\1$|) {\n-\t    $filename = $1;\n-\t    next;\n-\t}\n-\tif (/^@@ -\\S+ \\+(\\d+)/) {\n-\t    $lineno = $1 - 1;\n-\t    next;\n-\t}\n-\tif (/^ /) {\n-\t    $lineno++;\n-\t    next;\n-\t}\n-\tif (s/^\\+//) {\n-\t    $lineno++;\n-\t    chomp;\n-\t    if (/\\s$/) {\n-\t\tbad_line(\"trailing whitespace\", $_);\n-\t    }\n-\t    if (/^\\s* \\t/) {\n-\t\tbad_line(\"indent SP followed by a TAB\", $_);\n-\t    }\n-\t    if (/^(?:[<>=]){7}/) {\n-\t\tbad_line(\"unresolved merge conflict\", $_);\n-\t    }\n-\t}\n-    }\n-    exit($found_bad);\n-'\n+cat >&2 <<EOF\n+fatal: commit aborted due to whitespace problems\n+specify --no-verify to bypass these checks and commit anyway\n+EOF\n+\n+exit 1\n-- \n1.5.4.rc0.4.g50348\n"},{"id":"63008","messageId":"1197552751-53480-4-git-send-email-win@wincent.com","threadId":"11272","inReplyTo":"1197552751-53480-3-git-send-email-win@wincent.com","subject":"[PATCH 3/5] Unify whitespace checking","fromName":"Wincent Colaiuta","fromEmail":"win@wincent.com","sentAt":"2007-12-13T13:32:29Z","receivedAt":"2007-12-13T13:32:29Z","isPatch":true,"sender":{"key":"greg@hurrell.net","avatar":"https://avatars.githubusercontent.com/u/7074?v=4"},"body":"This commit unifies three separate places where whitespace checking was\nperformed:\n\n - the whitespace checking previously done in builtin-apply.c is\nextracted into a function in ws.c\n\n - the equivalent logic in \"git diff\" is removed\n\n - the emit_line_with_ws() function is also removed because that also\nrechecks the shitespace, and its functionality is rolled into ws.c\n\nThe new function is called check_and_emit_line() and it does two things:\nchecks a line for whitespace errors and optionally emits it. The checking\nis based on lines of content rather than patch lines (in other words, the\ncaller must strip the leading \"+\" or \"-\"); this was suggested by Junio on\nthe mailing list to allow for a future extension to \"git show\" to display\nwhitespace errors in blobs.\n\nAt the same time we teach it to report all classes of whitespace errors\nfound for a given line rather than reporting only the first found error.\n\nSigned-off-by: Wincent Colaiuta <win@wincent.com>\n---\n builtin-apply.c            |   54 +++--------------\n cache.h                    |    4 +\n diff.c                     |  138 ++++++--------------------------------------\n t/t4015-diff-whitespace.sh |    2 +-\n ws.c                       |  105 +++++++++++++++++++++++++++++++++\n 5 files changed, 139 insertions(+), 164 deletions(-)\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex f2e9a33..ab393f3 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -900,56 +900,22 @@ static int find_header(char *line, unsigned long size, int *hdrsize, struct patc\n \n static void check_whitespace(const char *line, int len, unsigned ws_rule)\n {\n-\tconst char *err = \"Adds trailing whitespace\";\n-\tint seen_space = 0;\n-\tint i;\n-\n-\t/*\n-\t * We know len is at least two, since we have a '+' and we\n-\t * checked that the last character was a '\\n' before calling\n-\t * this function.  That is, an addition of an empty line would\n-\t * check the '+' here.  Sneaky...\n-\t */\n-\tif ((ws_rule & WS_TRAILING_SPACE) && isspace(line[len-2]))\n-\t\tgoto error;\n-\n-\t/*\n-\t * Make sure that there is no space followed by a tab in\n-\t * indentation.\n-\t */\n-\tif (ws_rule & WS_SPACE_BEFORE_TAB) {\n-\t\terr = \"Space in indent is followed by a tab\";\n-\t\tfor (i = 1; i < len; i++) {\n-\t\t\tif (line[i] == '\\t') {\n-\t\t\t\tif (seen_space)\n-\t\t\t\t\tgoto error;\n-\t\t\t}\n-\t\t\telse if (line[i] == ' ')\n-\t\t\t\tseen_space = 1;\n-\t\t\telse\n-\t\t\t\tbreak;\n-\t\t}\n-\t}\n-\n-\t/*\n-\t * Make sure that the indentation does not contain more than\n-\t * 8 spaces.\n-\t */\n-\tif ((ws_rule & WS_INDENT_WITH_NON_TAB) &&\n-\t    (8 < len) && !strncmp(\"+        \", line, 9)) {\n-\t\terr = \"Indent more than 8 places with spaces\";\n-\t\tgoto error;\n-\t}\n-\treturn;\n+\tchar *err;\n+\tunsigned result = check_and_emit_line(line + 1, len - 1, ws_rule,\n+\t    NULL, NULL, NULL, NULL);\n+\tif (!result)\n+\t\treturn;\n \n- error:\n \twhitespace_error++;\n \tif (squelch_whitespace_errors &&\n \t    squelch_whitespace_errors < whitespace_error)\n \t\t;\n-\telse\n+\telse {\n+\t\terr = whitespace_error_string(result);\n \t\tfprintf(stderr, \"%s.\\n%s:%d:%.*s\\n\",\n-\t\t\terr, patch_input_file, linenr, len-2, line+1);\n+\t\t     err, patch_input_file, linenr, len - 2, line + 1);\n+\t\tfree(err);\n+\t}\n }\n \n /*\ndiff --git a/cache.h b/cache.h\nindex 27d90fe..39331c2 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -655,6 +655,10 @@ void shift_tree(const unsigned char *, const unsigned char *, unsigned char *, i\n extern unsigned whitespace_rule_cfg;\n extern unsigned whitespace_rule(const char *);\n extern unsigned parse_whitespace_rule(const char *);\n+extern unsigned check_and_emit_line(const char *line, int len, unsigned ws_rule,\n+    FILE *stream, const char *set,\n+    const char *reset, const char *ws);\n+extern char *whitespace_error_string(unsigned ws);\n \n /* ls-files */\n int pathspec_match(const char **spec, char *matched, const char *filename, int skiplen);\ndiff --git a/diff.c b/diff.c\nindex 39109a6..d96a4ac 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -486,88 +486,9 @@ const char *diff_get_color(int diff_use_color, enum color_diff ix)\n \n static void emit_line(const char *set, const char *reset, const char *line, int len)\n {\n-\tif (len > 0 && line[len-1] == '\\n')\n-\t\tlen--;\n \tfputs(set, stdout);\n \tfwrite(line, len, 1, stdout);\n-\tputs(reset);\n-}\n-\n-static void emit_line_with_ws(int nparents,\n-\t\t\t      const char *set, const char *reset, const char *ws,\n-\t\t\t      const char *line, int len, unsigned ws_rule)\n-{\n-\tint col0 = nparents;\n-\tint last_tab_in_indent = -1;\n-\tint last_space_in_indent = -1;\n-\tint i;\n-\tint tail = len;\n-\tint need_highlight_leading_space = 0;\n-\t/*\n-\t * The line is a newly added line.  Does it have funny leading\n-\t * whitespaces?  In indent, SP should never precede a TAB.  In\n-\t * addition, under \"indent with non tab\" rule, there should not\n-\t * be more than 8 consecutive spaces.\n-\t */\n-\tfor (i = col0; i < len; i++) {\n-\t\tif (line[i] == '\\t') {\n-\t\t\tlast_tab_in_indent = i;\n-\t\t\tif ((ws_rule & WS_SPACE_BEFORE_TAB) &&\n-\t\t\t    0 <= last_space_in_indent)\n-\t\t\t\tneed_highlight_leading_space = 1;\n-\t\t}\n-\t\telse if (line[i] == ' ')\n-\t\t\tlast_space_in_indent = i;\n-\t\telse\n-\t\t\tbreak;\n-\t}\n-\tif ((ws_rule & WS_INDENT_WITH_NON_TAB) &&\n-\t    0 <= last_space_in_indent &&\n-\t    last_tab_in_indent < 0 &&\n-\t    8 <= (i - col0)) {\n-\t\tlast_tab_in_indent = i;\n-\t\tneed_highlight_leading_space = 1;\n-\t}\n-\tfputs(set, stdout);\n-\tfwrite(line, col0, 1, stdout);\n \tfputs(reset, stdout);\n-\tif (((i == len) || line[i] == '\\n') && i != col0) {\n-\t\t/* The whole line was indent */\n-\t\temit_line(ws, reset, line + col0, len - col0);\n-\t\treturn;\n-\t}\n-\ti = col0;\n-\tif (need_highlight_leading_space) {\n-\t\twhile (i < last_tab_in_indent) {\n-\t\t\tif (line[i] == ' ') {\n-\t\t\t\tfputs(ws, stdout);\n-\t\t\t\tputchar(' ');\n-\t\t\t\tfputs(reset, stdout);\n-\t\t\t}\n-\t\t\telse\n-\t\t\t\tputchar(line[i]);\n-\t\t\ti++;\n-\t\t}\n-\t}\n-\ttail = len - 1;\n-\tif (line[tail] == '\\n' && i < tail)\n-\t\ttail--;\n-\tif (ws_rule & WS_TRAILING_SPACE) {\n-\t\twhile (i < tail) {\n-\t\t\tif (!isspace(line[tail]))\n-\t\t\t\tbreak;\n-\t\t\ttail--;\n-\t\t}\n-\t}\n-\tif ((i < tail && line[tail + 1] != '\\n')) {\n-\t\t/* This has whitespace between tail+1..len */\n-\t\tfputs(set, stdout);\n-\t\tfwrite(line + i, tail - i + 1, 1, stdout);\n-\t\tfputs(reset, stdout);\n-\t\temit_line(ws, reset, line + tail + 1, len - tail - 1);\n-\t}\n-\telse\n-\t\temit_line(set, reset, line + i, len - i);\n }\n \n static void emit_add_line(const char *reset, struct emit_callback *ecbdata, const char *line, int len)\n@@ -577,9 +498,13 @@ static void emit_add_line(const char *reset, struct emit_callback *ecbdata, cons\n \n \tif (!*ws)\n \t\temit_line(set, reset, line, len);\n-\telse\n-\t\temit_line_with_ws(ecbdata->nparents, set, reset, ws,\n-\t\t\t\t  line, len, ecbdata->ws_rule);\n+\telse {\n+\t\t/* Emit just the prefix, then the rest. */\n+\t\temit_line(set, reset, line, ecbdata->nparents);\n+\t\t(void)check_and_emit_line(line + ecbdata->nparents,\n+\t\t    len - ecbdata->nparents, ecbdata->ws_rule,\n+\t\t    stdout, set, reset, ws);\n+\t}\n }\n \n static void fn_out_consume(void *priv, char *line, unsigned long len)\n@@ -1040,45 +965,20 @@ static void checkdiff_consume(void *priv, char *line, unsigned long len)\n \tconst char *ws = diff_get_color(data->color_diff, DIFF_WHITESPACE);\n \tconst char *reset = diff_get_color(data->color_diff, DIFF_RESET);\n \tconst char *set = diff_get_color(data->color_diff, DIFF_FILE_NEW);\n+\tchar *err;\n \n \tif (line[0] == '+') {\n-\t\tint i, spaces = 0, space_before_tab = 0, white_space_at_end = 0;\n-\n-\t\t/* check space before tab */\n-\t\tfor (i = 1; i < len; i++) {\n-\t\t\tif (line[i] == ' ')\n-\t\t\t\tspaces++;\n-\t\t\telse if (line[i] == '\\t') {\n-\t\t\t\tif (spaces) {\n-\t\t\t\t\tspace_before_tab = 1;\n-\t\t\t\t\tbreak;\n-\t\t\t\t}\n-\t\t\t}\n-\t\t\telse\n-\t\t\t\tbreak;\n-\t\t}\n-\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-\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-\t\t\t\tif (white_space_at_end)\n-\t\t\t\t\tputchar(',');\n-\t\t\t}\n-\t\t\tif (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-\t\t}\n-\n+\t\tdata->status = check_and_emit_line(line + 1, len - 1,\n+\t\t    data->ws_rule, NULL, NULL, NULL, NULL);\n+\t\tif (!data->status)\n+\t\t\treturn;\n+\t\terr = whitespace_error_string(data->status);\n+\t\tprintf(\"%s:%d: %s%s:%s \", data->filename, data->lineno,\n+\t\t    ws, err, reset);\n+\t\tfree(err);\n+\t\temit_line(set, reset, line, 1);\n+\t\t(void)check_and_emit_line(line + 1, len - 1, data->ws_rule,\n+\t\t    stdout, set, reset, ws);\n \t\tdata->lineno++;\n \t} else if (line[0] == ' ')\n \t\tdata->lineno++;\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex dc538b3..a0a47dd 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -121,7 +121,7 @@ test_expect_success 'check mixed spaces and tabs in indent' '\n \n \t# This is indented with SP HT SP.\n \techo \" \t foo();\" > x &&\n-\tgit diff --check | grep \"space before tab\"\n+\tgit diff --check | grep \"Space in indent is followed by a tab\"\n \n '\n \ndiff --git a/ws.c b/ws.c\nindex 52c10ca..a0ce860 100644\n--- a/ws.c\n+++ b/ws.c\n@@ -94,3 +94,108 @@ unsigned whitespace_rule(const char *pathname)\n \t\treturn whitespace_rule_cfg;\n \t}\n }\n+\n+/* The returned string should be freed by the caller. */\n+char *whitespace_error_string(unsigned ws)\n+{\n+\tstruct strbuf err;\n+\tstrbuf_init(&err, 0);\n+\tif (ws & WS_TRAILING_SPACE)\n+\t\tstrbuf_addstr(&err, \"Adds trailing whitespace\");\n+\tif (ws & WS_SPACE_BEFORE_TAB) {\n+\t\tif (err.len)\n+\t\t\tstrbuf_addstr(&err, \", \");\n+\t\tstrbuf_addstr(&err, \"Space in indent is followed by a tab\");\n+\t}\n+\tif (ws & WS_INDENT_WITH_NON_TAB) {\n+\t\tif (err.len)\n+\t\t\tstrbuf_addstr(&err, \", \");\n+\t\tstrbuf_addstr(&err, \"Indent more than 8 places with spaces\");\n+\t}\n+\treturn strbuf_detach(&err, NULL);\n+}\n+\n+/* If stream is non-NULL, emits the line after checking. */\n+unsigned check_and_emit_line(const char *line, int len, unsigned ws_rule,\n+                             FILE *stream, const char *set,\n+                             const char *reset, const char *ws)\n+{\n+\tunsigned result = 0;\n+\tint leading_space = -1;\n+\tint trailing_whitespace = -1;\n+\tint trailing_newline = 0;\n+\tint i;\n+\n+\t/* Logic is simpler if we temporarily ignore the trailing newline. */\n+\tif (len > 0 && line[len - 1] == '\\n') {\n+\t\ttrailing_newline = 1;\n+\t\tlen--;\n+\t}\n+\n+\t/* Check for trailing whitespace. */\n+\tif (ws_rule & WS_TRAILING_SPACE) {\n+\t\tfor (i = len - 1; i >= 0; i--) {\n+\t\t\tif (isspace(line[i])) {\n+\t\t\t\ttrailing_whitespace = i;\n+\t\t\t\tresult |= WS_TRAILING_SPACE;\n+\t\t\t}\n+\t\t\telse\n+\t\t\t\tbreak;\n+\t\t}\n+\t}\n+\n+\t/* Check for space before tab in initial indent. */\n+\tfor (i = 0; i < len; i++) {\n+\t\tif (line[i] == '\\t') {\n+\t\t\tif ((ws_rule & WS_SPACE_BEFORE_TAB) &&\n+\t\t\t    (leading_space != -1))\n+\t\t\t\tresult |= WS_SPACE_BEFORE_TAB;\n+\t\t\tbreak;\n+\t\t}\n+\t\telse if (line[i] == ' ')\n+\t\t\tleading_space = i;\n+\t\telse\n+\t\t\tbreak;\n+\t}\n+\n+\t/* Check for indent using non-tab. */\n+\tif ((ws_rule & WS_INDENT_WITH_NON_TAB) && leading_space >= 8)\n+\t\tresult |= WS_INDENT_WITH_NON_TAB;\n+\n+\tif (stream) {\n+\t\t/* Highlight errors in leading whitespace. */\n+\t\tif ((result & WS_SPACE_BEFORE_TAB) ||\n+\t\t    (result & WS_INDENT_WITH_NON_TAB)) {\n+\t\t\tfputs(ws, stream);\n+\t\t\tfwrite(line, leading_space + 1, 1, stream);\n+\t\t\tfputs(reset, stream);\n+\t\t\tleading_space++;\n+\t\t}\n+\t\telse\n+\t\t\tleading_space = 0;\n+\n+\t\t/* Now the rest of the line starts at leading_space.\n+\t\t * The non-highlighted part ends at trailing_whitespace. */\n+\t\tif (trailing_whitespace == -1)\n+\t\t\ttrailing_whitespace = len;\n+\n+\t\t/* Emit non-highlighted (middle) segment. */\n+\t\tif (trailing_whitespace - leading_space > 0) {\n+\t\t\tfputs(set, stream);\n+\t\t\tfwrite(line + leading_space,\n+\t\t\t    trailing_whitespace - leading_space, 1, stream);\n+\t\t\tfputs(reset, stream);\n+\t\t}\n+\n+\t\t/* Highlight errors in trailing whitespace. */\n+\t\tif (trailing_whitespace != len) {\n+\t\t\tfputs(ws, stream);\n+\t\t\tfwrite(line + trailing_whitespace,\n+\t\t\t    len - trailing_whitespace, 1, stream);\n+\t\t\tfputs(reset, stream);\n+\t\t}\n+\t\tif (trailing_newline)\n+\t\t\tfputc('\\n', stream);\n+\t}\n+\treturn result;\n+}\n-- \n1.5.4.rc0.4.g50348\n"},{"id":"63010","messageId":"1197552751-53480-5-git-send-email-win@wincent.com","threadId":"11272","inReplyTo":"1197552751-53480-4-git-send-email-win@wincent.com","subject":"[PATCH 4/5] Make \"diff --check\" output match \"git apply\"","fromName":"Wincent Colaiuta","fromEmail":"win@wincent.com","sentAt":"2007-12-13T13:32:30Z","receivedAt":"2007-12-13T13:32:30Z","isPatch":true,"sender":{"key":"greg@hurrell.net","avatar":"https://avatars.githubusercontent.com/u/7074?v=4"},"body":"For consistency, make the two tools report whitespace errors in the\nsame way (the output of \"diff --check\" has been tweaked to match\nthat of \"git apply\").\n\nNote that although the textual content is basically the same only\n\"git diff --check\" provides a colorized version of the problematic\nlines; making \"git apply\" do colorization will require more extensive\nchanges (figuring out the diff colorization preferences of the user)\nand so that will be a subject for another commit.\n\nSigned-off-by: Wincent Colaiuta <win@wincent.com>\n---\n builtin-apply.c |    4 ++--\n diff.c          |    3 +--\n 2 files changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex ab393f3..2edd83b 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -912,8 +912,8 @@ static void check_whitespace(const char *line, int len, unsigned ws_rule)\n \t\t;\n \telse {\n \t\terr = whitespace_error_string(result);\n-\t\tfprintf(stderr, \"%s.\\n%s:%d:%.*s\\n\",\n-\t\t     err, patch_input_file, linenr, len - 2, line + 1);\n+\t\tfprintf(stderr, \"%s:%d: %s.\\n%.*s\\n\",\n+\t\t     patch_input_file, linenr, err, len - 2, line + 1);\n \t\tfree(err);\n \t}\n }\ndiff --git a/diff.c b/diff.c\nindex d96a4ac..73a723d 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -973,8 +973,7 @@ static void checkdiff_consume(void *priv, char *line, unsigned long len)\n \t\tif (!data->status)\n \t\t\treturn;\n \t\terr = whitespace_error_string(data->status);\n-\t\tprintf(\"%s:%d: %s%s:%s \", data->filename, data->lineno,\n-\t\t    ws, err, reset);\n+\t\tprintf(\"%s:%d: %s.\\n\", data->filename, data->lineno, err);\n \t\tfree(err);\n \t\temit_line(set, reset, line, 1);\n \t\t(void)check_and_emit_line(line + 1, len - 1, data->ws_rule,\n-- \n1.5.4.rc0.4.g50348\n"},{"id":"63007","messageId":"1197552751-53480-6-git-send-email-win@wincent.com","threadId":"11272","inReplyTo":"1197552751-53480-5-git-send-email-win@wincent.com","subject":"[PATCH 5/5] Add tests for \"git diff --check\" with core.whitespace options","fromName":"Wincent Colaiuta","fromEmail":"win@wincent.com","sentAt":"2007-12-13T13:32:31Z","receivedAt":"2007-12-13T13:32:31Z","isPatch":true,"sender":{"key":"greg@hurrell.net","avatar":"https://avatars.githubusercontent.com/u/7074?v=4"},"body":"Make sure that \"git diff --check\" does the right thing when the\ncore.whitespace options are set.\n\nSigned-off-by: Wincent Colaiuta <win@wincent.com>\n---\n t/t4015-diff-whitespace.sh |   50 ++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 50 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex a0a47dd..cb90ee7 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -261,4 +261,54 @@ test_expect_failure 'check with space before tab in indent (diff-tree)' '\n \n '\n \n+test_expect_success 'check trailing whitespace (trailing-space: off)' '\n+\n+\tgit config core.whitespace \"-trailing-space\" &&\n+\techo \"foo ();   \" > x &&\n+\tgit diff --check\n+\n+'\n+\n+test_expect_failure 'check trailing whitespace (trailing-space: on)' '\n+\n+\tgit config core.whitespace \"trailing-space\" &&\n+\techo \"foo ();   \" > x &&\n+\tgit diff --check\n+\n+'\n+\n+test_expect_success 'check space before tab in indent (space-before-tab: off)' '\n+\n+\t# indent contains space followed by HT\n+\tgit config core.whitespace \"-space-before-tab\" &&\n+\techo \" \tfoo ();\" > x &&\n+\tgit diff --check\n+\n+'\n+\n+test_expect_failure 'check space before tab in indent (space-before-tab: on)' '\n+\n+\t# indent contains space followed by HT\n+\tgit config core.whitespace \"space-before-tab\" &&\n+\techo \" \tfoo ();   \" > x &&\n+\tgit diff --check\n+\n+'\n+\n+test_expect_success 'check spaces as indentation (indent-with-non-tab: off)' '\n+\n+\tgit config core.whitespace \"-indent-with-non-tab\"\n+\techo \"                foo ();\" > x &&\n+\tgit diff --check\n+\n+'\n+\n+test_expect_failure 'check spaces as indentation (indent-with-non-tab: on)' '\n+\n+\tgit config core.whitespace \"indent-with-non-tab\" &&\n+\techo \"                foo ();\" > x &&\n+\tgit diff --check\n+\n+'\n+\n test_done\n-- \n1.5.4.rc0.4.g50348\n"},{"id":"63012","messageId":"47613662.6010203@op5.se","threadId":"11272","inReplyTo":"1197552751-53480-4-git-send-email-win@wincent.com","subject":"Re: [PATCH 3/5] Unify whitespace checking","fromName":"Andreas Ericsson","fromEmail":"ae@op5.se","sentAt":"2007-12-13T13:40:50Z","receivedAt":"2007-12-13T13:40:50Z","isPatch":true,"sender":{"key":"ae@op5.se","avatar":"https://gravatar.com/avatar/426e89595c75a8f5252dd0c989e5fabe5bcac616e68557427ad9aef6b0ca342a?d=mp&s=160"},"body":"Wincent Colaiuta wrote:\n> \n>  - the emit_line_with_ws() function is also removed because that also\n> rechecks the shitespace, and its functionality is rolled into ws.c\n> \n\nshitespace? It's fitting though. I'd stick with it :-)\n\n-- \nAndreas Ericsson                   andreas.ericsson@op5.se\nOP5 AB                             www.op5.se\nTel: +46 8-230225                  Fax: +46 8-230231\n"},{"id":"63047","messageId":"1197575138-58070-1-git-send-email-win@wincent.com","threadId":"11272","inReplyTo":"1197552751-53480-2-git-send-email-win@wincent.com","subject":"[PATCH] Don't use the pager when running \"git diff --check\"","fromName":"Wincent Colaiuta","fromEmail":"win@wincent.com","sentAt":"2007-12-13T19:45:38Z","receivedAt":"2007-12-13T19:45:38Z","isPatch":true,"sender":{"key":"greg@hurrell.net","avatar":"https://avatars.githubusercontent.com/u/7074?v=4"},"body":"In 89d07f75 \"git diff\" learnt to not run the pager if the user passes\nthe --exit-code switch. This commit does the same for the --check\nswitch for the same reason: we want the user to get the exit status\nfrom \"git diff\", not the pager.\n\nSigned-off-by: Wincent Colaiuta <win@wincent.com>\n---\n\nThis really should have been part of my original patch,\n([PATCH 1/5] \"diff --check\" should affect exit status\"), but I only\njust discovered this now.\n\n builtin-diff.c |    3 ++-\n 1 files changed, 2 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-diff.c b/builtin-diff.c\nindex 86d01a3..ed2d218 100644\n--- a/builtin-diff.c\n+++ b/builtin-diff.c\n@@ -247,7 +247,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t/* If the user asked for our exit code then don't start a\n \t * pager or we would end up reporting its exit code instead.\n \t */\n-\tif (!DIFF_OPT_TST(&rev.diffopt, EXIT_WITH_STATUS))\n+\tif (!DIFF_OPT_TST(&rev.diffopt, EXIT_WITH_STATUS) &&\n+\t    (!&rev.diffopt.output_format & DIFF_FORMAT_CHECKDIFF))\n \t\tsetup_pager();\n \n \t/* Do we have --cached and not have a pending object, then\n-- \n1.5.4.rc0.5.gd201-dirty\n"},{"id":"63062","messageId":"7vir32ywyz.fsf@gitster.siamese.dyndns.org","threadId":"11272","inReplyTo":"1197552751-53480-2-git-send-email-win@wincent.com","subject":"Re: [PATCH 1/5] \"diff --check\" should affect exit status","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-13T23:51:48Z","receivedAt":"2007-12-13T23:51:48Z","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> @@ -2965,8 +2976,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}\n\nIsn't this wrong when check is not in effect?\n\nI think highjacking the \"did we encounter problems\" return value of the\nentire callchain for the purpose of checkdiff is very ugly and wrong to\nbegin with, so please do not argue \"but if checkdiff is not in effect,\nthe caller does not check CHECK_FAILED\".\n\nWouldn't it be much cleaner to make diff_flush_checkdiff(), or its\nunderlying function run_checkdiff(), set that CHECK_FAILED flag to\noptions structure, and return success?  The toplevel caller can decide\nto exit with non-zero when --check is in effect and CHECK_FAILED flag is\nset.\n"},{"id":"63063","messageId":"7vd4taywfw.fsf@gitster.siamese.dyndns.org","threadId":"11272","inReplyTo":"1197552751-53480-4-git-send-email-win@wincent.com","subject":"Re: [PATCH 3/5] Unify whitespace checking","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-14T00:03:15Z","receivedAt":"2007-12-14T00:03:15Z","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> The new function is called check_and_emit_line() and it does two things:\n> checks a line for whitespace errors and optionally emits it. The checking\n> is based on lines of content rather than patch lines (in other words, the\n> caller must strip the leading \"+\" or \"-\"); this was suggested by Junio on\n> the mailing list to allow for a future extension to \"git show\" to display\n> whitespace errors in blobs.\n\nI do not think \"git show\" is a realistic \"future extension\", by the\nway.  But at least from the interface-cleanliness point of view, I think\nnot passing the leading \"+\" to the function is a right thing to do.\n\n> diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\n> index dc538b3..a0a47dd 100755\n> --- a/t/t4015-diff-whitespace.sh\n> +++ b/t/t4015-diff-whitespace.sh\n> @@ -121,7 +121,7 @@ test_expect_success 'check mixed spaces and tabs in indent' '\n>  \n>  \t# This is indented with SP HT SP.\n>  \techo \" \t foo();\" > x &&\n> -\tgit diff --check | grep \"space before tab\"\n> +\tgit diff --check | grep \"Space in indent is followed by a tab\"\n>  \n>  '\n\nHmph.  I think with the multiple detection this rewording would make the\nerror message very long to read.  Was the rewording really necessary?\n\n> +/* If stream is non-NULL, emits the line after checking. */\n> +unsigned check_and_emit_line(const char *line, int len, unsigned ws_rule,\n> +                             FILE *stream, const char *set,\n> +                             const char *reset, const char *ws)\n> +{\n\nHonestly, I regretted suggesting this, fearing that it might make the\nchecking too costly, but the code is clean, readable, and does not look\ncostly at all.  Nice job.\n"},{"id":"63065","messageId":"7v1w9qyvzo.fsf@gitster.siamese.dyndns.org","threadId":"11272","inReplyTo":"1197552751-53480-5-git-send-email-win@wincent.com","subject":"Re: [PATCH 4/5] Make \"diff --check\" output match \"git apply\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-14T00:12:59Z","receivedAt":"2007-12-14T00:12:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nice attention to the details.\n"},{"id":"63066","messageId":"fjsi37$7ji$1@ger.gmane.org","threadId":"11272","inReplyTo":"1197552751-53480-3-git-send-email-win@wincent.com","subject":"Re: [PATCH 2/5] New version of pre-commit hook","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2007-12-14T00:17:42Z","receivedAt":"2007-12-14T00:17:42Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Wincent Colaiuta wrote:\n\n> Now that \"git diff --check\" indicates problems with its exit code the\n> pre-commit hook becomes a trivial one-liner.\n\n> -         if (/^(?:[<>=]){7}/) {\n> -             bad_line(\"unresolved merge conflict\", $_);\n> -         }\n\nAren't you losing this check with rewrite?\n\n-- \nJakub Narebski\nWarsaw, Poland\nShadeHawk on #git\n"},{"id":"63067","messageId":"7vsl26xffw.fsf@gitster.siamese.dyndns.org","threadId":"11272","inReplyTo":"1197552751-53480-6-git-send-email-win@wincent.com","subject":"Re: [PATCH 5/5] Add tests for \"git diff --check\" with core.whitespace options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-14T00:55:47Z","receivedAt":"2007-12-14T00:55:47Z","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> +test_expect_success 'check spaces as indentation (indent-with-non-tab: off)' '\n> +\n> +\tgit config core.whitespace \"-indent-with-non-tab\"\n> +\techo \"                foo ();\" > x &&\n> +\tgit diff --check\n> +\n> +'\n> +\n> +test_expect_failure 'check spaces as indentation (indent-with-non-tab: on)' '\n> +\n> +\tgit config core.whitespace \"indent-with-non-tab\" &&\n> +\techo \"                foo ();\" > x &&\n> +\tgit diff --check\n> +\n> +'\n> +\n>  test_done\n\nIt is good to check both positive and negative cases, but I would prefer\navoiding test_expect_failure.  You cannot tell if the \"git config\"\nbefore the real test failed or \"git diff --check\" correctly reported\nwhitespace breakage --- both would cause the test to pass.\n"},{"id":"63075","messageId":"7vbq8uxbzx.fsf@gitster.siamese.dyndns.org","threadId":"11272","inReplyTo":"7vir32ywyz.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/5] \"diff --check\" should affect exit status","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-14T02:10:10Z","receivedAt":"2007-12-14T02:10:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> I think highjacking the \"did we encounter problems\" return value of the\n> entire callchain for the purpose of checkdiff is very ugly and wrong to\n> begin with,...\n\nHow about this on top of your 1/5?  It mostly is about reverting the\ndamage to the higher level callchain.  Instead, builtin_checkdiff()\ninspects the checkdiff data and sets the CHECK_FAILED flag to the\ndiffopt structure.  The callers are already checking that flag so there\nis nothing to change for them.\n\n--\n\ndiff --git a/diff.c b/diff.c\nindex 39109a6..fc496bf 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1456,7 +1456,7 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n \tdiff_free_filespec_data(two);\n }\n \n-static int builtin_checkdiff(const char *name_a, const char *name_b,\n+static void 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@@ -1464,7 +1464,7 @@ static int builtin_checkdiff(const char *name_a, const char *name_b,\n \tstruct checkdiff_t data;\n \n \tif (!two)\n-\t\treturn 0;\n+\t\treturn;\n \n \tmemset(&data, 0, sizeof(data));\n \tdata.xm.consume = checkdiff_consume;\n@@ -1493,7 +1493,8 @@ static int 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+\tif (data.status)\n+\t\tDIFF_OPT_SET(o, CHECK_FAILED);\n }\n \n struct diff_filespec *alloc_filespec(const char *path)\n@@ -2078,14 +2079,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 int run_checkdiff(struct diff_filepair *p, struct diff_options *o)\n+static void 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 0;\n+\t\treturn;\n \t}\n \n \tname = p->one->path;\n@@ -2094,7 +2095,7 @@ static int 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-\treturn builtin_checkdiff(name, other, p->one, p->two, o);\n+\tbuiltin_checkdiff(name, other, p->one, p->two, o);\n }\n \n void diff_setup(struct diff_options *options)\n@@ -2596,17 +2597,17 @@ static void diff_flush_stat(struct diff_filepair *p, struct diff_options *o,\n \trun_diffstat(p, o, diffstat);\n }\n \n-static int diff_flush_checkdiff(struct diff_filepair *p,\n+static void diff_flush_checkdiff(struct diff_filepair *p,\n \t\tstruct diff_options *o)\n {\n \tif (diff_unmodified_pair(p))\n-\t\treturn 0;\n+\t\treturn;\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 0; /* no tree diffs in patch format */\n+\t\treturn; /* no tree diffs in patch format */\n \n-\treturn run_checkdiff(p, o);\n+\trun_checkdiff(p, o);\n }\n \n int diff_queue_is_empty(void)\n@@ -2725,19 +2726,16 @@ static int check_pair_status(struct diff_filepair *p)\n \t}\n }\n \n-static int flush_one_pair(struct diff_filepair *p, struct diff_options *opt)\n+static void 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\treturn diff_flush_checkdiff(p, opt);\n+\t\tdiff_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@@ -2976,8 +2974,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) && flush_one_pair(p, options))\n-\t\t\t\tDIFF_OPT_SET(options, CHECK_FAILED);\n+\t\t\tif (check_pair_status(p))\n+\t\t\t\tflush_one_pair(p, options);\n \t\t}\n \t\tseparator++;\n \t}\n-- \n1.5.4.rc0.1.g37d0\n"},{"id":"63081","messageId":"20071214045127.GC10169@sigill.intra.peff.net","threadId":"11272","inReplyTo":"1197575138-58070-1-git-send-email-win@wincent.com","subject":"Re: [PATCH] Don't use the pager when running \"git diff --check\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-12-14T04:51:27Z","receivedAt":"2007-12-14T04:51:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 13, 2007 at 08:45:38PM +0100, Wincent Colaiuta wrote:\n\n> In 89d07f75 \"git diff\" learnt to not run the pager if the user passes\n> the --exit-code switch. This commit does the same for the --check\n> switch for the same reason: we want the user to get the exit status\n> from \"git diff\", not the pager.\n\nBut --check is also producing useful output, which might need paged. So\nyou are sacrificing existing interactive use of --check for scriptable\nexit-code uses. If you really want the exit code, why not \"git diff\n--check --exit-code\"?\n\nOTOH, I am not too sad to lose the paging behavior; it would take quite\na few whitespace errors to scroll off the screen.\n\n-Peff\n"},{"id":"63083","messageId":"7vmysdx3la.fsf@gitster.siamese.dyndns.org","threadId":"11272","inReplyTo":"20071214045127.GC10169@sigill.intra.peff.net","subject":"Re: [PATCH] Don't use the pager when running \"git diff --check\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-14T05:11:45Z","receivedAt":"2007-12-14T05:11:45Z","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> On Thu, Dec 13, 2007 at 08:45:38PM +0100, Wincent Colaiuta wrote:\n>\n>> In 89d07f75 \"git diff\" learnt to not run the pager if the user passes\n>> the --exit-code switch. This commit does the same for the --check\n>> switch for the same reason: we want the user to get the exit status\n>> from \"git diff\", not the pager.\n>\n> But --check is also producing useful output, which might need paged. So\n> you are sacrificing existing interactive use of --check for scriptable\n> exit-code uses. If you really want the exit code, why not \"git diff\n> --check --exit-code\"?\n>\n> OTOH, I am not too sad to lose the paging behavior; it would take quite\n> a few whitespace errors to scroll off the screen.\n\nYou are right.  While I do not personally miss paging output, it is a\nregression not to page --check output by default.\n\nBy the way, there is no reason to make --check and --exit-code mutually\nexclusive either.  You could say with --exit-code the command will exit\nwith status 01 or'ed in if trees are not identical, and with --check the\ncommand will exit with status 02 or'ed in.  Loosely written scripted\ncallers can continue doing:\n\n\tif git --no-pager diff --exit-code\n        then\n        \tthey are different\n        fi\n\n\tif git --no-pager diff --check\n\tthen\n        \tthere are funky blanks\n\tfi\n\nwhile the ones that are aware of the new behaviour of --check can:\n\n\tgit --no-pager diff --check --exit-code\n        case $? in\n        0)\tall is well ;;\n        1)\tclean difference there ;;\n        3)\tdirty difference there ;;\n        2)\tcannot happen ;;\n\t*)\tbombed out, as die exits with 128 ;;\n\tesac\n"},{"id":"63114","messageId":"06C937AD-24B3-496B-AE76-B63CA007BEBB@wincent.com","threadId":"11272","inReplyTo":"fjsi37$7ji$1@ger.gmane.org","subject":"Re: [PATCH 2/5] New version of pre-commit hook","fromName":"Wincent Colaiuta","fromEmail":"win@wincent.com","sentAt":"2007-12-14T07:24:04Z","receivedAt":"2007-12-14T07:24:04Z","isPatch":true,"sender":{"key":"greg@hurrell.net","avatar":"https://avatars.githubusercontent.com/u/7074?v=4"},"body":"El 14/12/2007, a las 1:17, Jakub Narebski escribió:\n\n> Wincent Colaiuta wrote:\n>\n>> Now that \"git diff --check\" indicates problems with its exit code the\n>> pre-commit hook becomes a trivial one-liner.\n>\n>> -         if (/^(?:[<>=]){7}/) {\n>> -             bad_line(\"unresolved merge conflict\", $_);\n>> -         }\n>\n> Aren't you losing this check with rewrite?\n\nYes. If that's a problem then this is definitely a \"no-goer\".\n\nCheers,\nWincent\n"},{"id":"63116","messageId":"7vy7bxu3w5.fsf@gitster.siamese.dyndns.org","threadId":"11272","inReplyTo":"7vmysdx3la.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Don't use the pager when running \"git diff --check\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-14T07:33:30Z","receivedAt":"2007-12-14T07:33:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> By the way, there is no reason to make --check and --exit-code mutually\n> exclusive either.\n\nPerhaps something like this.  Regardless of the exclusivity issue, I\nthink the \"diff_result_code()\" helper function is a good clean-up.\n\n---\n builtin-diff-files.c       |    6 +-----\n builtin-diff-index.c       |    6 +-----\n builtin-diff-tree.c        |    6 ++----\n builtin-diff.c             |   11 ++++-------\n diff.c                     |   19 ++++++++++++++-----\n diff.h                     |    2 ++\n t/t4015-diff-whitespace.sh |    4 ++--\n 7 files changed, 26 insertions(+), 28 deletions(-)\n\ndiff --git a/builtin-diff-files.c b/builtin-diff-files.c\nindex 4afc872..9c04111 100644\n--- a/builtin-diff-files.c\n+++ b/builtin-diff-files.c\n@@ -31,9 +31,5 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)\n \tif (!rev.diffopt.output_format)\n \t\trev.diffopt.output_format = DIFF_FORMAT_RAW;\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+\treturn diff_result_code(&rev.diffopt, result);\n }\ndiff --git a/builtin-diff-index.c b/builtin-diff-index.c\nindex 532b284..0f2390a 100644\n--- a/builtin-diff-index.c\n+++ b/builtin-diff-index.c\n@@ -44,9 +44,5 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)\n \t\treturn -1;\n \t}\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+\treturn diff_result_code(&rev.diffopt, result);\n }\ndiff --git a/builtin-diff-tree.c b/builtin-diff-tree.c\nindex 9ab90cb..ebc50ef 100644\n--- a/builtin-diff-tree.c\n+++ b/builtin-diff-tree.c\n@@ -132,8 +132,6 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n \t\t\t\tdiff_tree_stdin(line);\n \t\t}\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+\n+\treturn diff_result_code(&opt->diffopt, 0);\n }\ndiff --git a/builtin-diff.c b/builtin-diff.c\nindex 9d878f6..29365a0 100644\n--- a/builtin-diff.c\n+++ b/builtin-diff.c\n@@ -244,11 +244,11 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \tDIFF_OPT_SET(&rev.diffopt, ALLOW_EXTERNAL);\n \tDIFF_OPT_SET(&rev.diffopt, RECURSIVE);\n \n-\t/* If the user asked for our exit code then don't start a\n+\t/*\n+\t * If the user asked for our exit code then don't start a\n \t * pager or we would end up reporting its exit code instead.\n \t */\n-\tif (!DIFF_OPT_TST(&rev.diffopt, EXIT_WITH_STATUS) &&\n-\t    (!(rev.diffopt.output_format & DIFF_FORMAT_CHECKDIFF)))\n+\tif (!DIFF_OPT_TST(&rev.diffopt, EXIT_WITH_STATUS))\n \t\tsetup_pager();\n \n \t/* Do we have --cached and not have a pending object, then\n@@ -352,10 +352,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \telse\n \t\tresult = builtin_diff_combined(&rev, argc, argv,\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-\tif (rev.diffopt.output_format & DIFF_FORMAT_CHECKDIFF)\n-\t\treturn DIFF_OPT_TST(&rev.diffopt, CHECK_FAILED) != 0;\n+\tresult = diff_result_code(&rev.diffopt, result);\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 8237075..3e46ff8 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2125,12 +2125,7 @@ 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@@ -3180,6 +3175,20 @@ void diffcore_std(struct diff_options *options)\n \t\tDIFF_OPT_CLR(options, HAS_CHANGES);\n }\n \n+int diff_result_code(struct diff_options *opt, int status)\n+{\n+\tint result = 0;\n+\tif (!DIFF_OPT_TST(opt, EXIT_WITH_STATUS) &&\n+\t    !(opt->output_format & DIFF_FORMAT_CHECKDIFF))\n+\t\treturn status;\n+\tif (DIFF_OPT_TST(opt, EXIT_WITH_STATUS) &&\n+\t    DIFF_OPT_TST(opt, HAS_CHANGES))\n+\t\tresult |= 01;\n+\tif ((opt->output_format & DIFF_FORMAT_CHECKDIFF) &&\n+\t    DIFF_OPT_TST(opt, CHECK_FAILED))\n+\t\tresult |= 02;\n+\treturn result;\n+}\n \n void diff_addremove(struct diff_options *options,\n \t\t    int addremove, unsigned mode,\ndiff --git a/diff.h b/diff.h\nindex 5d50d93..7e8000a 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -247,4 +247,6 @@ extern int run_diff_index(struct rev_info *revs, int cached);\n extern int do_diff_cache(const unsigned char *, struct diff_options *);\n extern int diff_flush_patch_id(struct diff_options *, unsigned char *);\n \n+extern int diff_result_code(struct diff_options *, int);\n+\n #endif /* DIFF_H */\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex dc538b3..757a27a 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -148,14 +148,14 @@ test_expect_failure 'check with space before tab in indent' '\n \n '\n \n-test_expect_failure '--check and --exit-code are exclusive' '\n+test_expect_success '--check and --exit-code are not exclusive' '\n \n \tgit checkout x &&\n \tgit diff --check --exit-code\n \n '\n \n-test_expect_failure '--check and --quiet are exclusive' '\n+test_expect_success '--check and --quiet are not exclusive' '\n \n \tgit diff --check --quiet\n \n"},{"id":"63117","messageId":"85B8B116-78F6-4D37-A9DB-30EECAB97956@wincent.com","threadId":"11272","inReplyTo":"7vd4taywfw.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 3/5] Unify whitespace checking","fromName":"Wincent Colaiuta","fromEmail":"win@wincent.com","sentAt":"2007-12-14T07:36:09Z","receivedAt":"2007-12-14T07:36:09Z","isPatch":true,"sender":{"key":"greg@hurrell.net","avatar":"https://avatars.githubusercontent.com/u/7074?v=4"},"body":"El 14/12/2007, a las 1:03, Junio C Hamano escribió:\n\n>> -\tgit diff --check | grep \"space before tab\"\n>> +\tgit diff --check | grep \"Space in indent is followed by a tab\"\n>>\n> Hmph.  I think with the multiple detection this rewording would make  \n> the\n> error message very long to read.  Was the rewording really necessary?\n\nWell, that wording came from builtin-apply.c so it wasn't really a \"re- \nwording\" but an extraction. The other string comes from \"diff.c\". So I  \nhad to pick one of them in unifying them and basically the one from  \nbuiltin-apply.c won because it was the site where I extracted from  \nfirst. So basically it was arbitrary.\n\nIf I were to actually *re-word* the message I think something like  \nthis would be the compromise between clarity and conciseness:\n\n\"space before tab in indent\"\n\nAnd in the same spirit, the other two strings extracted from builtin- \napply.c into the new whitespace_error_string() function:\n\n\"Adds trailing whitespace\"\n\"Indent more than 8 places with spaces\"\n\nCould be shortened to:\n\n\"trailing whitespace\" (diff.c says \"white space at end\")\n\"indent using spaces\"\n\nBut I personally have no strong conviction about this, so I'm happy  \nfor it to be whatever you want.\n\n>> +/* If stream is non-NULL, emits the line after checking. */\n>> +unsigned check_and_emit_line(const char *line, int len, unsigned  \n>> ws_rule,\n>> +                             FILE *stream, const char *set,\n>> +                             const char *reset, const char *ws)\n>> +{\n>\n> Honestly, I regretted suggesting this, fearing that it might make the\n> checking too costly, but the code is clean, readable, and does not  \n> look\n> costly at all.  Nice job.\n\nThanks. The check_and_emit_line function itself is fairly clean  \ninternally (apart from the somewhat ugly parameter list, but that was  \nmostly inherited from the emit_line_with_ws function which it  \nreplaces). It's the sites where check_and_emit_line is called that  \nlook a bit ugly, but those can hopefully be cleaned up at some point  \nin the future.\n\nCheers,\nWincent\n"},{"id":"63118","messageId":"26AEA0A9-9F15-4245-9D27-61050DA57E6F@wincent.com","threadId":"11272","inReplyTo":"7vmysdx3la.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Don't use the pager when running \"git diff --check\"","fromName":"Wincent Colaiuta","fromEmail":"win@wincent.com","sentAt":"2007-12-14T07:47:44Z","receivedAt":"2007-12-14T07:47:44Z","isPatch":true,"sender":{"key":"greg@hurrell.net","avatar":"https://avatars.githubusercontent.com/u/7074?v=4"},"body":"El 14/12/2007, a las 6:11, Junio C Hamano escribió:\n\n> Jeff King <peff@peff.net> writes:\n>\n>> On Thu, Dec 13, 2007 at 08:45:38PM +0100, Wincent Colaiuta wrote:\n>>\n>>> In 89d07f75 \"git diff\" learnt to not run the pager if the user  \n>>> passes\n>>> the --exit-code switch. This commit does the same for the --check\n>>> switch for the same reason: we want the user to get the exit status\n>>> from \"git diff\", not the pager.\n>>\n>> But --check is also producing useful output, which might need  \n>> paged. So\n>> you are sacrificing existing interactive use of --check for  \n>> scriptable\n>> exit-code uses. If you really want the exit code, why not \"git diff\n>> --check --exit-code\"?\n>>\n>> OTOH, I am not too sad to lose the paging behavior; it would take  \n>> quite\n>> a few whitespace errors to scroll off the screen.\n>\n> You are right.  While I do not personally miss paging output, it is a\n> regression not to page --check output by default.\n\nI thought this was ok because \"git diff --exit-code\" also produces  \nuseful output and also turns off the pager.\n\n> By the way, there is no reason to make --check and --exit-code  \n> mutually\n> exclusive either.  You could say with --exit-code the command will  \n> exit\n> with status 01 or'ed in if trees are not identical, and with --check  \n> the\n> command will exit with status 02 or'ed in.\n\nYes, this sounds fine to me.\n\nCheers,\nWincent\n"},{"id":"63120","messageId":"5A086A2E-16DB-4A3C-92E6-F879189A87FE@wincent.com","threadId":"11272","inReplyTo":"7vy7bxu3w5.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Don't use the pager when running \"git diff --check\"","fromName":"Wincent Colaiuta","fromEmail":"win@wincent.com","sentAt":"2007-12-14T07:51:02Z","receivedAt":"2007-12-14T07:51:02Z","isPatch":true,"sender":{"key":"greg@hurrell.net","avatar":"https://avatars.githubusercontent.com/u/7074?v=4"},"body":"El 14/12/2007, a las 8:33, Junio C Hamano escribió:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> By the way, there is no reason to make --check and --exit-code  \n>> mutually\n>> exclusive either.\n>\n> Perhaps something like this.  Regardless of the exclusivity issue, I\n> think the \"diff_result_code()\" helper function is a good clean-up.\n\nTotally agreed. It replaces some groups of very-similar lines.\n\nI'll integrate these (and the other suggestions) some time this  \nmorning and post a new version of the series that incorporates all the  \nfeedback and patch suggestions.\n\nCheers,\nWincent\n"},{"id":"63121","messageId":"7vr6hpu2sq.fsf@gitster.siamese.dyndns.org","threadId":"11272","inReplyTo":"5A086A2E-16DB-4A3C-92E6-F879189A87FE@wincent.com","subject":"Re: [PATCH] Don't use the pager when running \"git diff --check\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-14T07:57:09Z","receivedAt":"2007-12-14T07:57:09Z","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> I'll integrate these (and the other suggestions) some time this  \n> morning and post a new version of the series that incorporates all the  \n> feedback and patch suggestions.\n\nHeh, I am about to push out fixed-up results, so it might save both of\nus some time if you looked at it first and then complained on my\nscrewups.\n"},{"id":"63134","messageId":"200712141123.27519.jnareb@gmail.com","threadId":"11272","inReplyTo":"06C937AD-24B3-496B-AE76-B63CA007BEBB@wincent.com","subject":"Re: [PATCH 2/5] New version of pre-commit hook","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2007-12-14T10:23:19Z","receivedAt":"2007-12-14T10:23:19Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Dnia piątek 14. grudnia 2007 08:24, Wincent Colaiuta napisał:\n> El 14/12/2007, a las 1:17, Jakub Narebski escribió:\n> \n>> Wincent Colaiuta wrote:\n>>\n>>> Now that \"git diff --check\" indicates problems with its exit code the\n>>> pre-commit hook becomes a trivial one-liner.\n>>\n>>> -         if (/^(?:[<>=]){7}/) {\n>>> -             bad_line(\"unresolved merge conflict\", $_);\n>>> -         }\n>>\n>> Aren't you losing this check with rewrite?\n> \n> Yes. If that's a problem then this is definitely a \"no-goer\".\n\nWell, this only means that it wouldn't be one-liner, but replacing\nall whitespace damage checks by \"git diff --check\" is still a good\nidea.\n\nPerhaps we could add 'merge conflict' check to git diff/git applyt too...\n-- \nJakub Narebski\nPoland\n"},{"id":"63206","messageId":"7v1w9pq9o9.fsf@gitster.siamese.dyndns.org","threadId":"11272","inReplyTo":"26AEA0A9-9F15-4245-9D27-61050DA57E6F@wincent.com","subject":"Re: [PATCH] Don't use the pager when running \"git diff --check\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-14T20:54:30Z","receivedAt":"2007-12-14T20:54:30Z","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> El 14/12/2007, a las 6:11, Junio C Hamano escribió:\n>\n>> You are right.  While I do not personally miss paging output, it is a\n>> regression not to page --check output by default.\n>\n> I thought this was ok because \"git diff --exit-code\" also produces\n> useful output and also turns off the pager.\n\nIt is different.  --exit-code was that way from day one.  The primary\nuse of --check has been (and I suspect it will continue to be) for\npeople to _view_ the diff, spot problems so that they can fix them up,\nand for that use case, exit code does not matter but pageability does.\nYou are introducing a new behaviour and a new use case --- that does not\ngive you a license to break other existing use cases.\n"}]}