Re: [PATCH v4 6/6] xdiff/xdl_cleanup_records: simplify INVESTIGATE handling for clarity

[Phillip Wood <phillip.wood123@gmail.com>]

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.

This patch changes the diff output. If I compare the output of

     git log --diff-merges=1 --diff-algorithm=myers -n1000 origin/master

with git built from 702083c4820 (xdiff/xdl_cleanup_records: make setting
action easier to follow, 2026-03-30) and from 7ff1460b62f (xdiff/
xdl_cleanup_records: simplify INVESTIGATE handling for clarity,
2026-03-30) I see the diff below. I think the problem is that
xdl_clean_mmatch() depends on the action arrays and this patch modifies
them because it converts INVESTIGATE to KEEP or DISCARD. We can probably
work around that by using a local variable rather than modifing
actionX[i].

Thanks

Phillip

diff --git a/tmp/p-sub-KADADDFP b/tmp/p-sub-PDCNCAOG
--- a/tmp/p-sub-KADADDFP
+++ b/tmp/p-sub-PDCNCAOG
@@ -4258,14 +4258,15 @@ index 6485cb67068..0ff2e45aa7a 100644
   		ctx.progress = NULL;

  -	ctx.to_include = packs_to_include;
+-
+-	for_each_file_in_pack_dir(source->path, add_pack_to_midx, &ctx);
  +	if (ctx.compact) {
  +		int bitmap_order = 0;
  +		if (opts->preferred_pack_name)
  +			bitmap_order |= 1;
  +		else if (opts->flags & (MIDX_WRITE_REV_INDEX | MIDX_WRITE_BITMAP))
  +			bitmap_order |= 1;
-
--	for_each_file_in_pack_dir(source->path, add_pack_to_midx, &ctx);
++
  +		fill_packs_from_midx_range(&ctx, bitmap_order);
  +	} else {
  +		ctx.to_include = opts->packs_to_include;
@@ -50839,7 +50840,8 @@ index 41b0750e5af..acaf42b2d93 100644
  -
  -	if (opts->in_place)
  -		outfile = create_in_place_tempfile(file);
--
++	read_input_file(&input, file);
+
  -	trailer_block = parse_trailers(opts, sb.buf, &head);
  -
  -	/* Print the lines before the trailer block */
@@ -50848,8 +50850,7 @@ index 41b0750e5af..acaf42b2d93 100644
  -
  -	if (!opts->only_trailers && !blank_line_before_trailer_block(trailer_block))
  -		fprintf(outfile, "\n");
-+	read_input_file(&input, file);
-
+-
  -
  -	if (!opts->only_input) {
  -		LIST_HEAD(config_head);
@@ -58880,12 +58881,12 @@ index 776de5356c9..84a31084d38 100644
   	odb_prepare_alternates(odb);
  -	for (source = odb->sources; source; source = source->next) {
  -		if (packfile_store_freshen_object(source->packfiles, oid))
+-			return 1;
+-
+-		if (odb_source_loose_freshen_object(source, oid))
  +	for (source = odb->sources; source; source = source->next)
  +		if (odb_source_freshen_object(source, oid))

Thanks

Phillip

> 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)
>   			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: