Re: [PATCH 04/10] xdiff: let patience and histogram benefit from xdl_trim_ends()
On 20/01/2026 15:02, Phillip Wood wrote:
Show 41 quoted lines
> 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
Show 42 quoted lines
> 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);
>
>