On 2/7/2018 4:21 AM, Nguyễn Thái Ngọc Duy wrote:
read_directory() code ignores all paths named ".git" even if it's not
a valid git repository. See treat_path() for details. Since ".git" is
basically invisible to read_directory(), when we are asked to
invalidate a path that contains ".git", we can safely ignore it
because the slow path would not consider it anyway.
This helps when fsmonitor is used and we have a real ".git" repo at
worktree top. Occasionally .git/index will be updated and if the
fsmonitor hook does not filter it, untracked cache is asked to
invalidate the path ".git/index".
Without this patch, we invalidate the root directory unncessarily,
which:
- makes read_directory() fall back to slow path for root directory
(slower)
- makes the index dirty (because UNTR extension is updated). Depending
on the index size, writing it down could also be slow.
Thank you again, this patch makes much more sense to me.
A note about the new "safe_path" knob. Since this new check could be
relatively expensive, avoid it when we know it's not needed. If the
path comes from the index, it can't contain ".git". If it does
contain, we may be screwed up at many more levels, not just this one.
I do have a simplifying suggestion to make. I noticed that other uses
of verify_path() check when the potentially erroneous path is passed in
and then all the underlying code can assume it is valid. I think that
makes sense here as well and it makes for a smaller patch.
diff --git a/fsmonitor.h b/fsmonitor.h
index cd3cc0ccf2..65f3743636 100644
--- a/fsmonitor.h
+++ b/fsmonitor.h
@@ -65,7 +65,7 @@ static inline void mark_fsmonitor_invalid(struct index_state *istate, struct cac
{
if (core_fsmonitor) {
ce->ce_flags &= ~CE_FSMONITOR_VALID;
- untracked_cache_invalidate_path(istate, ce->name);
+ untracked_cache_invalidate_path(istate, ce->name, 1);
This test isn't needed because we're pulling the name right out of the
cache entry so it doesn't need to be verified.
Here is a modified version of your patch for consideration:
================
read_directory() code ignores all paths named ".git" even if it's not
a valid git repository. See treat_path() for details. Since ".git" is
basically invisible to read_directory(), when we are asked to
invalidate a path that contains ".git", we can safely ignore it
because the slow path would not consider it anyway.
This helps when fsmonitor is used and we have a real ".git" repo at
worktree top. Occasionally .git/index will be updated and if the
fsmonitor hook does not filter it, untracked cache is asked to
invalidate the path ".git/index".
Without this patch, we invalidate the root directory unnecessarily,
which:
- makes read_directory() fall back to slow path for root directory
(slower)
- makes the index dirty (because UNTR extension is updated). Depending
on the index size, writing it down could also be slow.
Noticed-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx>
Signed-off-by: Ben Peart <benpeart@xxxxxxxxxxxxx>
---
Notes:
Base Ref: master
Web-Diff: https://github.com/benpeart/git/commit/218a577618
Checkout: git fetch https://github.com/benpeart/git verify_path-v1
&& git checkout 218a577618
dir.c | 2 +-
fsmonitor.c | 6 +++++-
t/t7519-status-fsmonitor.sh | 39 +++++++++++++++++++++++++++++++++++++++
3 files changed, 45 insertions(+), 2 deletions(-)
diff --git a/dir.c b/dir.c
index 7c4b45e30e..d431da46f5 100644
--- a/dir.c
+++ b/dir.c
@@ -1773,7 +1773,7 @@ static enum path_treatment treat_path(struct
dir_struct *dir,
if (!de)
return treat_path_fast(dir, untracked, cdir, istate, path,
baselen, pathspec);
- if (is_dot_or_dotdot(de->d_name) || !strcmp(de->d_name, ".git"))
+ if (is_dot_or_dotdot(de->d_name) || !fspathcmp(de->d_name, ".git"))
return path_none;
strbuf_setlen(path, baselen);
strbuf_addstr(path, de->d_name);
diff --git a/fsmonitor.c b/fsmonitor.c
index 0af7c4edba..019576f306 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -118,8 +118,12 @@ static int query_fsmonitor(int version, uint64_t
last_update, struct strbuf *que
static void fsmonitor_refresh_callback(struct index_state *istate,
const char *name)
{
- int pos = index_name_pos(istate, name, strlen(name));
+ int pos;
+ if (!verify_path(name))
+ return;
+
+ pos = index_name_pos(istate, name, strlen(name));
if (pos >= 0) {
struct cache_entry *ce = istate->cache[pos];
ce->ce_flags &= ~CE_FSMONITOR_VALID;
diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index eb2d13bbcf..756beb0d8e 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -314,4 +314,43 @@ test_expect_success 'splitting the index results in
the same state' '
test_cmp expect actual
'
+test_expect_success UNTRACKED_CACHE 'ignore .git changes when
invalidating UNTR' '
+ test_create_repo dot-git &&
+ (
+ cd dot-git &&
+ mkdir -p .git/hooks &&
+ : >tracked &&
+ : >modified &&
+ mkdir dir1 &&
+ : >dir1/tracked &&
+ : >dir1/modified &&
+ mkdir dir2 &&
+ : >dir2/tracked &&
+ : >dir2/modified &&
+ write_integration_script &&
+ git config core.fsmonitor .git/hooks/fsmonitor-test &&
+ git update-index --untracked-cache &&
+ git update-index --fsmonitor &&
+ GIT_TRACE_UNTRACKED_STATS="$TRASH_DIRECTORY/trace-before" \
+ git status &&
+ test-dump-untracked-cache >../before
+ ) &&
+ cat >>dot-git/.git/hooks/fsmonitor-test <<-\EOF &&
+ printf ".git\0"
+ printf ".git/index\0"
+ printf "dir1/.git\0"
+ printf "dir1/.git/index\0"
+ EOF
+ (
+ cd dot-git &&
+ GIT_TRACE_UNTRACKED_STATS="$TRASH_DIRECTORY/trace-after" \
+ git status &&
+ test-dump-untracked-cache >../after
+ ) &&
+ grep "directory invalidation" trace-before >>before &&
+ grep "directory invalidation" trace-after >>after &&
+ # UNTR extension unchanged, dir invalidation count unchanged
+ test_cmp before after
+'
+
test_done
base-commit: 5be1f00a9a701532232f57958efab4be8c959a29
--
2.15.0.windows.1
