From: Phillip Wood <phillip.wood123@gmail.com>
Date: Wed, 21 Jan 2026 14:49:58 GMT
Subject: Re: [PATCH 04/10] xdiff: let patience and histogram benefit from xdl_trim_ends()
Message-ID: <6d533cfd-d308-4004-8d8e-4ae730c76086@gmail.com>
In-Reply-To: <f17adb7a-8776-42b3-b753-f6306145250a@gmail.com>

On 20/01/2026 15:02, Phillip Wood wrote:
> 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 does change larger diffs. If you run

git show --diff-algorithm=patience --diff-merges=first-parent f406b89552

You get a different diff with this series applied.

Thanks

Phillip

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