undef/git-commit-vandalism
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Merge branch 'jt/diff-color-move-fix'

Browse Source 
A handful of bugfixes and an improvement to "diff --color-moved".

* jt/diff-color-move-fix:
  diff: define block by number of alphanumeric chars
  diff: respect MIN_BLOCK_LENGTH for last block
  diff: avoid redundantly clearing a flag
This commit is contained in:
Junio C Hamano 2017-08-26 22:55:04 -07:00
commit 0b96358479
4 changed files with 236 additions and 82 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
Documentation/diff-options.txt
View File

@ -254,13 +254,11 @@ plain::
moved line, but it is not very useful in a review to determine moved line, but it is not very useful in a review to determine
if a block of code was moved without permutation. if a block of code was moved without permutation.
zebra:: zebra::
Blocks of moved code are detected greedily. The detected blocks are Blocks of moved text of at least 20 alphanumeric characters
are detected greedily. The detected blocks are
painted using either the 'color.diff.{old,new}Moved' color or painted using either the 'color.diff.{old,new}Moved' color or
'color.diff.{old,new}MovedAlternative'. The change between 'color.diff.{old,new}MovedAlternative'. The change between
the two colors indicates that a new block was detected. If there the two colors indicates that a new block was detected.
are fewer than 3 adjacent moved lines, they are not marked up
as moved, but the regular colors 'color.diff.{old,new}' will be
used.
dimmed_zebra:: dimmed_zebra::
Similar to 'zebra', but additional dimming of uninteresting parts Similar to 'zebra', but additional dimming of uninteresting parts
of moved code is performed. The bordering lines of two adjacent of moved code is performed. The bordering lines of two adjacent

47
diff.c
View File

@ -855,6 +855,38 @@ static int shrink_potential_moved_blocks(struct moved_entry **pmb,
return rp + 1; return rp + 1;
} }
/*
* If o->color_moved is COLOR_MOVED_PLAIN, this function does nothing.
*
* Otherwise, if the last block has fewer alphanumeric characters than
* COLOR_MOVED_MIN_ALNUM_COUNT, unset DIFF_SYMBOL_MOVED_LINE on all lines in
* that block.
*
* The last block consists of the (n - block_length)'th line up to but not
* including the nth line.
*
* NEEDSWORK: This uses the same heuristic as blame_entry_score() in blame.c.
* Think of a way to unify them.
*/
static void adjust_last_block(struct diff_options *o, int n, int block_length)
{
int i, alnum_count = 0;
if (o->color_moved == COLOR_MOVED_PLAIN)
return;
for (i = 1; i < block_length + 1; i++) {
const char *c = o->emitted_symbols->buf[n - i].line;
for (; *c; c++) {
if (!isalnum(*c))
continue;
alnum_count++;
if (alnum_count >= COLOR_MOVED_MIN_ALNUM_COUNT)
return;
}
}
for (i = 1; i < block_length + 1; i++)
o->emitted_symbols->buf[n - i].flags &= ~DIFF_SYMBOL_MOVED_LINE;
}
/* Find blocks of moved code, delegate actual coloring decision to helper */ /* Find blocks of moved code, delegate actual coloring decision to helper */
static void mark_color_as_moved(struct diff_options *o, static void mark_color_as_moved(struct diff_options *o,
struct hashmap *add_lines, struct hashmap *add_lines,
@ -890,20 +922,13 @@ static void mark_color_as_moved(struct diff_options *o,
} }
if (!match) { if (!match) {
if (block_length < COLOR_MOVED_MIN_BLOCK_LENGTH && adjust_last_block(o, n, block_length);
o->color_moved != COLOR_MOVED_PLAIN) {
for (i = 0; i < block_length + 1; i++) {
l = &o->emitted_symbols->buf[n - i];
l->flags &= ~DIFF_SYMBOL_MOVED_LINE;
}
}
pmb_nr = 0; pmb_nr = 0;
block_length = 0; block_length = 0;
continue; continue;
} }
l->flags |= DIFF_SYMBOL_MOVED_LINE; l->flags |= DIFF_SYMBOL_MOVED_LINE;
block_length++;
if (o->color_moved == COLOR_MOVED_PLAIN) if (o->color_moved == COLOR_MOVED_PLAIN)
continue; continue;
@ -933,11 +958,17 @@ static void mark_color_as_moved(struct diff_options *o,
} }
flipped_block = (flipped_block + 1) % 2; flipped_block = (flipped_block + 1) % 2;
adjust_last_block(o, n, block_length);
block_length = 0;
} }
block_length++;
if (flipped_block) if (flipped_block)
l->flags |= DIFF_SYMBOL_MOVED_LINE_ALT; l->flags |= DIFF_SYMBOL_MOVED_LINE_ALT;
} }
adjust_last_block(o, n, block_length);
free(pmb); free(pmb);
} }

