Subject: Re: [PATCH v5 12/13] xdiff: use enum macros NONE(0), SOME(1), TOO_MANY(2) in xprepare.c

From: Phillip Wood <[email-masked]>
Date: Wed, 24 Sep 2025 16:18:28 +0100

On 24/09/2025 15:46, Ezekiel Newren wrote:
> On Wed, Sep 24, 2025 at 4:21 AM Phillip Wood <[email-masked]> wrote:
>>
>> On 23/09/2025 22:24, Ezekiel Newren via GitGitGadget wrote:
>>> From: Ezekiel Newren <[email-masked]>
>>>
>>> Rename dis1, dis2 to matches1, matches2.
>>>
>>> Define macros NONE(0), SOME(1), TOO_MANY(2) as the enum values for
>>> matches1 and matches2. These states will influence whether changed[i]
>>> is set to 1 or kept as 0.
>>
>> This message also says what is being changed rather than why it is being
>> changed. I think the rename here is a good idea but I'm not sure what
>> "rdis[01]" and "rpdis[01]" are used for and whether they should be
>> renamed if we're renaming "dis[01]"
> 
> "Rename dis1, dis2 to matches1, matches2 to give the variable names a
> more obvious meaning."
>
> Would something like that work, or do I need to refine it further?

I'd maybe add a sentence before that to explain that "dis1 and dis2 are 
used to record if a line has zero, one or many matches on the other side 
of the diff". I don't think any of these patches need huge commit 
messages but a couple of sentences explaining the reasoning would be 
helpful for anyone looking at them it the future.

> I
> would love to rename rdis, rpdis, etc... except that I don't
> understand what is happening or why. Could someone explain the purpose
> of these variables?

Good question, I'm not sure anyone has an intimate knowledge of this 
code. My understanding is that the code aims to remove runs of common 
lines when they occur between unique lines in order to reduce the number 
of lines we need to look at when we're calculating the diff. I haven't 
worked through the code in detail though.

>>>        /*
>>> -      * Limits the window the is examined during the similar-lines
>>> -      * scan. The loops below stops when dis[i - r] == 1 (line that
>>> +      * Limits the window that is examined during the similar-lines
>>> +      * scan. The loops below stops when matches[i - r] == SOME (line that
>>
>> Thanks for updating the comments. Not reflowing the lines makes the diff
>> easier to read but leaves the comments in a rather strange state with
>> random long lines.
> 
> What is the reflow limit for comments? 72? 80?

For code it's 80 columns with a little leaway if that makes things 
clearer. For comments I'd match whatever it is using at the moment.

>>
>>> +     bool need_min = !!(cf->flags & XDF_NEED_MINIMAL);
>>
>> Nice use of bool, strictly speaking I don't think we need the !! if
>> we're changing the type from int. I think Junio recently suggested that
>> we might start using (bool) instead of !! for cases like this.
>>
>> Everything below this looks good, though some of the lines are getting
>> quite long with the renamed variables and symbolic values so we might
>> want to break them.
> 
> I didn't add !! and thought it looked funny myself. I didn't remove it
> because I wasn't sure if I should.

Our coding guidelines say not to use "!!x" (I assume we're supposed to 
do "x != 0" instead) but in practice it's pretty common to see it in our 
codebase. I'd maybe try a (bool) cast and see what people say.

Thanks for cleaning up the xdiff code, it is much appreciated

Phillip