* Re: [PATCH v2 14/16] fsmonitor: support case-insensitive events
From: Jeff Hostetler @ 2024-02-26 20:47 UTC (permalink / raw)
To: Torsten Bögershausen, Jeff Hostetler via GitGitGadget
Cc: git, Patrick Steinhardt, Jeff Hostetler
In-Reply-To: <20240225131023.GC1952@tb-raspi4>
On 2/25/24 8:10 AM, Torsten Bögershausen wrote:
>> + if (!memcmp(name, canonical_path.buf, canonical_path.len)) {
>> + strbuf_release(&canonical_path);
>> + /*
>> + * 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 becaus we should never get here. Or we
>> + * could just get rid of the memcmp() and this "if"
>> + * clause completely.
>> + */
>> + return 0; /* should not happen */
>
> In very very theory, there may be a race-condition,
> when a directory is renamed very fast, more than once.
> I don't think, that the "it did not match exactly, but
> now it matches" is a problem.
> Question: Does it make sense to just remove this ?
> And, may be, find out that the "corrected spelling (tm)"
> of "DIR1" is not "dir1", neither "Dir1", but, exactly, "DIR1" ?
> Would that be a problem ?
>
I just meant that the dir-name-hash that we computed when
we loaded the index found an exact-case match here that
wasn't found when called index_name_pos() and the negative
"pos" didn't point to this exact-case prefix. This should
not happen.
Yeah I didn't think it should be a fatal condition, but
since it shouldn't happen, we can make it a BUG() and see.
Jeff
^ permalink raw reply
* Re: [PATCH v2 14/16] fsmonitor: support case-insensitive events
From: Junio C Hamano @ 2024-02-26 21:18 UTC (permalink / raw)
To: Jeff Hostetler
Cc: Jeff Hostetler via GitGitGadget, git, Patrick Steinhardt,
Jeff Hostetler
In-Reply-To: <b6cfe94a-adcf-04fa-2ed8-dfd4f0fdc77a@jeffhostetler.com>
Jeff Hostetler <git@jeffhostetler.com> writes:
> I'm not happy about this (and no, I haven't had time to measure the
> perf hit we'll take), but right now I'm just worried about the
> correctness -- I've had several reports of stale/incomplete status
> when IDE tools change file/directory case in unexpected ways....
Of course, it is a good discipline, and I fully support the
direction, to focus on the correctness first.
^ permalink raw reply
* [PATCH v3 00/14] FSMonitor edge cases on case-insensitive file systems
From: Jeff Hostetler via GitGitGadget @ 2024-02-26 21:39 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Jeff Hostetler, Torsten Bögershausen,
Jeff Hostetler
In-Reply-To: <pull.1662.v2.git.1708658300.gitgitgadget@gmail.com>
Here is version 3. I think I have addressed the remaining comments.
I cleaned up the test code to use the test_expect_failure at the beginning
and squashed in the test_expect_success version of tests into the final
commit in the series.
I moved the invalidate_ce_fsm() commit earlier in the series, so that the
final commit actually uses all of the up-to-this-point changes to fix the
problem.
I converted a few "should not happens" to BUG()s.
Thanks to everyone for their time and attention reviewing this. Jeff
Jeff Hostetler (14):
name-hash: add index_dir_find()
t7527: add case-insensitve test for FSMonitor
fsmonitor: refactor refresh callback on directory events
fsmonitor: clarify handling of directory events in callback helper
fsmonitor: refactor refresh callback for non-directory events
dir: create untracked_cache_invalidate_trimmed_path()
fsmonitor: refactor untracked-cache invalidation
fsmonitor: move untracked-cache invalidation into helper functions
fsmonitor: return invalidated cache-entry count on directory event
fsmonitor: remove custom loop from non-directory path handler
fsmonitor: return invalided cache-entry count on non-directory event
fsmonitor: trace the new invalidated cache-entry count
fsmonitor: refactor bit invalidation in refresh callback
fsmonitor: support case-insensitive events
dir.c | 20 +++
dir.h | 7 +
fsmonitor.c | 312 +++++++++++++++++++++++++++++------
name-hash.c | 9 +-
name-hash.h | 7 +-
t/t7527-builtin-fsmonitor.sh | 223 +++++++++++++++++++++++++
6 files changed, 522 insertions(+), 56 deletions(-)
base-commit: f41f85c9ec8d4d46de0fd5fded88db94d3ec8c11
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1662%2Fjeffhostetler%2Ffsmonitor-ignore-case-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1662/jeffhostetler/fsmonitor-ignore-case-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1662
Range-diff vs v2:
1: 03b07d9c25e ! 1: 64ae07aaeaa name-hash: add index_dir_find()
@@ Metadata
## Commit message ##
name-hash: add index_dir_find()
- Replace the index_dir_exists() function with index_dir_find() and
- change the API to take an optional strbuf to return the canonical
- spelling of the matched directory prefix.
+ index_dir_exists() returns a boolean to indicate if there is a
+ case-insensitive match in the directory name-hash, but does not
+ provide the caller with the exact spelling of that match.
- Create an index_dir_exists() wrapper macro for existing callers.
+ Create index_dir_find() to do the case-insensitive search *and*
+ optionally return the spelling of the matched directory prefix in a
+ provided strbuf.
- The existing index_dir_exists() returns a boolean to indicate if
- there is a case-insensitive match in the directory name-hash, but
- it doesn't tell the caller the exact spelling of that match.
-
- The new version also copies the matched spelling to a provided strbuf.
- This lets the caller, for example, then call index_name_pos() with the
- correct case to search the cache-entry array for the real insertion
- position.
+ To avoid code duplication, convert index_dir_exists() to be a trivial
+ wrapper around the new index_dir_find().
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
2: 7778cee1c10 ! 2: beeebf55963 t7527: add case-insensitve test for FSMonitor
@@ t/t7527-builtin-fsmonitor.sh: test_expect_success 'split-index and FSMonitor wor
+#
+# The setup is a little contrived.
+#
-+test_expect_success CASE_INSENSITIVE_FS 'fsmonitor subdir case wrong on disk' '
++test_expect_failure CASE_INSENSITIVE_FS 'fsmonitor subdir case wrong on disk' '
+ test_when_finished "stop_daemon_delete_repo subdir_case_wrong" &&
+
+ git init subdir_case_wrong &&
@@ t/t7527-builtin-fsmonitor.sh: test_expect_success 'split-index and FSMonitor wor
+ grep -q " M AAA" "$PWD/subdir_case_wrong.out" &&
+ grep -q " M zzz" "$PWD/subdir_case_wrong.out" &&
+
-+ # However, with the fsmonitor client bug, the "(pos -3)" causes
-+ # the client to not update the bit and never rescan the file
-+ # and therefore not report it as dirty.
-+ ! grep -q " M dir1/dir2/dir3/file3" "$PWD/subdir_case_wrong.out"
++ # Expect Breakage: with the case confusion, the "(pos -3)" causes
++ # the client to not clear the CE_FSMONITOR_VALID bit and therefore
++ # status will not rescan the file and therefore not report it as dirty.
++ grep -q " M dir1/dir2/dir3/file3" "$PWD/subdir_case_wrong.out"
+'
+
-+test_expect_success CASE_INSENSITIVE_FS 'fsmonitor file case wrong on disk' '
++test_expect_failure CASE_INSENSITIVE_FS 'fsmonitor file case wrong on disk' '
+ test_when_finished "stop_daemon_delete_repo file_case_wrong" &&
+
+ git init file_case_wrong &&
@@ t/t7527-builtin-fsmonitor.sh: test_expect_success 'split-index and FSMonitor wor
+ grep -q "fsmonitor_refresh_callback.*FILE-3-A.*pos -3" "$PWD/file_case_wrong-try3.log" &&
+ grep -q "fsmonitor_refresh_callback.*file-4-a.*pos -9" "$PWD/file_case_wrong-try3.log" &&
+
-+ # Status should say these files are modified, but with the case
-+ # bug, the "pos -3" cause the client to not update the FSM bit
-+ # and never cause the file to be rescanned and therefore to not
-+ # report it dirty.
-+ ! grep -q " M dir1/dir2/dir3/file-3-a" "$PWD/file_case_wrong-try3.out" &&
-+ ! grep -q " M dir1/dir2/dir4/FILE-4-A" "$PWD/file_case_wrong-try3.out"
++ # Expect Breakage: with the case confusion, the "(pos-3)" and
++ # "(pos -9)" causes the client to not clear the CE_FSMONITOR_VALID
++ # bit and therefore status will not rescan the files and therefore
++ # not report them as dirty.
++ grep -q " M dir1/dir2/dir3/file-3-a" "$PWD/file_case_wrong-try3.out" &&
++ grep -q " M dir1/dir2/dir4/FILE-4-A" "$PWD/file_case_wrong-try3.out"
+'
+
test_done
3: dad079ade7f < -: ----------- t7527: temporarily disable case-insensitive tests
4: 5516670e30e = 3: 518cb4dd5df fsmonitor: refactor refresh callback on directory events
5: c04fd4eae94 = 4: 9a4b5bf990b fsmonitor: clarify handling of directory events in callback helper
6: 7ee6ca1aefd ! 5: 348b9b0c94e fsmonitor: refactor refresh callback for non-directory events
@@ Metadata
## Commit message ##
fsmonitor: refactor refresh callback for non-directory events
- Move the code handle unqualified FSEvents (without a trailing slash)
- into a helper function.
+ Move the code that handles unqualified FSEvents (without a trailing
+ slash) into a helper function.
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
7: 99c0d3e0742 ! 6: ed735e3f1cb dir: create untracked_cache_invalidate_trimmed_path()
@@ dir.c: void untracked_cache_invalidate_path(struct index_state *istate,
+ size_t len = strlen(path);
+
+ if (!len)
-+ return; /* should not happen */
++ BUG("untracked_cache_invalidate_trimmed_path given zero length path");
+
+ if (path[len - 1] != '/') {
+ untracked_cache_invalidate_path(istate, path, safe_path);
8: f2d6765d84f = 7: 2a43c6cbe0d fsmonitor: refactor untracked-cache invalidation
9: af6f57ab3e6 ! 8: 6e87ea6deaf fsmonitor: move untracked invalidation into helper functions
@@ Metadata
Author: Jeff Hostetler <jeffhostetler@github.com>
## Commit message ##
- fsmonitor: move untracked invalidation into helper functions
+ fsmonitor: move untracked-cache invalidation into helper functions
- Move the call to invalidate the untracked cache for the FSEvent
+ Move the call to invalidate the untracked-cache for the FSEvent
pathname into the two helper functions.
In a later commit in this series, we will call these helpers
from other contexts and it safer to include the UC invalidation
- in the helper than to remember to also add it to each helper
+ in the helpers than to remember to also add it to each helper
call-site.
+ This has the side-effect of invalidating the UC *before* we
+ invalidate the ce_flags in the cache-entry. These activities
+ are independent and do not affect each other. Also, by doing
+ the UC work first, we can avoid worrying about "early returns"
+ or the need for the usual "goto the end" in each of the
+ handler functions.
+
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
## fsmonitor.c ##
10: 623c6f06e21 = 9: 5fea8b9476e fsmonitor: return invalidated cache-entry count on directory event
11: 1853f77d333 = 10: 3fa7536cf80 fsmonitor: remove custom loop from non-directory path handler
12: f77d68c78ad ! 11: 53f73c1515d fsmonitor: return invalided cache-entry count on non-directory event
@@ Metadata
## Commit message ##
fsmonitor: return invalided cache-entry count on non-directory event
- Teah the refresh callback helper function for unqualified FSEvents
+ Teach the refresh callback helper function for unqualified FSEvents
(pathnames without a trailing slash) to return the number of
cache-entries that were invalided in response to the event.
13: 58b36673e15 = 12: 0148319aea5 fsmonitor: trace the new invalidated cache-entry count
15: 3a20065dbf8 ! 13: 04867eccfcd fsmonitor: refactor bit invalidation in refresh callback
@@ Commit message
it to help debug edge cases.
This is similar to the existing `mark_fsmonitor_invalid()` function,
- but we don't need the extra stuff that it does.
+ but it also does untracked-cache invalidation and we've already
+ handled that in the refresh-callback handlers, so but we don't need
+ to repeat that.
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
## fsmonitor.c ##
@@ fsmonitor.c: static int query_fsmonitor_hook(struct repository *r,
- static size_t handle_path_with_trailing_slash(
- struct index_state *istate, const char *name, int pos);
+ return result;
+ }
+/*
+ * Invalidate the FSM bit on this CE. This is like mark_fsmonitor_invalid()
-+ * but we've already handled the untracked-cache and I want a different
-+ * trace message.
++ * 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)
+{
-+ if (ce->ce_flags & CE_FSMONITOR_VALID)
++ 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;
++ ce->ce_flags &= ~CE_FSMONITOR_VALID;
++ }
+}
+
- /*
- * Use the name-hash to do a case-insensitive cache-entry lookup with
- * the pathname and invalidate the cache-entry.
-@@ fsmonitor.c: static size_t handle_using_name_hash_icase(
-
- untracked_cache_invalidate_trimmed_path(istate, ce->name, 0);
-
-- ce->ce_flags &= ~CE_FSMONITOR_VALID;
-+ invalidate_ce_fsm(ce);
- return 1;
- }
+ static size_t handle_path_with_trailing_slash(
+ struct index_state *istate, const char *name, int pos);
@@ fsmonitor.c: static size_t handle_path_without_trailing_slash(
* cache-entry with the same pathname, nor for a cone
14: 288f3f4e54e ! 14: ec036c04d1b fsmonitor: support case-insensitive events
@@ Commit message
Update event handling to optionally use the name-hash and dir-name-hash
if necessary.
+ Also update t7527 to convert the "test_expect_failure" to "_success"
+ now that we have fixed the bug.
+
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
## fsmonitor.c ##
@@ fsmonitor.c
#include "run-command.h"
#include "strbuf.h"
#include "trace2.h"
-@@ fsmonitor.c: static int query_fsmonitor_hook(struct repository *r,
+@@ fsmonitor.c: static void invalidate_ce_fsm(struct cache_entry *ce)
static size_t handle_path_with_trailing_slash(
struct index_state *istate, const char *name, int pos);
@@ fsmonitor.c: static int query_fsmonitor_hook(struct repository *r,
+ "fsmonitor_refresh_callback MAP: '%s' '%s'",
+ name, ce->name);
+
++ /*
++ * 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);
+
-+ ce->ce_flags &= ~CE_FSMONITOR_VALID;
++ invalidate_ce_fsm(ce);
+ return 1;
+}
+
@@ fsmonitor.c: static int query_fsmonitor_hook(struct repository *r,
+ * 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 becaus we should never get here. Or we
++ * 0 and go on because we should never get here. Or we
+ * could just get rid of the memcmp() and this "if"
+ * clause completely.
+ */
-+ return 0; /* should not happen */
++ BUG("handle_using_dir_name_hash_icase(%s) did not exact match",
++ name);
+ }
+
+ trace_printf_key(&trace_fsmonitor,
@@ fsmonitor.c: static void fsmonitor_refresh_callback(struct index_state *istate,
if (nr_in_cone)
trace_printf_key(&trace_fsmonitor,
"fsmonitor_refresh_callback CNT: %d",
+
+ ## t/t7527-builtin-fsmonitor.sh ##
+@@ t/t7527-builtin-fsmonitor.sh: test_expect_success 'split-index and FSMonitor work well together' '
+ #
+ # The setup is a little contrived.
+ #
+-test_expect_failure CASE_INSENSITIVE_FS 'fsmonitor subdir case wrong on disk' '
++test_expect_success CASE_INSENSITIVE_FS 'fsmonitor subdir case wrong on disk' '
+ test_when_finished "stop_daemon_delete_repo subdir_case_wrong" &&
+
+ git init subdir_case_wrong &&
+@@ t/t7527-builtin-fsmonitor.sh: test_expect_failure CASE_INSENSITIVE_FS 'fsmonitor subdir case wrong on disk' '
+
+ grep -q "dir1/DIR2/dir3/file3.*pos -3" "$PWD/subdir_case_wrong.log1" &&
+
++ # Verify that we get a mapping event to correct the case.
++ grep -q "MAP:.*dir1/DIR2/dir3/file3.*dir1/dir2/dir3/file3" \
++ "$PWD/subdir_case_wrong.log1" &&
++
+ # The refresh-callbacks should have caused "git status" to clear
+ # the CE_FSMONITOR_VALID bit on each of those files and caused
+ # the worktree scan to visit them and mark them as modified.
+ grep -q " M AAA" "$PWD/subdir_case_wrong.out" &&
+ grep -q " M zzz" "$PWD/subdir_case_wrong.out" &&
+-
+- # Expect Breakage: with the case confusion, the "(pos -3)" causes
+- # the client to not clear the CE_FSMONITOR_VALID bit and therefore
+- # status will not rescan the file and therefore not report it as dirty.
+ grep -q " M dir1/dir2/dir3/file3" "$PWD/subdir_case_wrong.out"
+ '
+
+-test_expect_failure CASE_INSENSITIVE_FS 'fsmonitor file case wrong on disk' '
++test_expect_success CASE_INSENSITIVE_FS 'fsmonitor file case wrong on disk' '
+ test_when_finished "stop_daemon_delete_repo file_case_wrong" &&
+
+ git init file_case_wrong &&
+@@ t/t7527-builtin-fsmonitor.sh: test_expect_failure CASE_INSENSITIVE_FS 'fsmonitor file case wrong on disk' '
+ GIT_TRACE_FSMONITOR="$PWD/file_case_wrong-try3.log" \
+ git -C file_case_wrong --no-optional-locks status --short \
+ >"$PWD/file_case_wrong-try3.out" &&
++
++ # Verify that we get a mapping event to correct the case.
++ grep -q "fsmonitor_refresh_callback MAP:.*dir1/dir2/dir3/FILE-3-A.*dir1/dir2/dir3/file-3-a" \
++ "$PWD/file_case_wrong-try3.log" &&
++ grep -q "fsmonitor_refresh_callback MAP:.*dir1/dir2/dir4/file-4-a.*dir1/dir2/dir4/FILE-4-A" \
++ "$PWD/file_case_wrong-try3.log" &&
++
+ # FSEvents are in observed case.
+ grep -q "fsmonitor_refresh_callback.*FILE-3-A.*pos -3" "$PWD/file_case_wrong-try3.log" &&
+ grep -q "fsmonitor_refresh_callback.*file-4-a.*pos -9" "$PWD/file_case_wrong-try3.log" &&
+
+- # Expect Breakage: with the case confusion, the "(pos-3)" and
+- # "(pos -9)" causes the client to not clear the CE_FSMONITOR_VALID
+- # bit and therefore status will not rescan the files and therefore
+- # not report them as dirty.
++ # The refresh-callbacks should have caused "git status" to clear
++ # the CE_FSMONITOR_VALID bit on each of those files and caused
++ # the worktree scan to visit them and mark them as modified.
+ grep -q " M dir1/dir2/dir3/file-3-a" "$PWD/file_case_wrong-try3.out" &&
+ grep -q " M dir1/dir2/dir4/FILE-4-A" "$PWD/file_case_wrong-try3.out"
+ '
16: 467d3c1fe2c < -: ----------- t7527: update case-insenstive fsmonitor test
--
gitgitgadget
^ permalink raw reply
* [PATCH v3 01/14] name-hash: add index_dir_find()
From: Jeff Hostetler via GitGitGadget @ 2024-02-26 21:39 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Jeff Hostetler, Torsten Bögershausen,
Jeff Hostetler, Jeff Hostetler
In-Reply-To: <pull.1662.v3.git.1708983565.gitgitgadget@gmail.com>
From: Jeff Hostetler <jeffhostetler@github.com>
index_dir_exists() returns a boolean to indicate if there is a
case-insensitive match in the directory name-hash, but does not
provide the caller with the exact spelling of that match.
Create index_dir_find() to do the case-insensitive search *and*
optionally return the spelling of the matched directory prefix in a
provided strbuf.
To avoid code duplication, convert index_dir_exists() to be a trivial
wrapper around the new index_dir_find().
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
---
name-hash.c | 9 ++++++++-
name-hash.h | 7 ++++++-
2 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/name-hash.c b/name-hash.c
index 251f036eef6..3a58ce03d9c 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -685,13 +685,20 @@ static int same_name(const struct cache_entry *ce, const char *name, int namelen
return slow_same_name(name, namelen, ce->name, len);
}
-int index_dir_exists(struct index_state *istate, const char *name, int namelen)
+int index_dir_find(struct index_state *istate, const char *name, int namelen,
+ struct strbuf *canonical_path)
{
struct dir_entry *dir;
lazy_init_name_hash(istate);
expand_to_path(istate, name, namelen, 0);
dir = find_dir_entry(istate, name, namelen);
+
+ if (canonical_path && dir && dir->nr) {
+ strbuf_reset(canonical_path);
+ strbuf_add(canonical_path, dir->name, dir->namelen);
+ }
+
return dir && dir->nr;
}
diff --git a/name-hash.h b/name-hash.h
index b1b4b0fb337..0cbfc428631 100644
--- a/name-hash.h
+++ b/name-hash.h
@@ -4,7 +4,12 @@
struct cache_entry;
struct index_state;
-int index_dir_exists(struct index_state *istate, const char *name, int namelen);
+
+int index_dir_find(struct index_state *istate, const char *name, int namelen,
+ struct strbuf *canonical_path);
+
+#define index_dir_exists(i, n, l) index_dir_find((i), (n), (l), NULL)
+
void adjust_dirname_case(struct index_state *istate, char *name);
struct cache_entry *index_file_exists(struct index_state *istate, const char *name, int namelen, int igncase);
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 02/14] t7527: add case-insensitve test for FSMonitor
From: Jeff Hostetler via GitGitGadget @ 2024-02-26 21:39 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Jeff Hostetler, Torsten Bögershausen,
Jeff Hostetler, Jeff Hostetler
In-Reply-To: <pull.1662.v3.git.1708983565.gitgitgadget@gmail.com>
From: Jeff Hostetler <jeffhostetler@github.com>
The FSMonitor client code trusts the spelling of the pathnames in the
FSEvents received from the FSMonitor daemon. On case-insensitive file
systems, these OBSERVED pathnames may be spelled differently than the
EXPECTED pathnames listed in the .git/index. This causes a miss when
using `index_name_pos()` which expects the given case to be correct.
When this happens, the FSMonitor client code does not update the state
of the CE_FSMONITOR_VALID bit when refreshing the index (and before
starting to scan the worktree).
This results in modified files NOT being reported by `git status` when
there is a discrepancy in the case-spelling of a tracked file's
pathname.
This commit contains a (rather contrived) test case to demonstrate
this. A later commit in this series will update the FSMonitor client
code to recognize these discrepancies and update the CE_ bit accordingly.
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
---
t/t7527-builtin-fsmonitor.sh | 217 +++++++++++++++++++++++++++++++++++
1 file changed, 217 insertions(+)
diff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh
index 363f9dc0e41..830f2d9de33 100755
--- a/t/t7527-builtin-fsmonitor.sh
+++ b/t/t7527-builtin-fsmonitor.sh
@@ -1037,4 +1037,221 @@ test_expect_success 'split-index and FSMonitor work well together' '
)
'
+# The FSMonitor daemon reports the OBSERVED pathname of modified files
+# and thus contains the OBSERVED spelling on case-insensitive file
+# systems. The daemon does not (and should not) load the .git/index
+# file and therefore does not know the expected case-spelling. Since
+# it is possible for the user to create files/subdirectories with the
+# incorrect case, a modified file event for a tracked will not have
+# the EXPECTED case. This can cause `index_name_pos()` to incorrectly
+# report that the file is untracked. This causes the client to fail to
+# mark the file as possibly dirty (keeping the CE_FSMONITOR_VALID bit
+# set) so that `git status` will avoid inspecting it and thus not
+# present in the status output.
+#
+# The setup is a little contrived.
+#
+test_expect_failure CASE_INSENSITIVE_FS 'fsmonitor subdir case wrong on disk' '
+ test_when_finished "stop_daemon_delete_repo subdir_case_wrong" &&
+
+ git init subdir_case_wrong &&
+ (
+ cd subdir_case_wrong &&
+ echo x >AAA &&
+ echo x >BBB &&
+
+ mkdir dir1 &&
+ echo x >dir1/file1 &&
+ mkdir dir1/dir2 &&
+ echo x >dir1/dir2/file2 &&
+ mkdir dir1/dir2/dir3 &&
+ echo x >dir1/dir2/dir3/file3 &&
+
+ echo x >yyy &&
+ echo x >zzz &&
+ git add . &&
+ git commit -m "data" &&
+
+ # This will cause "dir1/" and everything under it
+ # to be deleted.
+ git sparse-checkout set --cone --sparse-index &&
+
+ # Create dir2 with the wrong case and then let Git
+ # repopulate dir3 -- it will not correct the spelling
+ # of dir2.
+ mkdir dir1 &&
+ mkdir dir1/DIR2 &&
+ git sparse-checkout add dir1/dir2/dir3
+ ) &&
+
+ start_daemon -C subdir_case_wrong --tf "$PWD/subdir_case_wrong.trace" &&
+
+ # Enable FSMonitor in the client. Run enough commands for
+ # the .git/index to sync up with the daemon with everything
+ # marked clean.
+ git -C subdir_case_wrong config core.fsmonitor true &&
+ git -C subdir_case_wrong update-index --fsmonitor &&
+ git -C subdir_case_wrong status &&
+
+ # Make some files dirty so that FSMonitor gets FSEvents for
+ # each of them.
+ echo xx >>subdir_case_wrong/AAA &&
+ echo xx >>subdir_case_wrong/dir1/DIR2/dir3/file3 &&
+ echo xx >>subdir_case_wrong/zzz &&
+
+ GIT_TRACE_FSMONITOR="$PWD/subdir_case_wrong.log" \
+ git -C subdir_case_wrong --no-optional-locks status --short \
+ >"$PWD/subdir_case_wrong.out" &&
+
+ # "git status" should have gotten file events for each of
+ # the 3 files.
+ #
+ # "dir2" should be in the observed case on disk.
+ grep "fsmonitor_refresh_callback" \
+ <"$PWD/subdir_case_wrong.log" \
+ >"$PWD/subdir_case_wrong.log1" &&
+
+ grep -q "AAA.*pos 0" "$PWD/subdir_case_wrong.log1" &&
+ grep -q "zzz.*pos 6" "$PWD/subdir_case_wrong.log1" &&
+
+ grep -q "dir1/DIR2/dir3/file3.*pos -3" "$PWD/subdir_case_wrong.log1" &&
+
+ # The refresh-callbacks should have caused "git status" to clear
+ # the CE_FSMONITOR_VALID bit on each of those files and caused
+ # the worktree scan to visit them and mark them as modified.
+ grep -q " M AAA" "$PWD/subdir_case_wrong.out" &&
+ grep -q " M zzz" "$PWD/subdir_case_wrong.out" &&
+
+ # Expect Breakage: with the case confusion, the "(pos -3)" causes
+ # the client to not clear the CE_FSMONITOR_VALID bit and therefore
+ # status will not rescan the file and therefore not report it as dirty.
+ grep -q " M dir1/dir2/dir3/file3" "$PWD/subdir_case_wrong.out"
+'
+
+test_expect_failure CASE_INSENSITIVE_FS 'fsmonitor file case wrong on disk' '
+ test_when_finished "stop_daemon_delete_repo file_case_wrong" &&
+
+ git init file_case_wrong &&
+ (
+ cd file_case_wrong &&
+ echo x >AAA &&
+ echo x >BBB &&
+
+ mkdir dir1 &&
+ mkdir dir1/dir2 &&
+ mkdir dir1/dir2/dir3 &&
+ echo x >dir1/dir2/dir3/FILE-3-B &&
+ echo x >dir1/dir2/dir3/XXXX-3-X &&
+ echo x >dir1/dir2/dir3/file-3-a &&
+ echo x >dir1/dir2/dir3/yyyy-3-y &&
+ mkdir dir1/dir2/dir4 &&
+ echo x >dir1/dir2/dir4/FILE-4-A &&
+ echo x >dir1/dir2/dir4/XXXX-4-X &&
+ echo x >dir1/dir2/dir4/file-4-b &&
+ echo x >dir1/dir2/dir4/yyyy-4-y &&
+
+ echo x >yyy &&
+ echo x >zzz &&
+ git add . &&
+ git commit -m "data"
+ ) &&
+
+ start_daemon -C file_case_wrong --tf "$PWD/file_case_wrong.trace" &&
+
+ # Enable FSMonitor in the client. Run enough commands for
+ # the .git/index to sync up with the daemon with everything
+ # marked clean.
+ git -C file_case_wrong config core.fsmonitor true &&
+ git -C file_case_wrong update-index --fsmonitor &&
+ git -C file_case_wrong status &&
+
+ # Make some files dirty so that FSMonitor gets FSEvents for
+ # each of them.
+ echo xx >>file_case_wrong/AAA &&
+ echo xx >>file_case_wrong/zzz &&
+
+ # Rename some files so that FSMonitor sees a create and delete
+ # FSEvent for each. (A simple "mv foo FOO" is not portable
+ # between macOS and Windows. It works on both platforms, but makes
+ # the test messy, since (1) one platform updates "ctime" on the
+ # moved file and one does not and (2) it causes a directory event
+ # on one platform and not on the other which causes additional
+ # scanning during "git status" which causes a "H" vs "h" discrepancy
+ # in "git ls-files -f".) So old-school it and move it out of the
+ # way and copy it to the case-incorrect name so that we get fresh
+ # "ctime" and "mtime" values.
+
+ mv file_case_wrong/dir1/dir2/dir3/file-3-a file_case_wrong/dir1/dir2/dir3/ORIG &&
+ cp file_case_wrong/dir1/dir2/dir3/ORIG file_case_wrong/dir1/dir2/dir3/FILE-3-A &&
+ rm file_case_wrong/dir1/dir2/dir3/ORIG &&
+ mv file_case_wrong/dir1/dir2/dir4/FILE-4-A file_case_wrong/dir1/dir2/dir4/ORIG &&
+ cp file_case_wrong/dir1/dir2/dir4/ORIG file_case_wrong/dir1/dir2/dir4/file-4-a &&
+ rm file_case_wrong/dir1/dir2/dir4/ORIG &&
+
+ # Run status enough times to fully sync.
+ #
+ # The first instance should get the create and delete FSEvents
+ # for each pair. Status should update the index with a new FSM
+ # token (so the next invocation will not see data for these
+ # events).
+
+ GIT_TRACE_FSMONITOR="$PWD/file_case_wrong-try1.log" \
+ git -C file_case_wrong status --short \
+ >"$PWD/file_case_wrong-try1.out" &&
+ grep -q "fsmonitor_refresh_callback.*FILE-3-A.*pos -3" "$PWD/file_case_wrong-try1.log" &&
+ grep -q "fsmonitor_refresh_callback.*file-3-a.*pos 4" "$PWD/file_case_wrong-try1.log" &&
+ grep -q "fsmonitor_refresh_callback.*FILE-4-A.*pos 6" "$PWD/file_case_wrong-try1.log" &&
+ grep -q "fsmonitor_refresh_callback.*file-4-a.*pos -9" "$PWD/file_case_wrong-try1.log" &&
+
+ # FSM refresh will have invalidated the FSM bit and cause a regular
+ # (real) scan of these tracked files, so they should have "H" status.
+ # (We will not see a "h" status until the next refresh (on the next
+ # command).)
+
+ git -C file_case_wrong ls-files -f >"$PWD/file_case_wrong-lsf1.out" &&
+ grep -q "H dir1/dir2/dir3/file-3-a" "$PWD/file_case_wrong-lsf1.out" &&
+ grep -q "H dir1/dir2/dir4/FILE-4-A" "$PWD/file_case_wrong-lsf1.out" &&
+
+
+ # Try the status again. We assume that the above status command
+ # advanced the token so that the next one will not see those events.
+
+ GIT_TRACE_FSMONITOR="$PWD/file_case_wrong-try2.log" \
+ git -C file_case_wrong status --short \
+ >"$PWD/file_case_wrong-try2.out" &&
+ ! grep -q "fsmonitor_refresh_callback.*FILE-3-A.*pos" "$PWD/file_case_wrong-try2.log" &&
+ ! grep -q "fsmonitor_refresh_callback.*file-3-a.*pos" "$PWD/file_case_wrong-try2.log" &&
+ ! grep -q "fsmonitor_refresh_callback.*FILE-4-A.*pos" "$PWD/file_case_wrong-try2.log" &&
+ ! grep -q "fsmonitor_refresh_callback.*file-4-a.*pos" "$PWD/file_case_wrong-try2.log" &&
+
+ # FSM refresh saw nothing, so it will mark all files as valid,
+ # so they should now have "h" status.
+
+ git -C file_case_wrong ls-files -f >"$PWD/file_case_wrong-lsf2.out" &&
+ grep -q "h dir1/dir2/dir3/file-3-a" "$PWD/file_case_wrong-lsf2.out" &&
+ grep -q "h dir1/dir2/dir4/FILE-4-A" "$PWD/file_case_wrong-lsf2.out" &&
+
+
+ # We now have files with clean content, but with case-incorrect
+ # file names. Modify them to see if status properly reports
+ # them.
+
+ echo xx >>file_case_wrong/dir1/dir2/dir3/FILE-3-A &&
+ echo xx >>file_case_wrong/dir1/dir2/dir4/file-4-a &&
+
+ GIT_TRACE_FSMONITOR="$PWD/file_case_wrong-try3.log" \
+ git -C file_case_wrong --no-optional-locks status --short \
+ >"$PWD/file_case_wrong-try3.out" &&
+ # FSEvents are in observed case.
+ grep -q "fsmonitor_refresh_callback.*FILE-3-A.*pos -3" "$PWD/file_case_wrong-try3.log" &&
+ grep -q "fsmonitor_refresh_callback.*file-4-a.*pos -9" "$PWD/file_case_wrong-try3.log" &&
+
+ # Expect Breakage: with the case confusion, the "(pos-3)" and
+ # "(pos -9)" causes the client to not clear the CE_FSMONITOR_VALID
+ # bit and therefore status will not rescan the files and therefore
+ # not report them as dirty.
+ grep -q " M dir1/dir2/dir3/file-3-a" "$PWD/file_case_wrong-try3.out" &&
+ grep -q " M dir1/dir2/dir4/FILE-4-A" "$PWD/file_case_wrong-try3.out"
+'
+
test_done
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 03/14] fsmonitor: refactor refresh callback on directory events
From: Jeff Hostetler via GitGitGadget @ 2024-02-26 21:39 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Jeff Hostetler, Torsten Bögershausen,
Jeff Hostetler, Jeff Hostetler
In-Reply-To: <pull.1662.v3.git.1708983565.gitgitgadget@gmail.com>
From: Jeff Hostetler <jeffhostetler@github.com>
Move the code to handle directory FSEvents (containing pathnames with
a trailing slash) into a helper function.
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
---
fsmonitor.c | 52 ++++++++++++++++++++++++++++++----------------------
1 file changed, 30 insertions(+), 22 deletions(-)
diff --git a/fsmonitor.c b/fsmonitor.c
index f670c509378..6fecae9aeb2 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -183,6 +183,35 @@ static int query_fsmonitor_hook(struct repository *r,
return result;
}
+static void handle_path_with_trailing_slash(
+ struct index_state *istate, const char *name, 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;
- }
+ handle_path_with_trailing_slash(istate, name, pos);
/*
* We need to remove the traling "/" from the path
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 04/14] fsmonitor: clarify handling of directory events in callback helper
From: Jeff Hostetler via GitGitGadget @ 2024-02-26 21:39 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Jeff Hostetler, Torsten Bögershausen,
Jeff Hostetler, Jeff Hostetler
In-Reply-To: <pull.1662.v3.git.1708983565.gitgitgadget@gmail.com>
From: Jeff Hostetler <jeffhostetler@github.com>
Improve documentation of the refresh callback helper function
used for directory FSEvents.
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
---
fsmonitor.c | 37 ++++++++++++++++++++++++-------------
1 file changed, 24 insertions(+), 13 deletions(-)
diff --git a/fsmonitor.c b/fsmonitor.c
index 6fecae9aeb2..29cce32d81c 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -183,24 +183,35 @@ static int query_fsmonitor_hook(struct repository *r,
return result;
}
+/*
+ * 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).
+ */
static void handle_path_with_trailing_slash(
struct index_state *istate, const char *name, 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;
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 05/14] fsmonitor: refactor refresh callback for non-directory events
From: Jeff Hostetler via GitGitGadget @ 2024-02-26 21:39 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Jeff Hostetler, Torsten Bögershausen,
Jeff Hostetler, Jeff Hostetler
In-Reply-To: <pull.1662.v3.git.1708983565.gitgitgadget@gmail.com>
From: Jeff Hostetler <jeffhostetler@github.com>
Move the code that handles 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);
+ 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);
}
/*
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 06/14] dir: create untracked_cache_invalidate_trimmed_path()
From: Jeff Hostetler via GitGitGadget @ 2024-02-26 21:39 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Jeff Hostetler, Torsten Bögershausen,
Jeff Hostetler, Jeff Hostetler
In-Reply-To: <pull.1662.v3.git.1708983565.gitgitgadget@gmail.com>
From: Jeff Hostetler <jeffhostetler@github.com>
Create a wrapper function for untracked_cache_invalidate_path()
that silently trims a trailing slash, if present, before calling
the wrapped function.
The untracked cache expects to be called with a pathname that
does not contain a trailing slash. This can make it inconvenient
for callers that have a directory path. Lets hide this complexity.
This will be used by a later commit in the FSMonitor code which
may receive directory pathnames from an FSEvent.
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
---
dir.c | 20 ++++++++++++++++++++
dir.h | 7 +++++++
2 files changed, 27 insertions(+)
diff --git a/dir.c b/dir.c
index ac699542302..20ebe4cba26 100644
--- a/dir.c
+++ b/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)
{
diff --git a/dir.h b/dir.h
index 98aa85fcc0e..45a7b9ec5f2 100644
--- a/dir.h
+++ b/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 *);
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 07/14] fsmonitor: refactor untracked-cache invalidation
From: Jeff Hostetler via GitGitGadget @ 2024-02-26 21:39 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Jeff Hostetler, Torsten Bögershausen,
Jeff Hostetler, Jeff Hostetler
In-Reply-To: <pull.1662.v3.git.1708983565.gitgitgadget@gmail.com>
From: Jeff Hostetler <jeffhostetler@github.com>
Update fsmonitor_refresh_callback() to use the new
untracked_cache_invalidate_trimmed_path() to invalidate
the cache using the observed pathname without needing to
modify the caller's buffer.
Previously, we modified the caller's buffer when the observed pathname
contained a trailing slash (and did not restore it). This wasn't a
problem for the single use-case caller, but felt dirty nontheless. In
a later commit we will want to invalidate case-corrected versions of
the pathname (using possibly borrowed pathnames from the name-hash or
dir-name-hash) and we may not want to keep the tradition of altering
the passed-in pathname.
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
---
fsmonitor.c | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/fsmonitor.c b/fsmonitor.c
index 364198d258f..2787f7ca5d1 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -271,21 +271,16 @@ static void fsmonitor_refresh_callback(struct index_state *istate, char *name)
if (name[len - 1] == '/') {
handle_path_with_trailing_slash(istate, name, pos);
-
- /*
- * We need to remove the traling "/" from the path
- * for the untracked cache.
- */
- name[len - 1] = '\0';
} else {
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.
+ * as it could be a new untracked file. (Let the untracked cache
+ * layer silently deal with any trailing slash.)
*/
- untracked_cache_invalidate_path(istate, name, 0);
+ untracked_cache_invalidate_trimmed_path(istate, name, 0);
}
/*
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 08/14] fsmonitor: move untracked-cache invalidation into helper functions
From: Jeff Hostetler via GitGitGadget @ 2024-02-26 21:39 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Jeff Hostetler, Torsten Bögershausen,
Jeff Hostetler, Jeff Hostetler
In-Reply-To: <pull.1662.v3.git.1708983565.gitgitgadget@gmail.com>
From: Jeff Hostetler <jeffhostetler@github.com>
Move the call to invalidate the untracked-cache for the FSEvent
pathname into the two helper functions.
In a later commit in this series, we will call these helpers
from other contexts and it safer to include the UC invalidation
in the helpers than to remember to also add it to each helper
call-site.
This has the side-effect of invalidating the UC *before* we
invalidate the ce_flags in the cache-entry. These activities
are independent and do not affect each other. Also, by doing
the UC work first, we can avoid worrying about "early returns"
or the need for the usual "goto the end" in each of the
handler functions.
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
---
fsmonitor.c | 26 +++++++++++++++++++-------
1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/fsmonitor.c b/fsmonitor.c
index 2787f7ca5d1..2f58ee2fe5a 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -188,6 +188,16 @@ static void handle_path_without_trailing_slash(
{
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).
+ * 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
@@ -249,6 +259,15 @@ static void handle_path_with_trailing_slash(
{
int i;
+ /*
+ * 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;
@@ -274,13 +293,6 @@ static void fsmonitor_refresh_callback(struct index_state *istate, char *name)
} else {
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. (Let the untracked cache
- * layer silently deal with any trailing slash.)
- */
- untracked_cache_invalidate_trimmed_path(istate, name, 0);
}
/*
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 09/14] fsmonitor: return invalidated cache-entry count on directory event
From: Jeff Hostetler via GitGitGadget @ 2024-02-26 21:39 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Jeff Hostetler, Torsten Bögershausen,
Jeff Hostetler, Jeff Hostetler
In-Reply-To: <pull.1662.v3.git.1708983565.gitgitgadget@gmail.com>
From: Jeff Hostetler <jeffhostetler@github.com>
Teach the refresh callback helper function for directory FSEvents to
return the number of cache-entries that were invalidated in response
to a directory event.
This will be used in a later commit to help determine if the observed
pathname in the FSEvent was a (possibly) case-incorrect directory
prefix (on a case-insensitive filesystem) of one or more actual
cache-entries.
If there exists at least one case-insensitive prefix match, then we
can assume that the directory is a (case-incorrect) prefix of at least
one tracked item rather than a completely unknown/untracked file or
directory.
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
---
fsmonitor.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/fsmonitor.c b/fsmonitor.c
index 2f58ee2fe5a..9424bd17230 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -253,11 +253,20 @@ static void handle_path_without_trailing_slash(
* 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 void handle_path_with_trailing_slash(
+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
@@ -276,7 +285,10 @@ static void handle_path_with_trailing_slash(
if (!starts_with(istate->cache[i]->name, name))
break;
istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;
+ nr_in_cone++;
}
+
+ return nr_in_cone;
}
static void fsmonitor_refresh_callback(struct index_state *istate, char *name)
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 10/14] fsmonitor: remove custom loop from non-directory path handler
From: Jeff Hostetler via GitGitGadget @ 2024-02-26 21:39 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Jeff Hostetler, Torsten Bögershausen,
Jeff Hostetler, Jeff Hostetler
In-Reply-To: <pull.1662.v3.git.1708983565.gitgitgadget@gmail.com>
From: Jeff Hostetler <jeffhostetler@github.com>
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 <jeffhostetler@github.com>
---
fsmonitor.c | 55 ++++++++++++++++++++++++++++++-----------------------
1 file changed, 31 insertions(+), 24 deletions(-)
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);
}
}
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 11/14] fsmonitor: return invalided cache-entry count on non-directory event
From: Jeff Hostetler via GitGitGadget @ 2024-02-26 21:39 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Jeff Hostetler, Torsten Bögershausen,
Jeff Hostetler, Jeff Hostetler
In-Reply-To: <pull.1662.v3.git.1708983565.gitgitgadget@gmail.com>
From: Jeff Hostetler <jeffhostetler@github.com>
Teach the refresh callback helper function for unqualified FSEvents
(pathnames without a trailing slash) to return the number of
cache-entries that were invalided in response to the event.
This will be used in a later commit to help determine if the observed
pathname was (possibly) case-incorrect when (on a case-insensitive
file system).
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
---
fsmonitor.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/fsmonitor.c b/fsmonitor.c
index a51c17cda70..c16ed5d8758 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -196,8 +196,10 @@ static size_t handle_path_with_trailing_slash(
* 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 void handle_path_without_trailing_slash(
+static size_t handle_path_without_trailing_slash(
struct index_state *istate, const char *name, int pos)
{
/*
@@ -218,7 +220,9 @@ static void handle_path_without_trailing_slash(
* at that directory. (That is, assume no D/F conflicts.)
*/
istate->cache[pos]->ce_flags &= ~CE_FSMONITOR_VALID;
+ return 1;
} else {
+ size_t nr_in_cone;
struct strbuf work_path = STRBUF_INIT;
/*
@@ -232,8 +236,10 @@ static void handle_path_without_trailing_slash(
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);
+ nr_in_cone = handle_path_with_trailing_slash(
+ istate, work_path.buf, pos);
strbuf_release(&work_path);
+ return nr_in_cone;
}
}
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 12/14] fsmonitor: trace the new invalidated cache-entry count
From: Jeff Hostetler via GitGitGadget @ 2024-02-26 21:39 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Jeff Hostetler, Torsten Bögershausen,
Jeff Hostetler, Jeff Hostetler
In-Reply-To: <pull.1662.v3.git.1708983565.gitgitgadget@gmail.com>
From: Jeff Hostetler <jeffhostetler@github.com>
Consolidate the directory/non-directory calls to the refresh handler
code. Log the resulting count of invalidated cache-entries.
The nr_in_cone value will be used in a later commit to decide if
we also need to try to do case-insensitive lookups.
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
---
fsmonitor.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/fsmonitor.c b/fsmonitor.c
index c16ed5d8758..739ddbf7aca 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -308,16 +308,21 @@ 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] == '/') {
- handle_path_with_trailing_slash(istate, name, pos);
- } else {
- handle_path_without_trailing_slash(istate, 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);
+
+ if (nr_in_cone)
+ trace_printf_key(&trace_fsmonitor,
+ "fsmonitor_refresh_callback CNT: %d",
+ (int)nr_in_cone);
}
/*
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 13/14] fsmonitor: refactor bit invalidation in refresh callback
From: Jeff Hostetler via GitGitGadget @ 2024-02-26 21:39 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Jeff Hostetler, Torsten Bögershausen,
Jeff Hostetler, Jeff Hostetler
In-Reply-To: <pull.1662.v3.git.1708983565.gitgitgadget@gmail.com>
From: Jeff Hostetler <jeffhostetler@github.com>
Refactor code in the fsmonitor_refresh_callback() call chain dealing
with invalidating the CE_FSMONITOR_VALID bit and add a trace message.
During the refresh, we clear the CE_FSMONITOR_VALID bit in response to
data from the FSMonitor daemon (so that a later phase will lstat() and
verify the true state of the file).
Create a new function to clear the bit and add some unique tracing for
it to help debug edge cases.
This is similar to the existing `mark_fsmonitor_invalid()` function,
but it also does untracked-cache invalidation and we've already
handled that in the refresh-callback handlers, so but we don't need
to repeat that.
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
---
fsmonitor.c | 20 ++++++++++++++++++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/fsmonitor.c b/fsmonitor.c
index 739ddbf7aca..3c87449be87 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -183,6 +183,22 @@ static int query_fsmonitor_hook(struct repository *r,
return result;
}
+/*
+ * 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)
+{
+ 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);
@@ -219,7 +235,7 @@ static size_t handle_path_without_trailing_slash(
* 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;
@@ -297,7 +313,7 @@ static size_t handle_path_with_trailing_slash(
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(istate->cache[i]);
nr_in_cone++;
}
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 14/14] fsmonitor: support case-insensitive events
From: Jeff Hostetler via GitGitGadget @ 2024-02-26 21:39 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Jeff Hostetler, Torsten Bögershausen,
Jeff Hostetler, Jeff Hostetler
In-Reply-To: <pull.1662.v3.git.1708983565.gitgitgadget@gmail.com>
From: Jeff Hostetler <jeffhostetler@github.com>
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 event handling to optionally use the name-hash and dir-name-hash
if necessary.
Also update t7527 to convert the "test_expect_failure" to "_success"
now that we have fixed the bug.
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
---
fsmonitor.c | 121 +++++++++++++++++++++++++++++++++++
t/t7527-builtin-fsmonitor.sh | 26 +++++---
2 files changed, 137 insertions(+), 10 deletions(-)
diff --git a/fsmonitor.c b/fsmonitor.c
index 3c87449be87..2b17d60bbbe 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"
@@ -202,6 +203,113 @@ static void invalidate_ce_fsm(struct cache_entry *ce)
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 MAP: '%s' '%s'",
+ name, ce->name);
+
+ /*
+ * 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);
+
+ 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);
+ /*
+ * 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.
+ */
+ 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
@@ -335,6 +443,19 @@ static void fsmonitor_refresh_callback(struct index_state *istate, char *name)
else
nr_in_cone = handle_path_without_trailing_slash(istate, name, pos);
+ /*
+ * 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.
+ */
+ 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",
diff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh
index 830f2d9de33..730f3c7f810 100755
--- a/t/t7527-builtin-fsmonitor.sh
+++ b/t/t7527-builtin-fsmonitor.sh
@@ -1051,7 +1051,7 @@ test_expect_success 'split-index and FSMonitor work well together' '
#
# The setup is a little contrived.
#
-test_expect_failure CASE_INSENSITIVE_FS 'fsmonitor subdir case wrong on disk' '
+test_expect_success CASE_INSENSITIVE_FS 'fsmonitor subdir case wrong on disk' '
test_when_finished "stop_daemon_delete_repo subdir_case_wrong" &&
git init subdir_case_wrong &&
@@ -1116,19 +1116,19 @@ test_expect_failure CASE_INSENSITIVE_FS 'fsmonitor subdir case wrong on disk' '
grep -q "dir1/DIR2/dir3/file3.*pos -3" "$PWD/subdir_case_wrong.log1" &&
+ # Verify that we get a mapping event to correct the case.
+ grep -q "MAP:.*dir1/DIR2/dir3/file3.*dir1/dir2/dir3/file3" \
+ "$PWD/subdir_case_wrong.log1" &&
+
# The refresh-callbacks should have caused "git status" to clear
# the CE_FSMONITOR_VALID bit on each of those files and caused
# the worktree scan to visit them and mark them as modified.
grep -q " M AAA" "$PWD/subdir_case_wrong.out" &&
grep -q " M zzz" "$PWD/subdir_case_wrong.out" &&
-
- # Expect Breakage: with the case confusion, the "(pos -3)" causes
- # the client to not clear the CE_FSMONITOR_VALID bit and therefore
- # status will not rescan the file and therefore not report it as dirty.
grep -q " M dir1/dir2/dir3/file3" "$PWD/subdir_case_wrong.out"
'
-test_expect_failure CASE_INSENSITIVE_FS 'fsmonitor file case wrong on disk' '
+test_expect_success CASE_INSENSITIVE_FS 'fsmonitor file case wrong on disk' '
test_when_finished "stop_daemon_delete_repo file_case_wrong" &&
git init file_case_wrong &&
@@ -1242,14 +1242,20 @@ test_expect_failure CASE_INSENSITIVE_FS 'fsmonitor file case wrong on disk' '
GIT_TRACE_FSMONITOR="$PWD/file_case_wrong-try3.log" \
git -C file_case_wrong --no-optional-locks status --short \
>"$PWD/file_case_wrong-try3.out" &&
+
+ # Verify that we get a mapping event to correct the case.
+ grep -q "fsmonitor_refresh_callback MAP:.*dir1/dir2/dir3/FILE-3-A.*dir1/dir2/dir3/file-3-a" \
+ "$PWD/file_case_wrong-try3.log" &&
+ grep -q "fsmonitor_refresh_callback MAP:.*dir1/dir2/dir4/file-4-a.*dir1/dir2/dir4/FILE-4-A" \
+ "$PWD/file_case_wrong-try3.log" &&
+
# FSEvents are in observed case.
grep -q "fsmonitor_refresh_callback.*FILE-3-A.*pos -3" "$PWD/file_case_wrong-try3.log" &&
grep -q "fsmonitor_refresh_callback.*file-4-a.*pos -9" "$PWD/file_case_wrong-try3.log" &&
- # Expect Breakage: with the case confusion, the "(pos-3)" and
- # "(pos -9)" causes the client to not clear the CE_FSMONITOR_VALID
- # bit and therefore status will not rescan the files and therefore
- # not report them as dirty.
+ # The refresh-callbacks should have caused "git status" to clear
+ # the CE_FSMONITOR_VALID bit on each of those files and caused
+ # the worktree scan to visit them and mark them as modified.
grep -q " M dir1/dir2/dir3/file-3-a" "$PWD/file_case_wrong-try3.out" &&
grep -q " M dir1/dir2/dir4/FILE-4-A" "$PWD/file_case_wrong-try3.out"
'
--
gitgitgadget
^ permalink raw reply related
* [PATCH v1 0/4] Change xwrite() to write_in_full() in builtins.
From: Randall S. Becker @ 2024-02-26 22:05 UTC (permalink / raw)
To: git; +Cc: Randall S. Becker
From: "Randall S. Becker" <rsbecker@nexbridge.com>
This series replaces xwrite to write_in_full in builtins/. The change is
required to fix critical problems that prevent full writes to be
processed by on platforms where xwrite may be limited to a platform size
limit. Further changes outside of builtins/ may be required but do not
appear to be as urgent as this change, which causes test breakage in
t7704. A separate series will be contributed for changes outside of
builtins/ at a later date.
Randall S. Becker (4):
builtin/index-pack.c: change xwrite to write_in_full to allow large
sizes.
builtin/receive-pack.c: change xwrite to write_in_full to allow large
sizes.
builtin/repack.c: change xwrite to write_in_full to allow large sizes.
builtin/unpack-objects.c: change xwrite to write_in_full to allow
large sizes.
builtin/index-pack.c | 2 +-
builtin/receive-pack.c | 5 +++--
builtin/repack.c | 9 +++++++--
builtin/unpack-objects.c | 2 +-
4 files changed, 12 insertions(+), 6 deletions(-)
--
2.42.1
^ permalink raw reply
* [PATCH v1 1/4] builtin/index-pack.c: change xwrite to write_in_full to allow large sizes.
From: Randall S. Becker @ 2024-02-26 22:05 UTC (permalink / raw)
To: git; +Cc: Randall S. Becker
In-Reply-To: <20240226220539.3494-1-randall.becker@nexbridge.ca>
From: "Randall S. Becker" <rsbecker@nexbridge.com>
This change is required because some platforms do not support file writes of
arbitrary sizes (e.g, NonStop). xwrite ends up truncating the output to the
maximum single I/O size possible for the destination device.
Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
---
builtin/index-pack.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index a3a37bd215..f80b8d101a 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1571,7 +1571,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
* the last part of the input buffer to stdout.
*/
while (input_len) {
- err = xwrite(1, input_buffer + input_offset, input_len);
+ err = write_in_full(1, input_buffer + input_offset, input_len);
if (err <= 0)
break;
input_len -= err;
--
2.42.1
^ permalink raw reply related
* [PATCH v1 2/4] builtin/receive-pack.c: change xwrite to write_in_full to allow large sizes.
From: Randall S. Becker @ 2024-02-26 22:05 UTC (permalink / raw)
To: git; +Cc: Randall S. Becker
In-Reply-To: <20240226220539.3494-1-randall.becker@nexbridge.ca>
From: "Randall S. Becker" <rsbecker@nexbridge.com>
This change is required because some platforms do not support file writes of
arbitrary sizes (e.g, NonStop). xwrite ends up truncating the output to the
maximum single I/O size possible for the destination device.
Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
---
builtin/receive-pack.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index db65607485..5064f3d300 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -455,8 +455,9 @@ static void report_message(const char *prefix, const char *err, va_list params)
if (use_sideband)
send_sideband(1, 2, msg, sz, use_sideband);
- else
- xwrite(2, msg, sz);
+ else {
+ write_in_full(2, msg, sz);
+ }
}
__attribute__((format (printf, 1, 2)))
--
2.42.1
^ permalink raw reply related
* [PATCH v1 3/4] builtin/repack.c: change xwrite to write_in_full to allow large sizes.
From: Randall S. Becker @ 2024-02-26 22:05 UTC (permalink / raw)
To: git; +Cc: Randall S. Becker
In-Reply-To: <20240226220539.3494-1-randall.becker@nexbridge.ca>
From: "Randall S. Becker" <rsbecker@nexbridge.com>
This change is required because some platforms do not support file writes of
arbitrary sizes (e.g, NonStop). xwrite ends up truncating the output to the
maximum single I/O size possible for the destination device. The result of
write_in_full() is also passed to the caller, which was previously ignored.
Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
---
builtin/repack.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index ede36328a3..932d24c60b 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -307,6 +307,7 @@ static int write_oid(const struct object_id *oid,
struct packed_git *pack UNUSED,
uint32_t pos UNUSED, void *data)
{
+ int err;
struct child_process *cmd = data;
if (cmd->in == -1) {
@@ -314,8 +315,12 @@ static int write_oid(const struct object_id *oid,
die(_("could not start pack-objects to repack promisor objects"));
}
- xwrite(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz);
- xwrite(cmd->in, "\n", 1);
+ err = write_in_full(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz);
+ if (err <= 0)
+ return err;
+ err = write_in_full(cmd->in, "\n", 1);
+ if (err <= 0)
+ return err;
return 0;
}
--
2.42.1
^ permalink raw reply related
* [PATCH v1 4/4] builtin/unpack-objects.c: change xwrite to write_in_full to allow large sizes.
From: Randall S. Becker @ 2024-02-26 22:05 UTC (permalink / raw)
To: git; +Cc: Randall S. Becker
In-Reply-To: <20240226220539.3494-1-randall.becker@nexbridge.ca>
From: "Randall S. Becker" <rsbecker@nexbridge.com>
This change is required because some platforms do not support file writes of
arbitrary sizes (e.g, NonStop). xwrite ends up truncating the output to the
maximum single I/O size possible for the destination device.
Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
---
builtin/unpack-objects.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index e0a701f2b3..6935c4574e 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -680,7 +680,7 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix UNUSED)
/* Write the last part of the buffer to stdout */
while (len) {
- int ret = xwrite(1, buffer + offset, len);
+ int ret = write_in_full(1, buffer + offset, len);
if (ret <= 0)
break;
len -= ret;
--
2.42.1
^ permalink raw reply related
* Re: [PATCH v1 1/4] builtin/index-pack.c: change xwrite to write_in_full to allow large sizes.
From: Taylor Blau @ 2024-02-26 22:38 UTC (permalink / raw)
To: Randall S. Becker; +Cc: git, Randall S. Becker
In-Reply-To: <20240226220539.3494-2-randall.becker@nexbridge.ca>
On Mon, Feb 26, 2024 at 05:05:35PM -0500, Randall S. Becker wrote:
> From: "Randall S. Becker" <rsbecker@nexbridge.com>
>
> This change is required because some platforms do not support file writes of
> arbitrary sizes (e.g, NonStop). xwrite ends up truncating the output to the
> maximum single I/O size possible for the destination device.
Hmm. I'm not sure I understand what NonStop's behavior is here...
> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index a3a37bd215..f80b8d101a 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -1571,7 +1571,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
> * the last part of the input buffer to stdout.
> */
> while (input_len) {
> - err = xwrite(1, input_buffer + input_offset, input_len);
> + err = write_in_full(1, input_buffer + input_offset, input_len);
> if (err <= 0)
> break;
> input_len -= err;
> --
> 2.42.1
The code above loops while input_len is non-zero, and correctly
decrements it by the number of bytes written by xwrite() after each
iteration.
Assuming that xwrite()/write(2) works how I think it does on NonStop,
I'm not sure I understand why this change is necessary.
Thanks,
Taylor
^ permalink raw reply
* RE: [PATCH v1 1/4] builtin/index-pack.c: change xwrite to write_in_full to allow large sizes.
From: rsbecker @ 2024-02-26 22:51 UTC (permalink / raw)
To: 'Taylor Blau', 'Randall S. Becker'; +Cc: git
In-Reply-To: <Zd0S7aUIG1bhGkaX@nand.local>
On Monday, February 26, 2024 5:39 PM, Taylor Blau wrote:
>On Mon, Feb 26, 2024 at 05:05:35PM -0500, Randall S. Becker wrote:
>> From: "Randall S. Becker" <rsbecker@nexbridge.com>
>>
>> This change is required because some platforms do not support file
>> writes of arbitrary sizes (e.g, NonStop). xwrite ends up truncating
>> the output to the maximum single I/O size possible for the destination device.
>
>Hmm. I'm not sure I understand what NonStop's behavior is here...
>
>> diff --git a/builtin/index-pack.c b/builtin/index-pack.c index
>> a3a37bd215..f80b8d101a 100644
>> --- a/builtin/index-pack.c
>> +++ b/builtin/index-pack.c
>> @@ -1571,7 +1571,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
>> * the last part of the input buffer to stdout.
>> */
>> while (input_len) {
>> - err = xwrite(1, input_buffer + input_offset, input_len);
>> + err = write_in_full(1, input_buffer + input_offset, input_len);
>> if (err <= 0)
>> break;
>> input_len -= err;
>> --
>> 2.42.1
>
>The code above loops while input_len is non-zero, and correctly decrements it by the number of bytes written by xwrite() after each
>iteration.
>
>Assuming that xwrite()/write(2) works how I think it does on NonStop, I'm not sure I understand why this change is necessary.
NonStop has a limited SSIZE_MAX. xwrite only handles that much so anything beyond that gets dropped (not in the above code but in other builtins); hence the critical nature of getting this fix out. This particular change probably could be tightened up on a re-roll to just call write_in_full instead of the while loop. I can fix that for v2. The goal suggested by Phillip W was to change xwrite to write_in_full, so I guess I went a little too far.
^ permalink raw reply
* Re: [GSoC][PATCH 1/1] add: use unsigned type for collection of bits
From: Junio C Hamano @ 2024-02-26 22:58 UTC (permalink / raw)
To: Christian Couder; +Cc: Eugenio Gigante, git, sunshine
In-Reply-To: <CAP8UFD3qR8E0gvUQtzzkLPWv4Db45kFS4pEqHKQr5siciVJ-zQ@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> On Sat, Feb 24, 2024 at 12:28 PM Eugenio Gigante
> <giganteeugenio2@gmail.com> wrote:
>>
>> The function 'refresh' in 'builtin/add.c' declares 'flags' as
>> signed, while the function 'refresh_index' defined in
>> 'read-cache-ll.h' expects an unsigned value.
>
> It's not clear from the patch that refresh() passes 'flags' as an
> argument to refresh_index(), so it might help reviewers a bit if you
> could tell that.
Perhaps.
>> Since in this case 'flags' represents a bag of bits, whose MSB is
>> not used in special ways, this commit changes the type of 'flags'
>> to unsigned.
>
> We prefer to use "let's change this and that" or just "change this and
> that" rather than "this commit changes this and that", see
> https://git-scm.com/docs/SubmittingPatches/#imperative-mood.
Very true.
> It might help if you could add a bit more explanation about why it's a
> good thing to use an unsigned variable instead of a signed one. For
> example you could say that it documents that we are not doing anything
> funny with the MSB.
But doesn't the proposed log message already say so?
In any case, it would very much help to fold long lines and have a
blank line in between paragraphs to make the log message more
readable.
Thanks, both.
>
>> Signed-off-by: Eugenio Gigante <giganteeugenio2@gmail.com>
>> ---
>> builtin/add.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> The patch looks correct, thanks!
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox