Skip to content

Commit 25e6190

Browse files
phillipwoodgitster
phillipwood
authored and
gitster
committed
diff --color-moved-ws=allow-indentation-change: improve hash lookups
As libxdiff does not have a whitespace flag to ignore the indentation the code for --color-moved-ws=allow-indentation-change uses XDF_IGNORE_WHITESPACE and then filters out any hash lookups where there are non-indentation changes. This filtering is inefficient as we have to perform another string comparison. By using the offset data that we have already computed to skip the indentation we can avoid using XDF_IGNORE_WHITESPACE and safely remove the extra checks which improves the performance by 11% and paves the way for the elimination of string comparisons in the next commit. This change slightly increases the run time of other --color-moved modes. This could be avoided by using different comparison functions for the different modes but after the next two commits there is no measurable benefit in doing so. There is a change in behavior for lines that begin with a form-feed or vertical-tab character. Since b46054b ("xdiff: use git-compat-util", 2019-04-11) xdiff does not treat '\f' or '\v' as whitespace characters. This means that lines starting with those characters are never considered to be blank and never match a line that does not start with the same character. After this patch a line matching "^[\f\v\r]*[ \t]*$" is considered to be blank by --color-moved-ws=allow-indentation-change and lines beginning "^[\f\v\r]*[ \t]*" can match another line if the suffixes match. This changes the output of git show for d18f76d ("compat/regex: use the regex engine from gawk for compat", 2010-08-17) as some lines in the pre-image before a moved block that contain '\f' are now considered moved as well as they match a blank line before the moved lines in the post-image. This commit updates one of the tests to reflect this change. Test HEAD^ HEAD -------------------------------------------------------------------------------------------------------------- 4002.1: diff --no-color-moved --no-color-moved-ws large change 0.38(0.33+0.05) 0.38(0.33+0.05) +0.0% 4002.2: diff --color-moved --no-color-moved-ws large change 0.86(0.82+0.04) 0.88(0.84+0.04) +2.3% 4002.3: diff --color-moved-ws=allow-indentation-change large change 0.97(0.94+0.03) 0.86(0.81+0.05) -11.3% 4002.4: log --no-color-moved --no-color-moved-ws 1.16(1.07+0.09) 1.16(1.06+0.09) +0.0% 4002.5: log --color-moved --no-color-moved-ws 1.32(1.26+0.06) 1.33(1.27+0.05) +0.8% 4002.6: log --color-moved-ws=allow-indentation-change 1.35(1.29+0.06) 1.33(1.24+0.08) -1.5% Signed-off-by: Phillip Wood <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
1 parent eec7f53 commit 25e6190

File tree

2 files changed

+30
-57
lines changed

2 files changed

+30
-57
lines changed

‎diff.c

Lines changed: 19 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -850,28 +850,15 @@ static void fill_es_indent_data(struct emitted_diff_symbol *es)
850850
}
851851

852852
static int compute_ws_delta(const struct emitted_diff_symbol *a,
853-
const struct emitted_diff_symbol *b,
854-
int *out)
855-
{
856-
int a_len = a->len,
857-
b_len = b->len,
858-
a_off = a->indent_off,
859-
a_width = a->indent_width,
860-
b_off = b->indent_off,
853+
const struct emitted_diff_symbol *b)
854+
{
855+
int a_width = a->indent_width,
861856
b_width = b->indent_width;
862857

863-
if (a_width == INDENT_BLANKLINE && b_width == INDENT_BLANKLINE) {
864-
*out = INDENT_BLANKLINE;
865-
return 1;
866-
}
867-
868-
if (a_len - a_off != b_len - b_off ||
869-
memcmp(a->line + a_off, b->line + b_off, a_len - a_off))
870-
return 0;
871-
872-
*out = a_width - b_width;
858+
if (a_width == INDENT_BLANKLINE && b_width == INDENT_BLANKLINE)
859+
return INDENT_BLANKLINE;
873860

874-
return 1;
861+
return a_width - b_width;
875862
}
876863

