diff: define block by number of alphanumeric chars
The existing behavior of diff --color-moved=zebra does not define the minimum size of a block at all, instead relying on a heuristic applied later to filter out sets of adjacent moved lines that are shorter than 3 lines long. This can be confusing, because a block could thus be colored as moved at the source but not at the destination (or vice versa), depending on its neighbors. Instead, teach diff that the minimum size of a block is 20 alphanumeric characters, the same heuristic used by "git blame". This allows diff to still exclude uninteresting lines appearing on their own (such as those solely consisting of one or a few closing braces), as was the intention of the adjacent-moved-line heuristic. This requires a change in some tests in that some of their lines are no longer considered to be part of a block, because they are too short. Signed-off-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jonathan Tan committed
Aug 15, 2017 at 18:27 UTC
f0b8fb6e591b50b72b921f2c4cf120ebd284f510
4 files changed
+183
-81
Documentation/diff-options.txt
+3
-5
@@ -254,13 +254,11 @@ plain::
254
moved line, but it is not very useful in a review to determine
255
if a block of code was moved without permutation.
256
zebra::
257
- Blocks of moved code are detected greedily. The detected blocks are
257
+ Blocks of moved text of at least 20 alphanumeric characters
258
+ are detected greedily. The detected blocks are
259
painted using either the 'color.diff.{old,new}Moved' color or
260
'color.diff.{old,new}MovedAlternative'. The change between
260
- the two colors indicates that a new block was detected. If there
261
- are fewer than 3 adjacent moved lines, they are not marked up
262
- as moved, but the regular colors 'color.diff.{old,new}' will be
263
- used.
261
+ the two colors indicates that a new block was detected.
262
dimmed_zebra::
263
Similar to 'zebra', but additional dimming of uninteresting parts
264
of moved code is performed. The bordering lines of two adjacent
diff.c
+22
-6
@@ -864,19 +864,31 @@ static int shrink_potential_moved_blocks(struct moved_entry **pmb,
864
/*
865
* If o->color_moved is COLOR_MOVED_PLAIN, this function does nothing.
866
*
867
- * Otherwise, if the last block has fewer lines than
868
- * COLOR_MOVED_MIN_BLOCK_LENGTH, unset DIFF_SYMBOL_MOVED_LINE on all lines in
867
+ * Otherwise, if the last block has fewer alphanumeric characters than
868
+ * COLOR_MOVED_MIN_ALNUM_COUNT, unset DIFF_SYMBOL_MOVED_LINE on all lines in
869
* that block.
870
*
871
* The last block consists of the (n - block_length)'th line up to but not
872
* including the nth line.
873
+ *
874
+ * NEEDSWORK: This uses the same heuristic as blame_entry_score() in blame.c.
875
+ * Think of a way to unify them.
876
*/
877
static void adjust_last_block(struct diff_options *o, int n, int block_length)
878
{
876
- int i;
877
- if (block_length >= COLOR_MOVED_MIN_BLOCK_LENGTH ||
878
- o->color_moved == COLOR_MOVED_PLAIN)
879
+ int i, alnum_count = 0;
880
+ if (o->color_moved == COLOR_MOVED_PLAIN)
881
return;
882
+ for (i = 1; i < block_length + 1; i++) {
883
+ const char *c = o->emitted_symbols->buf[n - i].line;
884
+ for (; *c; c++) {
885
+ if (!isalnum(*c))
886
+ continue;
887
+ alnum_count++;
888
+ if (alnum_count >= COLOR_MOVED_MIN_ALNUM_COUNT)
889
+ return;
890
+ }
891
+ }
892
for (i = 1; i < block_length + 1; i++)
893
o->emitted_symbols->buf[n - i].flags &= ~DIFF_SYMBOL_MOVED_LINE;
894
}
@@ -923,7 +935,6 @@ static void mark_color_as_moved(struct diff_options *o,
935
}
936
937
l->flags |= DIFF_SYMBOL_MOVED_LINE;
926
- block_length++;
938
939
if (o->color_moved == COLOR_MOVED_PLAIN)
940
continue;
@@ -953,8 +964,13 @@ static void mark_color_as_moved(struct diff_options *o,
964
}
965
966
flipped_block = (flipped_block + 1) % 2;
967
+
968
+ adjust_last_block(o, n, block_length);
969
+ block_length = 0;
970
}
971
972
+ block_length++;
973
+
974
if (flipped_block)
975
l->flags |= DIFF_SYMBOL_MOVED_LINE_ALT;
976
}
diff.h
+1
-1
@@ -195,7 +195,7 @@ struct diff_options {
195
COLOR_MOVED_ZEBRA_DIM = 3,
196
} color_moved;
197
#define COLOR_MOVED_DEFAULT COLOR_MOVED_ZEBRA
198
- #define COLOR_MOVED_MIN_BLOCK_LENGTH 3
198
+ #define COLOR_MOVED_MIN_ALNUM_COUNT 20
199
};
200
201
void diff_emit_submodule_del(struct diff_options *o, const char *line);
t/t4015-diff-whitespace.sh
+157
-69
@@ -1101,9 +1101,9 @@ test_expect_success 'detect malicious moved code, inside file' '
1101
<BRED>-{<RESET>
1102
<BLUE>-if (!u->is_allowed_foo)<RESET>
1103
<BLUE>-return;<RESET>
1104
- <BRED>-foo(u);<RESET>
1105
- <BLUE>-}<RESET>
1106
- <BLUE>-<RESET>
1104
+ <RED>-foo(u);<RESET>
1105
+ <RED>-}<RESET>
1106
+ <RED>-<RESET>
1107
int main()<RESET>
1108
{<RESET>
1109
foo();<RESET>
@@ -1117,11 +1117,11 @@ test_expect_success 'detect malicious moved code, inside file' '
1117
<RESET>
1118
<BGREEN>+<RESET><BGREEN>int secure_foo(struct user *u)<RESET>
1119
<BGREEN>+<RESET><BGREEN>{<RESET>
1120
- <YELLOW>+<RESET><YELLOW>foo(u);<RESET>
1120
+ <GREEN>+<RESET><GREEN>foo(u);<RESET>
1121
<BGREEN>+<RESET><BGREEN>if (!u->is_allowed_foo)<RESET>
1122
<BGREEN>+<RESET><BGREEN>return;<RESET>
1123
- <YELLOW>+<RESET><YELLOW>}<RESET>
1124
- <YELLOW>+<RESET>
1123
+ <GREEN>+<RESET><GREEN>}<RESET>
1124
+ <GREEN>+<RESET>
1125
int another_function()<RESET>
1126
{<RESET>
1127
bar();<RESET>
@@ -1182,9 +1182,9 @@ test_expect_success 'plain moved code, inside file' '
1182
test_expect_success 'detect permutations inside moved code -- dimmed_zebra' '
1183
git reset --hard &&
1184
cat <<-\EOF >lines.txt &&
1185
- line 1
1186
- line 2
1187
- line 3
1185
+ long line 1
1186
+ long line 2
1187
+ long line 3
1188
line 4
1189
line 5
1190
line 6
@@ -1195,9 +1195,9 @@ test_expect_success 'detect permutations inside moved code -- dimmed_zebra' '
1195
line 11
1196
line 12
1197
line 13
1198
- line 14
1199
- line 15
1200
- line 16
1198
+ long line 14
1199
+ long line 15
1200
+ long line 16
1201
EOF
1202
git add lines.txt &&
1203
git commit -m "add poetry" &&
@@ -1208,12 +1208,12 @@ test_expect_success 'detect permutations inside moved code -- dimmed_zebra' '
1208
line 7
1209
line 8
1210
line 9
1211
- line 1
1212
- line 2
1213
- line 3
1214
- line 14
1215
- line 15
1216
- line 16
1211
+ long line 1
1212
+ long line 2
1213
+ long line 3
1214
+ long line 14
1215
+ long line 15
1216
+ long line 16
1217
line 10
1218
line 11
1219
line 12
@@ -1227,35 +1227,36 @@ test_expect_success 'detect permutations inside moved code -- dimmed_zebra' '
1227
test_config color.diff.newMovedDimmed "normal cyan" &&
1228
test_config color.diff.oldMovedAlternativeDimmed "normal blue" &&
1229
test_config color.diff.newMovedAlternativeDimmed "normal yellow" &&
1230
- git diff HEAD --no-renames --color-moved=dimmed_zebra| test_decode_color >actual &&
1230
+ git diff HEAD --no-renames --color-moved=dimmed_zebra |
1231
+ grep -v "index" |
1232
+ test_decode_color >actual &&
1233
cat <<-\EOF >expected &&
1234
<BOLD>diff --git a/lines.txt b/lines.txt<RESET>
1233
- <BOLD>index 47ea9c3..ba96a38 100644<RESET>
1235
<BOLD>--- a/lines.txt<RESET>
1236
<BOLD>+++ b/lines.txt<RESET>
1237
<CYAN>@@ -1,16 +1,16 @@<RESET>
1237
- <BMAGENTA>-line 1<RESET>
1238
- <BMAGENTA>-line 2<RESET>
1239
- <BMAGENTA>-line 3<RESET>
1238
+ <BMAGENTA>-long line 1<RESET>
1239
+ <BMAGENTA>-long line 2<RESET>
1240
+ <BMAGENTA>-long line 3<RESET>
1241
line 4<RESET>
1242
line 5<RESET>
1243
line 6<RESET>
1244
line 7<RESET>
1245
line 8<RESET>
1246
line 9<RESET>
1246
- <BCYAN>+<RESET><BCYAN>line 1<RESET>
1247
- <BCYAN>+<RESET><BCYAN>line 2<RESET>
1248
- <CYAN>+<RESET><CYAN>line 3<RESET>
1249
- <YELLOW>+<RESET><YELLOW>line 14<RESET>
1250
- <BYELLOW>+<RESET><BYELLOW>line 15<RESET>
1251
- <BYELLOW>+<RESET><BYELLOW>line 16<RESET>
1247
+ <BCYAN>+<RESET><BCYAN>long line 1<RESET>
1248
+ <BCYAN>+<RESET><BCYAN>long line 2<RESET>
1249
+ <CYAN>+<RESET><CYAN>long line 3<RESET>
1250
+ <YELLOW>+<RESET><YELLOW>long line 14<RESET>
1251
+ <BYELLOW>+<RESET><BYELLOW>long line 15<RESET>
1252
+ <BYELLOW>+<RESET><BYELLOW>long line 16<RESET>
1253
line 10<RESET>
1254
line 11<RESET>
1255
line 12<RESET>
1256
line 13<RESET>
1256
- <BMAGENTA>-line 14<RESET>
1257
- <BMAGENTA>-line 15<RESET>
1258
- <BMAGENTA>-line 16<RESET>
1257
+ <BMAGENTA>-long line 14<RESET>
1258
+ <BMAGENTA>-long line 15<RESET>
1259
+ <BMAGENTA>-long line 16<RESET>
1260
EOF
1261
test_cmp expected actual
1262
'
@@ -1270,35 +1271,36 @@ test_expect_success 'cmd option assumes configured colored-moved' '
1271
test_config color.diff.oldMovedAlternativeDimmed "normal blue" &&
1272
test_config color.diff.newMovedAlternativeDimmed "normal yellow" &&
1273
test_config diff.colorMoved zebra &&
1273
- git diff HEAD --no-renames --color-moved| test_decode_color >actual &&
1274
+ git diff HEAD --no-renames --color-moved |
1275
+ grep -v "index" |
1276
+ test_decode_color >actual &&
1277
cat <<-\EOF >expected &&
1278
<BOLD>diff --git a/lines.txt b/lines.txt<RESET>
1276
- <BOLD>index 47ea9c3..ba96a38 100644<RESET>
1279
<BOLD>--- a/lines.txt<RESET>
1280
<BOLD>+++ b/lines.txt<RESET>
1281
<CYAN>@@ -1,16 +1,16 @@<RESET>
1280
- <MAGENTA>-line 1<RESET>
1281
- <MAGENTA>-line 2<RESET>
1282
- <MAGENTA>-line 3<RESET>
1282
+ <MAGENTA>-long line 1<RESET>
1283
+ <MAGENTA>-long line 2<RESET>
1284
+ <MAGENTA>-long line 3<RESET>
1285
line 4<RESET>
1286
line 5<RESET>
1287
line 6<RESET>
1288
line 7<RESET>
1289
line 8<RESET>
1290
line 9<RESET>
1289
- <CYAN>+<RESET><CYAN>line 1<RESET>
1290
- <CYAN>+<RESET><CYAN>line 2<RESET>
1291
- <CYAN>+<RESET><CYAN>line 3<RESET>
1292
- <YELLOW>+<RESET><YELLOW>line 14<RESET>
1293
- <YELLOW>+<RESET><YELLOW>line 15<RESET>
1294
- <YELLOW>+<RESET><YELLOW>line 16<RESET>
1291
+ <CYAN>+<RESET><CYAN>long line 1<RESET>
1292
+ <CYAN>+<RESET><CYAN>long line 2<RESET>
1293
+ <CYAN>+<RESET><CYAN>long line 3<RESET>
1294
+ <YELLOW>+<RESET><YELLOW>long line 14<RESET>
1295
+ <YELLOW>+<RESET><YELLOW>long line 15<RESET>
1296
+ <YELLOW>+<RESET><YELLOW>long line 16<RESET>
1297
line 10<RESET>
1298
line 11<RESET>
1299
line 12<RESET>
1300
line 13<RESET>
1299
- <MAGENTA>-line 14<RESET>
1300
- <MAGENTA>-line 15<RESET>
1301
- <MAGENTA>-line 16<RESET>
1301
+ <MAGENTA>-long line 14<RESET>
1302
+ <MAGENTA>-long line 15<RESET>
1303
+ <MAGENTA>-long line 16<RESET>
1304
EOF
1305
test_cmp expected actual
1306
'
@@ -1324,16 +1326,16 @@ line 1
1326
line 2
1327
line 3
1328
line 4
1327
-line 5
1328
-line 6
1329
-line 7
1329
+long line 5
1330
+long line 6
1331
+long line 7
1332
EOF
1333
git add lines.txt &&
1334
git commit -m "add poetry" &&
1335
cat <<\EOF >lines.txt &&
1334
- line 5
1335
- line 6
1336
- line 7
1336
+ long line 5
1337
+ long line 6
1338
+ long line 7
1339
line 1
1340
line 2
1341
line 3
@@ -1341,48 +1343,50 @@ line 4
1343
EOF
1344
test_config color.diff.oldMoved "magenta" &&
1345
test_config color.diff.newMoved "cyan" &&
1344
- git diff HEAD --no-renames --color-moved| test_decode_color >actual &&
1346
+ git diff HEAD --no-renames --color-moved |
1347
+ grep -v "index" |
1348
+ test_decode_color >actual &&
1349
cat <<-\EOF >expected &&
1350
<BOLD>diff --git a/lines.txt b/lines.txt<RESET>
1347
- <BOLD>index 734156d..eb89ead 100644<RESET>
1351
<BOLD>--- a/lines.txt<RESET>
1352
<BOLD>+++ b/lines.txt<RESET>
1353
<CYAN>@@ -1,7 +1,7 @@<RESET>
1351
- <GREEN>+<RESET> <GREEN>line 5<RESET>
1352
- <GREEN>+<RESET> <GREEN>line 6<RESET>
1353
- <GREEN>+<RESET> <GREEN>line 7<RESET>
1354
+ <GREEN>+<RESET> <GREEN>long line 5<RESET>
1355
+ <GREEN>+<RESET> <GREEN>long line 6<RESET>
1356
+ <GREEN>+<RESET> <GREEN>long line 7<RESET>
1357
line 1<RESET>
1358
line 2<RESET>
1359
line 3<RESET>
1360
line 4<RESET>
1358
- <RED>-line 5<RESET>
1359
- <RED>-line 6<RESET>
1360
- <RED>-line 7<RESET>
1361
+ <RED>-long line 5<RESET>
1362
+ <RED>-long line 6<RESET>
1363
+ <RED>-long line 7<RESET>
1364
EOF
1365
test_cmp expected actual &&
1366
1364
- git diff HEAD --no-renames -w --color-moved| test_decode_color >actual &&
1367
+ git diff HEAD --no-renames -w --color-moved |
1368
+ grep -v "index" |
1369
+ test_decode_color >actual &&
1370
cat <<-\EOF >expected &&
1371
<BOLD>diff --git a/lines.txt b/lines.txt<RESET>
1367
- <BOLD>index 734156d..eb89ead 100644<RESET>
1372
<BOLD>--- a/lines.txt<RESET>
1373
<BOLD>+++ b/lines.txt<RESET>
1374
<CYAN>@@ -1,7 +1,7 @@<RESET>
1371
- <CYAN>+<RESET> <CYAN>line 5<RESET>
1372
- <CYAN>+<RESET> <CYAN>line 6<RESET>
1373
- <CYAN>+<RESET> <CYAN>line 7<RESET>
1375
+ <CYAN>+<RESET> <CYAN>long line 5<RESET>
1376
+ <CYAN>+<RESET> <CYAN>long line 6<RESET>
1377
+ <CYAN>+<RESET> <CYAN>long line 7<RESET>
1378
line 1<RESET>
1379
line 2<RESET>
1380
line 3<RESET>
1381
line 4<RESET>
1378
- <MAGENTA>-line 5<RESET>
1379
- <MAGENTA>-line 6<RESET>
1380
- <MAGENTA>-line 7<RESET>
1382
+ <MAGENTA>-long line 5<RESET>
1383
+ <MAGENTA>-long line 6<RESET>
1384
+ <MAGENTA>-long line 7<RESET>
1385
EOF
1386
test_cmp expected actual
1387
'
1388
1385
-test_expect_success '--color-moved block at end of diff output respects MIN_BLOCK_LENGTH' '
1389
+test_expect_success '--color-moved block at end of diff output respects MIN_ALNUM_COUNT' '
1390
git reset --hard &&
1391
>bar &&
1392
cat <<-\EOF >foo &&
@@ -1419,6 +1423,90 @@ test_expect_success '--color-moved block at end of diff output respects MIN_BLOC
1423
test_cmp expected actual
1424
'
1425
1426
+test_expect_success '--color-moved respects MIN_ALNUM_COUNT' '
1427
+ git reset --hard &&
1428
+ cat <<-\EOF >foo &&
1429
+ nineteen chars 456789
1430
+ irrelevant_line
1431
+ twenty chars 234567890
1432
+ EOF
1433
+ >bar &&
1434
+ git add foo bar &&
1435
+ git commit -m x &&
1436
+
1437
+ cat <<-\EOF >foo &&
1438
+ irrelevant_line
1439
+ EOF
1440
+ cat <<-\EOF >bar &&
1441
+ twenty chars 234567890
1442
+ nineteen chars 456789
1443
+ EOF
1444
+
1445
+ git diff HEAD --color-moved=zebra --no-renames |
1446
+ grep -v "index" |
1447
+ test_decode_color >actual &&
1448
+ cat >expected <<-\EOF &&
1449
+ <BOLD>diff --git a/bar b/bar<RESET>
1450
+ <BOLD>--- a/bar<RESET>
1451
+ <BOLD>+++ b/bar<RESET>
1452
+ <CYAN>@@ -0,0 +1,2 @@<RESET>
1453
+ <BOLD;CYAN>+<RESET><BOLD;CYAN>twenty chars 234567890<RESET>
1454
+ <GREEN>+<RESET><GREEN>nineteen chars 456789<RESET>
1455
+ <BOLD>diff --git a/foo b/foo<RESET>
1456
+ <BOLD>--- a/foo<RESET>
1457
+ <BOLD>+++ b/foo<RESET>
1458
+ <CYAN>@@ -1,3 +1 @@<RESET>
1459
+ <RED>-nineteen chars 456789<RESET>
1460
+ irrelevant_line<RESET>
1461
+ <BOLD;MAGENTA>-twenty chars 234567890<RESET>
1462
+ EOF
1463
+
1464
+ test_cmp expected actual
1465
+'
1466
+
1467
+test_expect_success '--color-moved treats adjacent blocks as separate for MIN_ALNUM_COUNT' '
1468
+ git reset --hard &&
1469
+ cat <<-\EOF >foo &&
1470
+ 7charsA
1471
+ irrelevant_line
1472
+ 7charsB
1473
+ 7charsC
1474
+ EOF
1475
+ >bar &&
1476
+ git add foo bar &&
1477
+ git commit -m x &&
1478
+
1479
+ cat <<-\EOF >foo &&
1480
+ irrelevant_line
1481
+ EOF
1482
+ cat <<-\EOF >bar &&
1483
+ 7charsB
1484
+ 7charsC
1485
+ 7charsA
1486
+ EOF
1487
+
1488
+ git diff HEAD --color-moved=zebra --no-renames | grep -v "index" | test_decode_color >actual &&
1489
+ cat >expected <<-\EOF &&
1490
+ <BOLD>diff --git a/bar b/bar<RESET>
1491
+ <BOLD>--- a/bar<RESET>
1492
+ <BOLD>+++ b/bar<RESET>
1493
+ <CYAN>@@ -0,0 +1,3 @@<RESET>
1494
+ <GREEN>+<RESET><GREEN>7charsB<RESET>
1495
+ <GREEN>+<RESET><GREEN>7charsC<RESET>
1496
+ <GREEN>+<RESET><GREEN>7charsA<RESET>
1497
+ <BOLD>diff --git a/foo b/foo<RESET>
1498
+ <BOLD>--- a/foo<RESET>
1499
+ <BOLD>+++ b/foo<RESET>
1500
+ <CYAN>@@ -1,4 +1 @@<RESET>
1501
+ <RED>-7charsA<RESET>
1502
+ irrelevant_line<RESET>
1503
+ <RED>-7charsB<RESET>
1504
+ <RED>-7charsC<RESET>
1505
+ EOF
1506
+
1507
+ test_cmp expected actual
1508
+'
1509
+
1510
test_expect_success 'move detection with submodules' '
1511
test_create_repo bananas &&
1512
echo ripe >bananas/recipe &&