{"thread":{"id":"60232","subject":"[PATCH] cache: add fake_lstat()","startedAt":"2023-09-14T21:46:52Z","lastAt":"2023-09-15T00:17:44Z","messageCount":3,"participants":["Junio C Hamano","Josip Sokcevic"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"481853","messageId":"xmqqcyykig1l.fsf@gitster.g","threadId":"60232","inReplyTo":null,"subject":"[PATCH] cache: add fake_lstat()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-14T21:46:46Z","receivedAt":"2023-09-14T21:46:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"At times, we may already know that a path represented by a\ncache_entry ce has no changes via some out-of-line means, like\nfsmonitor, and yet need the control to go through a codepath that\nrequires us to have \"struct stat\" obtained by lstat() on the path,\nfor various purposes (e.g. \"ie_match_stat()\" wants cached stat-info\nis still current wrt \"struct stat\", \"diff\" wants to know st_mode).\n\nThe callers of lstat() on a tracked file, when its cache_entry knows\nit is up-to-date, can instead call this helper to pretend that it\ncalled lstat() by faking the \"struct stat\" information.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * Just setting the .st_mode member in check_removed() relies on\n   the assumption that other members of \"struct stat\" are never\n   looked at a bit too much.  We could use something like this to\n   bypass calling lstat() and instead populate from the data we have\n   in cache_entry.\n\n builtin/ls-files.c |  5 ++++-\n read-cache-ll.h    |  8 ++++++++\n read-cache.c       | 27 +++++++++++++++++++++++++++\n statinfo.c         | 27 +++++++++++++++++++++++++++\n statinfo.h         |  8 ++++++++\n 5 files changed, 74 insertions(+), 1 deletion(-)\n\ndiff --git c/builtin/ls-files.c w/builtin/ls-files.c\nindex a0229c3277..7031ffcb0a 100644\n--- c/builtin/ls-files.c\n+++ w/builtin/ls-files.c\n@@ -450,7 +450,10 @@ static void show_files(struct repository *repo, struct dir_struct *dir)\n \t\t\tcontinue;\n \t\tif (ce_skip_worktree(ce))\n \t\t\tcontinue;\n-\t\tstat_err = lstat(fullname.buf, &st);\n+\t\tif (!(ce->ce_flags & CE_FSMONITOR_VALID))\n+\t\t\tstat_err = lstat(fullname.buf, &st);\n+\t\telse\n+\t\t\tstat_err = fake_lstat(ce, &st);\n \t\tif (stat_err && (errno != ENOENT && errno != ENOTDIR))\n \t\t\terror_errno(\"cannot lstat '%s'\", fullname.buf);\n \t\tif (stat_err && show_deleted) {\ndiff --git c/read-cache-ll.h w/read-cache-ll.h\nindex 9a1a7edc5a..2a50a784f0 100644\n--- c/read-cache-ll.h\n+++ w/read-cache-ll.h\n@@ -436,6 +436,14 @@ int match_stat_data_racy(const struct index_state *istate,\n \n void fill_stat_cache_info(struct index_state *istate, struct cache_entry *ce, struct stat *st);\n \n+/*\n+ * Fill members of st by members of sd enough to convince match_stat()\n+ * to consider that they match.  It should be usable as a replacement\n+ * for lstat() for a tracked path that is known to be up-to-date via\n+ * some out-of-line means (like fsmonitor).\n+ */\n+int fake_lstat(const struct cache_entry *ce, struct stat *st);\n+\n #define REFRESH_REALLY                   (1 << 0) /* ignore_valid */\n #define REFRESH_UNMERGED                 (1 << 1) /* allow unmerged */\n #define REFRESH_QUIET                    (1 << 2) /* be quiet about it */\ndiff --git c/read-cache.c w/read-cache.c\nindex 080bd39713..eb750e2e49 100644\n--- c/read-cache.c\n+++ w/read-cache.c\n@@ -197,6 +197,33 @@ void fill_stat_cache_info(struct index_state *istate, struct cache_entry *ce, st\n \t}\n }\n \n+static unsigned int st_mode_from_ce(const struct cache_entry *ce)\n+{\n+\textern int trust_executable_bit, has_symlinks;\n+\n+\tswitch (ce->ce_mode & S_IFMT) {\n+\tcase S_IFLNK:\n+\t\treturn has_symlinks ? S_IFLNK : (S_IFREG | 0644);\n+\tcase S_IFREG:\n+\t\treturn (ce->ce_mode & (trust_executable_bit ? 0755 : 0644)) | S_IFREG;\n+\tcase S_IFGITLINK:\n+\t\treturn S_IFDIR | 0755;\n+\tcase S_IFDIR:\n+\t\treturn ce->ce_mode;\n+\tdefault:\n+\t\tBUG(\"unsupported ce_mode: %o\", ce->ce_mode);\n+\t}\n+}\n+\n+int fake_lstat(const struct cache_entry *ce, struct stat *st)\n+{\n+\tfake_lstat_data(&ce->ce_stat_data, st);\n+\tst->st_mode = st_mode_from_ce(ce);\n+\n+\t/* always succeed as lstat() replacement */\n+\treturn 0;\n+}\n+\n static int ce_compare_data(struct index_state *istate,\n \t\t\t   const struct cache_entry *ce,\n \t\t\t   struct stat *st)\ndiff --git c/statinfo.c w/statinfo.c\nindex 17bb8966c3..45156109de 100644\n--- c/statinfo.c\n+++ w/statinfo.c\n@@ -15,6 +15,33 @@ void fill_stat_data(struct stat_data *sd, struct stat *st)\n \tsd->sd_size = st->st_size;\n }\n \n+static void set_times(struct stat *st, const struct stat_data *sd)\n+{\n+\tst->st_ctime = sd->sd_ctime.sec;\n+\tst->st_mtime = sd->sd_mtime.sec;\n+#ifdef NO_NSEC\n+\t; /* nothing */\n+#else\n+#ifdef USE_ST_TIMESPEC\n+\tst->st_ctimespec.tv_nsec = sd->sd_ctime.nsec;\n+\tst->st_mtimespec.tv_nsec = sd->sd_mtime.nsec;\n+#else\n+\tst->st_ctim.tv_nsec = sd->sd_ctime.nsec;\n+\tst->st_mtim.tv_nsec = sd->sd_mtime.nsec;\n+#endif\n+#endif\n+}\n+\n+void fake_lstat_data(const struct stat_data *sd, struct stat *st)\n+{\n+\tset_times(st, sd);\n+\tst->st_dev = sd->sd_dev;\n+\tst->st_ino = sd->sd_ino;\n+\tst->st_uid = sd->sd_uid;\n+\tst->st_gid = sd->sd_gid;\n+\tst->st_size = sd->sd_size;\n+}\n+\n int match_stat_data(const struct stat_data *sd, struct stat *st)\n {\n \tint changed = 0;\ndiff --git c/statinfo.h w/statinfo.h\nindex 700f502ac0..5b21a30f90 100644\n--- c/statinfo.h\n+++ w/statinfo.h\n@@ -46,6 +46,14 @@ struct stat_validity {\n  */\n void fill_stat_data(struct stat_data *sd, struct stat *st);\n \n+/*\n+ * The inverse of the above.  When we know the cache_entry that\n+ * contains sd is up-to-date, but still need to pretend we called\n+ * lstat() to learn that fact, this function fills \"st\" enough to\n+ * fool ie_match_stat().\n+ */\n+void fake_lstat_data(const struct stat_data *sd, struct stat *st);\n+\n /*\n  * Return 0 if st is consistent with a file not having been changed\n  * since sd was filled.  If there are differences, return a\n"},{"id":"481855","messageId":"xmqqr0n0h0tw.fsf@gitster.g","threadId":"60232","inReplyTo":"xmqqcyykig1l.fsf@gitster.g","subject":"Re: [PATCH] cache: add fake_lstat()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-14T22:00:43Z","receivedAt":"2023-09-14T22:00:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"And your diff-lib change may look like this on top.  This one\nunfortunately is completely untested, as I do not use macOS.\n\nReviewing, testing, and improving it is highly appreciated.\n\n----- >8 ---------- >8 ---------- >8 -----\nSubject: diff-lib: fix check_removed() when fsmonitor is active\n\n`git diff-index` may return incorrect deleted entries when fsmonitor\nis used in a repository with git submodules. This can be observed on\nMac machines, but it can affect all other supported platforms too.\n\nIf fsmonitor is used, `stat *st` is left uninitialied if cache_entry\nhas CE_FSMONITOR_VALID bit set.  But, there are three call sites\nthat rely on stat afterwards, which can result in incorrect results.\n\nWe can fill members of \"struct stat\" that matters well enough using\nthe information we have in \"struct cache_entry\" that fsmonitor told\nus is up-to-date to solve this.\n\nHelped-by: Josip Sokcevic <sokcevic@google.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n diff-lib.c                   | 9 ++++++++-\n t/t7527-builtin-fsmonitor.sh | 5 +++++\n 2 files changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git c/diff-lib.c w/diff-lib.c\nindex d8aa777a73..7799747a97 100644\n--- c/diff-lib.c\n+++ w/diff-lib.c\n@@ -38,8 +38,15 @@\n  */\n static int check_removed(const struct index_state *istate, const struct cache_entry *ce, struct stat *st)\n {\n+\tint stat_err;\n+\n \tassert(is_fsmonitor_refreshed(istate));\n-\tif (!(ce->ce_flags & CE_FSMONITOR_VALID) && lstat(ce->name, st) < 0) {\n+\n+\tif (!(ce->ce_flags & CE_FSMONITOR_VALID))\n+\t\tstat_err = lstat(ce->name, st);\n+\telse\n+\t\tstat_err = fake_lstat(ce, st);\n+\tif (stat_err < 0) {\n \t\tif (!is_missing_file_error(errno))\n \t\t\treturn -1;\n \t\treturn 1;\ndiff --git c/t/t7527-builtin-fsmonitor.sh w/t/t7527-builtin-fsmonitor.sh\nindex 0c241d6c14..78503158fd 100755\n--- c/t/t7527-builtin-fsmonitor.sh\n+++ w/t/t7527-builtin-fsmonitor.sh\n@@ -809,6 +809,11 @@ my_match_and_clean () {\n \t\tstatus --porcelain=v2 >actual.without &&\n \ttest_cmp actual.with actual.without &&\n \n+\tgit -C super --no-optional-locks diff-index --name-status HEAD >actual.with &&\n+\tgit -C super --no-optional-locks -c core.fsmonitor=false \\\n+\t\tdiff-index --name-status HEAD >actual.without &&\n+\ttest_cmp actual.with actual.without &&\n+\n \tgit -C super/dir_1/dir_2/sub reset --hard &&\n \tgit -C super/dir_1/dir_2/sub clean -d -f\n }\n"},{"id":"481863","messageId":"CAJiyOigiBABJMgXXrExHd0Dd0E9RypVC3sWh15hrU0r+=+i7Qg@mail.gmail.com","threadId":"60232","inReplyTo":"xmqqr0n0h0tw.fsf@gitster.g","subject":"Re: [PATCH] cache: add fake_lstat()","fromName":"Josip Sokcevic","fromEmail":"sokcevic@google.com","sentAt":"2023-09-15T00:17:30Z","receivedAt":"2023-09-15T00:17:44Z","isPatch":true,"sender":{"key":"sokcevic@google.com","avatar":"https://avatars.githubusercontent.com/u/89933621?v=4"},"body":"On Thu, Sep 14, 2023 at 3:00 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> And your diff-lib change may look like this on top.  This one\n> unfortunately is completely untested, as I do not use macOS.\n>\n> Reviewing, testing, and improving it is highly appreciated.\n\nThe patches look good to me, and all tests pass on macOS.\n\n-- \nJosip Sokcevic\n"}]}