From: Junio C Hamano Date: Tue, 17 Oct 2017 04:29:19 +0000 (+0900) Subject: Merge branch 'sb/diff-color-move' X-Git-Tag: v2.15.0-rc2~10 X-Git-Url: https://git.lorimer.id.au/gitweb.git/diff_plain/91ccfb85176fbe2ed416751ff7884cdaf61311cb?ds=inline;hp=-c Merge branch 'sb/diff-color-move' A recently added "--color-moved" feature of "diff" fell into infinite loop when ignoring whitespace changes, which has been fixed. * sb/diff-color-move: diff: fix infinite loop with --color-moved --ignore-space-change --- 91ccfb85176fbe2ed416751ff7884cdaf61311cb diff --combined diff.c index 69f03570ad,af9b952886..d76bb937c1 --- a/diff.c +++ b/diff.c @@@ -21,7 -21,6 +21,7 @@@ #include "string-list.h" #include "argv-array.h" #include "graph.h" +#include "packfile.h" #ifdef NO_FAST_WORKING_DIRECTORY #define FAST_WORKING_DIRECTORY 0 @@@ -358,6 -357,9 +358,6 @@@ int git_diff_ui_config(const char *var return 0; } - if (git_color_config(var, value, cb) < 0) - return -1; - return git_diff_basic_config(var, value, cb); } @@@ -402,6 -404,9 +402,6 @@@ int git_diff_basic_config(const char *v return 0; } - if (starts_with(var, "submodule.")) - return parse_submodule_config_option(var, value); - if (git_diff_heuristic_config(var, value, cb) < 0) return -1; @@@ -459,9 -464,11 +459,9 @@@ static struct diff_tempfile * If this diff_tempfile instance refers to a temporary file, * this tempfile object is used to manage its lifetime. */ - struct tempfile tempfile; + struct tempfile *tempfile; } diff_temp[2]; -typedef unsigned long (*sane_truncate_fn)(char *line, unsigned long len); - struct emit_callback { int color_diff; unsigned ws_rule; @@@ -469,6 -476,7 +469,6 @@@ int blank_at_eof_in_postimage; int lno_in_preimage; int lno_in_postimage; - sane_truncate_fn truncate; const char **label_path; struct diff_words_data *diff_words; struct diff_options *opt; @@@ -712,20 -720,22 +712,22 @@@ static int next_byte(const char **cp, c if (*cp > *endp) return -1; - if (DIFF_XDL_TST(diffopt, IGNORE_WHITESPACE_CHANGE)) { - while (*cp < *endp && isspace(**cp)) - (*cp)++; - /* - * After skipping a couple of whitespaces, we still have to - * account for one space. - */ - return (int)' '; - } + if (isspace(**cp)) { + if (DIFF_XDL_TST(diffopt, IGNORE_WHITESPACE_CHANGE)) { + while (*cp < *endp && isspace(**cp)) + (*cp)++; + /* + * After skipping a couple of whitespaces, + * we still have to account for one space. + */ + return (int)' '; + } - if (DIFF_XDL_TST(diffopt, IGNORE_WHITESPACE)) { - while (*cp < *endp && isspace(**cp)) - (*cp)++; - /* return the first non-ws character via the usual below */ + if (DIFF_XDL_TST(diffopt, IGNORE_WHITESPACE)) { + while (*cp < *endp && isspace(**cp)) + (*cp)++; + /* return the first non-ws character via the usual below */ + } } retval = (unsigned char)(**cp); @@@ -853,38 -863,6 +855,38 @@@ static int shrink_potential_moved_block 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, @@@ -920,13 -898,20 +922,13 @@@ } 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; @@@ -956,17 -941,11 +958,17 @@@ } 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); } @@@ -1414,7 -1393,7 +1416,7 @@@ static void remove_tempfile(void { int i; for (i = 0; i < ARRAY_SIZE(diff_temp); i++) { - if (is_tempfile_active(&diff_temp[i].tempfile)) + if (is_tempfile_active(diff_temp[i].tempfile)) delete_tempfile(&diff_temp[i].tempfile); diff_temp[i].name = NULL; } @@@ -1541,7 -1520,7 +1543,7 @@@ static void emit_rewrite_diff(const cha struct diff_words_buffer { mmfile_t text; - long alloc; + unsigned long alloc; struct diff_words_orig { const char *begin, *end; } *orig; @@@ -1994,6 -1973,8 +1996,6 @@@ static unsigned long sane_truncate_line unsigned long allot; size_t l = len; - if (ecb->truncate) - return ecb->truncate(line, len); cp = line; allot = l; while (0 < l) { @@@ -2583,7 -2564,6 +2585,7 @@@ static void show_stats(struct diffstat_ } print_stat_summary_inserts_deletes(options, total_files, adds, dels); + strbuf_release(&out); } static void show_shortstats(struct diffstat_t *data, struct diff_options *options) @@@ -2839,7 -2819,7 +2841,7 @@@ static void show_dirstat_by_line(struc * bytes per "line". * This is stupid and ugly, but very cheap... */ - damage = (damage + 63) / 64; + damage = DIV_ROUND_UP(damage, 64); ALLOC_GROW(dir.files, dir.nr + 1, dir.alloc); dir.files[dir.nr].name = file->name; dir.files[dir.nr].changed = damage; @@@ -3721,6 -3701,7 +3723,6 @@@ static void prep_temp_blob(const char * const struct object_id *oid, int mode) { - int fd; struct strbuf buf = STRBUF_INIT; struct strbuf template = STRBUF_INIT; char *path_dup = xstrdup(path); @@@ -3730,18 -3711,18 +3732,18 @@@ strbuf_addstr(&template, "XXXXXX_"); strbuf_addstr(&template, base); - fd = mks_tempfile_ts(&temp->tempfile, template.buf, strlen(base) + 1); - if (fd < 0) + temp->tempfile = mks_tempfile_ts(template.buf, strlen(base) + 1); + if (!temp->tempfile) die_errno("unable to create temp-file"); if (convert_to_working_tree(path, (const char *)blob, (size_t)size, &buf)) { blob = buf.buf; size = buf.len; } - if (write_in_full(fd, blob, size) != size) + if (write_in_full(temp->tempfile->fd, blob, size) < 0 || + close_tempfile_gently(temp->tempfile)) die_errno("unable to write temp-file"); - close_tempfile(&temp->tempfile); - temp->name = get_tempfile_path(&temp->tempfile); + temp->name = get_tempfile_path(temp->tempfile); oid_to_hex_r(temp->hex, oid); xsnprintf(temp->mode, sizeof(temp->mode), "%06o", mode); strbuf_release(&buf); @@@ -4009,7 -3990,7 +4011,7 @@@ static void diff_fill_oid_info(struct d } if (lstat(one->path, &st) < 0) die_errno("stat '%s'", one->path); - if (index_path(one->oid.hash, one->path, &st, 0)) + if (index_path(&one->oid, one->path, &st, 0)) die("cannot hash %s", one->path); } } @@@ -5290,7 -5271,6 +5292,7 @@@ static void show_rename_copy(struct dif emit_diff_symbol(opt, DIFF_SYMBOL_SUMMARY, sb.buf, sb.len, 0); show_mode_change(opt, p, 0); + strbuf_release(&sb); } static void diff_summary(struct diff_options *opt, struct diff_filepair *p) @@@ -5316,7 -5296,6 +5318,7 @@@ strbuf_addf(&sb, " (%d%%)\n", similarity_index(p)); emit_diff_symbol(opt, DIFF_SYMBOL_SUMMARY, sb.buf, sb.len, 0); + strbuf_release(&sb); } show_mode_change(opt, p, !p->score); break; diff --combined t/t4015-diff-whitespace.sh index bd0f75d9f7,66cad4d629..87083f728f --- a/t/t4015-diff-whitespace.sh +++ b/t/t4015-diff-whitespace.sh @@@ -155,7 -155,7 +155,7 @@@ test_expect_success 'ignore-blank-lines " >x && git diff --ignore-blank-lines >out && >expect && - test_cmp out expect + test_cmp expect out ' test_expect_success 'ignore-blank-lines: only new lines with space' ' @@@ -165,7 -165,7 +165,7 @@@ " >x && git diff -w --ignore-blank-lines >out && >expect && - test_cmp out expect + test_cmp expect out ' test_expect_success 'ignore-blank-lines: after change' ' @@@ -802,6 -802,7 +802,6 @@@ test_expect_success 'combined diff wit # Start testing the colored format for whitespace checks test_expect_success 'setup diff colors' ' - git config color.diff always && git config color.diff.plain normal && git config color.diff.meta bold && git config color.diff.frag cyan && @@@ -820,7 -821,7 +820,7 @@@ test_expect_success 'diff that introduc echo "test" >x && git commit -m "initial" x && echo "{NTN}" | tr "NT" "\n\t" >>x && - git -c color.diff=always diff | test_decode_color >current && + git diff --color | test_decode_color >current && cat >expected <<-\EOF && diff --git a/x b/x @@@ -850,7 -851,7 +850,7 @@@ test_expect_success 'diff that introduc echo "2. and a new line " } >x && - git -c color.diff=always diff | + git diff --color | test_decode_color >current && cat >expected <<-\EOF && @@@ -922,15 -923,15 +922,15 @@@ test_expect_success 'ws-error-highligh test_expect_success 'test --ws-error-highlight option' ' - git -c color.diff=always diff --ws-error-highlight=default,old | + git diff --color --ws-error-highlight=default,old | test_decode_color >current && test_cmp expect.default-old current && - git -c color.diff=always diff --ws-error-highlight=all | + git diff --color --ws-error-highlight=all | test_decode_color >current && test_cmp expect.all current && - git -c color.diff=always diff --ws-error-highlight=none | + git diff --color --ws-error-highlight=none | test_decode_color >current && test_cmp expect.none current @@@ -938,15 -939,15 +938,15 @@@ test_expect_success 'test diff.wsErrorHighlight config' ' - git -c color.diff=always -c diff.wsErrorHighlight=default,old diff | + git -c diff.wsErrorHighlight=default,old diff --color | test_decode_color >current && test_cmp expect.default-old current && - git -c color.diff=always -c diff.wsErrorHighlight=all diff | + git -c diff.wsErrorHighlight=all diff --color | test_decode_color >current && test_cmp expect.all current && - git -c color.diff=always -c diff.wsErrorHighlight=none diff | + git -c diff.wsErrorHighlight=none diff --color | test_decode_color >current && test_cmp expect.none current @@@ -954,18 -955,18 +954,18 @@@ test_expect_success 'option overrides diff.wsErrorHighlight' ' - git -c color.diff=always -c diff.wsErrorHighlight=none \ - diff --ws-error-highlight=default,old | + git -c diff.wsErrorHighlight=none \ + diff --color --ws-error-highlight=default,old | test_decode_color >current && test_cmp expect.default-old current && - git -c color.diff=always -c diff.wsErrorHighlight=default \ - diff --ws-error-highlight=all | + git -c diff.wsErrorHighlight=default \ + diff --color --ws-error-highlight=all | test_decode_color >current && test_cmp expect.all current && - git -c color.diff=always -c diff.wsErrorHighlight=all \ - diff --ws-error-highlight=none | + git -c diff.wsErrorHighlight=all \ + diff --color --ws-error-highlight=none | test_decode_color >current && test_cmp expect.none current @@@ -985,7 -986,7 +985,7 @@@ test_expect_success 'detect moved code git mv test.c main.c && test_config color.diff.oldMoved "normal red" && test_config color.diff.newMoved "normal green" && - git diff HEAD --color-moved=zebra --no-renames | test_decode_color >actual && + git diff HEAD --color-moved=zebra --color --no-renames | test_decode_color >actual && cat >expected <<-\EOF && diff --git a/main.c b/main.c new file mode 100644 @@@ -1086,7 -1087,7 +1086,7 @@@ test_expect_success 'detect malicious m bar(); } EOF - git diff HEAD --no-renames --color-moved=zebra| test_decode_color >actual && + git diff HEAD --no-renames --color-moved=zebra --color | test_decode_color >actual && cat <<-\EOF >expected && diff --git a/main.c b/main.c index 27a619c..7cf9336 100644 @@@ -1100,9 -1101,9 +1100,9 @@@ -{ -if (!u->is_allowed_foo) -return; - -foo(u); - -} - - + -foo(u); + -} + - int main() { foo(); @@@ -1116,11 -1117,11 +1116,11 @@@ +int secure_foo(struct user *u) +{ - +foo(u); + +foo(u); +if (!u->is_allowed_foo) +return; - +} - + + +} + + int another_function() { bar(); @@@ -1135,7 -1136,7 +1135,7 @@@ test_expect_success 'plain moved code, test_config color.diff.oldMovedAlternative "blue" && test_config color.diff.newMovedAlternative "yellow" && # needs previous test as setup - git diff HEAD --no-renames --color-moved=plain| test_decode_color >actual && + git diff HEAD --no-renames --color-moved=plain --color | test_decode_color >actual && cat <<-\EOF >expected && diff --git a/main.c b/main.c index 27a619c..7cf9336 100644 @@@ -1181,9 -1182,9 +1181,9 @@@ 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 @@@ -1194,9 -1195,9 +1194,9 @@@ 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" && @@@ -1207,12 -1208,12 +1207,12 @@@ 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 @@@ -1226,36 -1227,35 +1226,36 @@@ 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 --color | + 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,36 -1270,35 +1270,36 @@@ test_expect_success 'cmd option assume 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 --color | + 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 ' @@@ -1325,16 -1324,16 +1325,16 @@@ line 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 @@@ -1342,167 -1341,44 +1342,167 @@@ line 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 --color | + 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 --color | + 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 --color --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 --color --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 --color --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 ' @@@ -1518,7 -1394,7 +1518,7 @@@ test_expect_success 'move detection wit echo foul >bananas/recipe && echo ripe >fruit.t && - git diff --submodule=diff --color-moved >actual && + git diff --submodule=diff --color-moved --color >actual && # no move detection as the moved line is across repository boundaries. test_decode_color decoded_actual && @@@ -1526,8 -1402,17 +1526,17 @@@ ! grep BRED decoded_actual && # nor did we mess with it another way - git diff --submodule=diff | test_decode_color >expect && + git diff --submodule=diff --color | test_decode_color >expect && test_cmp expect decoded_actual ' + test_expect_success 'move detection with whitespace changes' ' + test_when_finished "git reset --hard" && + test_seq 10 >test && + git add test && + sed s/3/42/ test.tmp && + mv test.tmp test && + git -c diff.colormoved diff --ignore-space-change -- test + ' + test_done