From 62c64895cfcf3bbf34969a69fa96a631f7d5b14e Mon Sep 17 00:00:00 2001 From: Wincent Colaiuta Date: Thu, 13 Dec 2007 21:24:52 +0100 Subject: [PATCH 1/7] "diff --check" should affect exit status "git diff" has a --check option that can be used to check for whitespace problems but it only reported by printing warnings to the console. Now when the --check option is used we give a non-zero exit status, making "git diff --check" nicer to use in scripts and hooks. Signed-off-by: Wincent Colaiuta Signed-off-by: Junio C Hamano --- Documentation/diff-options.txt | 4 +- builtin-diff-files.c | 2 + builtin-diff-index.c | 2 + builtin-diff-tree.c | 28 +++---- builtin-diff.c | 6 +- diff.c | 9 +++ diff.h | 1 + t/t4015-diff-whitespace.sh | 137 ++++++++++++++++++++++++++++++++- 8 files changed, 171 insertions(+), 18 deletions(-) diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt index 5d22b7b58c..9ecc1d7bc4 100644 --- a/Documentation/diff-options.txt +++ b/Documentation/diff-options.txt @@ -93,7 +93,9 @@ endif::git-format-patch[] --check:: Warn if changes introduce trailing whitespace - or an indent that uses a space before a tab. + or an indent that uses a space before a tab. Exits with + non-zero status if problems are found. Not compatible with + --exit-code. --full-index:: Instead of the first handful characters, show full diff --git a/builtin-diff-files.c b/builtin-diff-files.c index 046b7e34b5..4afc8724e7 100644 --- a/builtin-diff-files.c +++ b/builtin-diff-files.c @@ -33,5 +33,7 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix) result = run_diff_files_cmd(&rev, argc, argv); if (DIFF_OPT_TST(&rev.diffopt, EXIT_WITH_STATUS)) return DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES) != 0; + if (rev.diffopt.output_format & DIFF_FORMAT_CHECKDIFF) + return DIFF_OPT_TST(&rev.diffopt, CHECK_FAILED) != 0; return result; } diff --git a/builtin-diff-index.c b/builtin-diff-index.c index 556c506bfa..532b284b10 100644 --- a/builtin-diff-index.c +++ b/builtin-diff-index.c @@ -46,5 +46,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix) result = run_diff_index(&rev, cached); if (DIFF_OPT_TST(&rev.diffopt, EXIT_WITH_STATUS)) return DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES) != 0; + if (rev.diffopt.output_format & DIFF_FORMAT_CHECKDIFF) + return DIFF_OPT_TST(&rev.diffopt, CHECK_FAILED) != 0; return result; } diff --git a/builtin-diff-tree.c b/builtin-diff-tree.c index 2e13716eec..9ab90cb4c5 100644 --- a/builtin-diff-tree.c +++ b/builtin-diff-tree.c @@ -117,23 +117,23 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix) break; } - if (!read_stdin) - return DIFF_OPT_TST(&opt->diffopt, EXIT_WITH_STATUS) - && DIFF_OPT_TST(&opt->diffopt, HAS_CHANGES); + if (read_stdin) { + if (opt->diffopt.detect_rename) + opt->diffopt.setup |= (DIFF_SETUP_USE_SIZE_CACHE | + DIFF_SETUP_USE_CACHE); + while (fgets(line, sizeof(line), stdin)) { + unsigned char sha1[20]; - if (opt->diffopt.detect_rename) - opt->diffopt.setup |= (DIFF_SETUP_USE_SIZE_CACHE | - DIFF_SETUP_USE_CACHE); - while (fgets(line, sizeof(line), stdin)) { - unsigned char sha1[20]; - - if (get_sha1_hex(line, sha1)) { - fputs(line, stdout); - fflush(stdout); + if (get_sha1_hex(line, sha1)) { + fputs(line, stdout); + fflush(stdout); + } + else + diff_tree_stdin(line); } - else - diff_tree_stdin(line); } + if (opt->diffopt.output_format & DIFF_FORMAT_CHECKDIFF) + return DIFF_OPT_TST(&opt->diffopt, CHECK_FAILED) != 0; return DIFF_OPT_TST(&opt->diffopt, EXIT_WITH_STATUS) && DIFF_OPT_TST(&opt->diffopt, HAS_CHANGES); } diff --git a/builtin-diff.c b/builtin-diff.c index 55fb84c730..9d878f6a71 100644 --- a/builtin-diff.c +++ b/builtin-diff.c @@ -247,7 +247,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix) /* If the user asked for our exit code then don't start a * pager or we would end up reporting its exit code instead. */ - if (!DIFF_OPT_TST(&rev.diffopt, EXIT_WITH_STATUS)) + if (!DIFF_OPT_TST(&rev.diffopt, EXIT_WITH_STATUS) && + (!(rev.diffopt.output_format & DIFF_FORMAT_CHECKDIFF))) setup_pager(); /* Do we have --cached and not have a pending object, then @@ -353,7 +354,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix) ent, ents); if (DIFF_OPT_TST(&rev.diffopt, EXIT_WITH_STATUS)) result = DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES) != 0; - + if (rev.diffopt.output_format & DIFF_FORMAT_CHECKDIFF) + return DIFF_OPT_TST(&rev.diffopt, CHECK_FAILED) != 0; if (1 < rev.diffopt.skip_stat_unmatch) refresh_index_quietly(); return result; diff --git a/diff.c b/diff.c index 3dd2f35f73..8237075484 100644 --- a/diff.c +++ b/diff.c @@ -1031,6 +1031,7 @@ struct checkdiff_t { const char *filename; int lineno, color_diff; unsigned ws_rule; + unsigned status; }; static void checkdiff_consume(void *priv, char *line, unsigned long len) @@ -1064,6 +1065,7 @@ static void checkdiff_consume(void *priv, char *line, unsigned long len) white_space_at_end = 1; if (space_before_tab || white_space_at_end) { + data->status = 1; printf("%s:%d: %s", data->filename, data->lineno, ws); if (space_before_tab) { printf("space before tab"); @@ -1491,6 +1493,8 @@ static void builtin_checkdiff(const char *name_a, const char *name_b, free_and_return: diff_free_filespec_data(one); diff_free_filespec_data(two); + if (data.status) + DIFF_OPT_SET(o, CHECK_FAILED); } struct diff_filespec *alloc_filespec(const char *path) @@ -2121,7 +2125,12 @@ int diff_setup_done(struct diff_options *options) if (options->output_format & DIFF_FORMAT_NAME_STATUS) count++; if (options->output_format & DIFF_FORMAT_CHECKDIFF) + { count++; + if (DIFF_OPT_TST(options, QUIET) || + DIFF_OPT_TST(options, EXIT_WITH_STATUS)) + die("--check may not be used with --quiet or --exit-code"); + } if (options->output_format & DIFF_FORMAT_NO_OUTPUT) count++; if (count > 1) diff --git a/diff.h b/diff.h index a52496a108..5d50d93a52 100644 --- a/diff.h +++ b/diff.h @@ -59,6 +59,7 @@ typedef void (*diff_format_fn_t)(struct diff_queue_struct *q, #define DIFF_OPT_ALLOW_EXTERNAL (1 << 13) #define DIFF_OPT_EXIT_WITH_STATUS (1 << 14) #define DIFF_OPT_REVERSE_DIFF (1 << 15) +#define DIFF_OPT_CHECK_FAILED (1 << 16) #define DIFF_OPT_TST(opts, flag) ((opts)->flags & DIFF_OPT_##flag) #define DIFF_OPT_SET(opts, flag) ((opts)->flags |= DIFF_OPT_##flag) #define DIFF_OPT_CLR(opts, flag) ((opts)->flags &= ~DIFF_OPT_##flag) diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh index 6adf9d11d0..dc538b3e33 100755 --- a/t/t4015-diff-whitespace.sh +++ b/t/t4015-diff-whitespace.sh @@ -117,7 +117,6 @@ EOF git diff -b > out test_expect_success 'another test, with -b' 'git diff expect out' - test_expect_success 'check mixed spaces and tabs in indent' ' # This is indented with SP HT SP. @@ -126,4 +125,140 @@ test_expect_success 'check mixed spaces and tabs in indent' ' ' +test_expect_success 'check with no whitespace errors' ' + + git commit -m "snapshot" && + echo "foo();" > x && + git diff --check + +' + +test_expect_failure 'check with trailing whitespace' ' + + echo "foo(); " > x && + git diff --check + +' + +test_expect_failure 'check with space before tab in indent' ' + + # indent has space followed by hard tab + echo " foo();" > x && + git diff --check + +' + +test_expect_failure '--check and --exit-code are exclusive' ' + + git checkout x && + git diff --check --exit-code + +' + +test_expect_failure '--check and --quiet are exclusive' ' + + git diff --check --quiet + +' + +test_expect_success 'check staged with no whitespace errors' ' + + echo "foo();" > x && + git add x && + git diff --cached --check + +' + +test_expect_failure 'check staged with trailing whitespace' ' + + echo "foo(); " > x && + git add x && + git diff --cached --check + +' + +test_expect_failure 'check staged with space before tab in indent' ' + + # indent has space followed by hard tab + echo " foo();" > x && + git add x && + git diff --cached --check + +' + +test_expect_success 'check with no whitespace errors (diff-index)' ' + + echo "foo();" > x && + git add x && + git diff-index --check HEAD + +' + +test_expect_failure 'check with trailing whitespace (diff-index)' ' + + echo "foo(); " > x && + git add x && + git diff-index --check HEAD + +' + +test_expect_failure 'check with space before tab in indent (diff-index)' ' + + # indent has space followed by hard tab + echo " foo();" > x && + git add x && + git diff-index --check HEAD + +' + +test_expect_success 'check staged with no whitespace errors (diff-index)' ' + + echo "foo();" > x && + git add x && + git diff-index --cached --check HEAD + +' + +test_expect_failure 'check staged with trailing whitespace (diff-index)' ' + + echo "foo(); " > x && + git add x && + git diff-index --cached --check HEAD + +' + +test_expect_failure 'check staged with space before tab in indent (diff-index)' ' + + # indent has space followed by hard tab + echo " foo();" > x && + git add x && + git diff-index --cached --check HEAD + +' + +test_expect_success 'check with no whitespace errors (diff-tree)' ' + + echo "foo();" > x && + git commit -m "new commit" x && + git diff-tree --check HEAD^ HEAD + +' + +test_expect_failure 'check with trailing whitespace (diff-tree)' ' + + echo "foo(); " > x && + git commit -m "another commit" x && + git diff-tree --check HEAD^ HEAD + +' + +test_expect_failure 'check with space before tab in indent (diff-tree)' ' + + # indent has space followed by hard tab + echo " foo();" > x && + git commit -m "yet another" x && + git diff-tree --check HEAD^ HEAD + +' + test_done From da31b358fb39b32622c14343ffe157a765f3948b Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 13 Dec 2007 23:40:27 -0800 Subject: [PATCH 2/7] diff --check: minor fixups There is no reason --exit-code and --check-diff must be mutually exclusive, so assign different bits to different results and allow them to be returned from the command. Introduce diff_result_code() to factor out the common code to decide final status code based on diffopt settings and use it everywhere. Update tests to match the above fix. Turning pager off when "diff --check" is used is a regression. Signed-off-by: Junio C Hamano --- builtin-diff-files.c | 6 +----- builtin-diff-index.c | 6 +----- builtin-diff-tree.c | 6 ++---- builtin-diff.c | 11 ++++------- diff.c | 19 ++++++++++++++----- diff.h | 2 ++ t/t4015-diff-whitespace.sh | 4 ++-- 7 files changed, 26 insertions(+), 28 deletions(-) diff --git a/builtin-diff-files.c b/builtin-diff-files.c index 4afc8724e7..9c04111656 100644 --- a/builtin-diff-files.c +++ b/builtin-diff-files.c @@ -31,9 +31,5 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix) if (!rev.diffopt.output_format) rev.diffopt.output_format = DIFF_FORMAT_RAW; result = run_diff_files_cmd(&rev, argc, argv); - if (DIFF_OPT_TST(&rev.diffopt, EXIT_WITH_STATUS)) - return DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES) != 0; - if (rev.diffopt.output_format & DIFF_FORMAT_CHECKDIFF) - return DIFF_OPT_TST(&rev.diffopt, CHECK_FAILED) != 0; - return result; + return diff_result_code(&rev.diffopt, result); } diff --git a/builtin-diff-index.c b/builtin-diff-index.c index 532b284b10..0f2390a20a 100644 --- a/builtin-diff-index.c +++ b/builtin-diff-index.c @@ -44,9 +44,5 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix) return -1; } result = run_diff_index(&rev, cached); - if (DIFF_OPT_TST(&rev.diffopt, EXIT_WITH_STATUS)) - return DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES) != 0; - if (rev.diffopt.output_format & DIFF_FORMAT_CHECKDIFF) - return DIFF_OPT_TST(&rev.diffopt, CHECK_FAILED) != 0; - return result; + return diff_result_code(&rev.diffopt, result); } diff --git a/builtin-diff-tree.c b/builtin-diff-tree.c index 9ab90cb4c5..ebc50efbd2 100644 --- a/builtin-diff-tree.c +++ b/builtin-diff-tree.c @@ -132,8 +132,6 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix) diff_tree_stdin(line); } } - if (opt->diffopt.output_format & DIFF_FORMAT_CHECKDIFF) - return DIFF_OPT_TST(&opt->diffopt, CHECK_FAILED) != 0; - return DIFF_OPT_TST(&opt->diffopt, EXIT_WITH_STATUS) - && DIFF_OPT_TST(&opt->diffopt, HAS_CHANGES); + + return diff_result_code(&opt->diffopt, 0); } diff --git a/builtin-diff.c b/builtin-diff.c index 9d878f6a71..29365a0b17 100644 --- a/builtin-diff.c +++ b/builtin-diff.c @@ -244,11 +244,11 @@ int cmd_diff(int argc, const char **argv, const char *prefix) DIFF_OPT_SET(&rev.diffopt, ALLOW_EXTERNAL); DIFF_OPT_SET(&rev.diffopt, RECURSIVE); - /* If the user asked for our exit code then don't start a + /* + * If the user asked for our exit code then don't start a * pager or we would end up reporting its exit code instead. */ - if (!DIFF_OPT_TST(&rev.diffopt, EXIT_WITH_STATUS) && - (!(rev.diffopt.output_format & DIFF_FORMAT_CHECKDIFF))) + if (!DIFF_OPT_TST(&rev.diffopt, EXIT_WITH_STATUS)) setup_pager(); /* Do we have --cached and not have a pending object, then @@ -352,10 +352,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix) else result = builtin_diff_combined(&rev, argc, argv, ent, ents); - if (DIFF_OPT_TST(&rev.diffopt, EXIT_WITH_STATUS)) - result = DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES) != 0; - if (rev.diffopt.output_format & DIFF_FORMAT_CHECKDIFF) - return DIFF_OPT_TST(&rev.diffopt, CHECK_FAILED) != 0; + result = diff_result_code(&rev.diffopt, result); if (1 < rev.diffopt.skip_stat_unmatch) refresh_index_quietly(); return result; diff --git a/diff.c b/diff.c index 8237075484..3e46ff8a75 100644 --- a/diff.c +++ b/diff.c @@ -2125,12 +2125,7 @@ int diff_setup_done(struct diff_options *options) if (options->output_format & DIFF_FORMAT_NAME_STATUS) count++; if (options->output_format & DIFF_FORMAT_CHECKDIFF) - { count++; - if (DIFF_OPT_TST(options, QUIET) || - DIFF_OPT_TST(options, EXIT_WITH_STATUS)) - die("--check may not be used with --quiet or --exit-code"); - } if (options->output_format & DIFF_FORMAT_NO_OUTPUT) count++; if (count > 1) @@ -3180,6 +3175,20 @@ void diffcore_std(struct diff_options *options) DIFF_OPT_CLR(options, HAS_CHANGES); } +int diff_result_code(struct diff_options *opt, int status) +{ + int result = 0; + if (!DIFF_OPT_TST(opt, EXIT_WITH_STATUS) && + !(opt->output_format & DIFF_FORMAT_CHECKDIFF)) + return status; + if (DIFF_OPT_TST(opt, EXIT_WITH_STATUS) && + DIFF_OPT_TST(opt, HAS_CHANGES)) + result |= 01; + if ((opt->output_format & DIFF_FORMAT_CHECKDIFF) && + DIFF_OPT_TST(opt, CHECK_FAILED)) + result |= 02; + return result; +} void diff_addremove(struct diff_options *options, int addremove, unsigned mode, diff --git a/diff.h b/diff.h index 5d50d93a52..7e8000a5d7 100644 --- a/diff.h +++ b/diff.h @@ -247,4 +247,6 @@ extern int run_diff_index(struct rev_info *revs, int cached); extern int do_diff_cache(const unsigned char *, struct diff_options *); extern int diff_flush_patch_id(struct diff_options *, unsigned char *); +extern int diff_result_code(struct diff_options *, int); + #endif /* DIFF_H */ diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh index dc538b3e33..757a27abdb 100755 --- a/t/t4015-diff-whitespace.sh +++ b/t/t4015-diff-whitespace.sh @@ -148,14 +148,14 @@ test_expect_failure 'check with space before tab in indent' ' ' -test_expect_failure '--check and --exit-code are exclusive' ' +test_expect_success '--check and --exit-code are not exclusive' ' git checkout x && git diff --check --exit-code ' -test_expect_failure '--check and --quiet are exclusive' ' +test_expect_success '--check and --quiet are not exclusive' ' git diff --check --quiet From c1795bb08aae9fb7e4dc1a01e292b85e59b1c640 Mon Sep 17 00:00:00 2001 From: Wincent Colaiuta Date: Thu, 13 Dec 2007 14:32:29 +0100 Subject: [PATCH 3/7] Unify whitespace checking This commit unifies three separate places where whitespace checking was performed: - the whitespace checking previously done in builtin-apply.c is extracted into a function in ws.c - the equivalent logic in "git diff" is removed - the emit_line_with_ws() function is also removed because that also rechecks the whitespace, and its functionality is rolled into ws.c The new function is called check_and_emit_line() and it does two things: checks a line for whitespace errors and optionally emits it. The checking is based on lines of content rather than patch lines (in other words, the caller must strip the leading "+" or "-"); this was suggested by Junio on the mailing list to allow for a future extension to "git show" to display whitespace errors in blobs. At the same time we teach it to report all classes of whitespace errors found for a given line rather than reporting only the first found error. Signed-off-by: Wincent Colaiuta Signed-off-by: Junio C Hamano --- builtin-apply.c | 54 +++------------ cache.h | 4 ++ diff.c | 138 +++++-------------------------------- t/t4015-diff-whitespace.sh | 2 +- ws.c | 105 ++++++++++++++++++++++++++++ 5 files changed, 139 insertions(+), 164 deletions(-) diff --git a/builtin-apply.c b/builtin-apply.c index f2e9a332ca..ab393f32e9 100644 --- a/builtin-apply.c +++ b/builtin-apply.c @@ -900,56 +900,22 @@ static int find_header(char *line, unsigned long size, int *hdrsize, struct patc static void check_whitespace(const char *line, int len, unsigned ws_rule) { - const char *err = "Adds trailing whitespace"; - int seen_space = 0; - int i; + char *err; + unsigned result = check_and_emit_line(line + 1, len - 1, ws_rule, + NULL, NULL, NULL, NULL); + if (!result) + return; - /* - * We know len is at least two, since we have a '+' and we - * checked that the last character was a '\n' before calling - * this function. That is, an addition of an empty line would - * check the '+' here. Sneaky... - */ - if ((ws_rule & WS_TRAILING_SPACE) && isspace(line[len-2])) - goto error; - - /* - * Make sure that there is no space followed by a tab in - * indentation. - */ - if (ws_rule & WS_SPACE_BEFORE_TAB) { - err = "Space in indent is followed by a tab"; - for (i = 1; i < len; i++) { - if (line[i] == '\t') { - if (seen_space) - goto error; - } - else if (line[i] == ' ') - seen_space = 1; - else - break; - } - } - - /* - * Make sure that the indentation does not contain more than - * 8 spaces. - */ - if ((ws_rule & WS_INDENT_WITH_NON_TAB) && - (8 < len) && !strncmp("+ ", line, 9)) { - err = "Indent more than 8 places with spaces"; - goto error; - } - return; - - error: whitespace_error++; if (squelch_whitespace_errors && squelch_whitespace_errors < whitespace_error) ; - else + else { + err = whitespace_error_string(result); fprintf(stderr, "%s.\n%s:%d:%.*s\n", - err, patch_input_file, linenr, len-2, line+1); + err, patch_input_file, linenr, len - 2, line + 1); + free(err); + } } /* diff --git a/cache.h b/cache.h index 27d90fe543..39331c28be 100644 --- a/cache.h +++ b/cache.h @@ -655,6 +655,10 @@ void shift_tree(const unsigned char *, const unsigned char *, unsigned char *, i extern unsigned whitespace_rule_cfg; extern unsigned whitespace_rule(const char *); extern unsigned parse_whitespace_rule(const char *); +extern unsigned check_and_emit_line(const char *line, int len, unsigned ws_rule, + FILE *stream, const char *set, + const char *reset, const char *ws); +extern char *whitespace_error_string(unsigned ws); /* ls-files */ int pathspec_match(const char **spec, char *matched, const char *filename, int skiplen); diff --git a/diff.c b/diff.c index 3e46ff8a75..577c773c3e 100644 --- a/diff.c +++ b/diff.c @@ -486,88 +486,9 @@ const char *diff_get_color(int diff_use_color, enum color_diff ix) static void emit_line(const char *set, const char *reset, const char *line, int len) { - if (len > 0 && line[len-1] == '\n') - len--; fputs(set, stdout); fwrite(line, len, 1, stdout); - puts(reset); -} - -static void emit_line_with_ws(int nparents, - const char *set, const char *reset, const char *ws, - const char *line, int len, unsigned ws_rule) -{ - int col0 = nparents; - int last_tab_in_indent = -1; - int last_space_in_indent = -1; - int i; - int tail = len; - int need_highlight_leading_space = 0; - /* - * The line is a newly added line. Does it have funny leading - * whitespaces? In indent, SP should never precede a TAB. In - * addition, under "indent with non tab" rule, there should not - * be more than 8 consecutive spaces. - */ - for (i = col0; i < len; i++) { - if (line[i] == '\t') { - last_tab_in_indent = i; - if ((ws_rule & WS_SPACE_BEFORE_TAB) && - 0 <= last_space_in_indent) - need_highlight_leading_space = 1; - } - else if (line[i] == ' ') - last_space_in_indent = i; - else - break; - } - if ((ws_rule & WS_INDENT_WITH_NON_TAB) && - 0 <= last_space_in_indent && - last_tab_in_indent < 0 && - 8 <= (i - col0)) { - last_tab_in_indent = i; - need_highlight_leading_space = 1; - } - fputs(set, stdout); - fwrite(line, col0, 1, stdout); fputs(reset, stdout); - if (((i == len) || line[i] == '\n') && i != col0) { - /* The whole line was indent */ - emit_line(ws, reset, line + col0, len - col0); - return; - } - i = col0; - if (need_highlight_leading_space) { - while (i < last_tab_in_indent) { - if (line[i] == ' ') { - fputs(ws, stdout); - putchar(' '); - fputs(reset, stdout); - } - else - putchar(line[i]); - i++; - } - } - tail = len - 1; - if (line[tail] == '\n' && i < tail) - tail--; - if (ws_rule & WS_TRAILING_SPACE) { - while (i < tail) { - if (!isspace(line[tail])) - break; - tail--; - } - } - if ((i < tail && line[tail + 1] != '\n')) { - /* This has whitespace between tail+1..len */ - fputs(set, stdout); - fwrite(line + i, tail - i + 1, 1, stdout); - fputs(reset, stdout); - emit_line(ws, reset, line + tail + 1, len - tail - 1); - } - else - emit_line(set, reset, line + i, len - i); } static void emit_add_line(const char *reset, struct emit_callback *ecbdata, const char *line, int len) @@ -577,9 +498,13 @@ static void emit_add_line(const char *reset, struct emit_callback *ecbdata, cons if (!*ws) emit_line(set, reset, line, len); - else - emit_line_with_ws(ecbdata->nparents, set, reset, ws, - line, len, ecbdata->ws_rule); + else { + /* Emit just the prefix, then the rest. */ + emit_line(set, reset, line, ecbdata->nparents); + (void)check_and_emit_line(line + ecbdata->nparents, + len - ecbdata->nparents, ecbdata->ws_rule, + stdout, set, reset, ws); + } } static void fn_out_consume(void *priv, char *line, unsigned long len) @@ -1040,45 +965,20 @@ static void checkdiff_consume(void *priv, char *line, unsigned long len) const char *ws = diff_get_color(data->color_diff, DIFF_WHITESPACE); const char *reset = diff_get_color(data->color_diff, DIFF_RESET); const char *set = diff_get_color(data->color_diff, DIFF_FILE_NEW); + char *err; if (line[0] == '+') { - int i, spaces = 0, space_before_tab = 0, white_space_at_end = 0; - - /* check space before tab */ - for (i = 1; i < len; i++) { - if (line[i] == ' ') - spaces++; - else if (line[i] == '\t') { - if (spaces) { - space_before_tab = 1; - break; - } - } - else - break; - } - - /* check whitespace at line end */ - if (line[len - 1] == '\n') - len--; - if (isspace(line[len - 1])) - white_space_at_end = 1; - - if (space_before_tab || white_space_at_end) { - data->status = 1; - printf("%s:%d: %s", data->filename, data->lineno, ws); - if (space_before_tab) { - printf("space before tab"); - if (white_space_at_end) - putchar(','); - } - if (white_space_at_end) - printf("whitespace at end"); - printf(":%s ", reset); - emit_line_with_ws(1, set, reset, ws, line, len, - data->ws_rule); - } - + data->status = check_and_emit_line(line + 1, len - 1, + data->ws_rule, NULL, NULL, NULL, NULL); + if (!data->status) + return; + err = whitespace_error_string(data->status); + printf("%s:%d: %s%s:%s ", data->filename, data->lineno, + ws, err, reset); + free(err); + emit_line(set, reset, line, 1); + (void)check_and_emit_line(line + 1, len - 1, data->ws_rule, + stdout, set, reset, ws); data->lineno++; } else if (line[0] == ' ') data->lineno++; diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh index 757a27abdb..595cd60623 100755 --- a/t/t4015-diff-whitespace.sh +++ b/t/t4015-diff-whitespace.sh @@ -121,7 +121,7 @@ test_expect_success 'check mixed spaces and tabs in indent' ' # This is indented with SP HT SP. echo " foo();" > x && - git diff --check | grep "space before tab" + git diff --check | grep "Space in indent is followed by a tab" ' diff --git a/ws.c b/ws.c index 52c10caf35..d7d1522f8a 100644 --- a/ws.c +++ b/ws.c @@ -94,3 +94,108 @@ unsigned whitespace_rule(const char *pathname) return whitespace_rule_cfg; } } + +/* The returned string should be freed by the caller. */ +char *whitespace_error_string(unsigned ws) +{ + struct strbuf err; + strbuf_init(&err, 0); + if (ws & WS_TRAILING_SPACE) + strbuf_addstr(&err, "Adds trailing whitespace"); + if (ws & WS_SPACE_BEFORE_TAB) { + if (err.len) + strbuf_addstr(&err, ", "); + strbuf_addstr(&err, "Space in indent is followed by a tab"); + } + if (ws & WS_INDENT_WITH_NON_TAB) { + if (err.len) + strbuf_addstr(&err, ", "); + strbuf_addstr(&err, "Indent more than 8 places with spaces"); + } + return strbuf_detach(&err, NULL); +} + +/* If stream is non-NULL, emits the line after checking. */ +unsigned check_and_emit_line(const char *line, int len, unsigned ws_rule, + FILE *stream, const char *set, + const char *reset, const char *ws) +{ + unsigned result = 0; + int leading_space = -1; + int trailing_whitespace = -1; + int trailing_newline = 0; + int i; + + /* Logic is simpler if we temporarily ignore the trailing newline. */ + if (len > 0 && line[len - 1] == '\n') { + trailing_newline = 1; + len--; + } + + /* Check for trailing whitespace. */ + if (ws_rule & WS_TRAILING_SPACE) { + for (i = len - 1; i >= 0; i--) { + if (isspace(line[i])) { + trailing_whitespace = i; + result |= WS_TRAILING_SPACE; + } + else + break; + } + } + + /* Check for space before tab in initial indent. */ + for (i = 0; i < len; i++) { + if (line[i] == '\t') { + if ((ws_rule & WS_SPACE_BEFORE_TAB) && + (leading_space != -1)) + result |= WS_SPACE_BEFORE_TAB; + break; + } + else if (line[i] == ' ') + leading_space = i; + else + break; + } + + /* Check for indent using non-tab. */ + if ((ws_rule & WS_INDENT_WITH_NON_TAB) && leading_space >= 8) + result |= WS_INDENT_WITH_NON_TAB; + + if (stream) { + /* Highlight errors in leading whitespace. */ + if ((result & WS_SPACE_BEFORE_TAB) || + (result & WS_INDENT_WITH_NON_TAB)) { + fputs(ws, stream); + fwrite(line, leading_space + 1, 1, stream); + fputs(reset, stream); + leading_space++; + } + else + leading_space = 0; + + /* Now the rest of the line starts at leading_space. + * The non-highlighted part ends at trailing_whitespace. */ + if (trailing_whitespace == -1) + trailing_whitespace = len; + + /* Emit non-highlighted (middle) segment. */ + if (trailing_whitespace - leading_space > 0) { + fputs(set, stream); + fwrite(line + leading_space, + trailing_whitespace - leading_space, 1, stream); + fputs(reset, stream); + } + + /* Highlight errors in trailing whitespace. */ + if (trailing_whitespace != len) { + fputs(ws, stream); + fwrite(line + trailing_whitespace, + len - trailing_whitespace, 1, stream); + fputs(reset, stream); + } + if (trailing_newline) + fputc('\n', stream); + } + return result; +} From 45e2a4b2b0a07cda05dda08b47906100c4711e4e Mon Sep 17 00:00:00 2001 From: Wincent Colaiuta Date: Thu, 13 Dec 2007 14:32:30 +0100 Subject: [PATCH 4/7] Make "diff --check" output match "git apply" For consistency, make the two tools report whitespace errors in the same way (the output of "diff --check" has been tweaked to match that of "git apply"). Note that although the textual content is basically the same only "git diff --check" provides a colorized version of the problematic lines; making "git apply" do colorization will require more extensive changes (figuring out the diff colorization preferences of the user) and so that will be a subject for another commit. Signed-off-by: Wincent Colaiuta Signed-off-by: Junio C Hamano --- builtin-apply.c | 4 ++-- diff.c | 3 +-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/builtin-apply.c b/builtin-apply.c index ab393f32e9..2edd83bf40 100644 --- a/builtin-apply.c +++ b/builtin-apply.c @@ -912,8 +912,8 @@ static void check_whitespace(const char *line, int len, unsigned ws_rule) ; else { err = whitespace_error_string(result); - fprintf(stderr, "%s.\n%s:%d:%.*s\n", - err, patch_input_file, linenr, len - 2, line + 1); + fprintf(stderr, "%s:%d: %s.\n%.*s\n", + patch_input_file, linenr, err, len - 2, line + 1); free(err); } } diff --git a/diff.c b/diff.c index 577c773c3e..08ec66c794 100644 --- a/diff.c +++ b/diff.c @@ -973,8 +973,7 @@ static void checkdiff_consume(void *priv, char *line, unsigned long len) if (!data->status) return; err = whitespace_error_string(data->status); - printf("%s:%d: %s%s:%s ", data->filename, data->lineno, - ws, err, reset); + printf("%s:%d: %s.\n", data->filename, data->lineno, err); free(err); emit_line(set, reset, line, 1); (void)check_and_emit_line(line + 1, len - 1, data->ws_rule, From f817546652812ad5f20a8ab4a4a7290a9759ac4e Mon Sep 17 00:00:00 2001 From: Wincent Colaiuta Date: Thu, 13 Dec 2007 14:32:31 +0100 Subject: [PATCH 5/7] Add tests for "git diff --check" with core.whitespace options Make sure that "git diff --check" does the right thing when the core.whitespace options are set. While we are at it, correct many uses of test_expect_failure that ran sequence of commands. Signed-off-by: Wincent Colaiuta Signed-off-by: Junio C Hamano --- t/t4015-diff-whitespace.sh | 90 +++++++++++++++++++++++++++++--------- 1 file changed, 70 insertions(+), 20 deletions(-) diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh index 595cd60623..05ef78b2c0 100755 --- a/t/t4015-diff-whitespace.sh +++ b/t/t4015-diff-whitespace.sh @@ -133,18 +133,18 @@ test_expect_success 'check with no whitespace errors' ' ' -test_expect_failure 'check with trailing whitespace' ' +test_expect_success 'check with trailing whitespace' ' echo "foo(); " > x && - git diff --check + ! git diff --check ' -test_expect_failure 'check with space before tab in indent' ' +test_expect_success 'check with space before tab in indent' ' # indent has space followed by hard tab echo " foo();" > x && - git diff --check + ! git diff --check ' @@ -169,20 +169,20 @@ test_expect_success 'check staged with no whitespace errors' ' ' -test_expect_failure 'check staged with trailing whitespace' ' +test_expect_success 'check staged with trailing whitespace' ' echo "foo(); " > x && git add x && - git diff --cached --check + ! git diff --cached --check ' -test_expect_failure 'check staged with space before tab in indent' ' +test_expect_success 'check staged with space before tab in indent' ' # indent has space followed by hard tab echo " foo();" > x && git add x && - git diff --cached --check + ! git diff --cached --check ' @@ -194,20 +194,20 @@ test_expect_success 'check with no whitespace errors (diff-index)' ' ' -test_expect_failure 'check with trailing whitespace (diff-index)' ' +test_expect_success 'check with trailing whitespace (diff-index)' ' echo "foo(); " > x && git add x && - git diff-index --check HEAD + ! git diff-index --check HEAD ' -test_expect_failure 'check with space before tab in indent (diff-index)' ' +test_expect_success 'check with space before tab in indent (diff-index)' ' # indent has space followed by hard tab echo " foo();" > x && git add x && - git diff-index --check HEAD + ! git diff-index --check HEAD ' @@ -219,20 +219,20 @@ test_expect_success 'check staged with no whitespace errors (diff-index)' ' ' -test_expect_failure 'check staged with trailing whitespace (diff-index)' ' +test_expect_success 'check staged with trailing whitespace (diff-index)' ' echo "foo(); " > x && git add x && - git diff-index --cached --check HEAD + ! git diff-index --cached --check HEAD ' -test_expect_failure 'check staged with space before tab in indent (diff-index)' ' +test_expect_success 'check staged with space before tab in indent (diff-index)' ' # indent has space followed by hard tab echo " foo();" > x && git add x && - git diff-index --cached --check HEAD + ! git diff-index --cached --check HEAD ' @@ -244,20 +244,70 @@ test_expect_success 'check with no whitespace errors (diff-tree)' ' ' -test_expect_failure 'check with trailing whitespace (diff-tree)' ' +test_expect_success 'check with trailing whitespace (diff-tree)' ' echo "foo(); " > x && git commit -m "another commit" x && - git diff-tree --check HEAD^ HEAD + ! git diff-tree --check HEAD^ HEAD ' -test_expect_failure 'check with space before tab in indent (diff-tree)' ' +test_expect_success 'check with space before tab in indent (diff-tree)' ' # indent has space followed by hard tab echo " foo();" > x && git commit -m "yet another" x && - git diff-tree --check HEAD^ HEAD + ! git diff-tree --check HEAD^ HEAD + +' + +test_expect_success 'check trailing whitespace (trailing-space: off)' ' + + git config core.whitespace "-trailing-space" && + echo "foo (); " > x && + git diff --check + +' + +test_expect_success 'check trailing whitespace (trailing-space: on)' ' + + git config core.whitespace "trailing-space" && + echo "foo (); " > x && + ! git diff --check + +' + +test_expect_success 'check space before tab in indent (space-before-tab: off)' ' + + # indent contains space followed by HT + git config core.whitespace "-space-before-tab" && + echo " foo ();" > x && + git diff --check + +' + +test_expect_success 'check space before tab in indent (space-before-tab: on)' ' + + # indent contains space followed by HT + git config core.whitespace "space-before-tab" && + echo " foo (); " > x && + ! git diff --check + +' + +test_expect_success 'check spaces as indentation (indent-with-non-tab: off)' ' + + git config core.whitespace "-indent-with-non-tab" + echo " foo ();" > x && + git diff --check + +' + +test_expect_success 'check spaces as indentation (indent-with-non-tab: on)' ' + + git config core.whitespace "indent-with-non-tab" && + echo " foo ();" > x && + ! git diff --check ' From 420f4f04de9b3790da695918ac168d4115665d92 Mon Sep 17 00:00:00 2001 From: Wincent Colaiuta Date: Fri, 14 Dec 2007 12:23:43 +0100 Subject: [PATCH 6/7] Use shorter error messages for whitespace problems The initial version of the whitespace_error_string() function took the messages from builtin-apply.c rather than the shorter messages from diff.c. This commit addresses Junio's concern that these messages might be too long (now that we can emit multiple warnings per line). Signed-off-by: Wincent Colaiuta Signed-off-by: Junio C Hamano --- t/t4015-diff-whitespace.sh | 2 +- ws.c | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh index 05ef78b2c0..9bff8f5e4b 100755 --- a/t/t4015-diff-whitespace.sh +++ b/t/t4015-diff-whitespace.sh @@ -121,7 +121,7 @@ test_expect_success 'check mixed spaces and tabs in indent' ' # This is indented with SP HT SP. echo " foo();" > x && - git diff --check | grep "Space in indent is followed by a tab" + git diff --check | grep "space before tab in indent" ' diff --git a/ws.c b/ws.c index d7d1522f8a..46cbdd6379 100644 --- a/ws.c +++ b/ws.c @@ -101,16 +101,16 @@ char *whitespace_error_string(unsigned ws) struct strbuf err; strbuf_init(&err, 0); if (ws & WS_TRAILING_SPACE) - strbuf_addstr(&err, "Adds trailing whitespace"); + strbuf_addstr(&err, "trailing whitespace"); if (ws & WS_SPACE_BEFORE_TAB) { if (err.len) strbuf_addstr(&err, ", "); - strbuf_addstr(&err, "Space in indent is followed by a tab"); + strbuf_addstr(&err, "space before tab in indent"); } if (ws & WS_INDENT_WITH_NON_TAB) { if (err.len) strbuf_addstr(&err, ", "); - strbuf_addstr(&err, "Indent more than 8 places with spaces"); + strbuf_addstr(&err, "indent with spaces"); } return strbuf_detach(&err, NULL); } From 5973a07937f2c64b595c6784b06eb4441c07b5ef Mon Sep 17 00:00:00 2001 From: Wincent Colaiuta Date: Fri, 14 Dec 2007 12:23:44 +0100 Subject: [PATCH 7/7] Test interaction between diff --check and --exit-code Make sure that it works as advertised in the man page. Signed-off-by: Wincent Colaiuta Signed-off-by: Junio C Hamano --- t/t4017-diff-retval.sh | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/t/t4017-diff-retval.sh b/t/t4017-diff-retval.sh index 68731908be..dc0b7126cc 100755 --- a/t/t4017-diff-retval.sh +++ b/t/t4017-diff-retval.sh @@ -76,4 +76,33 @@ test_expect_success 'git diff-index --cached HEAD' ' } ' +test_expect_success '--check --exit-code returns 0 for no difference' ' + + git diff --check --exit-code + +' + +test_expect_success '--check --exit-code returns 1 for a clean difference' ' + + echo "good" > a && + git diff --check --exit-code + test $? = 1 + +' + +test_expect_success '--check --exit-code returns 3 for a dirty difference' ' + + echo "bad " >> a && + git diff --check --exit-code + test $? = 3 + +' + +test_expect_success '--check with --no-pager returns 2 for dirty difference' ' + + git --no-pager diff --check + test $? = 2 + +' + test_done