diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt index b1ab96477b..a88c76741e 100644 --- a/Documentation/diff-options.txt +++ b/Documentation/diff-options.txt @@ -254,13 +254,11 @@ plain:: moved line, but it is not very useful in a review to determine if a block of code was moved without permutation. 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 'color.diff.{old,new}MovedAlternative'. The change between - the two colors indicates that a new block was detected. If there - 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. + the two colors indicates that a new block was detected. dimmed_zebra:: Similar to 'zebra', but additional dimming of uninteresting parts of moved code is performed. The bordering lines of two adjacent diff --git a/diff.c b/diff.c index 8c7e360b69..ac4023d30b 100644 --- a/diff.c +++ b/diff.c @@ -855,6 +855,38 @@ static int shrink_potential_moved_blocks(struct moved_entry **pmb, 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 */ static void mark_color_as_moved(struct diff_options *o, struct hashmap *add_lines, @@ -890,20 +922,13 @@ static void mark_color_as_moved(struct diff_options *o, } if (!match) { - if (block_length < COLOR_MOVED_MIN_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; - } - } + adjust_last_block(o, n, block_length); pmb_nr = 0; block_length = 0; continue; } l->flags |= DIFF_SYMBOL_MOVED_LINE; - block_length++; if (o->color_moved == COLOR_MOVED_PLAIN) continue; @@ -933,11 +958,17 @@ static void mark_color_as_moved(struct diff_options *o, } flipped_block = (flipped_block + 1) % 2; + + adjust_last_block(o, n, block_length); + block_length = 0; } + block_length++; + if (flipped_block) l->flags |= DIFF_SYMBOL_MOVED_LINE_ALT; } + adjust_last_block(o, n, block_length); free(pmb); } diff --git a/diff.h b/diff.h index 5755f465de..aca150ba2e 100644 --- a/diff.h +++ b/diff.h @@ -195,7 +195,7 @@ struct diff_options { COLOR_MOVED_ZEBRA_DIM = 3, } color_moved; #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); diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh index c3b697411a..12d182dc1b 100755 --- a/t/t4015-diff-whitespace.sh +++ b/t/t4015-diff-whitespace.sh @@ -1101,9 +1101,9 @@ test_expect_success 'detect malicious moved code, inside file' ' -{ -if (!u->is_allowed_foo) -return; - -foo(u); - -} - - + -foo(u); + -} + - int main() { foo(); @@ -1117,11 +1117,11 @@ test_expect_success 'detect malicious moved code, inside file' ' +int secure_foo(struct user *u) +{ - +foo(u); + +foo(u); +if (!u->is_allowed_foo) +return; - +} - + + +} + + int another_function() { bar(); @@ -1182,9 +1182,9 @@ test_expect_success 'plain moved code, inside file' ' test_expect_success 'detect permutations inside moved code -- dimmed_zebra' ' git reset --hard && cat <<-\EOF >lines.txt && - line 1 - line 2 - line 3 + long line 1 + long line 2 + long line 3 line 4 line 5 line 6 @@ -1195,9 +1195,9 @@ test_expect_success 'detect permutations inside moved code -- dimmed_zebra' ' line 11 line 12 line 13 - line 14 - line 15 - line 16 + long line 14 + long line 15 + long line 16 EOF git add lines.txt && git commit -m "add poetry" && @@ -1208,12 +1208,12 @@ test_expect_success 'detect permutations inside moved code -- dimmed_zebra' ' line 7 line 8 line 9 - line 1 - line 2 - line 3 - line 14 - line 15 - line 16 + long line 1 + long line 2 + long line 3 + long line 14 + long line 15 + long line 16 line 10 line 11 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.oldMovedAlternativeDimmed "normal blue" && 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 && diff --git a/lines.txt b/lines.txt - index 47ea9c3..ba96a38 100644 --- a/lines.txt +++ b/lines.txt @@ -1,16 +1,16 @@ - -line 1 - -line 2 - -line 3 + -long line 1 + -long line 2 + -long line 3 line 4 line 5 line 6 line 7 line 8 line 9 - +line 1 - +line 2 - +line 3 - +line 14 - +line 15 - +line 16 + +long line 1 + +long line 2 + +long line 3 + +long line 14 + +long line 15 + +long line 16 line 10 line 11 line 12 line 13 - -line 14 - -line 15 - -line 16 + -long line 14 + -long line 15 + -long line 16 EOF 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.newMovedAlternativeDimmed "normal yellow" && 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 && diff --git a/lines.txt b/lines.txt - index 47ea9c3..ba96a38 100644 --- a/lines.txt +++ b/lines.txt @@ -1,16 +1,16 @@ - -line 1 - -line 2 - -line 3 + -long line 1 + -long line 2 + -long line 3 line 4 line 5 line 6 line 7 line 8 line 9 - +line 1 - +line 2 - +line 3 - +line 14 - +line 15 - +line 16 + +long line 1 + +long line 2 + +long line 3 + +long line 14 + +long line 15 + +long line 16 line 10 line 11 line 12 line 13 - -line 14 - -line 15 - -line 16 + -long line 14 + -long line 15 + -long line 16 EOF test_cmp expected actual ' @@ -1324,16 +1326,16 @@ line 1 line 2 line 3 line 4 -line 5 -line 6 -line 7 +long line 5 +long line 6 +long line 7 EOF git add lines.txt && git commit -m "add poetry" && cat <<\EOF >lines.txt && - line 5 - line 6 - line 7 + long line 5 + long line 6 + long line 7 line 1 line 2 line 3 @@ -1341,47 +1343,170 @@ line 4 EOF test_config color.diff.oldMoved "magenta" && 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 && diff --git a/lines.txt b/lines.txt - index 734156d..eb89ead 100644 --- a/lines.txt +++ b/lines.txt @@ -1,7 +1,7 @@ - + line 5 - + line 6 - + line 7 + + long line 5 + + long line 6 + + long line 7 line 1 line 2 line 3 line 4 - -line 5 - -line 6 - -line 7 + -long line 5 + -long line 6 + -long line 7 EOF 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 && diff --git a/lines.txt b/lines.txt - index 734156d..eb89ead 100644 --- a/lines.txt +++ b/lines.txt @@ -1,7 +1,7 @@ - + line 5 - + line 6 - + line 7 + + long line 5 + + long line 6 + + long line 7 line 1 line 2 line 3 line 4 - -line 5 - -line 6 - -line 7 + -long line 5 + -long line 6 + -long line 7 EOF 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 && + diff --git a/bar b/bar + --- a/bar + +++ b/bar + @@ -0,0 +1 @@ + +line1 + diff --git a/foo b/foo + --- a/foo + +++ b/foo + @@ -1,2 +1 @@ + irrelevant_line + -line1 + 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 && + diff --git a/bar b/bar + --- a/bar + +++ b/bar + @@ -0,0 +1,2 @@ + +twenty chars 234567890 + +nineteen chars 456789 + diff --git a/foo b/foo + --- a/foo + +++ b/foo + @@ -1,3 +1 @@ + -nineteen chars 456789 + irrelevant_line + -twenty chars 234567890 + 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 && + diff --git a/bar b/bar + --- a/bar + +++ b/bar + @@ -0,0 +1,3 @@ + +7charsB + +7charsC + +7charsA + diff --git a/foo b/foo + --- a/foo + +++ b/foo + @@ -1,4 +1 @@ + -7charsA + irrelevant_line + -7charsB + -7charsC + EOF + + test_cmp expected actual +' + test_expect_success 'move detection with submodules' ' test_create_repo bananas && echo ripe >bananas/recipe &&