From: Phillip Wood <phillip.wood123@gmail•com>
To: Ezekiel Newren <ezekielnewren@gmail•com>
Cc: Ezekiel Newren via GitGitGadget <gitgitgadget@gmail•com>,
git@vger•kernel.org, Elijah Newren <newren@gmail•com>,
Ben Knoble <ben.knoble@gmail•com>, Jeff King <peff@peff•net>
Subject: Re: [PATCH v5 12/13] xdiff: use enum macros NONE(0), SOME(1), TOO_MANY(2) in xprepare.c
Date: Wed, 24 Sep 2025 16:18:28 +0100 [thread overview]
Message-ID: <1c3461c3-09bb-404d-a4dc-a895baba68f2@gmail.com> (raw)
In-Reply-To: <CAH=ZcbBcEbTqQ-FpYSdr_QjSM5sKerECKsSVdTyHoSQqG-V9iA@mail.gmail.com>
On 24/09/2025 15:46, Ezekiel Newren wrote:
> 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.
>>> /*
>>> - * 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
next prev parent reply other threads:[~2025-09-24 15:18 UTC|newest]
Thread overview: 158+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-09-07 19:45 [PATCH 00/17] Use rust types in xdiff Ezekiel Newren via GitGitGadget
2025-09-07 19:45 ` [PATCH 01/17] xdiff: delete static forward declarations in xprepare Ezekiel Newren via GitGitGadget
2025-09-09 8:55 ` Elijah Newren
2025-09-07 19:45 ` [PATCH 02/17] xdiff: delete local variables and initialize/free xdfile_t directly Ezekiel Newren via GitGitGadget
2025-09-09 8:56 ` Elijah Newren
2025-09-07 19:45 ` [PATCH 03/17] xdiff: delete unnecessary fields from xrecord_t and xdfile_t Ezekiel Newren via GitGitGadget
2025-09-09 8:56 ` Elijah Newren
2025-09-07 19:45 ` [PATCH 04/17] xdiff: delete xdl_get_rec() in xemit Ezekiel Newren via GitGitGadget
2025-09-09 8:56 ` Elijah Newren
2025-09-07 19:45 ` [PATCH 05/17] xdiff: delete struct diffdata_t Ezekiel Newren via GitGitGadget
2025-09-09 8:56 ` Elijah Newren
2025-09-07 19:45 ` [PATCH 06/17] xdiff: delete redundant array xdfile_t.ha Ezekiel Newren via GitGitGadget
2025-09-09 8:57 ` Elijah Newren
2025-09-07 19:45 ` [PATCH 07/17] xdiff: delete fields ha, line, size in xdlclass_t in favor of an xrecord_t Ezekiel Newren via GitGitGadget
2025-09-09 8:57 ` Elijah Newren
2025-09-07 19:45 ` [PATCH 08/17] xdiff: delete chastore from xdfile_t, view with --color-words Ezekiel Newren via GitGitGadget
2025-09-09 8:58 ` Elijah Newren
2025-09-09 13:50 ` Phillip Wood
2025-09-09 20:33 ` Junio C Hamano
2025-09-10 22:02 ` Ben Knoble
2025-09-07 19:45 ` [PATCH 09/17] xdiff: treat xdfile_t.rchg like an enum Ezekiel Newren via GitGitGadget
2025-09-09 8:58 ` Elijah Newren
2025-09-07 19:45 ` [PATCH 10/17] compat/rust_types.h: define rust primitive types Ezekiel Newren via GitGitGadget
2025-09-08 15:08 ` Junio C Hamano
2025-09-08 16:15 ` Ezekiel Newren
2025-09-07 19:45 ` [PATCH 11/17] xdiff: include compat/rust_types.h Ezekiel Newren via GitGitGadget
2025-09-07 19:45 ` [PATCH 12/17] xdiff: make xrecord_t.ptr a u8 instead of char Ezekiel Newren via GitGitGadget
2025-09-07 19:45 ` [PATCH 13/17] xdiff: make xrecord_t.size a usize instead of long Ezekiel Newren via GitGitGadget
2025-09-07 19:45 ` [PATCH 14/17] xdiff: split xrecord_t.ha into line_hash and minimal_perfect_hash Ezekiel Newren via GitGitGadget
2025-09-07 19:45 ` [PATCH 15/17] xdiff: make xdfile_t.nrec a usize instead of long Ezekiel Newren via GitGitGadget
2025-09-07 19:45 ` [PATCH 16/17] xdiff: make xdfile_t.nreff " Ezekiel Newren via GitGitGadget
2025-09-07 19:45 ` [PATCH 17/17] xdiff: change the types of dstart, dend, rchg, and rindex in xdfile_t Ezekiel Newren via GitGitGadget
2025-09-16 21:56 ` [PATCH 00/17] Use rust types in xdiff Junio C Hamano
2025-09-16 22:01 ` Ezekiel Newren
2025-09-17 2:16 ` Elijah Newren
2025-09-17 13:53 ` Junio C Hamano
2025-09-17 6:22 ` Junio C Hamano
2025-09-18 23:56 ` [PATCH v2 00/10] " Ezekiel Newren via GitGitGadget
2025-09-18 23:56 ` [PATCH v2 01/10] xdiff: delete static forward declarations in xprepare Ezekiel Newren via GitGitGadget
2025-09-18 23:56 ` [PATCH v2 02/10] xdiff: delete local variables and initialize/free xdfile_t directly Ezekiel Newren via GitGitGadget
2025-09-18 23:56 ` [PATCH v2 03/10] xdiff: delete unnecessary fields from xrecord_t and xdfile_t Ezekiel Newren via GitGitGadget
2025-09-18 23:56 ` [PATCH v2 04/10] xdiff: delete xdl_get_rec() in xemit Ezekiel Newren via GitGitGadget
2025-09-18 23:56 ` [PATCH v2 05/10] xdiff: delete struct diffdata_t Ezekiel Newren via GitGitGadget
2025-09-18 23:56 ` [PATCH v2 06/10] xdiff: delete redundant array xdfile_t.ha Ezekiel Newren via GitGitGadget
2025-09-18 23:56 ` [PATCH v2 07/10] xdiff: delete fields ha, line, size in xdlclass_t in favor of an xrecord_t Ezekiel Newren via GitGitGadget
2025-09-18 23:56 ` [PATCH v2 08/10] xdiff: delete chastore from xdfile_t Ezekiel Newren via GitGitGadget
2025-09-18 23:56 ` [PATCH v2 10/10] xdiff: treat xdfile_t.rchg like an enum Ezekiel Newren via GitGitGadget
2025-09-19 0:33 ` [PATCH v2 00/10] Use rust types in xdiff Junio C Hamano
2025-09-19 0:41 ` Ezekiel Newren
2025-09-19 15:15 ` Ezekiel Newren
2025-09-19 15:16 ` [PATCH v3 00/10] Cleanup xdfile_t and xrecord_t " Ezekiel Newren via GitGitGadget
2025-09-19 15:16 ` [PATCH v3 01/10] xdiff: delete static forward declarations in xprepare Ezekiel Newren via GitGitGadget
2025-09-20 17:16 ` Junio C Hamano
2025-09-20 17:41 ` Ezekiel Newren
2025-09-20 18:31 ` Elijah Newren
2025-09-20 22:25 ` Ben Knoble
2025-09-20 22:43 ` Junio C Hamano
2025-09-20 17:46 ` Ben Knoble
2025-09-20 18:46 ` Jeff King
2025-09-20 22:25 ` Ben Knoble
2025-09-20 22:52 ` Junio C Hamano
2025-09-20 23:15 ` Jeff King
2025-09-19 15:16 ` [PATCH v3 02/10] xdiff: delete local variables and initialize/free xdfile_t directly Ezekiel Newren via GitGitGadget
2025-09-20 17:36 ` Junio C Hamano
2025-09-19 15:16 ` [PATCH v3 03/10] xdiff: delete unnecessary fields from xrecord_t and xdfile_t Ezekiel Newren via GitGitGadget
2025-09-19 15:16 ` [PATCH v3 04/10] xdiff: delete xdl_get_rec() in xemit Ezekiel Newren via GitGitGadget
2025-09-20 17:48 ` Junio C Hamano
2025-09-21 13:06 ` Phillip Wood
2025-09-21 15:07 ` Ezekiel Newren
2025-09-19 15:16 ` [PATCH v3 05/10] xdiff: delete struct diffdata_t Ezekiel Newren via GitGitGadget
2025-09-21 13:06 ` Phillip Wood
2025-09-21 16:03 ` Ezekiel Newren
2025-09-19 15:16 ` [PATCH v3 06/10] xdiff: delete redundant array xdfile_t.ha Ezekiel Newren via GitGitGadget
2025-09-19 15:16 ` [PATCH v3 07/10] xdiff: delete fields ha, line, size in xdlclass_t in favor of an xrecord_t Ezekiel Newren via GitGitGadget
2025-09-21 13:06 ` Phillip Wood
2025-09-21 16:07 ` Ezekiel Newren
2025-09-19 15:16 ` [PATCH v3 08/10] xdiff: delete chastore from xdfile_t Ezekiel Newren via GitGitGadget
2025-09-19 15:16 ` [PATCH v3 09/10] xdiff: delete rchg aliasing Ezekiel Newren via GitGitGadget
2025-09-21 13:07 ` Phillip Wood
2025-09-21 16:37 ` Ezekiel Newren
2025-09-19 15:16 ` [PATCH v3 10/10] xdiff: treat xdfile_t.rchg like an enum Ezekiel Newren via GitGitGadget
2025-09-21 0:00 ` Junio C Hamano
2025-09-21 0:38 ` Ezekiel Newren
2025-09-21 9:19 ` Phillip Wood
2025-09-21 16:11 ` Ezekiel Newren
2025-09-19 23:30 ` [PATCH v3 00/10] Cleanup xdfile_t and xrecord_t in xdiff Elijah Newren
2025-09-19 23:37 ` Ezekiel Newren
2025-09-22 19:51 ` [PATCH v4 00/12] " Ezekiel Newren via GitGitGadget
2025-09-22 19:51 ` [PATCH v4 01/12] xdiff: delete static forward declarations in xprepare Ezekiel Newren via GitGitGadget
2025-09-22 19:51 ` [PATCH v4 02/12] xdiff: delete local variables and initialize/free xdfile_t directly Ezekiel Newren via GitGitGadget
2025-09-22 19:51 ` [PATCH v4 03/12] xdiff: delete unnecessary fields from xrecord_t and xdfile_t Ezekiel Newren via GitGitGadget
2025-09-22 19:51 ` [PATCH v4 04/12] xdiff: delete superfluous function xdl_get_rec() in xemit Ezekiel Newren via GitGitGadget
2025-09-22 19:51 ` [PATCH v4 05/12] xdiff: delete superfluous local variables that alias fields in xrecord_t Ezekiel Newren via GitGitGadget
2025-09-22 19:51 ` [PATCH v4 06/12] xdiff: delete struct diffdata_t Ezekiel Newren via GitGitGadget
2025-09-22 19:51 ` [PATCH v4 07/12] xdiff: delete redundant array xdfile_t.ha Ezekiel Newren via GitGitGadget
2025-09-22 19:51 ` [PATCH v4 08/12] xdiff: delete fields ha, line, size in xdlclass_t in favor of an xrecord_t Ezekiel Newren via GitGitGadget
2025-09-22 19:51 ` [PATCH v4 09/12] xdiff: delete chastore from xdfile_t Ezekiel Newren via GitGitGadget
2025-09-22 19:51 ` [PATCH v4 10/12] xdiff: delete rchg aliasing Ezekiel Newren via GitGitGadget
2025-09-22 19:51 ` [PATCH v4 11/12] xdiff: use bool literals for xdfile_t.rchg Ezekiel Newren via GitGitGadget
2025-09-22 19:51 ` [PATCH v4 12/12] xdiff: refactor 'char *rchg' to 'bool *changed' in xdfile_t Ezekiel Newren via GitGitGadget
2025-09-22 22:39 ` [PATCH v4 00/12] Cleanup xdfile_t and xrecord_t in xdiff Junio C Hamano
2025-09-23 0:13 ` Ezekiel Newren
2025-09-23 1:06 ` Junio C Hamano
2025-09-23 1:30 ` Ezekiel Newren
2025-09-23 14:12 ` Junio C Hamano
2025-09-23 16:50 ` Ezekiel Newren
2025-09-23 21:24 ` [PATCH v5 00/13] " Ezekiel Newren via GitGitGadget
2025-09-23 21:24 ` [PATCH v5 01/13] xdiff: delete static forward declarations in xprepare Ezekiel Newren via GitGitGadget
2025-09-23 21:24 ` [PATCH v5 02/13] xdiff: delete local variables and initialize/free xdfile_t directly Ezekiel Newren via GitGitGadget
2025-09-23 21:24 ` [PATCH v5 03/13] xdiff: delete unnecessary fields from xrecord_t and xdfile_t Ezekiel Newren via GitGitGadget
2025-09-23 21:24 ` [PATCH v5 04/13] xdiff: delete superfluous function xdl_get_rec() in xemit Ezekiel Newren via GitGitGadget
2025-09-30 13:31 ` Kristoffer Haugsbakk
2025-09-30 19:35 ` Ezekiel Newren
2025-09-30 20:05 ` Junio C Hamano
2025-09-23 21:24 ` [PATCH v5 05/13] xdiff: delete superfluous local variables that alias fields in xrecord_t Ezekiel Newren via GitGitGadget
2025-09-24 10:22 ` Phillip Wood
2025-09-24 14:52 ` Ezekiel Newren
2025-09-23 21:24 ` [PATCH v5 06/13] xdiff: delete struct diffdata_t Ezekiel Newren via GitGitGadget
2025-09-23 21:24 ` [PATCH v5 07/13] xdiff: delete redundant array xdfile_t.ha Ezekiel Newren via GitGitGadget
2025-09-23 21:24 ` [PATCH v5 08/13] xdiff: delete fields ha, line, size in xdlclass_t in favor of an xrecord_t Ezekiel Newren via GitGitGadget
2025-09-23 21:24 ` [PATCH v5 09/13] xdiff: delete chastore from xdfile_t Ezekiel Newren via GitGitGadget
2025-09-23 21:24 ` [PATCH v5 10/13] xdiff: delete rchg aliasing Ezekiel Newren via GitGitGadget
2025-09-24 10:22 ` Phillip Wood
2025-09-24 15:01 ` Ezekiel Newren
2025-09-24 15:34 ` Junio C Hamano
2025-09-24 15:58 ` Ezekiel Newren
2025-09-24 21:31 ` Junio C Hamano
2025-09-24 22:46 ` Ezekiel Newren
2025-09-25 7:09 ` Junio C Hamano
2025-09-25 22:02 ` Ezekiel Newren
2025-09-23 21:24 ` [PATCH v5 11/13] xdiff: rename rchg -> changed in xdfile_t Ezekiel Newren via GitGitGadget
2025-09-24 10:22 ` Phillip Wood
2025-09-24 15:10 ` Ezekiel Newren
2025-09-24 15:18 ` Phillip Wood
2025-09-23 21:24 ` [PATCH v5 12/13] xdiff: use enum macros NONE(0), SOME(1), TOO_MANY(2) in xprepare.c Ezekiel Newren via GitGitGadget
2025-09-24 10:21 ` Phillip Wood
2025-09-24 14:46 ` Ezekiel Newren
2025-09-24 15:18 ` Phillip Wood [this message]
2025-09-24 17:29 ` Junio C Hamano
2025-09-25 18:40 ` Ezekiel Newren
2025-09-26 2:29 ` Ezekiel Newren
2025-09-23 21:24 ` [PATCH v5 13/13] xdiff: change type of xdfile_t.changed from char to bool Ezekiel Newren via GitGitGadget
2025-09-24 10:21 ` Phillip Wood
2025-09-24 15:14 ` Ezekiel Newren
2025-09-26 22:41 ` [PATCH v6 00/12] Cleanup xdfile_t and xrecord_t in xdiff Ezekiel Newren via GitGitGadget
2025-09-26 22:41 ` [PATCH v6 01/12] xdiff: delete static forward declarations in xprepare Ezekiel Newren via GitGitGadget
2025-09-26 22:41 ` [PATCH v6 02/12] xdiff: delete local variables and initialize/free xdfile_t directly Ezekiel Newren via GitGitGadget
2025-09-26 22:41 ` [PATCH v6 03/12] xdiff: delete unnecessary fields from xrecord_t and xdfile_t Ezekiel Newren via GitGitGadget
2025-09-26 22:41 ` [PATCH v6 04/12] xdiff: delete superfluous function xdl_get_rec() in xemit Ezekiel Newren via GitGitGadget
2025-09-26 22:41 ` [PATCH v6 05/12] xdiff: delete local variables that alias fields in xrecord_t Ezekiel Newren via GitGitGadget
2025-09-26 22:41 ` [PATCH v6 06/12] xdiff: delete struct diffdata_t Ezekiel Newren via GitGitGadget
2025-09-26 22:41 ` [PATCH v6 07/12] xdiff: delete redundant array xdfile_t.ha Ezekiel Newren via GitGitGadget
2025-09-26 22:41 ` [PATCH v6 08/12] xdiff: delete fields ha, line, size in xdlclass_t in favor of an xrecord_t Ezekiel Newren via GitGitGadget
2025-09-26 22:41 ` [PATCH v6 09/12] xdiff: delete chastore from xdfile_t Ezekiel Newren via GitGitGadget
2025-09-26 22:41 ` [PATCH v6 10/12] xdiff: rename rchg -> changed in xdfile_t Ezekiel Newren via GitGitGadget
2025-09-26 22:41 ` [PATCH v6 11/12] xdiff: add macros DISCARD(0), KEEP(1), INVESTIGATE(2) in xprepare.c Ezekiel Newren via GitGitGadget
2025-09-26 22:41 ` [PATCH v6 12/12] xdiff: change type of xdfile_t.changed from char to bool Ezekiel Newren via GitGitGadget
2025-10-03 13:47 ` [PATCH v6 00/12] Cleanup xdfile_t and xrecord_t in xdiff Phillip Wood
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1c3461c3-09bb-404d-a4dc-a895baba68f2@gmail.com \
--to=phillip.wood123@gmail$(echo .)com \
--cc=ben.knoble@gmail$(echo .)com \
--cc=ezekielnewren@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitgitgadget@gmail$(echo .)com \
--cc=newren@gmail$(echo .)com \
--cc=peff@peff$(echo .)net \
--cc=phillip.wood@dunelm$(echo .)org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox