From: Junio C Hamano Date: Thu, 29 Jan 2026 20:58:53 GMT Subject: Re: [PATCH] xdiff: re-diff shifted change groups when using histogram algorithm Message-ID: In-Reply-To: Yee Cheng Chin writes: > 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 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.