Subject: Re: [PATCH v5 0/6] Xdiff cleanup part 3

From: Phillip Wood <[email-masked]>
Date: Tue, 14 Apr 2026 11:08:11 +0100

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

Phillip

[1] https://lore.kernel.org/git/[email-masked]
[2] https://lore.kernel.org/git/[email-masked]
[3] https://lore.kernel.org/git/[email-masked]

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