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

[Phillip Wood <phillip.wood123@xxxxxxxxx>]

Hi Junio

On 28/10/2021 22:51, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
> > From: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
> >
> > 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"?

rather that "-w" one can use "--color-moved-ws=allow-indentation-change" 
to highlight moved lines where the indentation has changed. It took me a 
while to realize why "-w" does not do anything here but it is because 
the lines are moved as well as having their indentation changed.

> ... 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.

Yes that's a shame, it seemed overkill to have one commit moving the 
code as is and then another reformatting it. All of the moved lines 
apart from the one that is unwrapped are highlighted with 
"--color-moved-ws=allow-indentation-change".

Best Wishes

Phillip

> > Signed-off-by: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
> > ---
> >   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;