From: Phillip Wood <phillip.wood123@gmail.com>
Date: Tue, 31 Mar 2026 09:43:49 GMT
Subject: Re: [PATCH v4 6/6] xdiff/xdl_cleanup_records: simplify INVESTIGATE handling for clarity
Message-ID: <40589b6f-6694-4d9c-8367-3f6352e45e7b@gmail.com>
In-Reply-To: <fd14ccafc494aeda4bb9d05b83ac09f35bec8b52.1774890003.git.gitgitgadget@gmail.com>

Hi Ezekiel

On 30/03/2026 18:00, Ezekiel Newren via GitGitGadget wrote:
> From: Ezekiel Newren <ezekielnewren@gmail.com>
> 
> Make it clear that INVESTIGATE is turned into KEEP or DISCARD based on
> the result of xdl_clean_mmatch() which reduces actionX[i] into a
> boolean value.
> 
> Signed-off-by: Ezekiel Newren <ezekielnewren@gmail.com>
> ---
>   xdiff/xprepare.c | 34 ++++++++++++++++++++++++----------
>   1 file changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/xdiff/xprepare.c b/xdiff/xprepare.c
> index 471d9567c9..1f2e8c6b4b 100644
> --- a/xdiff/xprepare.c
> +++ b/xdiff/xprepare.c
> @@ -329,24 +329,38 @@ 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))) {
> +		if (action1[i] == INVESTIGATE) {
> +			if (!xdl_clean_mmatch(action1, i, xdf1->dstart, xdf1->dend))
> +				action1[i] = KEEP;
> +			else
> +				action1[i] = DISCARD;
> +		}
> +
> +		if (action1[i] == KEEP) {
>   			xdf1->reference_index[xdf1->nreff++] = i;
> -			/* changed[i] remains false, i.e. keep */
> -		} else
> +			/* changed[i] remains false */
> +		} else if (action1[i] == DISCARD)

As one clause uses braces, they all should. Apart from that this looks 
like another nice improvement in readability.

Thanks

Phillip

>   			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))) {
> +		if (action2[i] == INVESTIGATE) {
> +			if (!xdl_clean_mmatch(action2, i, xdf2->dstart, xdf2->dend))
> +				action2[i] = KEEP;
> +			else
> +				action2[i] = DISCARD;
> +		}
> +
> +		if (action2[i] == KEEP) {
>   			xdf2->reference_index[xdf2->nreff++] = i;
> -			/* changed[i] remains false, i.e. keep */
> -		} else
> +			/* changed[i] remains false */
> +		} else if (action2[i] == DISCARD)
>   			xdf2->changed[i] = true;
> -			/* i.e. discard */
> +		else
> +			BUG("Illegal state for action2[i]");
>   	}
>   
>   cleanup: