Merge branch 'jc/checkdiff'
authorJunio C Hamano <gitster@pobox.com>
Tue, 1 Jul 2008 23:22:35 +0000 (16:22 -0700)
committerJunio C Hamano <gitster@pobox.com>
Tue, 1 Jul 2008 23:22:35 +0000 (16:22 -0700)
* jc/checkdiff:
Fix t4017-diff-retval for white-space from wc
Update sample pre-commit hook to use "diff --check"
diff --check: detect leftover conflict markers
Teach "diff --check" about new blank lines at end
checkdiff: pass diff_options to the callback
check_and_emit_line(): rename and refactor
diff --check: explain why we do not care whether old side is binary

builtin-apply.c
cache.h
diff.c
t/t4015-diff-whitespace.sh
t/t4017-diff-retval.sh
templates/hooks--pre-commit.sample
ws.c
index 318504a03700d3b50ce2848a0ab374ac5f519950..985ca3be5ab1ff0e5b4071d2ef19384dc1e9641e 100644 (file)
@@ -987,8 +987,7 @@ 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)
 {
        char *err;
-       unsigned result = check_and_emit_line(line + 1, len - 1, ws_rule,
-           NULL, NULL, NULL, NULL);
+       unsigned result = ws_check(line + 1, len - 1, ws_rule);
        if (!result)
                return;
 
@@ -999,7 +998,7 @@ static void check_whitespace(const char *line, int len, unsigned ws_rule)
        else {
                err = whitespace_error_string(result);
                fprintf(stderr, "%s:%d: %s.\n%.*s\n",
-                    patch_input_file, linenr, err, len - 2, line + 1);
+                       patch_input_file, linenr, err, len - 2, line + 1);
                free(err);
        }
 }
diff --git a/cache.h b/cache.h
index 64ef86e129337fde851d701a30e70cb84b54ecb3..188428dd26b7d9bf8dc0b9cd1a02ebca6d459400 100644 (file)
--- a/cache.h
+++ b/cache.h
@@ -819,11 +819,11 @@ 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 unsigned ws_check(const char *line, int len, unsigned ws_rule);
+extern void ws_check_emit(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);
 extern int ws_fix_copy(char *, const char *, int, unsigned, int *);
+extern int ws_blank_line(const char *line, int len, unsigned ws_rule);
 
 /* ls-files */
 int pathspec_match(const char **spec, char *matched, const char *filename, int skiplen);
diff --git a/diff.c b/diff.c
index 66851b5647c2a1a1b4853bb869d3eb707c117943..803fbba451dfe8226097dcb791c339ee2ccd6735 100644 (file)
--- a/diff.c
+++ b/diff.c
@@ -535,9 +535,9 @@ static void emit_add_line(const char *reset, struct emit_callback *ecbdata, cons
        else {
                /* Emit just the prefix, then the rest. */
                emit_line(ecbdata->file, set, reset, line, ecbdata->nparents);
-               (void)check_and_emit_line(line + ecbdata->nparents,
-                   len - ecbdata->nparents, ecbdata->ws_rule,
-                   ecbdata->file, set, reset, ws);
+               ws_check_emit(line + ecbdata->nparents,
+                             len - ecbdata->nparents, ecbdata->ws_rule,
+                             ecbdata->file, set, reset, ws);
        }
 }
 
@@ -1136,42 +1136,85 @@ static void free_diffstat_info(struct diffstat_t *diffstat)
 struct checkdiff_t {
        struct xdiff_emit_state xm;
        const char *filename;
-       int lineno, color_diff;
+       int lineno;
+       struct diff_options *o;
        unsigned ws_rule;
        unsigned status;
-       FILE *file;
+       int trailing_blanks_start;
 };
 
+static int is_conflict_marker(const char *line, unsigned long len)
+{
+       char firstchar;
+       int cnt;
+
+       if (len < 8)
+               return 0;
+       firstchar = line[0];
+       switch (firstchar) {
+       case '=': case '>': case '<':
+               break;
+       default:
+               return 0;
+       }
+       for (cnt = 1; cnt < 7; cnt++)
+               if (line[cnt] != firstchar)
+                       return 0;
+       /* line[0] thru line[6] are same as firstchar */
+       if (firstchar == '=') {
+               /* divider between ours and theirs? */
+               if (len != 8 || line[7] != '\n')
+                       return 0;
+       } else if (len < 8 || !isspace(line[7])) {
+               /* not divider before ours nor after theirs */
+               return 0;
+       }
+       return 1;
+}
+
 static void checkdiff_consume(void *priv, char *line, unsigned long len)
 {
        struct checkdiff_t *data = priv;
-       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);
+       int color_diff = DIFF_OPT_TST(data->o, COLOR_DIFF);
+       const char *ws = diff_get_color(color_diff, DIFF_WHITESPACE);
+       const char *reset = diff_get_color(color_diff, DIFF_RESET);
+       const char *set = diff_get_color(color_diff, DIFF_FILE_NEW);
        char *err;
 
        if (line[0] == '+') {
                unsigned bad;
                data->lineno++;
-               bad = check_and_emit_line(line + 1, len - 1,
-                   data->ws_rule, NULL, NULL, NULL, NULL);
+               if (!ws_blank_line(line + 1, len - 1, data->ws_rule))
+                       data->trailing_blanks_start = 0;
+               else if (!data->trailing_blanks_start)
+                       data->trailing_blanks_start = data->lineno;
+               if (is_conflict_marker(line + 1, len - 1)) {
+                       data->status |= 1;
+                       fprintf(data->o->file,
+                               "%s:%d: leftover conflict marker\n",
+                               data->filename, data->lineno);
+               }
+               bad = ws_check(line + 1, len - 1, data->ws_rule);
                if (!bad)
                        return;
                data->status |= bad;
                err = whitespace_error_string(bad);
-               fprintf(data->file, "%s:%d: %s.\n", data->filename, data->lineno, err);
+               fprintf(data->o->file, "%s:%d: %s.\n",
+                       data->filename, data->lineno, err);
                free(err);
-               emit_line(data->file, set, reset, line, 1);
-               (void)check_and_emit_line(line + 1, len - 1, data->ws_rule,
-                   data->file, set, reset, ws);
-       } else if (line[0] == ' ')
+               emit_line(data->o->file, set, reset, line, 1);
+               ws_check_emit(line + 1, len - 1, data->ws_rule,
+                             data->o->file, set, reset, ws);
+       } else if (line[0] == ' ') {
                data->lineno++;
-       else if (line[0] == '@') {
+               data->trailing_blanks_start = 0;
+       } else if (line[0] == '@') {
                char *plus = strchr(line, '+');
                if (plus)
                        data->lineno = strtol(plus, NULL, 10) - 1;
                else
                        die("invalid diff");
+               data->trailing_blanks_start = 0;
        }
 }
 
@@ -1544,8 +1587,9 @@ static void builtin_diffstat(const char *name_a, const char *name_b,
 
 static void builtin_checkdiff(const char *name_a, const char *name_b,
                              const char *attr_path,
-                            struct diff_filespec *one,
-                            struct diff_filespec *two, struct diff_options *o)
+                             struct diff_filespec *one,
+                             struct diff_filespec *two,
+                             struct diff_options *o)
 {
        mmfile_t mf1, mf2;
        struct checkdiff_t data;
@@ -1557,13 +1601,18 @@ static void builtin_checkdiff(const char *name_a, const char *name_b,
        data.xm.consume = checkdiff_consume;
        data.filename = name_b ? name_b : name_a;
        data.lineno = 0;
-       data.color_diff = DIFF_OPT_TST(o, COLOR_DIFF);
+       data.o = o;
        data.ws_rule = whitespace_rule(attr_path);
-       data.file = o->file;
 
        if (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0)
                die("unable to read files to diff");
 
+       /*
+        * All the other codepaths check both sides, but not checking
+        * the "old" side here is deliberate.  We are checking the newly
+        * introduced changes, and as long as the "new" side is text, we
+        * can and should check what it introduces.
+        */
        if (diff_filespec_is_binary(two))
                goto free_and_return;
        else {
@@ -1577,6 +1626,12 @@ static void builtin_checkdiff(const char *name_a, const char *name_b,
                ecb.outf = xdiff_outf;
                ecb.priv = &data;
                xdi_diff(&mf1, &mf2, &xpp, &xecfg, &ecb);
+
+               if (data.trailing_blanks_start) {
+                       fprintf(o->file, "%s:%d: ends with blank lines.\n",
+                               data.filename, data.trailing_blanks_start);
+                       data.status = 1; /* report errors */
+               }
        }
  free_and_return:
        diff_free_filespec_data(one);
index b7cc6b28e61e73af3206a9613620a635a467ea0e..0922c708f16fea490a85d16d5d4d366df0fb2088 100755 (executable)
@@ -335,4 +335,10 @@ test_expect_success 'line numbers in --check output are correct' '
 
 '
 
+test_expect_success 'checkdiff detects trailing blank lines' '
+       echo "foo();" >x &&
+       echo "" >>x &&
+       git diff --check | grep "ends with blank"
+'
+
 test_done
index 0d0fb87f5732e3ecbca8a195843070539353701e..60dd2014d5ae5d5e9e168b8b60278d90ef93cc53 100755 (executable)
@@ -113,4 +113,18 @@ test_expect_success 'check should test not just the last line' '
 
 '
 
+test_expect_success 'check detects leftover conflict markers' '
+       git reset --hard &&
+       git checkout HEAD^ &&
+       echo binary >>b &&
+       git commit -m "side" b &&
+       test_must_fail git merge master &&
+       git add b && (
+               git --no-pager diff --cached --check >test.out
+               test $? = 2
+       ) &&
+       test 3 = $(grep "conflict marker" test.out | wc -l) &&
+       git reset --hard
+'
+
 test_done
index 71c10f25f485f4a995f4617a5fa7b0e424fcaae8..0e49279c7f7b805c78f1bc4760a0d1c70a84a0d9 100755 (executable)
@@ -7,64 +7,12 @@
 #
 # To enable this hook, rename this file to "pre-commit".
 
-# This is slightly modified from Andrew Morton's Perfect Patch.
-# Lines you introduce should not have trailing whitespace.
-# Also check for an indentation that has SP before a TAB.
-
 if git-rev-parse --verify HEAD 2>/dev/null
 then
-       git-diff-index -p -M --cached HEAD --
+       against=HEAD
 else
-       # NEEDSWORK: we should produce a diff with an empty tree here
-       # if we want to do the same verification for the initial import.
-       :
-fi |
-perl -e '
-    my $found_bad = 0;
-    my $filename;
-    my $reported_filename = "";
-    my $lineno;
-    sub bad_line {
-       my ($why, $line) = @_;
-       if (!$found_bad) {
-           print STDERR "*\n";
-           print STDERR "* You have some suspicious patch lines:\n";
-           print STDERR "*\n";
-           $found_bad = 1;
-       }
-       if ($reported_filename ne $filename) {
-           print STDERR "* In $filename\n";
-           $reported_filename = $filename;
-       }
-       print STDERR "* $why (line $lineno)\n";
-       print STDERR "$filename:$lineno:$line\n";
-    }
-    while (<>) {
-       if (m|^diff --git a/(.*) b/\1$|) {
-           $filename = $1;
-           next;
-       }
-       if (/^@@ -\S+ \+(\d+)/) {
-           $lineno = $1 - 1;
-           next;
-       }
-       if (/^ /) {
-           $lineno++;
-           next;
-       }
-       if (s/^\+//) {
-           $lineno++;
-           chomp;
-           if (/\s$/) {
-               bad_line("trailing whitespace", $_);
-           }
-           if (/^\s* \t/) {
-               bad_line("indent SP followed by a TAB", $_);
-           }
-           if (/^([<>])\1{6} |^={7}$/) {
-               bad_line("unresolved merge conflict", $_);
-           }
-       }
-    }
-    exit($found_bad);
-'
+       # Initial commit: diff against an empty tree object
+       against=4b825dc642cb6eb9a060e54bf8d69288fbee4904
+fi
+
+exec git diff-index --check --cached $against --
diff --git a/ws.c b/ws.c
index ba7e834ca819b1d2ccce6cf125aa9f34efea8d5c..7a7ff130a34942506e6068105ac5946c9404bf18 100644 (file)
--- a/ws.c
+++ b/ws.c
@@ -117,9 +117,9 @@ char *whitespace_error_string(unsigned ws)
 }
 
 /* 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)
+static unsigned ws_check_emit_1(const char *line, int len, unsigned ws_rule,
+                               FILE *stream, const char *set,
+                               const char *reset, const char *ws)
 {
        unsigned result = 0;
        int written = 0;
@@ -213,6 +213,33 @@ unsigned check_and_emit_line(const char *line, int len, unsigned ws_rule,
        return result;
 }
 
+void ws_check_emit(const char *line, int len, unsigned ws_rule,
+                  FILE *stream, const char *set,
+                  const char *reset, const char *ws)
+{
+       (void)ws_check_emit_1(line, len, ws_rule, stream, set, reset, ws);
+}
+
+unsigned ws_check(const char *line, int len, unsigned ws_rule)
+{
+       return ws_check_emit_1(line, len, ws_rule, NULL, NULL, NULL, NULL);
+}
+
+int ws_blank_line(const char *line, int len, unsigned ws_rule)
+{
+       /*
+        * We _might_ want to treat CR differently from other
+        * whitespace characters when ws_rule has WS_CR_AT_EOL, but
+        * for now we just use this stupid definition.
+        */
+       while (len-- > 0) {
+               if (!isspace(*line))
+                       return 0;
+               line++;
+       }
+       return 1;
+}
+
 /* Copy the line to the buffer while fixing whitespaces */
 int ws_fix_copy(char *dst, const char *src, int len, unsigned ws_rule, int *error_count)
 {