From: Phillip Wood <phillip.wood123@gmail.com>
Date: Tue, 20 Jan 2026 15:02:40 GMT
Subject: Re: [PATCH 04/10] xdiff: let patience and histogram benefit from xdl_trim_ends()
Message-ID: <f17adb7a-8776-42b3-b753-f6306145250a@gmail.com>
In-Reply-To: <70040ea1351451243be90d59d26cf1a403f3000a.1767379944.git.gitgitgadget@gmail.com>

On 02/01/2026 18:52, Ezekiel Newren via GitGitGadget wrote:
> From: Ezekiel Newren <ezekielnewren@gmail.com>
> 
> The patience diff is set up the exact same way as histogram, see
> xdl_do_historgram_diff() in xhistogram.c. xdl_optimize_ctxs() is
> redundant now, delete it.

Does this change the output? The patience diff looks for unique context 
lines and builds the context out from those. For files that look like

Old	New
A	A
B	B
C	A
B	B
A	C
	B
	A

That will give a hunk

@@ -1,3 +0,5 @@
+A
+B
  A
  B
  C

but trimming the common prefix first would give

@@ -1,5 +1,7
  A
  B
+A
+B
  C
  B
  A

Though it seems like the diff silder causes us to output the same diff 
in both cases for that simple test so maybe it is not an issue. It would 
certainly be helpful to comment on any possible changes in the commit 
message as it could have been a deliberate choice not to trim the ends 
for those algorithms.

> -static int xdl_optimize_ctxs(xdlclassifier_t *cf, xdfile_t *xdf1, xdfile_t *xdf2) {
> -
> -	if (xdl_trim_ends(xdf1, xdf2) < 0 ||
> -	    xdl_cleanup_records(cf, xdf1, xdf2) < 0) {
> -
> -		return -1;
> -	}
> -
> -	return 0;
> -}
> -
>   int xdl_prepare_env(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
>   		    xdfenv_t *xe) {
>   	xdlclassifier_t cf;
> @@ -404,9 +393,10 @@ int xdl_prepare_env(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
>   		xdl_classify_record(2, &cf, rec);
>   	}
>   
> +	xdl_trim_ends(&xe->xdf1, &xe->xdf2);

It would be clear that this was safe if you changed the function 
signature to return void as the way it is called in xdl_optimize_ctxs() 
makes it look like it can return an error.

Thanks

Phillip

>   	if ((XDF_DIFF_ALG(xpp->flags) != XDF_PATIENCE_DIFF) &&
>   	    (XDF_DIFF_ALG(xpp->flags) != XDF_HISTOGRAM_DIFF) &&
> -	    xdl_optimize_ctxs(&cf, &xe->xdf1, &xe->xdf2) < 0) {
> +	    xdl_cleanup_records(&cf, &xe->xdf1, &xe->xdf2) < 0) {
>   
>   		xdl_free_ctx(&xe->xdf2);
>   		xdl_free_ctx(&xe->xdf1);