2
diff.h
View File

@ -195,7 +195,7 @@ struct diff_options {
COLOR_MOVED_ZEBRA_DIM = 3, COLOR_MOVED_ZEBRA_DIM = 3,
} color_moved; } color_moved;
#define COLOR_MOVED_DEFAULT COLOR_MOVED_ZEBRA #define COLOR_MOVED_DEFAULT COLOR_MOVED_ZEBRA
#define COLOR_MOVED_MIN_BLOCK_LENGTH 3 #define COLOR_MOVED_MIN_ALNUM_COUNT 20
}; };
void diff_emit_submodule_del(struct diff_options *o, const char *line); void diff_emit_submodule_del(struct diff_options *o, const char *line);

261
t/t4015-diff-whitespace.sh
View File

@ -1101,9 +1101,9 @@ test_expect_success 'detect malicious moved code, inside file' '
<BRED>-{<RESET> <BRED>-{<RESET>
<BLUE>-if (!u->is_allowed_foo)<RESET> <BLUE>-if (!u->is_allowed_foo)<RESET>
<BLUE>-return;<RESET> <BLUE>-return;<RESET>
<BRED>-foo(u);<RESET> <RED>-foo(u);<RESET>
<BLUE>-}<RESET> <RED>-}<RESET>
<BLUE>-<RESET> <RED>-<RESET>
int main()<RESET> int main()<RESET>
{<RESET> {<RESET>
foo();<RESET> foo();<RESET>
@ -1117,11 +1117,11 @@ test_expect_success 'detect malicious moved code, inside file' '
<RESET> <RESET>
<BGREEN>+<RESET><BGREEN>int secure_foo(struct user *u)<RESET> <BGREEN>+<RESET><BGREEN>int secure_foo(struct user *u)<RESET>
<BGREEN>+<RESET><BGREEN>{<RESET> <BGREEN>+<RESET><BGREEN>{<RESET>
<YELLOW>+<RESET><YELLOW>foo(u);<RESET> <GREEN>+<RESET><GREEN>foo(u);<RESET>
<BGREEN>+<RESET><BGREEN>if (!u->is_allowed_foo)<RESET> <BGREEN>+<RESET><BGREEN>if (!u->is_allowed_foo)<RESET>
<BGREEN>+<RESET><BGREEN>return;<RESET> <BGREEN>+<RESET><BGREEN>return;<RESET>
<YELLOW>+<RESET><YELLOW>}<RESET> <GREEN>+<RESET><GREEN>}<RESET>
<YELLOW>+<RESET> <GREEN>+<RESET>
int another_function()<RESET> int another_function()<RESET>
{<RESET> {<RESET>
bar();<RESET> bar();<RESET>
@ -1182,9 +1182,9 @@ test_expect_success 'plain moved code, inside file' '
test_expect_success 'detect permutations inside moved code -- dimmed_zebra' ' test_expect_success 'detect permutations inside moved code -- dimmed_zebra' '
git reset --hard && git reset --hard &&
cat <<-\EOF >lines.txt && cat <<-\EOF >lines.txt &&
line 1 long line 1
line 2 long line 2
line 3 long line 3
line 4 line 4
line 5 line 5
line 6 line 6
@ -1195,9 +1195,9 @@ test_expect_success 'detect permutations inside moved code -- dimmed_zebra' '
line 11 line 11
line 12 line 12
line 13 line 13
line 14 long line 14
line 15 long line 15
line 16 long line 16
EOF EOF
git add lines.txt && git add lines.txt &&
git commit -m "add poetry" && git commit -m "add poetry" &&
@ -1208,12 +1208,12 @@ test_expect_success 'detect permutations inside moved code -- dimmed_zebra' '
line 7 line 7
line 8 line 8
line 9 line 9
line 1 long line 1
line 2 long line 2
line 3 long line 3
line 14 long line 14
line 15 long line 15
line 16 long line 16
line 10 line 10
line 11 line 11
line 12 line 12
@ -1227,35 +1227,36 @@ test_expect_success 'detect permutations inside moved code -- dimmed_zebra' '
test_config color.diff.newMovedDimmed "normal cyan" && test_config color.diff.newMovedDimmed "normal cyan" &&
test_config color.diff.oldMovedAlternativeDimmed "normal blue" && test_config color.diff.oldMovedAlternativeDimmed "normal blue" &&
test_config color.diff.newMovedAlternativeDimmed "normal yellow" && test_config color.diff.newMovedAlternativeDimmed "normal yellow" &&
git diff HEAD --no-renames --color-moved=dimmed_zebra| test_decode_color >actual && git diff HEAD --no-renames --color-moved=dimmed_zebra |
grep -v "index" |
test_decode_color >actual &&
cat <<-\EOF >expected && cat <<-\EOF >expected &&
<BOLD>diff --git a/lines.txt b/lines.txt<RESET> <BOLD>diff --git a/lines.txt b/lines.txt<RESET>
<BOLD>index 47ea9c3..ba96a38 100644<RESET>
<BOLD>--- a/lines.txt<RESET> <BOLD>--- a/lines.txt<RESET>
<BOLD>+++ b/lines.txt<RESET> <BOLD>+++ b/lines.txt<RESET>
<CYAN>@@ -1,16 +1,16 @@<RESET> <CYAN>@@ -1,16 +1,16 @@<RESET>
<BMAGENTA>-line 1<RESET> <BMAGENTA>-long line 1<RESET>
<BMAGENTA>-line 2<RESET> <BMAGENTA>-long line 2<RESET>
<BMAGENTA>-line 3<RESET> <BMAGENTA>-long line 3<RESET>
line 4<RESET> line 4<RESET>
line 5<RESET> line 5<RESET>
line 6<RESET> line 6<RESET>
line 7<RESET> line 7<RESET>
line 8<RESET> line 8<RESET>
line 9<RESET> line 9<RESET>
<BCYAN>+<RESET><BCYAN>line 1<RESET> <BCYAN>+<RESET><BCYAN>long line 1<RESET>
<BCYAN>+<RESET><BCYAN>line 2<RESET> <BCYAN>+<RESET><BCYAN>long line 2<RESET>
<CYAN>+<RESET><CYAN>line 3<RESET> <CYAN>+<RESET><CYAN>long line 3<RESET>
<YELLOW>+<RESET><YELLOW>line 14<RESET> <YELLOW>+<RESET><YELLOW>long line 14<RESET>
<BYELLOW>+<RESET><BYELLOW>line 15<RESET> <BYELLOW>+<RESET><BYELLOW>long line 15<RESET>
<BYELLOW>+<RESET><BYELLOW>line 16<RESET> <BYELLOW>+<RESET><BYELLOW>long line 16<RESET>
line 10<RESET> line 10<RESET>
line 11<RESET> line 11<RESET>
line 12<RESET> line 12<RESET>
line 13<RESET> line 13<RESET>
<BMAGENTA>-line 14<RESET> <BMAGENTA>-long line 14<RESET>
<BMAGENTA>-line 15<RESET> <BMAGENTA>-long line 15<RESET>
<BMAGENTA>-line 16<RESET> <BMAGENTA>-long line 16<RESET>
EOF EOF
test_cmp expected actual test_cmp expected actual
' '
@ -1270,35 +1271,36 @@ test_expect_success 'cmd option assumes configured colored-moved' '
test_config color.diff.oldMovedAlternativeDimmed "normal blue" && test_config color.diff.oldMovedAlternativeDimmed "normal blue" &&
test_config color.diff.newMovedAlternativeDimmed "normal yellow" && test_config color.diff.newMovedAlternativeDimmed "normal yellow" &&
test_config diff.colorMoved zebra && test_config diff.colorMoved zebra &&
git diff HEAD --no-renames --color-moved| test_decode_color >actual && git diff HEAD --no-renames --color-moved |
grep -v "index" |
test_decode_color >actual &&
cat <<-\EOF >expected && cat <<-\EOF >expected &&
<BOLD>diff --git a/lines.txt b/lines.txt<RESET> <BOLD>diff --git a/lines.txt b/lines.txt<RESET>
<BOLD>index 47ea9c3..ba96a38 100644<RESET>
<BOLD>--- a/lines.txt<RESET> <BOLD>--- a/lines.txt<RESET>
<BOLD>+++ b/lines.txt<RESET> <BOLD>+++ b/lines.txt<RESET>
<CYAN>@@ -1,16 +1,16 @@<RESET> <CYAN>@@ -1,16 +1,16 @@<RESET>
<MAGENTA>-line 1<RESET> <MAGENTA>-long line 1<RESET>
<MAGENTA>-line 2<RESET> <MAGENTA>-long line 2<RESET>
<MAGENTA>-line 3<RESET> <MAGENTA>-long line 3<RESET>
line 4<RESET> line 4<RESET>
line 5<RESET> line 5<RESET>
line 6<RESET> line 6<RESET>
line 7<RESET> line 7<RESET>
line 8<RESET> line 8<RESET>
line 9<RESET> line 9<RESET>
<CYAN>+<RESET><CYAN>line 1<RESET> <CYAN>+<RESET><CYAN>long line 1<RESET>
<CYAN>+<RESET><CYAN>line 2<RESET> <CYAN>+<RESET><CYAN>long line 2<RESET>
<CYAN>+<RESET><CYAN>line 3<RESET> <CYAN>+<RESET><CYAN>long line 3<RESET>
<YELLOW>+<RESET><YELLOW>line 14<RESET> <YELLOW>+<RESET><YELLOW>long line 14<RESET>
<YELLOW>+<RESET><YELLOW>line 15<RESET> <YELLOW>+<RESET><YELLOW>long line 15<RESET>
<YELLOW>+<RESET><YELLOW>line 16<RESET> <YELLOW>+<RESET><YELLOW>long line 16<RESET>
line 10<RESET> line 10<RESET>
line 11<RESET> line 11<RESET>
line 12<RESET> line 12<RESET>
line 13<RESET> line 13<RESET>
<MAGENTA>-line 14<RESET> <MAGENTA>-long line 14<RESET>
<MAGENTA>-line 15<RESET> <MAGENTA>-long line 15<RESET>
<MAGENTA>-line 16<RESET> <MAGENTA>-long line 16<RESET>
EOF EOF
test_cmp expected actual test_cmp expected actual
' '
@ -1324,16 +1326,16 @@ line 1
line 2 line 2
line 3 line 3
line 4 line 4
line 5 long line 5
line 6 long line 6
line 7 long line 7
EOF EOF
git add lines.txt && git add lines.txt &&
git commit -m "add poetry" && git commit -m "add poetry" &&
cat <<\EOF >lines.txt && cat <<\EOF >lines.txt &&
line 5 long line 5
line 6 long line 6
line 7 long line 7
line 1 line 1
line 2 line 2
line 3 line 3
@ -1341,47 +1343,170 @@ line 4
EOF EOF
test_config color.diff.oldMoved "magenta" && test_config color.diff.oldMoved "magenta" &&
test_config color.diff.newMoved "cyan" && test_config color.diff.newMoved "cyan" &&
git diff HEAD --no-renames --color-moved| test_decode_color >actual && git diff HEAD --no-renames --color-moved |
grep -v "index" |
test_decode_color >actual &&
cat <<-\EOF >expected && cat <<-\EOF >expected &&
<BOLD>diff --git a/lines.txt b/lines.txt<RESET> <BOLD>diff --git a/lines.txt b/lines.txt<RESET>
<BOLD>index 734156d..eb89ead 100644<RESET>
<BOLD>--- a/lines.txt<RESET> <BOLD>--- a/lines.txt<RESET>
<BOLD>+++ b/lines.txt<RESET> <BOLD>+++ b/lines.txt<RESET>
<CYAN>@@ -1,7 +1,7 @@<RESET> <CYAN>@@ -1,7 +1,7 @@<RESET>
<GREEN>+<RESET> <GREEN>line 5<RESET> <GREEN>+<RESET> <GREEN>long line 5<RESET>
<GREEN>+<RESET> <GREEN>line 6<RESET> <GREEN>+<RESET> <GREEN>long line 6<RESET>
<GREEN>+<RESET> <GREEN>line 7<RESET> <GREEN>+<RESET> <GREEN>long line 7<RESET>
line 1<RESET> line 1<RESET>
line 2<RESET> line 2<RESET>
line 3<RESET> line 3<RESET>
line 4<RESET> line 4<RESET>
<RED>-line 5<RESET> <RED>-long line 5<RESET>
<RED>-line 6<RESET> <RED>-long line 6<RESET>
<RED>-line 7<RESET> <RED>-long line 7<RESET>
EOF EOF
test_cmp expected actual && test_cmp expected actual &&
git diff HEAD --no-renames -w --color-moved| test_decode_color >actual && git diff HEAD --no-renames -w --color-moved |
grep -v "index" |
test_decode_color >actual &&
cat <<-\EOF >expected && cat <<-\EOF >expected &&
<BOLD>diff --git a/lines.txt b/lines.txt<RESET> <BOLD>diff --git a/lines.txt b/lines.txt<RESET>
<BOLD>index 734156d..eb89ead 100644<RESET>
<BOLD>--- a/lines.txt<RESET> <BOLD>--- a/lines.txt<RESET>
<BOLD>+++ b/lines.txt<RESET> <BOLD>+++ b/lines.txt<RESET>
<CYAN>@@ -1,7 +1,7 @@<RESET> <CYAN>@@ -1,7 +1,7 @@<RESET>
<CYAN>+<RESET> <CYAN>line 5<RESET> <CYAN>+<RESET> <CYAN>long line 5<RESET>
<CYAN>+<RESET> <CYAN>line 6<RESET> <CYAN>+<RESET> <CYAN>long line 6<RESET>
<CYAN>+<RESET> <CYAN>line 7<RESET> <CYAN>+<RESET> <CYAN>long line 7<RESET>
line 1<RESET> line 1<RESET>
line 2<RESET> line 2<RESET>
line 3<RESET> line 3<RESET>
line 4<RESET> line 4<RESET>
<MAGENTA>-line 5<RESET> <MAGENTA>-long line 5<RESET>
<MAGENTA>-line 6<RESET> <MAGENTA>-long line 6<RESET>
<MAGENTA>-line 7<RESET> <MAGENTA>-long line 7<RESET>
EOF EOF
test_cmp expected actual test_cmp expected actual
' '
test_expect_success '--color-moved block at end of diff output respects MIN_ALNUM_COUNT' '
git reset --hard &&
>bar &&
cat <<-\EOF >foo &&
irrelevant_line
line1
EOF
git add foo bar &&
git commit -m x &&
cat <<-\EOF >bar &&
line1
EOF
cat <<-\EOF >foo &&
irrelevant_line
EOF
git diff HEAD --color-moved=zebra --no-renames |
grep -v "index" |
test_decode_color >actual &&
cat >expected <<-\EOF &&
<BOLD>diff --git a/bar b/bar<RESET>
<BOLD>--- a/bar<RESET>
<BOLD>+++ b/bar<RESET>
<CYAN>@@ -0,0 +1 @@<RESET>
<GREEN>+<RESET><GREEN>line1<RESET>
<BOLD>diff --git a/foo b/foo<RESET>
<BOLD>--- a/foo<RESET>
<BOLD>+++ b/foo<RESET>
<CYAN>@@ -1,2 +1 @@<RESET>
irrelevant_line<RESET>
<RED>-line1<RESET>
EOF
test_cmp expected actual
'
test_expect_success '--color-moved respects MIN_ALNUM_COUNT' '
git reset --hard &&
cat <<-\EOF >foo &&
nineteen chars 456789
irrelevant_line
twenty chars 234567890
EOF
>bar &&
git add foo bar &&
git commit -m x &&
cat <<-\EOF >foo &&
irrelevant_line
EOF
cat <<-\EOF >bar &&
twenty chars 234567890
nineteen chars 456789
EOF
git diff HEAD --color-moved=zebra --no-renames |
grep -v "index" |
test_decode_color >actual &&
cat >expected <<-\EOF &&
<BOLD>diff --git a/bar b/bar<RESET>
<BOLD>--- a/bar<RESET>
<BOLD>+++ b/bar<RESET>
<CYAN>@@ -0,0 +1,2 @@<RESET>
<BOLD;CYAN>+<RESET><BOLD;CYAN>twenty chars 234567890<RESET>
<GREEN>+<RESET><GREEN>nineteen chars 456789<RESET>
<BOLD>diff --git a/foo b/foo<RESET>
<BOLD>--- a/foo<RESET>
<BOLD>+++ b/foo<RESET>
<CYAN>@@ -1,3 +1 @@<RESET>
<RED>-nineteen chars 456789<RESET>
irrelevant_line<RESET>
<BOLD;MAGENTA>-twenty chars 234567890<RESET>
EOF
test_cmp expected actual
'
test_expect_success '--color-moved treats adjacent blocks as separate for MIN_ALNUM_COUNT' '
git reset --hard &&
cat <<-\EOF >foo &&
7charsA
irrelevant_line
7charsB
7charsC
EOF
>bar &&
git add foo bar &&
git commit -m x &&
cat <<-\EOF >foo &&
irrelevant_line
EOF
cat <<-\EOF >bar &&
7charsB
7charsC
7charsA
EOF
git diff HEAD --color-moved=zebra --no-renames | grep -v "index" | test_decode_color >actual &&
cat >expected <<-\EOF &&
<BOLD>diff --git a/bar b/bar<RESET>
<BOLD>--- a/bar<RESET>
<BOLD>+++ b/bar<RESET>
<CYAN>@@ -0,0 +1,3 @@<RESET>
<GREEN>+<RESET><GREEN>7charsB<RESET>
<GREEN>+<RESET><GREEN>7charsC<RESET>
<GREEN>+<RESET><GREEN>7charsA<RESET>
<BOLD>diff --git a/foo b/foo<RESET>
<BOLD>--- a/foo<RESET>
<BOLD>+++ b/foo<RESET>
<CYAN>@@ -1,4 +1 @@<RESET>
<RED>-7charsA<RESET>
irrelevant_line<RESET>
<RED>-7charsB<RESET>
<RED>-7charsC<RESET>
EOF
test_cmp expected actual
'
test_expect_success 'move detection with submodules' ' test_expect_success 'move detection with submodules' '
test_create_repo bananas && test_create_repo bananas &&
echo ripe >bananas/recipe && echo ripe >bananas/recipe &&