From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 14 Apr 2026 17:06:38 GMT
Subject: Re: [PATCH v5 0/6] Xdiff cleanup part 3
Message-ID: <xmqqeckhbhep.fsf@gitster.g>
In-Reply-To: <df244360-e9a9-44c0-946d-29288e6dd269@gmail.com>

Phillip Wood <phillip.wood123@gmail.com> writes:

> On 08/04/2026 21:26, Ezekiel Newren via GitGitGadget wrote:
>> Changes in v5:
>> 
>>   * drop commit "xdiff/xdl_cleanup_records: simplify INVESTIGATE handling for
>>     clarity".
>>   * add braces around the else clause
>> 
>> I didn't see a better way to rewrite how action is used so I reverted to
>> what it used to be.
>
> That's a shame, the diff below uses a local variable to avoid altering the
> arrays as suggested in [1]. The comments about the double evaluation of
> xdl_bogosort() in patch 4 [2,3] also seem to have been overlooked.

Thanks for keeping an eye on this topic.  Very much appreciated.

>
> Thanks
>
> Phillip
>
> [1] https://lore.kernel.org/git/87a54698-396d-4de8-bd9d-cd72f8d1e8df@gmail.com
> [2] https://lore.kernel.org/git/32c34d0d-9358-43e3-9d58-5999b3ffd6c2@gmail.com
> [3] https://lore.kernel.org/git/xmqqcy0oj2s1.fsf@gitster.g
>
> ---- 8< ----
> diff --git a/xdiff/xprepare.c b/xdiff/xprepare.c
> index 471d9567c9..9966b4715d 100644
> --- a/xdiff/xprepare.c
> +++ b/xdiff/xprepare.c
> @@ -329,24 +329,42 @@ static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t *xdf1, xdfile_t *xd
>   	 */
>   	xdf1->nreff = 0;
>   	for (i = xdf1->dstart; i <= xdf1->dend; i++) {
> -		if (action1[i] == KEEP ||
> -		    (action1[i] == INVESTIGATE && !xdl_clean_mmatch(action1, i, xdf1->dstart, xdf1->dend))) {
> +		uint8_t action = action1[i];
> +
> +		if (action == INVESTIGATE) {
> +			if (!xdl_clean_mmatch(action1, i, xdf1->dstart, xdf1->dend))
> +				action = KEEP;
> +			else
> +				action = DISCARD;
> +		}
> +
> +		if (action == KEEP) {
>   			xdf1->reference_index[xdf1->nreff++] = i;
> -			/* changed[i] remains false, i.e. keep */
> -		} else
> +			/* changed[i] remains false */
> +		} else if (action == DISCARD)
>   			xdf1->changed[i] = true;
> -			/* i.e. discard */
> +		else
> +			BUG("Illegal state for action1[i]");
>   	}
>
>   	xdf2->nreff = 0;
>   	for (i = xdf2->dstart; i <= xdf2->dend; i++) {
> -		if (action2[i] == KEEP ||
> -		    (action2[i] == INVESTIGATE && !xdl_clean_mmatch(action2, i, xdf2->dstart, xdf2->dend))) {
> +		uint8_t action = action2[i];
> +
> +		if (action == INVESTIGATE) {
> +			if (!xdl_clean_mmatch(action2, i, xdf2->dstart, xdf2->dend))
> +				action = KEEP;
> +			else
> +				action = DISCARD;
> +		}
> +
> +		if (action == KEEP) {
>   			xdf2->reference_index[xdf2->nreff++] = i;
> -			/* changed[i] remains false, i.e. keep */
> -		} else
> +			/* changed[i] remains false */
> +		} else if (action == DISCARD)
>   			xdf2->changed[i] = true;
> -			/* i.e. discard */
> +		else
> +			BUG("Illegal state for action2[i]");
>   	}
>
>   cleanup: