diff: fix whitespace-skipping with --color-moved

The code for handling whitespace with --color-moved represents partial strings as a pair of pointers. There are two possible conventions for the end pointer: 1. It points to the byte right after the end of the string. 2. It points to the final byte of the string. But we seem to use both conventions in the code: a. we assign the initial pointers from the NUL-terminated string using (1) b. we eat trailing whitespace by checking the second pointer for isspace(), which needs (2) c. the next_byte() function checks for end-of-string with "if (cp > endp)", which is (2) d. in next_byte() we skip past internal whitespace with "while (cp < end)", which is (1) This creates fewer bugs than you might think, because there are some subtle interactions. Because of (a) and (c), we always return the NUL-terminator from next_byte(). But all of the callers of next_byte() happen to handle that gracefully. Because of the mismatch between (d) and (c), next_byte() could accidentally return a whitespace character right at endp. But because of the interaction of (a) and (b), we fail to actually chomp trailing whitespace, meaning our endp _always_ points to a NUL, canceling out the problem. But that does leave (b) as a real bug: when ignoring whitespace only at the end-of-line, we don't correctly trim it, and fail to match up lines. We can fix the whole thing by moving consistently to one convention. Since convention (1) is idiomatic in our code base, we'll pick that one. The existing "-w" and "-b" tests continue to pass, and a new "--ignore-space-at-eol" shows off the breakage we're fixing. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Oct 19, 2017 at 16:29 UTC da58318e76b37c345e4d0da4c42987ad45b4f155
2 files changed +77 -5
diff.c
+10 -5
@@ -712,7 +712,7 @@ static int next_byte(const char **cp, const char **endp,
712 {
713 int retval;
714
715 - if (*cp > *endp)
715 + if (*cp >= *endp)
716 return -1;
717
718 if (isspace(**cp)) {
@@ -729,7 +729,12 @@ static int next_byte(const char **cp, const char **endp,
729 if (DIFF_XDL_TST(diffopt, IGNORE_WHITESPACE)) {
730 while (*cp < *endp && isspace(**cp))
731 (*cp)++;
732 - /* return the first non-ws character via the usual below */
732 + /*
733 + * return the first non-ws character via the usual
734 + * below, unless we ate all of the bytes
735 + */
736 + if (*cp >= *endp)
737 + return -1;
738 }
739 }
740
@@ -750,9 +755,9 @@ static int moved_entry_cmp(const struct diff_options *diffopt,
755 return a->es->len != b->es->len || memcmp(ap, bp, a->es->len);
756
757 if (DIFF_XDL_TST(diffopt, IGNORE_WHITESPACE_AT_EOL)) {
753 - while (ae > ap && isspace(*ae))
758 + while (ae > ap && isspace(ae[-1]))
759 ae--;
755 - while (be > bp && isspace(*be))
760 + while (be > bp && isspace(be[-1]))
761 be--;
762 }
763
@@ -775,7 +780,7 @@ static unsigned get_string_hash(struct emitted_diff_symbol *es, struct diff_opti
780 int c;
781
782 strbuf_reset(&sb);
778 - while (ae > ap && isspace(*ae))
783 + while (ae > ap && isspace(ae[-1]))
784 ae--;
785 while ((c = next_byte(&ap, &ae, o)) > 0)
786 strbuf_addch(&sb, c);
t/t4015-diff-whitespace.sh
+67
@@ -1463,6 +1463,73 @@ test_expect_success 'move detection ignoring whitespace changes' '
1463 test_cmp expected actual
1464 '
1465
1466 +test_expect_success 'move detection ignoring whitespace at eol' '
1467 + git reset --hard &&
1468 + # Lines 6-9 have new eol whitespace, but 9 also has it in the middle
1469 + q_to_tab <<-\EOF >lines.txt &&
1470 + long line 6Q
1471 + long line 7Q
1472 + long line 8Q
1473 + longQline 9Q
1474 + line 1
1475 + line 2
1476 + line 3
1477 + line 4
1478 + line 5
1479 + EOF
1480 +
1481 + # avoid cluttering the output with complaints about our eol whitespace
1482 + test_config core.whitespace -blank-at-eol &&
1483 +
1484 + git diff HEAD --no-renames --color-moved --color |
1485 + grep -v "index" |
1486 + test_decode_color >actual &&
1487 + cat <<-\EOF >expected &&
1488 + <BOLD>diff --git a/lines.txt b/lines.txt<RESET>
1489 + <BOLD>--- a/lines.txt<RESET>
1490 + <BOLD>+++ b/lines.txt<RESET>
1491 + <CYAN>@@ -1,9 +1,9 @@<RESET>
1492 + <GREEN>+<RESET><GREEN>long line 6 <RESET>
1493 + <GREEN>+<RESET><GREEN>long line 7 <RESET>
1494 + <GREEN>+<RESET><GREEN>long line 8 <RESET>
1495 + <GREEN>+<RESET><GREEN>long line 9 <RESET>
1496 + line 1<RESET>
1497 + line 2<RESET>
1498 + line 3<RESET>
1499 + line 4<RESET>
1500 + line 5<RESET>
1501 + <RED>-long line 6<RESET>
1502 + <RED>-long line 7<RESET>
1503 + <RED>-long line 8<RESET>
1504 + <RED>-long line 9<RESET>
1505 + EOF
1506 + test_cmp expected actual &&
1507 +
1508 + git diff HEAD --no-renames --ignore-space-at-eol --color-moved --color |
1509 + grep -v "index" |
1510 + test_decode_color >actual &&
1511 + cat <<-\EOF >expected &&
1512 + <BOLD>diff --git a/lines.txt b/lines.txt<RESET>
1513 + <BOLD>--- a/lines.txt<RESET>
1514 + <BOLD>+++ b/lines.txt<RESET>
1515 + <CYAN>@@ -1,9 +1,9 @@<RESET>
1516 + <CYAN>+<RESET><CYAN>long line 6 <RESET>
1517 + <CYAN>+<RESET><CYAN>long line 7 <RESET>
1518 + <CYAN>+<RESET><CYAN>long line 8 <RESET>
1519 + <GREEN>+<RESET><GREEN>long line 9 <RESET>
1520 + line 1<RESET>
1521 + line 2<RESET>
1522 + line 3<RESET>
1523 + line 4<RESET>
1524 + line 5<RESET>
1525 + <MAGENTA>-long line 6<RESET>
1526 + <MAGENTA>-long line 7<RESET>
1527 + <MAGENTA>-long line 8<RESET>
1528 + <RED>-long line 9<RESET>
1529 + EOF
1530 + test_cmp expected actual
1531 +'
1532 +
1533 test_expect_success 'clean up whitespace-test colors' '
1534 git config --unset color.diff.oldMoved &&
1535 git config --unset color.diff.newMoved