Re: [PATCH v3 03/15] diff --color-moved: factor out function

From: Junio C Hamano <gitster@pobox.com>
To: "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, "Phillip Wood" <phillip.wood@dunelm.org.uk>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Elijah Newren" <newren@gmail.com>,
	"Phillip Wood" <phillip.wood123@gmail.com>
Subject: Re: [PATCH v3 03/15] diff --color-moved: factor out function
Date: Thu, 28 Oct 2021 14:51:55 -0700	[thread overview]
Message-ID: <xmqqmtmsq0lg.fsf@gitster.g> (raw)
In-Reply-To: <658aec2670c78f9753a5acccab20d3a1741403e6.1635336262.git.gitgitgadget@gmail.com> (Phillip Wood via GitGitGadget's message of "Wed, 27 Oct 2021 12:04:10 +0000")

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> This code is quite heavily indented and having it in its own function
> simplifies an upcoming change.

And this should show as "moved" lines correctly in the output from
"log -p --color-moved -w"?

... not really.  There is an unfortunate artificial line wrapping in
the original, which was unwrapped by this move, so the blocks do not
exactly match.

> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  diff.c | 51 ++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 34 insertions(+), 17 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index bd8e4ec9757..09af94e018c 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -1098,6 +1098,38 @@ static int shrink_potential_moved_blocks(struct moved_block *pmb,
>  	return rp + 1;
>  }
>  
> +static void fill_potential_moved_blocks(struct diff_options *o,
> +					struct hashmap *hm,
> +					struct moved_entry *match,
> +					struct emitted_diff_symbol *l,
> +					struct moved_block **pmb_p,
> +					int *pmb_alloc_p, int *pmb_nr_p)
> +
> +{
> +	struct moved_block *pmb = *pmb_p;
> +	int pmb_alloc = *pmb_alloc_p, pmb_nr = *pmb_nr_p;
> +
> +	/*
> +	 * The current line is the start of a new block.
> +	 * Setup the set of potential blocks.
> +	 */
> +	hashmap_for_each_entry_from(hm, match, ent) {
> +		ALLOC_GROW(pmb, pmb_nr + 1, pmb_alloc);
> +		if (o->color_moved_ws_handling &
> +		    COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE) {
> +			if (compute_ws_delta(l, match->es, &(pmb[pmb_nr]).wsd))
> +				pmb[pmb_nr++].match = match;
> +		} else {
> +			pmb[pmb_nr].wsd = 0;
> +			pmb[pmb_nr++].match = match;
> +		}
> +	}
> +
> +	*pmb_p = pmb;
> +	*pmb_alloc_p = pmb_alloc;
> +	*pmb_nr_p = pmb_nr;
> +}
> +
>  /*
>   * If o->color_moved is COLOR_MOVED_PLAIN, this function does nothing.
>   *
> @@ -1198,23 +1230,8 @@ static void mark_color_as_moved(struct diff_options *o,
>  		pmb_nr = shrink_potential_moved_blocks(pmb, pmb_nr);
>  
>  		if (pmb_nr == 0) {
> -			/*
> -			 * The current line is the start of a new block.
> -			 * Setup the set of potential blocks.
> -			 */
> -			hashmap_for_each_entry_from(hm, match, ent) {
> -				ALLOC_GROW(pmb, pmb_nr + 1, pmb_alloc);
> -				if (o->color_moved_ws_handling &
> -				    COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE) {
> -					if (compute_ws_delta(l, match->es,
> -							     &pmb[pmb_nr].wsd))
> -						pmb[pmb_nr++].match = match;
> -				} else {
> -					pmb[pmb_nr].wsd = 0;
> -					pmb[pmb_nr++].match = match;
> -				}
> -			}
> -
> +			fill_potential_moved_blocks(
> +				o, hm, match, l, &pmb, &pmb_alloc, &pmb_nr);
>  			if (adjust_last_block(o, n, block_length) &&
>  			    pmb_nr && last_symbol != l->s)
>  				flipped_block = (flipped_block + 1) % 2;