Re: [PATCH] xdiff: re-diff shifted change groups when using histogram algorithm
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 29, 2026, 20:58 UTC
- Message-ID
- <xmqqsebo9lv6.fsf@gitster.g>
- In-Reply-To
- <CAHTeOx8SOZmqvi0pkcheSjFpbEALmOwaUiX0tKLmNP7fqvjMXA@mail.gmail.com>
Yee Cheng Chin <ychin.git@gmail.com> writes:
Show 33 quoted lines
> Thanks for the review and sorry for being a little late in replying.
> Aggregating all my inline replies in one email if that's ok.
>
> On Wed, Jan 21, 2026 at 12:51 PM Junio C Hamano <gitster@pobox.com> wrote:
>> So the idea is to remember the original values in g and go (the
>> location of the group in the file and the other file) and if
>> shifting up and down changed any one of the four ends from the
>> original locations, we always take the fall-back route (if we are
>> doing histogram)?
>>
>> By the way, this appears after the if/else if/ cascade that has:
>>
>> if (g.end == earliest_end) {
>> ... do nothing case (case #1)
>> } else if (end_matching_other != -1) {
>> ... do the slide-up thing (case #2)
>> } else if (flags & XDF_INDENT_HEIRISTIC) {
>> ... do the indent heuristic thing (case #3)
>> }
>>
>> Am I reading the code correctly that, even though this new block
>> appears as if it is a post-clean-up phase that is independent from
>> which one of the three choices are taken in the previous if/elseif
>> cascade, it only is relevant to the second case? I am wondering if
>> it would make it easier to follow if the new code were made into a
>> small helper function that is called from the (case #2) arm of the
>> existing if/else if cascade.
>
> That's correct. This condition happens only in the 2nd case. The
> problematic scenario here only happens when the opposite side is
> non-empty. If the opposite is empty (case #3, where we run the indent
> heuristic algorithm), there's simply no need to re-diff anything
> because diff'ing against an empty hunk is pointless.OK. In the version posted, it appeard that it is possible, after not doing the slide-up thing but using indent heuristic thing, to fall into this compensation codepath because the new code was placed after the above if-else-if cascade as if it is an independent clean-up phase. Encapsulating that new code in a helper function and calling it at the end of "do the slide-up thing" block will make the intent clearer.
Thanks.