877864
static int cmp_in_block_with_wsd(const struct moved_entry *cur,
@@ -916,26 +903,17 @@ static int moved_entry_cmp(const void *hashmap_cmp_fn_data,
916903
const void *keydata)
917904
{
918905
const struct diff_options *diffopt = hashmap_cmp_fn_data;
919-
const struct moved_entry *a, *b;
906+
const struct emitted_diff_symbol *a, *b;
920907
unsigned flags = diffopt->color_moved_ws_handling
921908
& XDF_WHITESPACE_FLAGS;
922909

923-
a = container_of(eptr, const struct moved_entry, ent);
924-
b = container_of(entry_or_key, const struct moved_entry, ent);
910+
a = container_of(eptr, const struct moved_entry, ent)->es;
911+
b = container_of(entry_or_key, const struct moved_entry, ent)->es;
925912

926-
if (diffopt->color_moved_ws_handling &
927-
COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE)
928-
/*
929-
* As there is not specific white space config given,
930-
* we'd need to check for a new block, so ignore all
931-
* white space. The setup of the white space
932-
* configuration for the next block is done else where
933-
*/
934-
flags |= XDF_IGNORE_WHITESPACE;
935-
936-
return !xdiff_compare_lines(a->es->line, a->es->len,
937-
b->es->line, b->es->len,
938-
flags);
913+
return !xdiff_compare_lines(a->line + a->indent_off,
914+
a->len - a->indent_off,
915+
b->line + b->indent_off,
916+
b->len - b->indent_off, flags);
939917
}
940918

941919
static struct moved_entry *prepare_entry(struct diff_options *o,
@@ -944,7 +922,8 @@ static struct moved_entry *prepare_entry(struct diff_options *o,
944922
struct moved_entry *ret = xmalloc(sizeof(*ret));
945923
struct emitted_diff_symbol *l = &o->emitted_symbols->buf[line_no];
946924
unsigned flags = o->color_moved_ws_handling & XDF_WHITESPACE_FLAGS;
947-
unsigned int hash = xdiff_hash_string(l->line, l->len, flags);
925+
unsigned int hash = xdiff_hash_string(l->line + l->indent_off,
926+
l->len - l->indent_off, flags);
948927

949928
hashmap_entry_init(&ret->ent, hash);
950929
ret->es = l;
@@ -1036,13 +1015,11 @@ static void fill_potential_moved_blocks(struct diff_options *o,
10361015
hashmap_for_each_entry_from(hm, match, ent) {
10371016
ALLOC_GROW(pmb, pmb_nr + 1, pmb_alloc);
10381017
if (o->color_moved_ws_handling &
1039-
COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE) {
1040-
if (compute_ws_delta(l, match->es, &(pmb[pmb_nr]).wsd))
1041-
pmb[pmb_nr++].match = match;
1042-
} else {
1018+
COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE)
1019+
pmb[pmb_nr].wsd = compute_ws_delta(l, match->es);
1020+
else
10431021
pmb[pmb_nr].wsd = 0;
1044-
pmb[pmb_nr++].match = match;
1045-
}
1022+
pmb[pmb_nr++].match = match;
10461023
}
10471024

10481025
*pmb_p = pmb;
@@ -6276,10 +6253,6 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o)
62766253
if (o->color_moved) {
62776254
struct hashmap add_lines, del_lines;
62786255

6279-
if (o->color_moved_ws_handling &
6280-
COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE)
6281-
o->color_moved_ws_handling |= XDF_IGNORE_WHITESPACE;
6282-
62836256
hashmap_init(&del_lines, moved_entry_cmp, o, 0);
62846257
hashmap_init(&add_lines, moved_entry_cmp, o, 0);
62856258

‎t/t4015-diff-whitespace.sh

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2206,10 +2206,10 @@ EMPTY=''
22062206
test_expect_success 'compare mixed whitespace delta across moved blocks' '
22072207
22082208
git reset --hard &&
2209-
tr Q_ "\t " <<-EOF >text.txt &&
2210-
${EMPTY}
2211-
____too short without
2212-
${EMPTY}
2209+
tr "^|Q_" "\f\v\t " <<-EOF >text.txt &&
2210+
^__
2211+
|____too short without
2212+
^
22132213
___being grouped across blank line
22142214
${EMPTY}
22152215
context
@@ -2228,7 +2228,7 @@ test_expect_success 'compare mixed whitespace delta across moved blocks' '
22282228
git add text.txt &&
22292229
git commit -m "add text.txt" &&
22302230
2231-
tr Q_ "\t " <<-EOF >text.txt &&
2231+
tr "^|Q_" "\f\v\t " <<-EOF >text.txt &&
22322232
context
22332233
lines
22342234
to
@@ -2239,7 +2239,7 @@ test_expect_success 'compare mixed whitespace delta across moved blocks' '
22392239
${EMPTY}
22402240
QQtoo short without
22412241
${EMPTY}
2242-
Q_______being grouped across blank line
2242+
^Q_______being grouped across blank line
22432243
${EMPTY}
22442244
Q_QThese two lines have had their
22452245
indentation reduced by four spaces
@@ -2251,16 +2251,16 @@ test_expect_success 'compare mixed whitespace delta across moved blocks' '
22512251
-c core.whitespace=space-before-tab \
22522252
diff --color --color-moved --ws-error-highlight=all \
22532253
--color-moved-ws=allow-indentation-change >actual.raw &&
2254-
grep -v "index" actual.raw | test_decode_color >actual &&
2254+
grep -v "index" actual.raw | tr "\f\v" "^|" | test_decode_color >actual &&
22552255
22562256
cat <<-\EOF >expected &&
22572257
<BOLD>diff --git a/text.txt b/text.txt<RESET>
22582258
<BOLD>--- a/text.txt<RESET>
22592259
<BOLD>+++ b/text.txt<RESET>
22602260
<CYAN>@@ -1,16 +1,16 @@<RESET>
2261-
<BOLD;MAGENTA>-<RESET>
2262-
<BOLD;MAGENTA>-<RESET><BOLD;MAGENTA> too short without<RESET>
2263-
<BOLD;MAGENTA>-<RESET>
2261+
<BOLD;MAGENTA>-<RESET><BOLD;MAGENTA>^<RESET><BRED> <RESET>
2262+
<BOLD;MAGENTA>-<RESET><BOLD;MAGENTA>| too short without<RESET>
2263+
<BOLD;MAGENTA>-<RESET><BOLD;MAGENTA>^<RESET>
22642264
<BOLD;MAGENTA>-<RESET><BOLD;MAGENTA> being grouped across blank line<RESET>
22652265
<BOLD;MAGENTA>-<RESET>
22662266
<RESET>context<RESET>
@@ -2280,7 +2280,7 @@ test_expect_success 'compare mixed whitespace delta across moved blocks' '
22802280
<BOLD;YELLOW>+<RESET>
22812281
<BOLD;YELLOW>+<RESET> <BOLD;YELLOW>too short without<RESET>
22822282
<BOLD;YELLOW>+<RESET>
2283-
<BOLD;YELLOW>+<RESET> <BOLD;YELLOW> being grouped across blank line<RESET>
2283+
<BOLD;YELLOW>+<RESET><BOLD;YELLOW>^ being grouped across blank line<RESET>
22842284
<BOLD;YELLOW>+<RESET>
22852285
<BOLD;CYAN>+<RESET> <BRED> <RESET> <BOLD;CYAN>These two lines have had their<RESET>
22862286
<BOLD;CYAN>+<RESET><BOLD;CYAN>indentation reduced by four spaces<RESET>

0 commit comments

Comments
 (0)