[v2,11/16] fsmonitor: remove custom loop from non-directory path handler

Commit Message

Jeff Hostetler Feb. 23, 2024, 3:18 a.m. UTC

From: Jeff Hostetler <[e-mail]>

Refactor the code that handles refresh events for pathnames that do
not contain a trailing slash.  Instead of using a custom loop to try
to scan the index and detect if the FSEvent named a file or might be a
directory prefix, use the recently created helper function to do that.

Also update the comments to describe what and why we are doing this.

On platforms that DO NOT annotate FS events with a trailing
slash, if we fail to find an exact match for the pathname
in the index, we do not know if the pathname represents a
directory or simply an untracked file.  Pretend that the pathname
is a directory and try again before assuming it is an untracked
file.

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

Comments

Junio C Hamano Feb. 23, 2024, 5:47 p.m. UTC | #1

"Jeff Hostetler via GitGitGadget" <[e-mail]> writes:

>  static void handle_path_without_trailing_slash(
>  	struct index_state *istate, const char *name, int pos)
>  {
> -	int i;
> -
>  	/*
>  	 * Mark the untracked cache dirty for this path (regardless of
>  	 * whether or not we find an exact match for it in the index).
> @@ -200,33 +212,28 @@ static void handle_path_without_trailing_slash(
>  
>  	if (pos >= 0) {
>  		/*
> -		 * We have an exact match for this path and can just
> -		 * invalidate it.
> +		 * An exact match on a tracked file. We assume that we
> +		 * do not need to scan forward for a sparse-directory
> +		 * cache-entry with the same pathname, nor for a cone
> +		 * at that directory. (That is, assume no D/F conflicts.)
>  		 */
>  		istate->cache[pos]->ce_flags &= ~CE_FSMONITOR_VALID;
>  	} else {
> +		struct strbuf work_path = STRBUF_INIT;
> +
>  		/*
> +		strbuf_add(&work_path, name, strlen(name));
> +		strbuf_addch(&work_path, '/');
> +		pos = index_name_pos(istate, work_path.buf, work_path.len);
> +		handle_path_with_trailing_slash(istate, work_path.buf, pos);
> +		strbuf_release(&work_path);
>  	}
>  }

The "with trailing slash" variant is returning a useful value to
this caller that ignores it, but we do not yet return a value from
this function, so that is OK.  The name being a name that may be in
different case from what we know in the index is not yet handled in
this step (we have "Assume it is case-correct" in the comment) and
that applies for both the main array of cache entries as well as the
untracked cache.

It will be exciting to see how these are lifted.  The main array has
some helper functions that uses name-hash features to help icase
matches, but I do not offhand recall what we have for the untracked
cache side.

Patch

diff --git a/fsmonitor.c b/fsmonitor.c
index 9424bd17230..a51c17cda70 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -183,11 +183,23 @@  static int query_fsmonitor_hook(struct repository *r,
 	return result;
 }
 
+static size_t handle_path_with_trailing_slash(
+	struct index_state *istate, const char *name, int pos);
+
+/*
+ * The daemon sent an observed pathname without a trailing slash.
+ * (This is the normal case.)  We do not know if it is a tracked or
+ * untracked file, a sparse-directory, or a populated directory (on a
+ * platform such as Windows where FSEvents are not qualified).
+ *
+ * The pathname contains the observed case reported by the FS. We
+ * do not know it is case-correct or -incorrect.
+ *
+ * Assume it is case-correct and try an exact match.
+ */
 static void handle_path_without_trailing_slash(
 	struct index_state *istate, const char *name, int pos)
 {
-	int i;
-
 	/*
 	 * Mark the untracked cache dirty for this path (regardless of
 	 * whether or not we find an exact match for it in the index).
@@ -200,33 +212,28 @@  static void handle_path_without_trailing_slash(
 
 	if (pos >= 0) {
 		/*
-		 * We have an exact match for this path and can just
-		 * invalidate it.
+		 * An exact match on a tracked file. We assume that we
+		 * do not need to scan forward for a sparse-directory
+		 * cache-entry with the same pathname, nor for a cone
+		 * at that directory. (That is, assume no D/F conflicts.)
 		 */
 		istate->cache[pos]->ce_flags &= ~CE_FSMONITOR_VALID;
 	} else {
+		struct strbuf work_path = STRBUF_INIT;
+
 		/*
-		 * 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.".
+		 * The negative "pos" gives us the suggested insertion
+		 * point for the pathname (without the trailing slash).
+		 * We need to see if there is a directory with that
+		 * prefix, but there can be lots of pathnames between
+		 * "foo" and "foo/" like "foo-" or "foo-bar", so we
+		 * don't want to do our own scan.
 		 */
-		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;
-		}
+		strbuf_add(&work_path, name, strlen(name));
+		strbuf_addch(&work_path, '/');
+		pos = index_name_pos(istate, work_path.buf, work_path.len);
+		handle_path_with_trailing_slash(istate, work_path.buf, pos);
+		strbuf_release(&work_path);
 	}
 }