Re: [PATCH v4 09/15] diff --color-moved: call comparison function directly

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Phillip,

On Tue, 16 Nov 2021, Phillip Wood via GitGitGadget wrote:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> This change will allow us to easily combine pmb_advance_or_null() and
> pmb_advance_or_null_multi_match() in the next commit. Calling
> xdiff_compare_lines() directly rather than using a function pointer
> from the hash map has little effect on the run time.

Good. I verified that the function `moved_entry_cmp()`
(https://github.com/gitgitgadget/git/blob/c3e5dce191/diff.c#L918-L944)
calls `xdiff_compare_lines()`, and it is this function that is used for
both `del_lines` and `add_lines` which would have been passed as `hm`:
https://github.com/gitgitgadget/git/blob/c3e5dce191/diff.c#L6339-L6340

>
> Test                                                                  HEAD^             HEAD
> -------------------------------------------------------------------------------------------------------------
> 4002.1: diff --no-color-moved --no-color-moved-ws large change        0.38(0.35+0.03)   0.38(0.32+0.06) +0.0%
> 4002.2: diff --color-moved --no-color-moved-ws large change           0.87(0.83+0.04)   0.87(0.80+0.06) +0.0%
> 4002.3: diff --color-moved-ws=allow-indentation-change large change   0.97(0.92+0.04)   0.97(0.93+0.04) +0.0%
> 4002.4: log --no-color-moved --no-color-moved-ws                      1.17(1.06+0.10)   1.16(1.10+0.05) -0.9%
> 4002.5: log --color-moved --no-color-moved-ws                         1.32(1.24+0.08)   1.31(1.22+0.09) -0.8%
> 4002.6: log --color-moved-ws=allow-indentation-change                 1.36(1.25+0.10)   1.35(1.25+0.10) -0.7%

Honestly, I would have expected an improvement, given that
`moved_entry_cmp()` has to do a few things before it can call
`xdiff_compare_lines()`.

I love your attention to detail, providing performance numbers in the
commit message to prove that it at least has no negative impact on the
speed.

Thanks,
Dscho

>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  diff.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index 78a486021ab..22e0edac173 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -994,17 +994,20 @@ static void add_lines_to_move_detection(struct diff_options *o,
>  }
>
>  static void pmb_advance_or_null(struct diff_options *o,
> -				struct moved_entry *match,
> -				struct hashmap *hm,
> +				struct emitted_diff_symbol *l,
>  				struct moved_block *pmb,
>  				int pmb_nr)
>  {
>  	int i;
> +	unsigned flags = o->color_moved_ws_handling & XDF_WHITESPACE_FLAGS;
> +
>  	for (i = 0; i < pmb_nr; i++) {
>  		struct moved_entry *prev = pmb[i].match;
>  		struct moved_entry *cur = (prev && prev->next_line) ?
>  				prev->next_line : NULL;
> -		if (cur && !hm->cmpfn(o, &cur->ent, &match->ent, NULL)) {
> +		if (cur && xdiff_compare_lines(cur->es->line, cur->es->len,
> +						l->line, l->len,
> +						flags)) {
>  			pmb[i].match = cur;
>  		} else {
>  			pmb[i].match = NULL;
> @@ -1195,7 +1198,7 @@ static void mark_color_as_moved(struct diff_options *o,
>  		    COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE)
>  			pmb_advance_or_null_multi_match(o, l, pmb, pmb_nr);
>  		else
> -			pmb_advance_or_null(o, match, hm, pmb, pmb_nr);
> +			pmb_advance_or_null(o, l, pmb, pmb_nr);
>
>  		pmb_nr = shrink_potential_moved_blocks(pmb, pmb_nr);
>
> --
> gitgitgadget
>
>