@matheusbsbs / matheus-bagliano / commits / eb89352504

diff --color-moved: avoid false short line matches and bad zebra coloring

When marking moved lines it is possible for a block of potential matched lines to extend past a change in sign when there is a sequence of added lines whose text matches the text of a sequence of deleted and added lines. Most of the time either `match` will be NULL or `pmb_advance_or_null()` will fail when the loop encounters a change of sign but there are corner cases where `match` is non-NULL and `pmb_advance_or_null()` successfully advances the moved block despite the change in sign. One consequence of this is highlighting a short line as moved when it should not be. For example -moved line # Correctly highlighted as moved +short line # Wrongly highlighted as moved context +moved line # Correctly highlighted as moved +short line context -short line The other consequence is coloring a moved addition following a moved deletion in the wrong color. In the example below the first "+moved line 3" should be highlighted as newMoved not newMovedAlternate. -moved line 1 # Correctly highlighted as oldMoved -moved line 2 # Correctly highlighted as oldMovedAlternate +moved line 3 # Wrongly highlighted as newMovedAlternate context # Everything else is highlighted correctly +moved line 2 +moved line 3 context +moved line 1 -moved line 3 These false matches are more likely when using --color-moved-ws with the exception of --color-moved-ws=allow-indentation-change which ties the sign of the current whitespace delta to the sign of the line to avoid this problem. The fix is to check that the sign of the new line being matched is the same as the sign of the line that started the block of potential matches. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Phillip Wood committed Dec 9, 2021 at 10:30 UTC eb893525041a53b968a46000ed5cb66ffc725853

2 files changed +76 -6

diff.c

+11 -6

