On Tue, Feb 13, 2024 at 08:52:17PM +0000, Jeff Hostetler via GitGitGadget wrote:
> From: Jeff Hostetler <jeffhostetler@xxxxxxxxxx>
>
> Teach fsmonitor_refresh_callback() to handle case-insensitive
> lookups if case-sensitive lookups fail on case-insensitive systems.
> This can cause 'git status' to report stale status for files if there
> are case issues/errors in the worktree.
>
> The FSMonitor daemon sends FSEvents using the observed spelling
> of each pathname. On case-insensitive file systems this may be
> different than the expected case spelling.
>
> The existing code uses index_name_pos() to find the cache-entry for
> the pathname in the FSEvent and clear the CE_FSMONITOR_VALID bit so
> that the worktree scan/index refresh will revisit and revalidate the
> path.
>
> On a case-insensitive file system, the exact match lookup may fail
> to find the associated cache-entry. This causes status to think that
> the cached CE flags are correct and skip over the file.
>
> Update the handling of directory-style FSEvents (ones containing a
> path with a trailing slash) to optionally use the name-hash if the
> case-correct search does not find a match.
>
> (The FSMonitor daemon can send directory FSEvents if the OS provides
> that information.)
>
> Signed-off-by: Jeff Hostetler <jeffhostetler@xxxxxxxxxx>
> ---
> fsmonitor.c | 122 +++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 120 insertions(+), 2 deletions(-)
>
> diff --git a/fsmonitor.c b/fsmonitor.c
> index 14585b6c516..73e6ac82af7 100644
> --- a/fsmonitor.c
> +++ b/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,6 +184,9 @@ static int query_fsmonitor_hook(struct repository *r,
> return result;
> }
>
> +static int fsmonitor_refresh_callback_slash(
> + struct index_state *istate, const char *name, int len, int pos);
> +
> /*
> * Invalidate the untracked cache for the given pathname. Copy the
> * buffer to a proper null-terminated string (since the untracked
> @@ -205,6 +209,84 @@ static void my_invalidate_untracked_cache(
> strbuf_release(&work_path);
> }
>
> +/*
> + * Use the name-hash to lookup the pathname.
> + *
> + * Returns the number of cache-entries that we invalidated.
> + */
The function not only looks up the path name, but also invalidates the
corresponding cache entry. You imply this with the second sentence, but
this could be a bit more explicit.
> +static int my_callback_name_hash(
> + struct index_state *istate, const char *name, int len)
I find the naming conventions here to be weird with the `my_` prefix.
> +{
> + struct cache_entry *ce = NULL;
> +
> + ce = index_file_exists(istate, name, len, 1);
> + if (!ce)
> + return 0;
Okay, `index_file_exists()` is called with `icase == 1` here. But is
that the correct thing to do on case-sensitive platforms? I would have
expected us to honor `core.ignoreCase` here.
Turns out, we only end up calling this function when `ignore_case` is
set, so we already do. I think this can be clarified both by giving the
function a better name and by documenting this in the comment. Also,
neither of this or the next function really are callbacks -- they only
happen to be called by a callback function.
I'd think something like `lookup_and_invalidate_path_icase()` and
`lookup_and_invalidate_dir_icase()` could help to clarify intent.
> + /*
> + * The index contains a case-insensitive match for the pathname.
> + * This could either be a regular file or a sparse-index directory.
> + *
> + * 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 map '%s' '%s'",
> + name, ce->name);
> +
> + my_invalidate_untracked_cache(istate, ce->name, ce->ce_namelen);
> +
> + ce->ce_flags &= ~CE_FSMONITOR_VALID;
> + return 1;
> +}
> +
> +/*
> + * Use the directory 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.
> + *
> + * The pathname MUST NOT have a trailing slash.
> + *
> + * Returns the number of cache-entries that we invalidated.
> + */
> +static int my_callback_dir_name_hash(
> + struct index_state *istate, const char *name, int len)
> +{
> + struct strbuf canonical_path = STRBUF_INIT;
> + int pos;
> + int nr_in_cone;
> +
> + if (!index_dir_exists2(istate, name, len, &canonical_path))
> + return 0; /* name is untracked */
> + if (!memcmp(name, canonical_path.buf, len)) {
> + strbuf_release(&canonical_path);
> + return 0; /* should not happen */
> + }
So in other words, this function should only be called when we know that
casing differs, and thus the passed-in name and the canonical name
should never be the same? If this case shouldn't ever happen, shouldn't
we report this as an error or use `BUG()` instead of silently ignoring
this mismatch of expectations?
Patrick
> + trace_printf_key(&trace_fsmonitor,
> + "fsmonitor_refresh_callback map '%s' '%s'",
> + name, canonical_path.buf);
> +
> + /*
> + * The directory 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
> + * 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 = fsmonitor_refresh_callback_slash(
> + istate, canonical_path.buf, canonical_path.len, pos);
> + strbuf_release(&canonical_path);
> + return nr_in_cone;
> +}
> +
> static void fsmonitor_refresh_callback_unqualified(
> struct index_state *istate, const char *name, int len, int pos)
> {
> @@ -269,7 +351,10 @@ static void fsmonitor_refresh_callback_unqualified(
> *
> * 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.
> + * case-insensitive search. That is, if a observed-case search 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 int fsmonitor_refresh_callback_slash(
> struct index_state *istate, const char *name, int len, int pos)
> @@ -293,17 +378,50 @@ static int fsmonitor_refresh_callback_slash(
> return nr_in_cone;
> }
>
> +/*
> + * On a case-insensitive FS, use the name-hash and directory name-hash
> + * to map the case of the observed path to the canonical case expected
> + * by the index.
> + *
> + * The given pathname includes the trailing slash.
> + *
> + * Return the number of cache-entries that we invalidated.
> + */
> +static int fsmonitor_refresh_callback_slash_icase(
> + struct index_state *istate, const char *name, int len)
> +{
> + int nr_in_cone;
> +
> + /*
> + * Look for a case-incorrect sparse-index directory.
> + */
> + nr_in_cone = my_callback_name_hash(istate, name, len);
> + if (nr_in_cone)
> + return nr_in_cone;
> +
> + /*
> + * (len-1) because we do not include the trailing slash in the
> + * pathname.
> + */
> + nr_in_cone = my_callback_dir_name_hash(istate, name, len-1);
> + 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);
> + int nr_in_cone;
> +
>
> trace_printf_key(&trace_fsmonitor,
> "fsmonitor_refresh_callback '%s' (pos %d)",
> name, pos);
>
> if (name[len - 1] == '/') {
> - fsmonitor_refresh_callback_slash(istate, name, len, pos);
> + nr_in_cone = fsmonitor_refresh_callback_slash(istate, name, len, pos);
> + if (ignore_case && !nr_in_cone)
> + fsmonitor_refresh_callback_slash_icase(istate, name, len);
> } else {
> fsmonitor_refresh_callback_unqualified(istate, name, len, pos);
> }
> --
> gitgitgadget
>
>
Attachment:
signature.asc
Description: PGP signature
