Skip to content

Commit d4b76e1

Browse files
gitstergitster
committed
Merge branch 'jc/checkdiff'
* 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
2 parents f7c3cf8 + ab20fda commit d4b76e1

File tree

7 files changed

+136
-87
lines changed

7 files changed

+136
-87
lines changed

‎builtin-apply.c‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -987,8 +987,7 @@ static int find_header(char *line, unsigned long size, int *hdrsize, struct patc
987987
static void check_whitespace(const char *line, int len, unsigned ws_rule)
988988
{
989989
char *err;
990-
unsigned result = check_and_emit_line(line + 1, len - 1, ws_rule,
991-
NULL, NULL, NULL, NULL);
990+
unsigned result = ws_check(line + 1, len - 1, ws_rule);
992991
if (!result)
993992
return;
994993

@@ -999,7 +998,7 @@ static void check_whitespace(const char *line, int len, unsigned ws_rule)
999998
else {
1000999
err = whitespace_error_string(result);
10011000
fprintf(stderr, "%s:%d: %s.\n%.*s\n",
1002-
patch_input_file, linenr, err, len - 2, line + 1);
1001+
patch_input_file, linenr, err, len - 2, line + 1);
10031002
free(err);
10041003
}
10051004
}

‎cache.h‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -819,11 +819,11 @@ void shift_tree(const unsigned char *, const unsigned char *, unsigned char *, i
819819
extern unsigned whitespace_rule_cfg;
820820
extern unsigned whitespace_rule(const char *);
821821
extern unsigned parse_whitespace_rule(const char *);
822-
extern unsigned check_and_emit_line(const char *line, int len, unsigned ws_rule,
823-
FILE *stream, const char *set,
824-
const char *reset, const char *ws);
822+
extern unsigned ws_check(const char *line, int len, unsigned ws_rule);
823+
extern void ws_check_emit(const char *line, int len, unsigned ws_rule, FILE *stream, const char *set, const char *reset, const char *ws);
825824
extern char *whitespace_error_string(unsigned ws);
826825
extern int ws_fix_copy(char *, const char *, int, unsigned, int *);
826+
extern int ws_blank_line(const char *line, int len, unsigned ws_rule);
827827

828828
/* ls-files */
829829
int pathspec_match(const char **spec, char *matched, const char *filename, int skiplen);

‎diff.c‎

Lines changed: 75 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -535,9 +535,9 @@ static void emit_add_line(const char *reset, struct emit_callback *ecbdata, cons
535535
else {
536536
/* Emit just the prefix, then the rest. */
537537
emit_line(ecbdata->file, set, reset, line, ecbdata->nparents);
538-
(void)check_and_emit_line(line + ecbdata->nparents,
539-
len - ecbdata->nparents, ecbdata->ws_rule,
540-
ecbdata->file, set, reset, ws);
538+
ws_check_emit(line + ecbdata->nparents,
539+
len - ecbdata->nparents, ecbdata->ws_rule,
540+
ecbdata->file, set, reset, ws);
541541
}
542542
}
543543

@@ -1136,42 +1136,85 @@ static void free_diffstat_info(struct diffstat_t *diffstat)
11361136
struct checkdiff_t {
11371137
struct xdiff_emit_state xm;
11381138
const char *filename;
1139-
int lineno, color_diff;
1139+
int lineno;
1140+
struct diff_options *o;
11401141
unsigned ws_rule;
11411142
unsigned status;
1142-
FILE *file;
1143+
int trailing_blanks_start;
11431144
};
11441145

1146+
static int is_conflict_marker(const char *line, unsigned long len)
1147+
{
1148+
char firstchar;
1149+
int cnt;
1150+
1151+
if (len < 8)
1152+
return 0;
1153+
firstchar = line[0];
1154+
switch (firstchar) {
1155+
case '=': case '>': case '<':
1156+
break;
1157+
default:
1158+
return 0;
1159+
}
1160+
for (cnt = 1; cnt < 7; cnt++)
1161+
if (line[cnt] != firstchar)
1162+
return 0;
1163+
/* line[0] thru line[6] are same as firstchar */
1164+
if (firstchar == '=') {
1165+
/* divider between ours and theirs? */
1166+
if (len != 8 || line[7] != '\n')
1167+
return 0;
1168+
} else if (len < 8 || !isspace(line[7])) {
1169+
/* not divider before ours nor after theirs */
1170+
return 0;
1171+
}
1172+
return 1;
1173+
}
1174+
11451175
static void checkdiff_consume(void *priv, char *line, unsigned long len)
11461176
{
11471177
struct checkdiff_t *data = priv;
1148-
const char *ws = diff_get_color(data->color_diff, DIFF_WHITESPACE);
1149-
const char *reset = diff_get_color(data->color_diff, DIFF_RESET);
1150-
const char *set = diff_get_color(data->color_diff, DIFF_FILE_NEW);
1178+
int color_diff = DIFF_OPT_TST(data->o, COLOR_DIFF);
1179+
const char *ws = diff_get_color(color_diff, DIFF_WHITESPACE);
1180+
const char *reset = diff_get_color(color_diff, DIFF_RESET);
1181+
const char *set = diff_get_color(color_diff, DIFF_FILE_NEW);
11511182
char *err;
11521183

11531184
if (line[0] == '+') {
11541185
unsigned bad;
11551186
data->lineno++;
1156-
bad = check_and_emit_line(line + 1, len - 1,
1157-
data->ws_rule, NULL, NULL, NULL, NULL);
1187+
if (!ws_blank_line(line + 1, len - 1, data->ws_rule))
1188+
data->trailing_blanks_start = 0;
1189+
else if (!data->trailing_blanks_start)
1190+
data->trailing_blanks_start = data->lineno;
1191+
if (is_conflict_marker(line + 1, len - 1)) {
1192+
data->status |= 1;
1193+
fprintf(data->o->file,
1194+
"%s:%d: leftover conflict marker\n",
1195+
data->filename, data->lineno);
1196+
}
1197+
bad = ws_check(line + 1, len - 1, data->ws_rule);
11581198
if (!bad)
11591199
return;
11601200
data->status |= bad;
11611201
err = whitespace_error_string(bad);
1162-
fprintf(data->file, "%s:%d: %s.\n", data->filename, data->lineno, err);
1202+
fprintf(data->o->file, "%s:%d: %s.\n",
1203+
data->filename, data->lineno, err);
11631204
free(err);
1164-
emit_line(data->file, set, reset, line, 1);
1165-
(void)check_and_emit_line(line + 1, len - 1, data->ws_rule,
1166-
data->file, set, reset, ws);
1167-
} else if (line[0] == ' ')
1205+
emit_line(data->o->file, set, reset, line, 1);
1206+
ws_check_emit(line + 1, len - 1, data->ws_rule,
1207+
data->o->file, set, reset, ws);
1208+
} else if (line[0] == ' ') {
11681209
data->lineno++;
1169-
else if (line[0] == '@') {
1210+
data->trailing_blanks_start = 0;
1211+
} else if (line[0] == '@') {
11701212
char *plus = strchr(line, '+');
11711213
if (plus)
11721214
data->lineno = strtol(plus, NULL, 10) - 1;
11731215
else
11741216
die("invalid diff");
1217+
data->trailing_blanks_start = 0;
11751218
}
11761219
}
11771220

@@ -1544,8 +1587,9 @@ static void builtin_diffstat(const char *name_a, const char *name_b,
15441587

15451588
static void builtin_checkdiff(const char *name_a, const char *name_b,
15461589
const char *attr_path,
1547-
struct diff_filespec *one,
1548-
struct diff_filespec *two, struct diff_options *o)
1590+
struct diff_filespec *one,
1591+
struct diff_filespec *two,
1592+
struct diff_options *o)
15491593
{
15501594
mmfile_t mf1, mf2;
15511595
struct checkdiff_t data;
@@ -1557,13 +1601,18 @@ static void builtin_checkdiff(const char *name_a, const char *name_b,
15571601
data.xm.consume = checkdiff_consume;
15581602
data.filename = name_b ? name_b : name_a;
15591603
data.lineno = 0;
1560-
data.color_diff = DIFF_OPT_TST(o, COLOR_DIFF);
1604+
data.o = o;
15611605
data.ws_rule = whitespace_rule(attr_path);
1562-
data.file = o->file;
15631606

15641607
if (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0)
15651608
die("unable to read files to diff");
15661609

1610+
/*
1611+
* All the other codepaths check both sides, but not checking
1612+
* the "old" side here is deliberate. We are checking the newly
1613+
* introduced changes, and as long as the "new" side is text, we
1614+
* can and should check what it introduces.
1615+
*/
15671616
if (diff_filespec_is_binary(two))
15681617
goto free_and_return;
15691618
else {
@@ -1577,6 +1626,12 @@ static void builtin_checkdiff(const char *name_a, const char *name_b,
15771626
ecb.outf = xdiff_outf;
15781627
ecb.priv = &data;
15791628
xdi_diff(&mf1, &mf2, &xpp, &xecfg, &ecb);
1629+
1630+
if (data.trailing_blanks_start) {
1631+
fprintf(o->file, "%s:%d: ends with blank lines.\n",
1632+
data.filename, data.trailing_blanks_start);
1633+
data.status = 1; /* report errors */
1634+
}
15801635
}
15811636
free_and_return:
15821637
diff_free_filespec_data(one);

‎t/t4015-diff-whitespace.sh‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -335,4 +335,10 @@ test_expect_success 'line numbers in --check output are correct' '
335335
336336
'
337337

338+
test_expect_success 'checkdiff detects trailing blank lines' '
339+
echo "foo();" >x &&
340+
echo "" >>x &&
341+
git diff --check | grep "ends with blank"
342+
'
343+
338344
test_done

‎t/t4017-diff-retval.sh‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,4 +113,18 @@ test_expect_success 'check should test not just the last line' '
113113
114114
'
115115

116+
test_expect_success 'check detects leftover conflict markers' '
117+
git reset --hard &&
118+
git checkout HEAD^ &&
119+
echo binary >>b &&
120+
git commit -m "side" b &&
121+
test_must_fail git merge master &&
122+
git add b && (
123+
git --no-pager diff --cached --check >test.out
124+
test $? = 2
125+
) &&
126+
test 3 = $(grep "conflict marker" test.out | wc -l) &&
127+
git reset --hard
128+
'
129+
116130
test_done

‎templates/hooks--pre-commit.sample‎

Lines changed: 6 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -7,64 +7,12 @@
77
#
88
# To enable this hook, rename this file to "pre-commit".
99

10-
# This is slightly modified from Andrew Morton's Perfect Patch.
11-
# Lines you introduce should not have trailing whitespace.
12-
# Also check for an indentation that has SP before a TAB.
13-
1410
if git-rev-parse --verify HEAD 2>/dev/null
1511
then
16-
git-diff-index -p -M --cached HEAD --
12+
against=HEAD
1713
else
18-
# NEEDSWORK: we should produce a diff with an empty tree here
19-
# if we want to do the same verification for the initial import.
20-
:
21-
fi |
22-
perl -e '
23-
my $found_bad = 0;
24-
my $filename;
25-
my $reported_filename = "";
26-
my $lineno;
27-
sub bad_line {
28-
my ($why, $line) = @_;
29-
if (!$found_bad) {
30-
print STDERR "*\n";
31-
print STDERR "* You have some suspicious patch lines:\n";
32-
print STDERR "*\n";
33-
$found_bad = 1;
34-
}
35-
if ($reported_filename ne $filename) {
36-
print STDERR "* In $filename\n";
37-
$reported_filename = $filename;
38-
}
39-
print STDERR "* $why (line $lineno)\n";
40-
print STDERR "$filename:$lineno:$line\n";
41-
}
42-
while (<>) {
43-
if (m|^diff --git a/(.*) b/\1$|) {
44-
$filename = $1;
45-
next;
46-
}
47-
if (/^@@ -\S+ \+(\d+)/) {
48-
$lineno = $1 - 1;
49-
next;
50-
}
51-
if (/^ /) {
52-
$lineno++;
53-
next;
54-
}
55-
if (s/^\+//) {
56-
$lineno++;
57-
chomp;
58-
if (/\s$/) {
59-
bad_line("trailing whitespace", $_);
60-
}
61-
if (/^\s* \t/) {
62-
bad_line("indent SP followed by a TAB", $_);
63-
}
64-
if (/^([<>])\1{6} |^={7}$/) {
65-
bad_line("unresolved merge conflict", $_);
66-
}
67-
}
68-
}
69-
exit($found_bad);
70-
'
14+
# Initial commit: diff against an empty tree object
15+
against=4b825dc642cb6eb9a060e54bf8d69288fbee4904
16+
fi
17+
18+
exec git diff-index --check --cached $against --

‎ws.c‎

Lines changed: 30 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -117,9 +117,9 @@ char *whitespace_error_string(unsigned ws)
117117
}
118118

119119
/* If stream is non-NULL, emits the line after checking. */
120-
unsigned check_and_emit_line(const char *line, int len, unsigned ws_rule,
121-
FILE *stream, const char *set,
122-
const char *reset, const char *ws)
120+
static unsigned ws_check_emit_1(const char *line, int len, unsigned ws_rule,
121+
FILE *stream, const char *set,
122+
const char *reset, const char *ws)
123123
{
124124
unsigned result = 0;
125125
int written = 0;
@@ -213,6 +213,33 @@ unsigned check_and_emit_line(const char *line, int len, unsigned ws_rule,
213213
return result;
214214
}
215215

216+
void ws_check_emit(const char *line, int len, unsigned ws_rule,
217+
FILE *stream, const char *set,
218+
const char *reset, const char *ws)
219+
{
220+
(void)ws_check_emit_1(line, len, ws_rule, stream, set, reset, ws);
221+
}
222+
223+
unsigned ws_check(const char *line, int len, unsigned ws_rule)
224+
{
225+
return ws_check_emit_1(line, len, ws_rule, NULL, NULL, NULL, NULL);
226+
}
227+
228+
int ws_blank_line(const char *line, int len, unsigned ws_rule)
229+
{
230+
/*
231+
* We _might_ want to treat CR differently from other
232+
* whitespace characters when ws_rule has WS_CR_AT_EOL, but
233+
* for now we just use this stupid definition.
234+
*/
235+
while (len-- > 0) {
236+
if (!isspace(*line))
237+
return 0;
238+
line++;
239+
}
240+
return 1;
241+
}
242+
216243
/* Copy the line to the buffer while fixing whitespaces */
217244
int ws_fix_copy(char *dst, const char *src, int len, unsigned ws_rule, int *error_count)
218245
{

0 commit comments

Comments
 (0)