Merge branch 'jh/fsmonitor-icase-corner-case-fix'

FSMonitor client code was confused when FSEvents were given in a
different case on a case-insensitive filesystem, which has been
corrected.

Acked-by: Patrick Steinhardt <[email protected]>
cf. <ZehofMaSZyUq8S1N@tanuki>

* jh/fsmonitor-icase-corner-case-fix:
  fsmonitor: support case-insensitive events
  fsmonitor: refactor bit invalidation in refresh callback
  fsmonitor: trace the new invalidated cache-entry count
  fsmonitor: return invalidated cache-entry count on non-directory event
  fsmonitor: remove custom loop from non-directory path handler
  fsmonitor: return invalidated cache-entry count on directory event
  fsmonitor: move untracked-cache invalidation into helper functions
  fsmonitor: refactor untracked-cache invalidation
  dir: create untracked_cache_invalidate_trimmed_path()
  fsmonitor: refactor refresh callback for non-directory events
  fsmonitor: clarify handling of directory events in callback helper
  fsmonitor: refactor refresh callback on directory events
  t7527: add case-insensitve test for FSMonitor
  name-hash: add index_dir_find()

Author: gitster

dir.c

@@ -3918,6 +3918,26 @@ void untracked_cache_invalidate_path(struct index_state *istate,
 				 path, strlen(path));
 }
 
+void untracked_cache_invalidate_trimmed_path(struct index_state *istate,
+					     const char *path,
+					     int safe_path)
+{
+	size_t len = strlen(path);
+
+	if (!len)
+		BUG("untracked_cache_invalidate_trimmed_path given zero length path");
+
+	if (path[len - 1] != '/') {
+		untracked_cache_invalidate_path(istate, path, safe_path);
+	} else {
+		struct strbuf tmp = STRBUF_INIT;
+
+		strbuf_add(&tmp, path, len - 1);
+		untracked_cache_invalidate_path(istate, tmp.buf, safe_path);
+		strbuf_release(&tmp);
+	}
+}
+
 void untracked_cache_remove_from_index(struct index_state *istate,
 				       const char *path)
 {

dir.h

@@ -576,6 +576,13 @@ int cmp_dir_entry(const void *p1, const void *p2);
 int check_dir_entry_contains(const struct dir_entry *out, const struct dir_entry *in);
 
 void untracked_cache_invalidate_path(struct index_state *, const char *, int safe_path);
+/*
+ * Invalidate the untracked-cache for this path, but first strip
+ * off a trailing slash, if present.
+ */
+void untracked_cache_invalidate_trimmed_path(struct index_state *,
+					     const char *path,
+					     int safe_path);
 void untracked_cache_remove_from_index(struct index_state *, const char *);
 void untracked_cache_add_to_index(struct index_state *, const char *);
 

fsmonitor.c

@@ -5,6 +5,7 @@
 #include "ewah/ewok.h"
 #include "fsmonitor.h"
 #include "fsmonitor-ipc.h"
+#include "name-hash.h"
 #include "run-command.h"
 #include "strbuf.h"
 #include "trace2.h"
@@ -183,79 +184,282 @@ static int query_fsmonitor_hook(struct repository *r,
 	return result;
 }
 
-static void fsmonitor_refresh_callback(struct index_state *istate, char *name)
+/*
+ * Invalidate the FSM bit on this CE.  This is like mark_fsmonitor_invalid()
+ * but we've already handled the untracked-cache, so let's not repeat that
+ * work.  This also lets us have a different trace message so that we can
+ * see everything that was done as part of the refresh-callback.
+ */
+static void invalidate_ce_fsm(struct cache_entry *ce)
 {
-	int i, len = strlen(name);
-	int pos = index_name_pos(istate, name, len);
+	if (ce->ce_flags & CE_FSMONITOR_VALID) {
+		trace_printf_key(&trace_fsmonitor,
+				 "fsmonitor_refresh_callback INV: '%s'",
+				 ce->name);
+		ce->ce_flags &= ~CE_FSMONITOR_VALID;
+	}
+}
+
+static size_t handle_path_with_trailing_slash(
+	struct index_state *istate, const char *name, int pos);
+
+/*
+ * Use the name-hash to do a case-insensitive cache-entry lookup with
+ * the pathname and invalidate the cache-entry.
+ *
+ * Returns the number of cache-entries that we invalidated.
+ */
+static size_t handle_using_name_hash_icase(
+	struct index_state *istate, const char *name)
+{
+	struct cache_entry *ce = NULL;
+
+	ce = index_file_exists(istate, name, strlen(name), 1);
+	if (!ce)
+		return 0;
 
+	/*
+	 * A case-insensitive search in the name-hash using the
+	 * observed pathname found a cache-entry, so the observed path
+	 * is case-incorrect.  Invalidate the cache-entry and use the
+	 * correct spelling from the cache-entry to invalidate the
+	 * untracked-cache.  Since we now have sparse-directories in
+	 * the index, the observed pathname may represent a regular
+	 * file or a sparse-index directory.
+	 *
+	 * Note that we should not have seen FSEvents for a
+	 * sparse-index directory, but we handle it just in case.
+	 *
+	 * Either way, we know that there are not any cache-entries for
+	 * children inside the cone of the directory, so we don't need to
+	 * do the usual scan.
+	 */
 	trace_printf_key(&trace_fsmonitor,
-			 "fsmonitor_refresh_callback '%s' (pos %d)",
-			 name, pos);
+			 "fsmonitor_refresh_callback MAP: '%s' '%s'",
+			 name, ce->name);
 
-	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;
+	/*
+	 * NEEDSWORK: We used the name-hash to find the correct
+	 * case-spelling of the pathname in the cache-entry[], so
+	 * technically this is a tracked file or a sparse-directory.
+	 * It should not have any entries in the untracked-cache, so
+	 * we should not need to use the case-corrected spelling to
+	 * invalidate the the untracked-cache.  So we may not need to
+	 * do this.  For now, I'm going to be conservative and always
+	 * do it; we can revisit this later.
+	 */
+	untracked_cache_invalidate_trimmed_path(istate, ce->name, 0);
 
-		/* 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;
-		}
+	invalidate_ce_fsm(ce);
+	return 1;
+}
+
+/*
+ * Use the dir-name-hash to find the correct-case spelling of the
+ * directory.  Use the canonical spelling to invalidate all of the
+ * cache-entries within the matching cone.
+ *
+ * Returns the number of cache-entries that we invalidated.
+ */
+static size_t handle_using_dir_name_hash_icase(
+	struct index_state *istate, const char *name)
+{
+	struct strbuf canonical_path = STRBUF_INIT;
+	int pos;
+	size_t len = strlen(name);
+	size_t nr_in_cone;
+
+	if (name[len - 1] == '/')
+		len--;
+
+	if (!index_dir_find(istate, name, len, &canonical_path))
+		return 0; /* name is untracked */
 
+	if (!memcmp(name, canonical_path.buf, canonical_path.len)) {
+		strbuf_release(&canonical_path);
 		/*
-		 * We need to remove the traling "/" from the path
-		 * for the untracked cache.
+		 * NEEDSWORK: Our caller already tried an exact match
+		 * and failed to find one.  They called us to do an
+		 * ICASE match, so we should never get an exact match,
+		 * so we could promote this to a BUG() here if we
+		 * wanted to.  It doesn't hurt anything to just return
+		 * 0 and go on because we should never get here.  Or we
+		 * could just get rid of the memcmp() and this "if"
+		 * clause completely.
 		 */
-		name[len - 1] = '\0';
-	} else if (pos >= 0) {
+		BUG("handle_using_dir_name_hash_icase(%s) did not exact match",
+		    name);
+	}
+
+	trace_printf_key(&trace_fsmonitor,
+			 "fsmonitor_refresh_callback MAP: '%s' '%s'",
+			 name, canonical_path.buf);
+
+	/*
+	 * The dir-name-hash only tells us the corrected spelling of
+	 * the prefix.  We have to use this canonical path to do a
+	 * lookup in the cache-entry array so that we repeat the
+	 * original search using the case-corrected spelling.
+	 */
+	strbuf_addch(&canonical_path, '/');
+	pos = index_name_pos(istate, canonical_path.buf,
+			     canonical_path.len);
+	nr_in_cone = handle_path_with_trailing_slash(
+		istate, canonical_path.buf, pos);
+	strbuf_release(&canonical_path);
+	return nr_in_cone;
+}
+
+/*
+ * 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.
+ *
+ * Return the number of cache-entries that we invalidated.
+ */
+static size_t handle_path_without_trailing_slash(
+	struct index_state *istate, const char *name, int pos)
+{
+	/*
+	 * Mark the untracked cache dirty for this path (regardless of
+	 * whether or not we find an exact match for it in the index).
+	 * Since the path is unqualified (no trailing slash hint in the
+	 * FSEvent), it may refer to a file or directory. So we should
+	 * not assume one or the other and should always let the untracked
+	 * cache decide what needs to invalidated.
+	 */
+	untracked_cache_invalidate_trimmed_path(istate, name, 0);
+
+	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;
+		invalidate_ce_fsm(istate->cache[pos]);
+		return 1;
 	} else {
+		size_t nr_in_cone;
+		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.
 		 */
+		strbuf_add(&work_path, name, strlen(name));
+		strbuf_addch(&work_path, '/');
+		pos = index_name_pos(istate, work_path.buf, work_path.len);
+		nr_in_cone = handle_path_with_trailing_slash(
+			istate, work_path.buf, pos);
+		strbuf_release(&work_path);
+		return nr_in_cone;
+	}
+}
+
+/*
+ * The daemon can decorate directory events, such as a move or rename,
+ * by adding a trailing slash to the observed name.  Use this to
+ * explicitly invalidate the entire cone under that directory.
+ *
+ * The daemon can only reliably do that if the OS FSEvent contains
+ * sufficient information in the event.
+ *
+ * macOS FSEvents have enough information.
+ *
+ * Other platforms may or may not be able to do it (and it might
+ * depend on the type of event (for example, a daemon could lstat() an
+ * observed pathname after a rename, but not after a delete)).
+ *
+ * If we find an exact match in the index for a path with a trailing
+ * slash, it means that we matched a sparse-index directory in a
+ * cone-mode sparse-checkout (since that's the only time we have
+ * directories in the index).  We should never see this in practice
+ * (because sparse directories should not be present and therefore
+ * not generating FS events).  Either way, we can treat them in the
+ * same way and just invalidate the cache-entry and the untracked
+ * cache (and in this case, the forward cache-entry scan won't find
+ * anything and it doesn't hurt to let it run).
+ *
+ * Return the number of cache-entries that we invalidated.  We will
+ * use this later to determine if we need to attempt a second
+ * case-insensitive search on case-insensitive file systems.  That is,
+ * if the search using the observed-case in the FSEvent yields any
+ * results, we assume the prefix is case-correct.  If there are no
+ * matches, we still don't know if the observed path is simply
+ * untracked or case-incorrect.
+ */
+static size_t handle_path_with_trailing_slash(
+	struct index_state *istate, const char *name, int pos)
+{
+	int i;
+	size_t nr_in_cone = 0;
+
+	/*
+	 * Mark the untracked cache dirty for this directory path
+	 * (regardless of whether or not we find an exact match for it
+	 * in the index or find it to be proper prefix of one or more
+	 * files in the index), since the FSEvent is hinting that
+	 * there may be changes on or within the directory.
+	 */
+	untracked_cache_invalidate_trimmed_path(istate, name, 0);
+
+	if (pos < 0)
 		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;
-		}
+	/* Mark all entries for the folder invalid */
+	for (i = pos; i < istate->cache_nr; i++) {
+		if (!starts_with(istate->cache[i]->name, name))
+			break;
+		invalidate_ce_fsm(istate->cache[i]);
+		nr_in_cone++;
 	}
 
