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

Commit Message

Jeff Hostetler Feb. 13, 2024, 8:52 p.m. UTC

From: Jeff Hostetler <[e-mail]>

Signed-off-by: Jeff Hostetler <[e-mail]>
---
 fsmonitor.c | 52 ++++++++++++++++++++++++++++++----------------------
 1 file changed, 30 insertions(+), 22 deletions(-)

Comments

Patrick Steinhardt Feb. 15, 2024, 9:32 a.m. UTC | #1

On Tue, Feb 13, 2024 at 08:52:13PM +0000, Jeff Hostetler via GitGitGadget wrote:
> From: Jeff Hostetler <[e-mail]>
> 
> Signed-off-by: Jeff Hostetler <[e-mail]>
> ---
>  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

> +	/*
> +	 * The daemon can decorate directory events, such as
> +	 * moves or renames, with a trailing slash if the OS
> +	 * FS Event contains sufficient information, such as
> +	 * MacOS.
> +	 *
> +	 * Use this to invalidate the entire cone under that
> +	 * directory.
> +	 *
> +	 * We do not expect an exact match because the index
> +	 * does not normally contain directory entries, so we
> +	 * start at the insertion point and scan.
> +	 */
> +	if (pos < 0)
> +		pos = -pos - 1;
> +
> +	/* Mark all entries for the folder invalid */
> +	for (i = pos; i < istate->cache_nr; i++) {
> +		if (!starts_with(istate->cache[i]->name, name))
> +			break;
> +		istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;
> +	}
> +}
> +
>  static void fsmonitor_refresh_callback(struct index_state *istate, char *name)
>  {
>  	int i, len = strlen(name);
> @@ -193,28 +222,7 @@ static void fsmonitor_refresh_callback(struct index_state *istate, char *name)
>  			 name, pos);
>  
>  	if (name[len - 1] == '/') {
> -		/*
> -		 * The daemon can decorate directory events, such as
> -		 * moves or renames, with a trailing slash if the OS
> -		 * FS Event contains sufficient information, such as
> -		 * MacOS.
> -		 *
> -		 * Use this to invalidate the entire cone under that
> -		 * directory.
> -		 *
> -		 * We do not expect an exact match because the index
> -		 * does not normally contain directory entries, so we
> -		 * start at the insertion point and scan.
> -		 */
> -		if (pos < 0)
> -			pos = -pos - 1;
> -
> -		/* Mark all entries for the folder invalid */
> -		for (i = pos; i < istate->cache_nr; i++) {
> -			if (!starts_with(istate->cache[i]->name, name))
> -				break;
> -			istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;
> -		}
> +		fsmonitor_refresh_callback_slash(istate, name, len, pos);
>  
>  		/*
>  		 * We need to remove the traling "/" from the path
> -- 
> gitgitgadget
> 
>

Jeff Hostetler Feb. 20, 2024, 6:54 p.m. UTC | #2

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 <[e-mail]>
>>
>> Signed-off-by: Jeff Hostetler <[e-mail]>
>> ---
>>   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.

Jeff

Patrick Steinhardt Feb. 21, 2024, 12:54 p.m. UTC | #3

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 <[e-mail]>
> > > 
> > > Signed-off-by: Jeff Hostetler <[e-mail]>
> > > ---
> > >   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

Patch

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)
+{
+	int i;
+
+	/*
+	 * The daemon can decorate directory events, such as
+	 * moves or renames, with a trailing slash if the OS
+	 * FS Event contains sufficient information, such as
+	 * MacOS.
+	 *
+	 * Use this to invalidate the entire cone under that
+	 * directory.
+	 *
+	 * We do not expect an exact match because the index
+	 * does not normally contain directory entries, so we
+	 * start at the insertion point and scan.
+	 */
+	if (pos < 0)
+		pos = -pos - 1;
+
+	/* Mark all entries for the folder invalid */
+	for (i = pos; i < istate->cache_nr; i++) {
+		if (!starts_with(istate->cache[i]->name, name))
+			break;
+		istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;
+	}
+}
+
 static void fsmonitor_refresh_callback(struct index_state *istate, char *name)
 {
 	int i, len = strlen(name);
@@ -193,28 +222,7 @@  static void fsmonitor_refresh_callback(struct index_state *istate, char *name)
 			 name, pos);
 
 	if (name[len - 1] == '/') {
-		/*
-		 * The daemon can decorate directory events, such as
-		 * moves or renames, with a trailing slash if the OS
-		 * FS Event contains sufficient information, such as
-		 * MacOS.
-		 *
-		 * Use this to invalidate the entire cone under that
-		 * directory.
-		 *
-		 * We do not expect an exact match because the index
-		 * does not normally contain directory entries, so we
-		 * start at the insertion point and scan.
-		 */
-		if (pos < 0)
-			pos = -pos - 1;
-
-		/* Mark all entries for the folder invalid */
-		for (i = pos; i < istate->cache_nr; i++) {
-			if (!starts_with(istate->cache[i]->name, name))
-				break;
-			istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;
-		}
+		fsmonitor_refresh_callback_slash(istate, name, len, pos);
 
 		/*
 		 * We need to remove the traling "/" from the path