@@ -1176,7 +1176,7 @@ static void mark_color_as_moved(struct diff_options *o,

1176 struct moved_block *pmb = NULL; /* potentially moved blocks */

1177 int pmb_nr = 0, pmb_alloc = 0;

1178 int n, flipped_block = 0, block_length = 0;

1179 - enum diff_symbol last_symbol = 0;

1179 + enum diff_symbol moved_symbol = DIFF_SYMBOL_BINARY_DIFF_HEADER;

1180

1181

1182 for (n = 0; n < o->emitted_symbols->nr; n++) {

@@ -1202,7 +1202,7 @@ static void mark_color_as_moved(struct diff_options *o,

1202 flipped_block = 0;

1203 }

1204

1205 - if (!match) {

1205 + if (pmb_nr && (!match || l->s != moved_symbol)) {

1206 int i;

1207

1208 if (!adjust_last_block(o, n, block_length) &&

@@ -1219,12 +1219,13 @@ static void mark_color_as_moved(struct diff_options *o,

1219 pmb_nr = 0;

1220 block_length = 0;

1221 flipped_block = 0;

1222 - last_symbol = l->s;

1222 + }

1223 + if (!match) {

1224 + moved_symbol = DIFF_SYMBOL_BINARY_DIFF_HEADER;

1225 continue;

1226 }

1227

1228 if (o->color_moved == COLOR_MOVED_PLAIN) {

1227 - last_symbol = l->s;

1229 l->flags |= DIFF_SYMBOL_MOVED_LINE;

1230 continue;

1231 }

@@ -1251,11 +1252,16 @@ static void mark_color_as_moved(struct diff_options *o,

1252 &pmb, &pmb_alloc,

1253 &pmb_nr);

1254

1254 - if (contiguous && pmb_nr && last_symbol == l->s)

1255 + if (contiguous && pmb_nr && moved_symbol == l->s)

1256 flipped_block = (flipped_block + 1) % 2;

1257 else

1258 flipped_block = 0;

1259

1260 + if (pmb_nr)

1261 + moved_symbol = l->s;

1262 + else

1263 + moved_symbol = DIFF_SYMBOL_BINARY_DIFF_HEADER;

1264 +

1265 block_length = 0;

1266 }

1267

@@ -1265,7 +1271,6 @@ static void mark_color_as_moved(struct diff_options *o,

1271 if (flipped_block && o->color_moved != COLOR_MOVED_BLOCKS)

1272 l->flags |= DIFF_SYMBOL_MOVED_LINE_ALT;

1273 }

1268 - last_symbol = l->s;

1274 }

1275 adjust_last_block(o, n, block_length);

1276

t/t4015-diff-whitespace.sh

+65

@@ -1514,6 +1514,71 @@ test_expect_success 'zebra alternate color is only used when necessary' '

1514 test_cmp expected actual

1515 '

1516

1517 +test_expect_success 'short lines of opposite sign do not get marked as moved' '

1518 + cat >old.txt <<-\EOF &&

1519 + this line should be marked as moved

1520 + unchanged

1521 + unchanged

1522 + unchanged

1523 + unchanged

1524 + too short

1525 + this line should be marked as oldMoved newMoved

1526 + this line should be marked as oldMovedAlternate newMoved

1527 + unchanged 1

1528 + unchanged 2

1529 + unchanged 3

1530 + unchanged 4

1531 + this line should be marked as oldMoved newMoved/newMovedAlternate

1532 + EOF

1533 + cat >new.txt <<-\EOF &&

1534 + too short

1535 + unchanged

1536 + unchanged

1537 + this line should be marked as moved

1538 + too short

1539 + unchanged

1540 + unchanged

1541 + this line should be marked as oldMoved newMoved/newMovedAlternate

1542 + unchanged 1

1543 + unchanged 2

1544 + this line should be marked as oldMovedAlternate newMoved

1545 + this line should be marked as oldMoved newMoved/newMovedAlternate

1546 + unchanged 3

1547 + this line should be marked as oldMoved newMoved

1548 + unchanged 4

1549 + EOF

1550 + test_expect_code 1 git diff --no-index --color --color-moved=zebra \

1551 + old.txt new.txt >output && cat output &&

1552 + grep -v index output | test_decode_color >actual &&

1553 + cat >expect <<-\EOF &&

1554 + <BOLD>diff --git a/old.txt b/new.txt<RESET>

1555 + <BOLD>--- a/old.txt<RESET>

1556 + <BOLD>+++ b/new.txt<RESET>

1557 + <CYAN>@@ -1,13 +1,15 @@<RESET>

1558 + <BOLD;MAGENTA>-this line should be marked as moved<RESET>

1559 + <GREEN>+<RESET><GREEN>too short<RESET>

1560 + unchanged<RESET>

1561 + unchanged<RESET>

1562 + <BOLD;CYAN>+<RESET><BOLD;CYAN>this line should be marked as moved<RESET>

1563 + <GREEN>+<RESET><GREEN>too short<RESET>

1564 + unchanged<RESET>

1565 + unchanged<RESET>

1566 + <RED>-too short<RESET>

1567 + <BOLD;MAGENTA>-this line should be marked as oldMoved newMoved<RESET>

1568 + <BOLD;BLUE>-this line should be marked as oldMovedAlternate newMoved<RESET>

1569 + <BOLD;CYAN>+<RESET><BOLD;CYAN>this line should be marked as oldMoved newMoved/newMovedAlternate<RESET>

1570 + unchanged 1<RESET>

1571 + unchanged 2<RESET>

1572 + <BOLD;CYAN>+<RESET><BOLD;CYAN>this line should be marked as oldMovedAlternate newMoved<RESET>

1573 + <BOLD;YELLOW>+<RESET><BOLD;YELLOW>this line should be marked as oldMoved newMoved/newMovedAlternate<RESET>

1574 + unchanged 3<RESET>

1575 + <BOLD;CYAN>+<RESET><BOLD;CYAN>this line should be marked as oldMoved newMoved<RESET>

1576 + unchanged 4<RESET>

1577 + <BOLD;MAGENTA>-this line should be marked as oldMoved newMoved/newMovedAlternate<RESET>

1578 + EOF

1579 + test_cmp expect actual

1580 +'

1581 +

1582 test_expect_success 'cmd option assumes configured colored-moved' '

1583 test_config color.diff.oldMoved "magenta" &&

1584 test_config color.diff.newMoved "cyan" &&