diff mbox series

[v2,06/16] fsmonitor: refactor refresh callback for non-directory events

Message ID 7ee6ca1aefd34a37d749300e317df10d80ef2b29.1708658300.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series FSMonitor edge cases on case-insensitive file systems | expand

Commit Message

Jeff Hostetler Feb. 23, 2024, 3:18 a.m. UTC 
From: Jeff Hostetler <jeffhostetler@github.com>

Move the code handle unqualified FSEvents (without a trailing slash)
into a helper function.

Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
---
 fsmonitor.c | 67 +++++++++++++++++++++++++++++++----------------------
 1 file changed, 39 insertions(+), 28 deletions(-)

Comments

Junio C Hamano Feb. 23, 2024, 8:18 a.m. UTC | #1 
"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Jeff Hostetler <jeffhostetler@github.com>
>
> Move the code handle unqualified FSEvents (without a trailing slash)
> into a helper function.

-ECANNOTPARSE.  "code handle" -> "code that handles"?

In the patch text itself, there is nothing unexpected.  Looking
good.

Thanks.

> Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
> ---
>  fsmonitor.c | 67 +++++++++++++++++++++++++++++++----------------------
>  1 file changed, 39 insertions(+), 28 deletions(-)
>
> diff --git a/fsmonitor.c b/fsmonitor.c
> index 29cce32d81c..364198d258f 100644
> --- a/fsmonitor.c
> +++ b/fsmonitor.c
> @@ -183,6 +183,43 @@ static int query_fsmonitor_hook(struct repository *r,
>  	return result;
>  }
>  
> +static void handle_path_without_trailing_slash(
> +	struct index_state *istate, const char *name, int pos)
> +{
> +	int i;
> +
> +	if (pos >= 0) {
> +		/*
> +		 * We have an exact match for this path and can just
> +		 * invalidate it.
> +		 */
> +		istate->cache[pos]->ce_flags &= ~CE_FSMONITOR_VALID;
> +	} else {
> +		/*
> +		 * The path is not a tracked file -or- it is a
> +		 * directory event on a platform that cannot
> +		 * distinguish between file and directory events in
> +		 * the event handler, such as Windows.
> +		 *
> +		 * Scan as if it is a directory and invalidate the
> +		 * cone under it.  (But remember to ignore items
> +		 * between "name" and "name/", such as "name-" and
> +		 * "name.".
> +		 */
> +		int len = strlen(name);
> +		pos = -pos - 1;
> +
> +		for (i = pos; i < istate->cache_nr; i++) {
> +			if (!starts_with(istate->cache[i]->name, name))
> +				break;
> +			if ((unsigned char)istate->cache[i]->name[len] > '/')
> +				break;
> +			if (istate->cache[i]->name[len] == '/')
> +				istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;
> +		}
> +	}
> +}
> +
>  /*
>   * The daemon can decorate directory events, such as a move or rename,
>   * by adding a trailing slash to the observed name.  Use this to
> @@ -225,7 +262,7 @@ static void handle_path_with_trailing_slash(
>  
>  static void fsmonitor_refresh_callback(struct index_state *istate, char *name)
>  {
> -	int i, len = strlen(name);
> +	int len = strlen(name);
>  	int pos = index_name_pos(istate, name, len);
>  
>  	trace_printf_key(&trace_fsmonitor,
> @@ -240,34 +277,8 @@ static void fsmonitor_refresh_callback(struct index_state *istate, char *name)
>  		 * for the untracked cache.
>  		 */
>  		name[len - 1] = '\0';
> -	} else if (pos >= 0) {
> -		/*
> -		 * We have an exact match for this path and can just
> -		 * invalidate it.
> -		 */
> -		istate->cache[pos]->ce_flags &= ~CE_FSMONITOR_VALID;
>  	} else {
> -		/*
> -		 * The path is not a tracked file -or- it is a
> -		 * directory event on a platform that cannot
> -		 * distinguish between file and directory events in
> -		 * the event handler, such as Windows.
> -		 *
> -		 * Scan as if it is a directory and invalidate the
> -		 * cone under it.  (But remember to ignore items
> -		 * between "name" and "name/", such as "name-" and
> -		 * "name.".
> -		 */
> -		pos = -pos - 1;
> -
> -		for (i = pos; i < istate->cache_nr; i++) {
> -			if (!starts_with(istate->cache[i]->name, name))
> -				break;
> -			if ((unsigned char)istate->cache[i]->name[len] > '/')
> -				break;
> -			if (istate->cache[i]->name[len] == '/')
> -				istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;
> -		}
> +		handle_path_without_trailing_slash(istate, name, pos);
>  	}
>  
>  	/*
Torsten Bögershausen Feb. 25, 2024, 12:30 p.m. UTC | #2 
On Fri, Feb 23, 2024 at 03:18:10AM +0000, Jeff Hostetler via GitGitGadget wrote:
> From: Jeff Hostetler <jeffhostetler@github.com>
>
> Move the code handle unqualified FSEvents (without a trailing slash)
> into a helper function.
>
> Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
> ---
>  fsmonitor.c | 67 +++++++++++++++++++++++++++++++----------------------
>  1 file changed, 39 insertions(+), 28 deletions(-)
>
> diff --git a/fsmonitor.c b/fsmonitor.c
> index 29cce32d81c..364198d258f 100644
> --- a/fsmonitor.c
> +++ b/fsmonitor.c
> @@ -183,6 +183,43 @@ static int query_fsmonitor_hook(struct repository *r,
>  	return result;
>  }
>
> +static void handle_path_without_trailing_slash(
> +	struct index_state *istate, const char *name, int pos)
> +{
> +	int i;
> +
> +	if (pos >= 0) {
> +		/*
> +		 * We have an exact match for this path and can just
> +		 * invalidate it.
> +		 */
> +		istate->cache[pos]->ce_flags &= ~CE_FSMONITOR_VALID;
> +	} else {
> +		/*
> +		 * The path is not a tracked file -or- it is a
> +		 * directory event on a platform that cannot
> +		 * distinguish between file and directory events in
> +		 * the event handler, such as Windows.
> +		 *
> +		 * Scan as if it is a directory and invalidate the
> +		 * cone under it.  (But remember to ignore items
> +		 * between "name" and "name/", such as "name-" and
> +		 * "name.".
> +		 */
> +		int len = strlen(name);

should this be
	size_t len = strlen(name);

> +		pos = -pos - 1;
> +
> +		for (i = pos; i < istate->cache_nr; i++) {
> +			if (!starts_with(istate->cache[i]->name, name))
> +				break;
> +			if ((unsigned char)istate->cache[i]->name[len] > '/')
> +				break;

Hm, this covers all digits, letters, :;<=>?
but not e.g. !+-. (and others). What do i miss ?


> +			if (istate->cache[i]->name[len] == '/')
> +				istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;
> +		}
> +	}
> +}
> +
>  /*
>   * The daemon can decorate directory events, such as a move or rename,
>   * by adding a trailing slash to the observed name.  Use this to
> @@ -225,7 +262,7 @@ static void handle_path_with_trailing_slash(
>
>  static void fsmonitor_refresh_callback(struct index_state *istate, char *name)
>  {
> -	int i, len = strlen(name);
> +	int len = strlen(name);

Same here: size_t len = strlen() ?

>  	int pos = index_name_pos(istate, name, len);
>
>  	trace_printf_key(&trace_fsmonitor,
> @@ -240,34 +277,8 @@ static void fsmonitor_refresh_callback(struct index_state *istate, char *name)
>  		 * for the untracked cache.
>  		 */
>  		name[len - 1] = '\0';
> -	} else if (pos >= 0) {
> -		/*
> -		 * We have an exact match for this path and can just
> -		 * invalidate it.
> -		 */
> -		istate->cache[pos]->ce_flags &= ~CE_FSMONITOR_VALID;
>  	} else {
> -		/*
> -		 * The path is not a tracked file -or- it is a
> -		 * directory event on a platform that cannot
> -		 * distinguish between file and directory events in
> -		 * the event handler, such as Windows.
> -		 *
> -		 * Scan as if it is a directory and invalidate the
> -		 * cone under it.  (But remember to ignore items
> -		 * between "name" and "name/", such as "name-" and
> -		 * "name.".
> -		 */
> -		pos = -pos - 1;
> -
> -		for (i = pos; i < istate->cache_nr; i++) {
> -			if (!starts_with(istate->cache[i]->name, name))
> -				break;
> -			if ((unsigned char)istate->cache[i]->name[len] > '/')
> -				break;
> -			if (istate->cache[i]->name[len] == '/')
> -				istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;
> -		}
> +		handle_path_without_trailing_slash(istate, name, pos);
>  	}
>
>  	/*
> --
> gitgitgadget
>
>
Junio C Hamano Feb. 25, 2024, 5:24 p.m. UTC | #3 
Torsten Bögershausen <tboegi@web.de> writes:

>> +		pos = -pos - 1;
>> +
>> +		for (i = pos; i < istate->cache_nr; i++) {
>> +			if (!starts_with(istate->cache[i]->name, name))
>> +				break;
>> +			if ((unsigned char)istate->cache[i]->name[len] > '/')
>> +				break;
>
> Hm, this covers all digits, letters, :;<=>?
> but not e.g. !+-. (and others). What do i miss ?

This is scanning an in-core array of cache entries, which is sorted
by name in lexicographic order, and the loop knows that files under
the directory "foo", whose pathnames all share prefix "foo/", would
sort between "foo.h" and "foo00", because "." sorts before "/" and
"0" sorts after "/".

It is trying to find where in the array a hypothetical directory
would appear, if any of the files in it existed in the array, and
exiting early, taking advantage of the fact that after seeing
something that sorts after a '/', it will never see an entry that
shares cache[i]->name[] as a prefix.

It is not a new code in the patch, of course; merely got moved from
elsewhere below.

>> -		/*
>> -		 * The path is not a tracked file -or- it is a
>> -		 * directory event on a platform that cannot
>> -		 * distinguish between file and directory events in
>> -		 * the event handler, such as Windows.
>> -		 *
>> -		 * Scan as if it is a directory and invalidate the
>> -		 * cone under it.  (But remember to ignore items
>> -		 * between "name" and "name/", such as "name-" and
>> -		 * "name.".
>> -		 */
>> -		pos = -pos - 1;
>> -
>> -		for (i = pos; i < istate->cache_nr; i++) {
>> -			if (!starts_with(istate->cache[i]->name, name))
>> -				break;
>> -			if ((unsigned char)istate->cache[i]->name[len] > '/')
>> -				break;
>> -			if (istate->cache[i]->name[len] == '/')
>> -				istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;
>> -		}
>> +		handle_path_without_trailing_slash(istate, name, pos);
>>  	}
>>
>>  	/*
>> --
>> gitgitgadget
>>
>>
diff mbox series

Patch

diff --git a/fsmonitor.c b/fsmonitor.c
index 29cce32d81c..364198d258f 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -183,6 +183,43 @@  static int query_fsmonitor_hook(struct repository *r,
 	return result;
 }
 
+static void handle_path_without_trailing_slash(
+	struct index_state *istate, const char *name, int pos)
+{
+	int i;
+
+	if (pos >= 0) {
+		/*
+		 * We have an exact match for this path and can just
+		 * invalidate it.
+		 */
+		istate->cache[pos]->ce_flags &= ~CE_FSMONITOR_VALID;
+	} else {
+		/*
+		 * The path is not a tracked file -or- it is a
+		 * directory event on a platform that cannot
+		 * distinguish between file and directory events in
+		 * the event handler, such as Windows.
+		 *
+		 * Scan as if it is a directory and invalidate the
+		 * cone under it.  (But remember to ignore items
+		 * between "name" and "name/", such as "name-" and
+		 * "name.".
+		 */
+		int len = strlen(name);
+		pos = -pos - 1;
+
+		for (i = pos; i < istate->cache_nr; i++) {
+			if (!starts_with(istate->cache[i]->name, name))
+				break;
+			if ((unsigned char)istate->cache[i]->name[len] > '/')
+				break;
+			if (istate->cache[i]->name[len] == '/')
+				istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;
+		}
+	}
+}
+
 /*
  * The daemon can decorate directory events, such as a move or rename,
  * by adding a trailing slash to the observed name.  Use this to
@@ -225,7 +262,7 @@  static void handle_path_with_trailing_slash(
 
 static void fsmonitor_refresh_callback(struct index_state *istate, char *name)
 {
-	int i, len = strlen(name);
+	int len = strlen(name);
 	int pos = index_name_pos(istate, name, len);
 
 	trace_printf_key(&trace_fsmonitor,
@@ -240,34 +277,8 @@  static void fsmonitor_refresh_callback(struct index_state *istate, char *name)
 		 * for the untracked cache.
 		 */
 		name[len - 1] = '\0';
-	} else if (pos >= 0) {
-		/*
-		 * We have an exact match for this path and can just
-		 * invalidate it.
-		 */
-		istate->cache[pos]->ce_flags &= ~CE_FSMONITOR_VALID;
 	} else {
-		/*
-		 * The path is not a tracked file -or- it is a
-		 * directory event on a platform that cannot
-		 * distinguish between file and directory events in
-		 * the event handler, such as Windows.
-		 *
-		 * Scan as if it is a directory and invalidate the
-		 * cone under it.  (But remember to ignore items
-		 * between "name" and "name/", such as "name-" and
-		 * "name.".
-		 */
-		pos = -pos - 1;
-
-		for (i = pos; i < istate->cache_nr; i++) {
-			if (!starts_with(istate->cache[i]->name, name))
-				break;
-			if ((unsigned char)istate->cache[i]->name[len] > '/')
-				break;
-			if (istate->cache[i]->name[len] == '/')
-				istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;
-		}
+		handle_path_without_trailing_slash(istate, name, pos);
 	}
 
 	/*