Re: [PATCH 04/12] fsmonitor: refactor refresh callback on directory events

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 2672 bytes --]

On Tue, Feb 20, 2024 at 01:54:44PM -0500, Jeff Hostetler wrote:
> 
> 
> On 2/15/24 4:32 AM, Patrick Steinhardt wrote:
> > On Tue, Feb 13, 2024 at 08:52:13PM +0000, Jeff Hostetler via GitGitGadget wrote:
> > > From: Jeff Hostetler <jeffhostetler@github.com>
> > > 
> > > Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
> > > ---
> > >   fsmonitor.c | 52 ++++++++++++++++++++++++++++++----------------------
> > >   1 file changed, 30 insertions(+), 22 deletions(-)
> > > 
> > > diff --git a/fsmonitor.c b/fsmonitor.c
> > > index f670c509378..b1ef01bf3cd 100644
> > > --- a/fsmonitor.c
> > > +++ b/fsmonitor.c
> > > @@ -183,6 +183,35 @@ static int query_fsmonitor_hook(struct repository *r,
> > >   	return result;
> > >   }
> > > +static void fsmonitor_refresh_callback_slash(
> > > +	struct index_state *istate, const char *name, int len, int pos)
> > 
> > `len` should be `size_t` as it tracks the length of the name. This is
> > a preexisting issue already because `fsmonitor_refresh_callback()`
> > assigns `int len = strlen(name)`, which is wrong.
> > 
> > > +{
> > > +	int i;
> > 
> > `i` is used to iterate through `istate->cache_nr`, which is an `unsigned
> > int` and not an `int`. I really wish we would compile the Git code base
> > with `-Wconversion`, but that's a rather big undertaking.
> > 
> > Anyway, none of these issues are new as you merely move the code into a
> > new function.
> > 
> > Patrick
> > 
> 
> Yeah, the int types are bit of a mess, but I'd like to defer that to
> another series.
> 
> There are several things going on here as you point out.
> 
> (1) 'int len' for the length of a pathname buffer.  This could be fixed,
> but the odds of Git correctly working with a >2Gb pathname doesn't make
> this urgent.
> 
> (2) 'int i' is signed, but should be unsigned -- but elsewhere in this
> function we use 'int pos' which an index on the same array and has
> double duty as the suggested insertion point when negative.  So we can
> fix the type of 'i', but that just sets us up to hide errors with 'pos',
> since they'd have different types.  Also, since it is really unlikely
> for Git to work with >2G cache-entries, I think this one can wait too.
> 
> I don't mean to be confrontational, but I think these changes should
> wait until another series that is focused on just those types of issues.

No worries, this doesn't come across as confrontational at all. As I
said myself, the problems aren't really new to your patch series but are
are preexisting and run deeper than what you can see in the diffs here.
So I'm perfectly fine with deferring this.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]