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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Sep 24, 2025, 15:18 UTC
Message-ID: <1c3461c3-09bb-404d-a4dc-a895baba68f2@gmail.com>
In-Reply-To: <CAH=ZcbBcEbTqQ-FpYSdr_QjSM5sKerECKsSVdTyHoSQqG-V9iA@mail.gmail.com>

On 24/09/2025 15:46, Ezekiel Newren wrote:

Show 20 quoted lines

> On Wed, Sep 24, 2025 at 4:21 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> On 23/09/2025 22:24, Ezekiel Newren via GitGitGadget wrote:
>>> From: Ezekiel Newren <ezekielnewren@gmail.com>
>>>
>>> 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.

Show 11 quoted lines

>>>        /*
>>> -      * 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.

Show 13 quoted lines

>>
>>> +     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