+	return nr_in_cone;
+}
+
+static void fsmonitor_refresh_callback(struct index_state *istate, char *name)
+{
+	int len = strlen(name);
+	int pos = index_name_pos(istate, name, len);
+	size_t nr_in_cone;
+
+	trace_printf_key(&trace_fsmonitor,
+			 "fsmonitor_refresh_callback '%s' (pos %d)",
+			 name, pos);
+
+	if (name[len - 1] == '/')
+		nr_in_cone = handle_path_with_trailing_slash(istate, name, pos);
+	else
+		nr_in_cone = handle_path_without_trailing_slash(istate, name, pos);
+
 	/*
-	 * Mark the untracked cache dirty even if it wasn't found in the index
-	 * as it could be a new untracked file.
+	 * If we did not find an exact match for this pathname or any
+	 * cache-entries with this directory prefix and we're on a
+	 * case-insensitive file system, try again using the name-hash
+	 * and dir-name-hash.
 	 */
-	untracked_cache_invalidate_path(istate, name, 0);
+	if (!nr_in_cone && ignore_case) {
+		nr_in_cone = handle_using_name_hash_icase(istate, name);
+		if (!nr_in_cone)
+			nr_in_cone = handle_using_dir_name_hash_icase(
+				istate, name);
+	}
+
+	if (nr_in_cone)
+		trace_printf_key(&trace_fsmonitor,
+				 "fsmonitor_refresh_callback CNT: %d",
+				 (int)nr_in_cone);
 }
 
 /*