From 62c64895cfcf3bbf34969a69fa96a631f7d5b14e Mon Sep 17 00:00:00 2001 From: Wincent Colaiuta Date: Thu, 13 Dec 2007 21:24:52 +0100 Subject: "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 --- diff.c | 9 +++++++++ 1 file changed, 9 insertions(+) (limited to 'diff.c') 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) -- cgit v1.3 From da31b358fb39b32622c14343ffe157a765f3948b Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 13 Dec 2007 23:40:27 -0800 Subject: 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(-) (limited to 'diff.c') 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 -- cgit v1.3 From c1795bb08aae9fb7e4dc1a01e292b85e59b1c640 Mon Sep 17 00:00:00 2001 From: Wincent Colaiuta Date: Thu, 13 Dec 2007 14:32:29 +0100 Subject: 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(-) (limited to 'diff.c') 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; - - /* - * 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; + char *err; + unsigned result = check_and_emit_line(line + 1, len - 1, ws_rule, + NULL, NULL, NULL, NULL); + if (!result) + 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; +} -- cgit v1.3 From 45e2a4b2b0a07cda05dda08b47906100c4711e4e Mon Sep 17 00:00:00 2001 From: Wincent Colaiuta Date: Thu, 13 Dec 2007 14:32:30 +0100 Subject: 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(-) (limited to 'diff.c') 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, -- cgit v1.3