> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> On 21/01/2026 20:51, Junio C Hamano wrote:
>>> "Yee Cheng Chin via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>>
>>>> @@ -915,6 +919,45 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
>>>>    			}
>>>>    		}
>>>>    
>>>> +		/*
>>>> +		 * If this has a matching group from the other file, it could
>>>> +		 * either be the original match from the diff algorithm, or
>>>> +		 * arrived at by shifting and joining groups. When it's the
>>>> +		 * latter, it's possible for the two newly joined sides to have
>>>> +		 * matching lines. Re-diff the group to mark these matching
>>>> +		 * lines as unchanged and remove from the diff output.
> 
> Also, after reading the first paragraph of the big comment again, it
> makes me wonder if it is saying the same thing as "When histogram is
> being used, we shouldn't bother shifting up and down to join groups,
> as the result will always worse than the fallback", but is it that
> bad?

>>>> +		if (end_matching_other != -1 &&
>>>> +				XDF_DIFF_ALG(flags) == XDF_HISTOGRAM_DIFF &&
>>>> +				(g.start != g_orig.start ||
>>>> +				 g.end != g_orig.end ||
>>>> +				 go.start != go_orig.start ||
>>>> +				 go.end != go_orig.end)) {
>>>
>>> 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)?
>>
>> I'm a bit confused why we need to check both groups. I think they're
>> supposed to move together (if we move "g" by n context lines we also
>> move "go" by n context lines) so I can't see how we can have
>>
>> 	g.start == g_orig.start && g.end == g_orig.end
>>
>> when
>>
>> 	go.start != go.orig.start || go.end != go_orig.end
> 
> Interesting.
> 
>>> 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 a good point
>>
>>>> +			xpparam_t xpp;
>>>> +			xdfenv_t xe;
>>>> +
>>>> +			memset(&xpp, 0, sizeof(xpp));
>>>> +			xpp.flags = flags & ~XDF_DIFF_ALGORITHM_MASK;
>>>> +
>>>> +			memcpy(&xe.xdf1, xdf, sizeof(xdfile_t));
>>>> +			memcpy(&xe.xdf2, xdfo, sizeof(xdfile_t));
>>
>> These would be safer as "xe.xdf1 = *xdf" so we don't have to worry about
>> getting the size correct (sizeof(*xdf) would also be safer but there is
>> no need for memcpy() here).
> 
> Very good readability enhancement suggestion.
> 
> Thanks.