fsmonitor: check the untracked cache after a trivial response
PATCH 1/1 fsmonitor: check the untracked cache after a trivial response
From: Ilia K <[email protected]>
When the monitor sends a trivial response ("/"), for example after the daemon restarted or lost events, refresh_fsmonitor() marks every index entry dirty, which is saved in the index. For the untracked cache it only clears the in-memory flag use_fsmonitor. The valid bits of the cached directories are saved unchanged, although nothing checks them anymore: valid_cached_dir() does not lstat() a valid directory while the monitor is trusted.
A command that does not scan for untracked files, like "git add" or "git checkout", therefore saves the new token next to stale valid bits, and every later "git status" trusts them. A directory removed in the meantime keeps its untracked files listed, or, when its own entry is invalid, makes every run print
``` warning: could not open directory 'sub/': No such file or directory ```
Check every cached directory against its stat data before the cache is saved next to such a token, as valid_cached_dir() does without a monitor, and invalidate the ones that changed or disappeared. Doing this at write time skips commands that do not write the index, and sparse index writes, which drop the fsmonitor extension anyway. The cost is one lstat() per cached directory, once per trivial response.
---
fsmonitor: check the untracked cache after a trivial response
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2247%2Fk15tfu%2Funtracked-cache-trivial-response-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2247/k15tfu/untracked-cache-trivial-response-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2247
dir.c | 56 ++++++++++++++++++++++++++++++++++++ dir.h | 7 +++++ read-cache.c | 16 +++++++++++ t/t7519-status-fsmonitor.sh | 55 +++++++++++++++++++++++++++++++++++ t/t7527-builtin-fsmonitor.sh | 44 ++++++++++++++++++++++++++++ 5 files changed, 178 insertions(+)
dir.c +56 -0
@@ -4053,6 +4053,62 @@ void untracked_cache_invalidate_trimmed_path(struct index_state *istate, } } +static int invalidate_stale_dirs (struct untracked_cache *uc, +struct untracked_cache_dir *ucd, +struct index_state *istate, +struct strbuf *path) +{ +struct stat st; +size_t len = path->len; +int nr_invalidated =0 ; +unsigned int i; + +if (ucd->valid && + (lstat (path->buf, &st) || +match_stat_data_racy (istate, &ucd->stat_data, &st))) { +invalidate_one_directory (uc, ucd); + nr_invalidated++; + } + +for (i =0 ; i < ucd->dirs_nr; i++) { +/* not written to the index, see write_one_dir() */ +if (!ucd->dirs[i]->recurse) +continue ; +strbuf_addch (path,'/' ); +strbuf_addstr (path, ucd->dirs[i]->name); + nr_invalidated +=invalidate_stale_dirs (uc, ucd->dirs[i], + istate, path); +strbuf_setlen (path, len); + } + +return nr_invalidated; +} + +int untracked_cache_invalidate_stale_dirs (struct index_state *istate) +{ +struct strbuf path =STRBUF_INIT ; +const char *worktree; +int nr_invalidated; + +if (!istate->untracked || !istate->untracked->root) +return 0 ; + +/* +* The index is also read and written by commands that do not +* run in the top-level directory of the worktree. +*/ + worktree =repo_get_work_tree (istate->repo); +if (!worktree) +return 0 ; + +strbuf_addstr (&path, worktree); + nr_invalidated =invalidate_stale_dirs (istate->untracked, + istate->untracked->root, + istate, &path); +strbuf_release (&path); +return nr_invalidated; +} +void untracked_cache_remove_from_index (struct index_state *istate,const char *path) {
dir.h +7 -0
@@ -604,6 +604,13 @@ void untracked_cache_invalidate_path(struct index_state *, const char *, int safvoid untracked_cache_invalidate_trimmed_path (struct index_state *,const char *path,int safe_path); +/* +* Invalidate every cached directory that no longer exists or whose +* stat data no longer matches the working tree. valid_cached_dir() +* skips this check while the file system monitor is trusted. +* Returns the number of invalidated directories. +*/ +int untracked_cache_invalidate_stale_dirs (struct index_state *);void untracked_cache_remove_from_index (struct index_state *,const char *);void untracked_cache_add_to_index (struct index_state *,const char *);
read-cache.c +16 -0
@@ -3034,6 +3034,22 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, istate->untracked) {strbuf_reset (&sb); +/* +* The monitor could not say what changed (see the trivial +* response in refresh_fsmonitor()), so nothing kept the +* valid bits up to date. Check them before they are saved +* next to the new token, which later commands trust even +* when this command did not look for untracked files. +*/ +if (write_extensions &WRITE_FSMONITOR_EXTENSION && + istate->fsmonitor_last_update && + !istate->untracked->use_fsmonitor) { +int nr =untracked_cache_invalidate_stale_dirs (istate); + +trace2_data_intmax ("index" , istate->repo, +"extension/untr/invalidated" , nr); + } +write_untracked_extension (&sb, istate->untracked); err =write_index_ext_header (f, eoie_c,CACHE_EXT_UNTRACKED , sb.len) <0 ;
t/t7519-status-fsmonitor.sh +55 -0
61 lines not on this page · raw message
t/t7527-builtin-fsmonitor.sh +44 -0
48 lines not on this page · raw message
base-commit: c46c1e37724f0478939de636ab8ea5a89086d532
Signature · 1 line
gitgitgadget
Quoted 9 lines · Ilia via GitGitGadget
From: Ilia K <[email protected]>
When the monitor sends a trivial response ("/"), for example after the daemon restarted or lost events,
refresh_fsmonitor()marks every index entry dirty, which is saved in the index. For the untracked cache it only clears the in-memory flaguse_fsmonitor. The valid bits of the cached directories are saved unchanged, although nothing checks them anymore:valid_cached_dir()does notlstat()a valid directory while the monitor is trusted.
Sorry that I declared "mailing list bankruptcy" and didn't see your patch when I was preparing mine [1] for the same issue.
[1] https://lore.kernel.org/git/[email protected]/
...
Quoted 7 lines
Check every cached directory against its stat data before the cache is saved next to such a token, as
valid_cached_dir()does without a monitor, and invalidate the ones that changed or disappeared. Doing this at write time skips commands that do not write the index, and sparse index writes, which drop the fsmonitor extension anyway. The cost is onelstat()per cached directory, once per trivial response.
This is the main difference between your patch and mine. My patch does a full clear of the untracked cache, but you do the more efficient thing to only clear the entries that don't match the filesystem info.
This is visible when running p7527-bulitin-fsmonitor.sh on Linux with a small setting in fs.inotify.max_user_watches, causing the builtin fsmonitor to always fail. My "clear everything" patch gets a 50-75% penalty on the untracked cache tests while yours has no measurable impact.
For this reason, I think your approach is better and worth the slight increase in code complexity.
Quoted 15 lines
dir.c +15
-0+static int invalidate_stale_dirs (struct untracked_cache *uc, +struct untracked_cache_dir *ucd, +struct index_state *istate, +struct strbuf *path) +{ +struct stat st; +size_t len = path->len; +int nr_invalidated =0 ; +unsigned int i; + +if (ucd->valid && + (lstat (path->buf, &st) || +match_stat_data_racy (istate, &ucd->stat_data, &st))) { +invalidate_one_directory (uc, ucd); + nr_invalidated++;
dir.c void untracked_cache_invalidate_trimmed_path(struct index_state *istate, · line 4080
Quoted 10 lines
dir.c +10
-0+ } + +for (i =0 ; i < ucd->dirs_nr; i++) { +/* not written to the index, see write_one_dir() */ +if (!ucd->dirs[i]->recurse) +continue ; +strbuf_addch (path,'/' ); +strbuf_addstr (path, ucd->dirs[i]->name); + nr_invalidated +=invalidate_stale_dirs (uc, ucd->dirs[i], + istate, path);
I'm conditioned to worry about recursive calls without depth limits, but I suppose that we are limited by the untracked cache data structure itself, which was presumably created recursively without hitting stack limits.
We could make this iterative using a stack-based data structure, but that would be more complicated than this is worth, probably. These are not used in server environments and the .git/index file shouldn't be considered "unsafe" or else we have bigger safety problems.
Quoted 12 lines
dir.c +12
-0+strbuf_setlen (path, len); + } + +return nr_invalidated; +} + +int untracked_cache_invalidate_stale_dirs (struct index_state *istate) +{ +struct strbuf path =STRBUF_INIT ; +const char *worktree; +int nr_invalidated; +
dir.c void untracked_cache_invalidate_trimmed_path(struct index_state *istate, · line 4102
Quoted 10 lines
dir.c +10
-0+if (!istate->untracked || !istate->untracked->root) +return 0 ; + +/* +* The index is also read and written by commands that do not +* run in the top-level directory of the worktree. +*/ + worktree =repo_get_work_tree (istate->repo); +if (!worktree) +return 0 ;
I wonder if this is the right response if we fail to get a worktree. Maybe we should invalidate the entire untracked cache in this case, along with a warning that we couldn't evaluate the worktree? Or should we return -1 to signal an error?
Quoted 8 lines
dir.c +8
-0+ +strbuf_addstr (&path, worktree); + nr_invalidated =invalidate_stale_dirs (istate->untracked, + istate->untracked->root, + istate, &path); +strbuf_release (&path); +return nr_invalidated; +}
...
dir.h void untracked_cache_invalidate_path(struct index_state *, const char *, int saf · line 613
Quoted 7 lines
dir.h +7
-0+/* +* Invalidate every cached directory that no longer exists or whose +* stat data no longer matches the working tree. valid_cached_dir() +* skips this check while the file system monitor is trusted. +* Returns the number of invalidated directories. +*/ +int untracked_cache_invalidate_stale_dirs (struct index_state *);
We could update this doc comment if we return negative on error.
Quoted 13 lines
read-cache.c +5
-0@@ -3034,6 +3034,22 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, istate->untracked) {strbuf_reset (&sb); +/* +* The monitor could not say what changed (see the trivial +* response in refresh_fsmonitor()), so nothing kept the +* valid bits up to date. Check them before they are saved +* next to the new token, which later commands trust even
read-cache.c static int do_write_index(struct index_state *istate, struct tempfile *tempfile, · line 3051
Quoted 10 lines
read-cache.c +10
-0+* when this command did not look for untracked files. +*/ +if (write_extensions &WRITE_FSMONITOR_EXTENSION && + istate->fsmonitor_last_update && + !istate->untracked->use_fsmonitor) { +int nr =untracked_cache_invalidate_stale_dirs (istate); + +trace2_data_intmax ("index" , istate->repo, +"extension/untr/invalidated" , nr); + }
This surprised me, that you're computing this in do_write_index(). I suppose that this avoids doing this computation unless the index is written, but is it possible that between refresh_fsmonitor() and do_write_index() that the untracked cache is read and used to determine what this index update should be?
I used a coding agent to explore my concerns in this space, and it couldn't find an example where doing this in refresh_fsmonitor() would be better than its current placement.
read-cache.c static int do_write_index(struct index_state *istate, struct tempfile *tempfile, · line 3054
Quoted 3 lines
read-cache.c +3
-0+# After a trivial response ("/") the monitor cannot vouch for the +# untracked cache. Even a command that does not look for untracked +# files must drop the stale entries, or the next "git status" trusts them.
nit: Generally, we don't need these comments because the tests should be self-documenting.
Quoted 24 lines
read-cache.c +24
-0+test_expect_success UNTRACKED_CACHE 'untracked cache is checked after a trivial response' ' + test_when_finished "rm -rf trivial err" && + git init trivial && + ( + cd trivial && + mkdir -p dir/sub && + echo tracked >dir/sub/tracked && + git add dir && + git commit -m initial && + git config core.fsmonitor "$TEST_DIRECTORY/t7519/fsmonitor-none" && +# Version 1 only, or the hook prints a version complaint +# on stderr at every query. + git config core.fsmonitorHookVersion 1 && + git config core.untrackedCache true && +# With "normal", invalidating one path also invalidates +# its parents, and the stale parent below is never seen. + git config status.showUntrackedFiles all && + echo untracked >dir/sub/untracked && + echo "?? dir/sub/untracked" >../expect && + git status --porcelain >../actual && + test_cmp ../expect ../actual && + git status --porcelain >../actual && + test_cmp ../expect ../actual && +
read-cache.c static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
Quoted 10 lines
read-cache.c +10
-0+# The monitor misses the removal of dir/sub. "git add other" +# gets the trivial response and does not touch the entries +# of dir and dir/sub by itself. + rm -r dir/sub && + echo other >other && + git -c core.fsmonitor="$TEST_DIRECTORY/t7519/fsmonitor-all" \ + add other && + cat >../expect <<-\EOF && + D dir/sub/tracked + A other
I initially thought that this difference in left-padding was a typo, but it is functional. We need a space before D and a space after A to signal the different states.
read-cache.c
Quoted 11 lines
read-cache.c +11
-0+ EOF + git status --porcelain >../actual 2>../err && + test_must_be_empty ../err && + test_cmp ../expect ../actual && + +# Invalidate the entry of dir/sub. The stale entry of dir then +# makes "git status" open the removed directory and warn. + git update-index --remove dir/sub/tracked && + cat >../expect <<-\EOF && + D dir/sub/tracked +A other
And of course now the deletion is staged. Good.
read-cache.c
Quoted 11 lines
read-cache.c +10
-0+ EOF + git status --porcelain >../actual 2>../err && + test_must_be_empty ../err && + test_cmp ../expect ../actual + ) +' +# After a restart the daemon sends a trivial response ("/"), because it +# cannot know what changed while it was down. Even a command that does +# not look for untracked files must then drop the stale untracked cache +# entries , or the next "git status" trusts them.
same nit: comments shouldn't be necessary here.
Quoted 29 lines
read-cache.c +29
-0+test_expect_success UNTRACKED_CACHE 'untracked cache is checked after a trivial response' ' + test_when_finished"stop_daemon_delete_repo test_trivial" && + + git init test_trivial && + mkdir -p test_trivial/dir/sub && + echo tracked >test_trivial/dir/sub/tracked && + git -C test_trivial add dir && + git -C test_trivial commit -m initial && + git -C test_trivial config core.fsmonitor true && + git -C test_trivial config core.untrackedCache true && + echo untracked >test_trivial/dir/sub/untracked && + +# The first status starts the daemon and builds the untracked +# cache , the second one trusts it. + echo "?? dir/sub/untracked" >expect && + git -C test_trivial status --porcelain >actual && + test_cmp expect actual && + git -C test_trivial status --porcelain >actual && + test_cmp expect actual && + +# Remove dir/sub while no daemon is running. "git add" then +# starts a new daemon, receives its trivial response, and does +# not look for untracked files. + git -C test_trivial fsmonitor--daemon stop && + rm -r test_trivial/dir/sub && + echo other >test_trivial/other && +GIT_TRACE2_EVENT ="$PWD/trace_trivial" \ + git -C test_trivial add other && + have_t2_data_event fsm_client query/trivial-response <trace_trivial &&
read-cache.c
Quoted 10 lines
read-cache.c +10
-0+ git -C test_trivial fsmonitor--daemon status && + + cat >expect <<-\EOF && + D dir/sub/tracked + A other +EOF + git -C test_trivial status --porcelain >actual 2>err && + test_must_be_empty err && + test_cmp expect actual +'
Overall, this patch appears to solve the same problem that pointed me in this direction. I don't see any issues worth re-rolling this patch, so it LGTM.
Thanks,
-Stolee
Quoted 13 lines · Derrick Stolee
dir.c void untracked_cache_invalidate_trimmed_path(struct index_state *istate, · line 4080
Quoted 8 lines
dir.c +8
-0+for (i =0 ; i < ucd->dirs_nr; i++) { +/* not written to the index, see write_one_dir() */ +if (!ucd->dirs[i]->recurse) +continue ; +strbuf_addch (path,'/' ); +strbuf_addstr (path, ucd->dirs[i]->name); + nr_invalidated +=invalidate_stale_dirs (uc, ucd->dirs[i], + istate, path);I'm conditioned to worry about recursive calls without depth limits, but I suppose that we are limited by the untracked cache data structure itself, which was presumably created recursively without hitting stack limits.
Quoted 4 lines
We could make this iterative using a stack-based data structure, but that would be more complicated than this is worth, probably. These are not used in server environments and the .git/index file shouldn't be considered "unsafe" or else we have bigger safety problems.
Good reasoning. Thanks for spelling it out.
Quoted 4 lines
Quoted 4 lines
dir.c +4
-0+/* +* The index is also read and written by commands that do not +* run in the top-level directory of the worktree. +*/
Quoted 8 lines
dir.c void untracked_cache_invalidate_trimmed_path(struct index_state *istate, · line 4102
Quoted 3 lines
dir.c +3
-0+ worktree =repo_get_work_tree (istate->repo); +if (!worktree) +return 0 ;I wonder if this is the right response if we fail to get a worktree. Maybe we should invalidate the entire untracked cache in this case, along with a warning that we couldn't evaluate the worktree? Or should we return -1 to signal an error?
It probably is safer to invalidate and continue. If there is truly not a worktree, untracked cache entries do not matter ;-)
Thanks for a review.
Quoted 20 lines · Derrick Stolee
Quoted 20 lines
dir.c +19
-0+strbuf_setlen (path, len); + } + +return nr_invalidated; +} + +int untracked_cache_invalidate_stale_dirs (struct index_state *istate) +{ +struct strbuf path =STRBUF_INIT ; +const char *worktree; +int nr_invalidated; + +if (!istate->untracked || !istate->untracked->root) +return 0 ; + +/* +* The index is also read and written by commands that do not +* run in the top-level directory of the worktree. +*/
Quoted 8 lines
dir.c void untracked_cache_invalidate_trimmed_path(struct index_state *istate, · line 4102
Quoted 3 lines
dir.c +3
-0+ worktree =repo_get_work_tree (istate->repo); +if (!worktree) +return 0 ;I wonder if this is the right response if we fail to get a worktree. Maybe we should invalidate the entire untracked cache in this case, along with a warning that we couldn't evaluate the worktree? Or should we return -1 to signal an error?
Fixed, thank you!
Quoted 21 lines
Quoted 21 lines
read-cache.c +13
-0@@ -3034,6 +3034,22 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, istate->untracked) {strbuf_reset (&sb); +/* +* The monitor could not say what changed (see the trivial +* response in refresh_fsmonitor()), so nothing kept the +* valid bits up to date. Check them before they are saved +* next to the new token, which later commands trust even +* when this command did not look for untracked files. +*/ +if (write_extensions &WRITE_FSMONITOR_EXTENSION && + istate->fsmonitor_last_update && + !istate->untracked->use_fsmonitor) { +int nr =untracked_cache_invalidate_stale_dirs (istate); + + trace2_data_intmax("index", istate->repo,
Quoted 8 lines
read-cache.c static int do_write_index(struct index_state *istate, struct tempfile *tempfile, · line 3051
Quoted 2 lines
read-cache.c +2
-0+ "extension/untr/invalidated", nr); + }This surprised me, that you're computing this in
do_write_index(). I suppose that this avoids doing this computation unless the index is written, but is it possible that betweenrefresh_fsmonitor()anddo_write_index()that the untracked cache is read and used to determine what this index update should be?
No, nothing between refresh_fsmonitor() and do_write_index() trusts the cache because use_fsmonitor stays 0 until the index is written.
Quoted 6 lines
read-cache.c static int do_write_index(struct index_state *istate, struct tempfile *tempfile, · line 3054
Quoted 3 lines
read-cache.c +3
-0+# After a trivial response ("/") the monitor cannot vouch for the +# untracked cache. Even a command that does not look for untracked +# files must drop the stale entries, or the next "git status" trusts them.nit: Generally, we don't need these comments because the tests should be self-documenting.
Fixed, thank you!
Quoted 7 lines
Quoted 6 lines
read-cache.c +6
-0+ EOF + git status --porcelain >../actual 2>../err && + test_must_be_empty ../err && + test_cmp ../expect ../actual + ) +'
Quoted 6 lines
read-cache.c
Quoted 4 lines
read-cache.c +4
-0+# After a restart the daemon sends a trivial response ("/"), because it +# cannot know what changed while it was down. Even a command that does +# not look for untracked files must then drop the stale untracked cache +# entries , or the next "git status" trusts them.same nit: comments shouldn't be necessary here.
Fixed, thank you!
--
- Ilia
PATCH v2 1/1 fsmonitor: check the untracked cache after a trivial response
In reply to Ilia via GitGitGadgetFrom: Ilia K <[email protected]>
When the monitor sends a trivial response ("/"), for example after the daemon restarted or lost events, refresh_fsmonitor() marks every index entry dirty, which is saved in the index. For the untracked cache it only clears the in-memory flag use_fsmonitor. The valid bits of the cached directories are saved unchanged, although nothing checks them anymore: valid_cached_dir() does not lstat() a valid directory while the monitor is trusted.
A command that does not scan for untracked files, like "git add" or "git checkout", therefore saves the new token next to stale valid bits, and every later "git status" trusts them. A directory removed in the meantime keeps its untracked files listed, or, when its own entry is invalid, makes every run print
``` warning: could not open directory 'sub/': No such file or directory ```
Check every cached directory against its stat data before the cache is saved next to such a token, as valid_cached_dir() does without a monitor, and invalidate the ones that changed or disappeared. Within the command itself, valid_cached_dir() already checks every directory, because use_fsmonitor is cleared. Only what is saved needs the check. Doing this at write time skips commands that do not write the index, and sparse index writes, which drop the fsmonitor extension anyway. The cost is one lstat() per cached directory, once per trivial response.
---
fsmonitor: check the untracked cache after a trivial response
Changes since v1:
* the whole untracked cache is dropped if !worktree
* removed the comments as per nits
* rebased against ToT
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2247%2Fk15tfu%2Funtracked-cache-trivial-response-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2247/k15tfu/untracked-cache-trivial-response-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2247
Range-diff vs v1:
1: 84618b83fe ! 1: d2cad8abe6 fsmonitor: check the untracked cache after a trivial response
@@ Commit message
```
Check every cached directory against its stat data before the cache is saved next to such a token, as valid_cached_dir() does without a
- monitor, and invalidate the ones that changed or disappeared. Doing
- this at write time skips commands that do not write the index, and
- sparse index writes, which drop the fsmonitor extension anyway.
+ monitor, and invalidate the ones that changed or disappeared. Within
+ the command itself, valid_cached_dir() already checks every directory,
+ because use_fsmonitor is cleared. Only what is saved needs the check.
+ Doing this at write time skips commands that do not write the index,
+ and sparse index writes, which drop the fsmonitor extension anyway.
The cost is one lstat() per cached directory, once per trivial response.
@@ dir.c: void untracked_cache_invalidate_trimmed_path(struct index_state *istate,
+ * run in the top-level directory of the worktree.
+ */
+ worktree = repo_get_work_tree(istate->repo);
-+ if (!worktree)
-+ return 0;
++ if (!worktree) {
++ invalidate_directory(istate->untracked,
++ istate->untracked->root);
++ return 1;
++ }
+
+ strbuf_addstr(&path, worktree);
+ nr_invalidated = invalidate_stale_dirs(istate->untracked,
@@ t/t7519-status-fsmonitor.sh: do
done
done
-+# After a trivial response ("/") the monitor cannot vouch for the
-+# untracked cache. Even a command that does not look for untracked
-+# files must drop the stale entries, or the next "git status" trusts them.
+test_expect_success UNTRACKED_CACHE 'untracked cache is checked after a trivial response' '
+ test_when_finished "rm -rf trivial err" &&
+ git init trivial &&
@@ t/t7527-builtin-fsmonitor.sh: test_expect_success CASE_INSENSITIVE_FS 'fsmonitor
test_grep -q " M dir1/dir2/dir4/FILE-4-A" "$PWD/file_case_wrong-try3.out"
'
-+# After a restart the daemon sends a trivial response ("/"), because it
-+# cannot know what changed while it was down. Even a command that does
-+# not look for untracked files must then drop the stale untracked cache
-+# entries, or the next "git status" trusts them.
+test_expect_success UNTRACKED_CACHE 'untracked cache is checked after a trivial response' '
+ test_when_finished "stop_daemon_delete_repo test_trivial" &&
+
dir.c | 59 ++++++++++++++++++++++++++++++++++++ dir.h | 7 +++++ read-cache.c | 16 ++++++++++ t/t7519-status-fsmonitor.sh | 52 +++++++++++++++++++++++++++++++ t/t7527-builtin-fsmonitor.sh | 40 ++++++++++++++++++++++++ 5 files changed, 174 insertions(+)
dir.c +59 -0
@@ -4066,6 +4066,65 @@ void untracked_cache_invalidate_trimmed_path(struct index_state *istate, } } +static int invalidate_stale_dirs (struct untracked_cache *uc, +struct untracked_cache_dir *ucd, +struct index_state *istate, +struct strbuf *path) +{ +struct stat st; +size_t len = path->len; +int nr_invalidated =0 ; +unsigned int i; + +if (ucd->valid && + (lstat (path->buf, &st) || +match_stat_data_racy (istate, &ucd->stat_data, &st))) { +invalidate_one_directory (uc, ucd); + nr_invalidated++; + } + +for (i =0 ; i < ucd->dirs_nr; i++) { +/* not written to the index, see write_one_dir() */ +if (!ucd->dirs[i]->recurse) +continue ; +strbuf_addch (path,'/' ); +strbuf_addstr (path, ucd->dirs[i]->name); + nr_invalidated +=invalidate_stale_dirs (uc, ucd->dirs[i], + istate, path); +strbuf_setlen (path, len); + } + +return nr_invalidated; +} + +int untracked_cache_invalidate_stale_dirs (struct index_state *istate) +{ +struct strbuf path =STRBUF_INIT ; +const char *worktree; +int nr_invalidated; + +if (!istate->untracked || !istate->untracked->root) +return 0 ; + +/* +* The index is also read and written by commands that do not +* run in the top-level directory of the worktree. +*/ + worktree =repo_get_work_tree (istate->repo); +if (!worktree) { +invalidate_directory (istate->untracked, + istate->untracked->root); +return 1 ; + } + +strbuf_addstr (&path, worktree); + nr_invalidated =invalidate_stale_dirs (istate->untracked, + istate->untracked->root, + istate, &path); +strbuf_release (&path); +return nr_invalidated; +} +void untracked_cache_remove_from_index (struct index_state *istate,const char *path) {
dir.h +7 -0
@@ -604,6 +604,13 @@ void untracked_cache_invalidate_path(struct index_state *, const char *, int safvoid untracked_cache_invalidate_trimmed_path (struct index_state *,const char *path,int safe_path); +/* +* Invalidate every cached directory that no longer exists or whose +* stat data no longer matches the working tree. valid_cached_dir() +* skips this check while the file system monitor is trusted. +* Returns the number of invalidated directories. +*/ +int untracked_cache_invalidate_stale_dirs (struct index_state *);void untracked_cache_remove_from_index (struct index_state *,const char *);void untracked_cache_add_to_index (struct index_state *,const char *);
read-cache.c +16 -0
@@ -3084,6 +3084,22 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, istate->untracked) {strbuf_reset (&sb); +/* +* The monitor could not say what changed (see the trivial +* response in refresh_fsmonitor()), so nothing kept the +* valid bits up to date. Check them before they are saved +* next to the new token, which later commands trust even +* when this command did not look for untracked files. +*/ +if (write_extensions &WRITE_FSMONITOR_EXTENSION && + istate->fsmonitor_last_update && + !istate->untracked->use_fsmonitor) { +int nr =untracked_cache_invalidate_stale_dirs (istate); + +trace2_data_intmax ("index" , istate->repo, +"extension/untr/invalidated" , nr); + } +write_untracked_extension (&sb, istate->untracked); err =write_index_ext_header (f, eoie_c,CACHE_EXT_UNTRACKED , sb.len) <0 ;
t/t7519-status-fsmonitor.sh +52 -0
58 lines not on this page · raw message
t/t7527-builtin-fsmonitor.sh +40 -0
@@ -1389,4 +1389,44 @@ test_expect_success CASE_INSENSITIVE_FS 'fsmonitor file case wrong on disk' ' test_grep -q " M dir1/dir2/dir4/FILE-4-A" "$PWD/file_case_wrong-try3.out" ' +test_expect_success UNTRACKED_CACHE 'untracked cache is checked after a trivial response' ' + test_when_finished "stop_daemon_delete_repo test_trivial" && + + git init test_trivial && + mkdir -p test_trivial/dir/sub && + echo tracked >test_trivial/dir/sub/tracked && + git -C test_trivial add dir && + git -C test_trivial commit -m initial && + git -C test_trivial config core.fsmonitor true && + git -C test_trivial config core.untrackedCache true && + echo untracked >test_trivial/dir/sub/untracked && + + # The first status starts the daemon and builds the untracked + # cache, the second one trusts it. + echo "?? dir/sub/untracked" >expect && + git -C test_trivial status --porcelain >actual && + test_cmp expect actual && + git -C test_trivial status --porcelain >actual && + test_cmp expect actual && + + # Remove dir/sub while no daemon is running. "git add" then + # starts a new daemon, receives its trivial response, and does + # not look for untracked files. + git -C test_trivial fsmonitor--daemon stop && + rm -r test_trivial/dir/sub && + echo other >test_trivial/other && + GIT_TRACE2_EVENT="$PWD/trace_trivial" \ + git -C test_trivial add other && + have_t2_data_event fsm_client query/trivial-response <trace_trivial && + git -C test_trivial fsmonitor--daemon status && + + cat >expect <<-\EOF && + D dir/sub/tracked + A other + EOF + git -C test_trivial status --porcelain >actual 2>err && + test_must_be_empty err && + test_cmp expect actual +' + test_done
base-commit: fb255012acbcbe81189cb8014da0280b09c66ecc
Signature · 1 line
gitgitgadget
Quoted 4 lines · Ilia via GitGitGadget
Changes since v1:
* the whole untracked cache is dropped if !worktree
* removed the comments as per nits
Thanks for these improvements. The v2 patch LGTM.
Quoted 1 line
* rebased against ToT
Generally, you shouldn't do this as the maintainer will apply your patch in the same spot and handle most merge conflicts via rerere, but rebasing interrupts that. The main exception is when you rebase onto a specific topic or with regards to a large refactor that merged recently.
Thanks,
-Stolee
Quoted 8 lines · Derrick Stolee
Quoted 4 lines · Ilia via GitGitGadget
Changes since v1:
* the whole untracked cache is dropped if !worktree
* removed the comments as per nitsThanks for these improvements. The v2 patch LGTM.
Quoted 7 lines
Quoted 1 line
* rebased against ToT
Generally, you shouldn't do this as the maintainer will apply your patch in the same spot and handle most merge conflicts via rerere, but rebasing interrupts that. The main exception is when you rebase onto a specific topic or with regards to a large refactor that merged recently.
Good suggestion. Worse yet, the topic is already in 'next' so a wholesale replacement is not very welcome.
Quoted 4 lines · Junio C Hamano
Quoted 4 lines
Thanks for these improvements. The v2 patch LGTM.
Quoted 1 line
* rebased against ToT
Quoted 8 lines
Quoted 5 lines
Generally, you shouldn't do this as the maintainer will apply your patch in the same spot and handle most merge conflicts via rerere, but rebasing interrupts that. The main exception is when you rebase onto a specific topic or with regards to a large refactor that merged recently.
Good suggestion. Worse yet, the topic is already in 'next' so a wholesale replacement is not very welcome.
Well, I ended up reverting the merge to 'next', and queued this iteration (applied on exactly the same base as v1 was applied in my tree---the patch cleanly applied as if there were no rebase). I'll mark the topic again ready for 'next' after waiting for a few days to see if there are any more feedback.
Thanks, all.