{"thread":{"id":"60164","subject":"Strange diff-index with fsmonitor, submodules","startedAt":"2023-08-29T00:57:03Z","lastAt":"2023-09-18T16:40:23Z","messageCount":18,"participants":["Jonathan Tan","Junio C Hamano","Jeff Hostetler","Josip Sokcevic"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"481106","messageId":"20230829005606.136615-1-jonathantanmy@google.com","threadId":"60164","inReplyTo":null,"subject":"Strange diff-index with fsmonitor, submodules","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2023-08-29T00:56:04Z","receivedAt":"2023-08-29T00:57:03Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"There is a strange interaction where diff-index not only produces\ndifferent results when run with and without fsmonitor, but produces\ndifferent results for 2 entries that as far as I can tell, should behave\nidentically (sibling files in the same directory - file_11 and file_12,\nand both of these filenames are only mentioned once each in the entire\ntest).\n\nYou can see this with this patch:\n\ndiff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh\nindex 0c241d6c14..e9e5e32016 100755\n--- a/t/t7527-builtin-fsmonitor.sh\n+++ b/t/t7527-builtin-fsmonitor.sh\n@@ -809,6 +809,11 @@ my_match_and_clean () {\n status --porcelain=v2 >actual.without &&\n test_cmp actual.with actual.without &&\n \n+ git -C super --no-optional-locks diff-index --name-status HEAD >actual.with &&\n+ git -C super --no-optional-locks -c core.fsmonitor=false \\\n+ diff-index --name-status HEAD >actual.without &&\n+ test_cmp actual.with actual.without &&\n+\n git -C super/dir_1/dir_2/sub reset --hard &&\n git -C super/dir_1/dir_2/sub clean -d -f\n }\n@@ -837,6 +842,7 @@ test_expect_success 'submodule always visited' '\n # some dirt in the submodule and confirm matching output.\n \n # Completely clean status.\n+ echo Now running for clean status &&\n my_match_and_clean &&\n\nand this is the output:\n\n++ echo Now running for clean status\nNow running for clean status\n++ my_match_and_clean\n++ git -C super --no-optional-locks status --porcelain=v2\n++ git -C super --no-optional-locks -c core.fsmonitor=false status --porcelain=v2\n++ test_cmp actual.with actual.without\n++ test 2 -ne 2\n++ eval 'diff -u' '\"$@\"'\n+++ diff -u actual.with actual.without\n++ git -C super --no-optional-locks diff-index --name-status HEAD\n++ git -C super --no-optional-locks -c core.fsmonitor=false diff-index --name-status HEAD\n++ test_cmp actual.with actual.without\n++ test 2 -ne 2\n++ eval 'diff -u' '\"$@\"'\n+++ diff -u actual.with actual.without\n--- actual.with\t2023-08-29 00:39:26\n+++ actual.without\t2023-08-29 00:39:26\n@@ -1 +0,0 @@\n-D\tdir_1/file_11\nerror: last command exited with $?=1\nnot ok 61 - submodule always visited\n\nNotice that with fsmonitor, diff-index reports a \"D\" line that is not\npresent when fsmonitor is off. To add to that, it only reports \"D\" for\nfile_11 when I would expect that if it reported file_11, it would report\nfile_12 as well.\n\nI'll continue investigating this myself, but does anyone know what is\ngoing on?\n"},{"id":"481109","messageId":"xmqqcyz6fxvf.fsf@gitster.g","threadId":"60164","inReplyTo":"20230829005606.136615-1-jonathantanmy@google.com","subject":"Re: Strange diff-index with fsmonitor, submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-29T01:20:04Z","receivedAt":"2023-08-29T01:21:16Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> There is a strange interaction where diff-index not only produces\n> different results when run with and without fsmonitor, but produces\n> different results for 2 entries that as far as I can tell, should behave\n> identically (sibling files in the same directory - file_11 and file_12,\n> and both of these filenames are only mentioned once each in the entire\n> test).\n\nPicking those who had non-clean-up changes to fsmonitor out of the\noutput from\n\n $ git shortlog -n --since=2.year --no-merges ':(glob)**/*fsmonitor*'\n\nand adding them to CC: for their attention.\n\n> You can see this with this patch:\n>\n> diff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh\n> index 0c241d6c14..e9e5e32016 100755\n> --- a/t/t7527-builtin-fsmonitor.sh\n> +++ b/t/t7527-builtin-fsmonitor.sh\n> @@ -809,6 +809,11 @@ my_match_and_clean () {\n> status --porcelain=v2 >actual.without &&\n> test_cmp actual.with actual.without &&\n> \n> + git -C super --no-optional-locks diff-index --name-status HEAD >actual.with &&\n> + git -C super --no-optional-locks -c core.fsmonitor=false \\\n> + diff-index --name-status HEAD >actual.without &&\n> + test_cmp actual.with actual.without &&\n> +\n> git -C super/dir_1/dir_2/sub reset --hard &&\n> git -C super/dir_1/dir_2/sub clean -d -f\n> }\n> @@ -837,6 +842,7 @@ test_expect_success 'submodule always visited' '\n> # some dirt in the submodule and confirm matching output.\n> \n> # Completely clean status.\n> + echo Now running for clean status &&\n> my_match_and_clean &&\n>\n> and this is the output:\n>\n> ++ echo Now running for clean status\n> Now running for clean status\n> ++ my_match_and_clean\n> ++ git -C super --no-optional-locks status --porcelain=v2\n> ++ git -C super --no-optional-locks -c core.fsmonitor=false status --porcelain=v2\n> ++ test_cmp actual.with actual.without\n> ++ test 2 -ne 2\n> ++ eval 'diff -u' '\"$@\"'\n> +++ diff -u actual.with actual.without\n> ++ git -C super --no-optional-locks diff-index --name-status HEAD\n> ++ git -C super --no-optional-locks -c core.fsmonitor=false diff-index --name-status HEAD\n> ++ test_cmp actual.with actual.without\n> ++ test 2 -ne 2\n> ++ eval 'diff -u' '\"$@\"'\n> +++ diff -u actual.with actual.without\n> --- actual.with\t2023-08-29 00:39:26\n> +++ actual.without\t2023-08-29 00:39:26\n> @@ -1 +0,0 @@\n> -D\tdir_1/file_11\n> error: last command exited with $?=1\n> not ok 61 - submodule always visited\n>\n> Notice that with fsmonitor, diff-index reports a \"D\" line that is not\n> present when fsmonitor is off. To add to that, it only reports \"D\" for\n> file_11 when I would expect that if it reported file_11, it would report\n> file_12 as well.\n>\n> I'll continue investigating this myself, but does anyone know what is\n> going on?\n"},{"id":"481112","messageId":"2ea5c2aa-64bb-adc0-4145-470321a5c8d8@jeffhostetler.com","threadId":"60164","inReplyTo":"xmqqcyz6fxvf.fsf@gitster.g","subject":"Re: Strange diff-index with fsmonitor, submodules","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2023-08-29T12:45:02Z","receivedAt":"2023-08-29T12:46:07Z","isPatch":false,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 8/28/23 9:20 PM, Junio C Hamano wrote:\n> Jonathan Tan writes:\n> \n>> There is a strange interaction where diff-index not only produces\n>> different results when run with and without fsmonitor, but produces\n>> different results for 2 entries that as far as I can tell, should behave\n>> identically (sibling files in the same directory - file_11 and file_12,\n>> and both of these filenames are only mentioned once each in the entire\n>> test).\n...\n>> I'll continue investigating this myself, but does anyone know what is\n>> going on?\n\nJust a quick ACK. I'll take a look.\n\nThanks\nJeff\n"},{"id":"481122","messageId":"03464c34-2f83-ba26-69be-d3502ed2ed38@jeffhostetler.com","threadId":"60164","inReplyTo":"20230829005606.136615-1-jonathantanmy@google.com","subject":"Re: Strange diff-index with fsmonitor, submodules","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2023-08-29T16:57:51Z","receivedAt":"2023-08-29T16:58:43Z","isPatch":false,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 8/28/23 8:56 PM, Jonathan Tan wrote:\n> There is a strange interaction where diff-index not only produces\n> different results when run with and without fsmonitor, but produces\n> different results for 2 entries that as far as I can tell, should behave\n> identically (sibling files in the same directory - file_11 and file_12,\n> and both of these filenames are only mentioned once each in the entire\n> test).\n> \n> You can see this with this patch:\n\nJonathan, what platform are you seeing this on?\n\nJeff\n\n"},{"id":"481123","messageId":"20230829170128.370527-1-jonathantanmy@google.com","threadId":"60164","inReplyTo":"03464c34-2f83-ba26-69be-d3502ed2ed38@jeffhostetler.com","subject":"Re: Strange diff-index with fsmonitor, submodules","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2023-08-29T17:01:28Z","receivedAt":"2023-08-29T17:02:27Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Jeff Hostetler writes:\n> \n> \n> On 8/28/23 8:56 PM, Jonathan Tan wrote:\n> > There is a strange interaction where diff-index not only produces\n> > different results when run with and without fsmonitor, but produces\n> > different results for 2 entries that as far as I can tell, should behave\n> > identically (sibling files in the same directory - file_11 and file_12,\n> > and both of these filenames are only mentioned once each in the entire\n> > test).\n> > \n> > You can see this with this patch:\n> \n> Jonathan, what platform are you seeing this on?\n> \n> Jeff\n\nAh, I should have mentioned this. This is on Mac OS X.\n"},{"id":"481427","messageId":"20230906060241.944886-2-sokcevic@google.com","threadId":"60164","inReplyTo":"20230829005606.136615-1-jonathantanmy@google.com","subject":"[PATCH] [diff-lib] Fix check_removed when fsmonitor is on","fromName":"Josip Sokcevic","fromEmail":"sokcevic@google.com","sentAt":"2023-09-06T06:02:42Z","receivedAt":"2023-09-06T06:02:52Z","isPatch":true,"sender":{"key":"sokcevic@google.com","avatar":"https://avatars.githubusercontent.com/u/89933621?v=4"},"body":"git-diff-index may return incorrect deleted entries when fsmonitor is used in a\nrepository with git submodules. This can be observed on Mac machines, but it\ncan affect all other supported platforms too.\n\nIf fsmonitor is used, `stat *st` may not be initialized. Since `lstat` calls\naren't not desired when fsmonitor is on, skip the entire gitlink check using\nthe same condition used to initialize `stat *st`.\n---\nThis patch is using jonathantanmy@ test case, which now passes. It's\npossible there are similar edge cases with fsmonitor, so I'll do more\nextensive e2e testing tomorrow.\n diff-lib.c | 2 +-\n t/t7527-builtin-fsmonitor.sh | 6 ++++++\n 2 files changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex d8aa777a73..c67aa5ff89 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -46,7 +46,7 @@ static int check_removed(const struct index_state *istate, const struct cache_en\n \t}\n \tif (has_symlink_leading_path(ce->name, ce_namelen(ce)))\n \t\treturn 1;\n-\tif (S_ISDIR(st->st_mode)) {\n+\tif (!(ce->ce_flags & CE_FSMONITOR_VALID) && S_ISDIR(st->st_mode)) {\n \t\tstruct object_id sub;\n \n \t\t/*\ndiff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh\nindex 0c241d6c14..894a80fbe8 100755\n--- a/t/t7527-builtin-fsmonitor.sh\n+++ b/t/t7527-builtin-fsmonitor.sh\n@@ -824,6 +824,10 @@ test_expect_success 'submodule always visited' '\n \n \tcreate_super super &&\n \tcreate_sub sub &&\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 diff-index --name-status HEAD >actual.without &&\n+\t test_cmp actual.with actual.without &&\n \n \tgit -C super submodule add ../sub ./dir_1/dir_2/sub &&\n \tgit -C super commit -m \"add sub\" &&\n@@ -837,6 +841,8 @@ test_expect_success 'submodule always visited' '\n \t# some dirt in the submodule and confirm matching output.\n \n \t# Completely clean status.\n+\techo Now running for clean status &&\n+\n \tmy_match_and_clean &&\n \n \t# .M S..U\n-- \n2.42.0.283.g2d96d420d3-goog\n\n\n"},{"id":"481453","messageId":"20230906203726.1526272-1-jonathantanmy@google.com","threadId":"60164","inReplyTo":"20230906060241.944886-2-sokcevic@google.com","subject":"Re: [PATCH] [diff-lib] Fix check_removed when fsmonitor is on","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2023-09-06T20:37:26Z","receivedAt":"2023-09-06T20:37:34Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Josip Sokcevic writes:\n> git-diff-index may return incorrect deleted entries when fsmonitor is used in a\n> repository with git submodules. This can be observed on Mac machines, but it\n> can affect all other supported platforms too.\n> \n> If fsmonitor is used, `stat *st` may not be initialized. Since `lstat` calls\n> aren't not desired when fsmonitor is on, skip the entire gitlink check using\n> the same condition used to initialize `stat *st`.\n\nAh, that's a great catch. Thanks for narrowing it down to\ncheck_removed(). \n\nI see from \"git blame\" that this was introduced in 4f3d6d0261\n(fsmonitor: skip lstat deletion check during git diff-index, 2021-03-\n17). In that commit, subsequent code in check_removed() already used\n\"st\" when it was possibly uninitialized, so perhaps no one noticed it.\nI don't think anyone discussed it either on the mailing list thread that\nintroduced this patch [1].\n\n[1] https://lore.kernel.org/git/pull.903.git.1615760258.gitgitgadget@gmail.com/\n\nLooking at this function in more detail, though, I see that the solution\nin this patch set is incomplete - both before and after this patch,\nthere are code paths in which \"st\" is never initialized, but \"st->mode\"\nis used (not in check_removed(), but in its callers).\n\nI think a more complete solution needs to look something like this:\n\n diff --git a/diff-lib.c b/diff-lib.c\n index d8aa777a73..5637237003 100644\n --- a/diff-lib.c\n +++ b/diff-lib.c\n @@ -39,10 +39,20 @@\n static int check_removed(const struct index_state *istate, const struct cache_entry *ce, struct stat *st)\n {\n assert(is_fsmonitor_refreshed(istate));\n - if (!(ce->ce_flags & CE_FSMONITOR_VALID) && lstat(ce->name, st) < 0) {\n - if (!is_missing_file_error(errno))\n - return -1;\n - return 1;\n + if (ce->ce_flags & CE_FSMONITOR_VALID) {\n + /*\n + * Both check_removed() and its callers expect lstat() to have\n + * happened and, in particular, the st_mode field to be set.\n + * Simulate this with the contents of ce.\n + */\n + memset(st, 0, sizeof(*st));\n + st->st_mode = ce->ce_mode;\n + } else {\n + if (lstat(ce->name, st) < 0) {\n + if (!is_missing_file_error(errno))\n + return -1;\n + return 1;\n + }\n }\n if (has_symlink_leading_path(ce->name, ce_namelen(ce)))\n return 1;\n\nAlso, regarding the commit message, the title should be formatted like:\n\n diff-lib: simulate stat when using fsmonitor\n\nor something like that (file, then colon, then message).\n\nYou'll also need a sign-off. See the [[sign-off]] section in\nDocumentation/SubmittingPatches for more information.\n\n> diff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh\n> index 0c241d6c14..894a80fbe8 100755\n> --- a/t/t7527-builtin-fsmonitor.sh\n> +++ b/t/t7527-builtin-fsmonitor.sh\n> @@ -824,6 +824,10 @@ test_expect_success 'submodule always visited' '\n> \n> \tcreate_super super &&\n> \tcreate_sub sub &&\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 diff-index --name-status HEAD >actual.without &&\n> +\t test_cmp actual.with actual.without &&\n> \n> \tgit -C super submodule add ../sub ./dir_1/dir_2/sub &&\n> \tgit -C super commit -m \"add sub\" &&\n\nAny reason why these additions are here instead of where I put them (in\nmy_match_and_clean())? I think it makes more sense where I put them,\nas then they would automatically apply to all tests.\n\n> @@ -837,6 +841,8 @@ test_expect_success 'submodule always visited' '\n> \t# some dirt in the submodule and confirm matching output.\n> \n> \t# Completely clean status.\n> +\techo Now running for clean status &&\n> +\n> \tmy_match_and_clean &&\n> \n> \t# .M S..U\n\nYou can remove this - I included this to show exactly where in the code\nthe test fails.\n"},{"id":"481482","messageId":"20230907170119.1536694-1-sokcevic@google.com","threadId":"60164","inReplyTo":"20230906203726.1526272-1-jonathantanmy@google.com","subject":"[PATCH v2] diff-lib: Fix check_removed when fsmonitor is on","fromName":"Josip Sokcevic","fromEmail":"sokcevic@google.com","sentAt":"2023-09-07T17:01:19Z","receivedAt":"2023-09-07T17:03:24Z","isPatch":true,"sender":{"key":"sokcevic@google.com","avatar":"https://avatars.githubusercontent.com/u/89933621?v=4"},"body":"git-diff-index may return incorrect deleted entries when fsmonitor is used in a\nrepository with git submodules. This can be observed on Mac machines, but it\ncan affect all other supported platforms too.\n\nIf fsmonitor is used, `stat *st` may not be initialized. Since `lstat` calls\naren't not desired when fsmonitor is on, skip the entire gitlink check using\nthe same condition used to initialize `stat *st`.\n\nSigned-off-by: Josip Sokcevic \n---\n diff-lib.c | 19 +++++++++++++++----\n t/t7527-builtin-fsmonitor.sh | 5 +++++\n 2 files changed, 20 insertions(+), 4 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex d8aa777a73..664613bb1b 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -39,11 +39,22 @@\n static int check_removed(const struct index_state *istate, const struct cache_entry *ce, struct stat *st)\n {\n \tassert(is_fsmonitor_refreshed(istate));\n-\tif (!(ce->ce_flags & CE_FSMONITOR_VALID) && lstat(ce->name, st) < 0) {\n-\t\tif (!is_missing_file_error(errno))\n-\t\t\treturn -1;\n-\t\treturn 1;\n+\tif (ce->ce_flags & CE_FSMONITOR_VALID) {\n+\t\t/*\n+\t\t * Both check_removed() and its callers expect lstat() to have\n+\t\t * happened and, in particular, the st_mode field to be set.\n+\t\t * Simulate this with the contents of ce.\n+\t\t */\n+\t\tmemset(st, 0, sizeof(*st));\n+\t\tst->st_mode = ce->ce_mode;\n+\t} else {\n+\t\tif (lstat(ce->name, st) < 0) {\n+\t\t\tif (!is_missing_file_error(errno))\n+\t\t\t\treturn -1;\n+\t\t\treturn 1;\n+\t\t}\n \t}\n+\n \tif (has_symlink_leading_path(ce->name, ce_namelen(ce)))\n \t\treturn 1;\n \tif (S_ISDIR(st->st_mode)) {\ndiff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh\nindex 0c241d6c14..78503158fd 100755\n--- a/t/t7527-builtin-fsmonitor.sh\n+++ b/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-- \n2.42.0.283.g2d96d420d3-goog\n\n\n"},{"id":"481486","messageId":"20230907172248.1789451-1-jonathantanmy@google.com","threadId":"60164","inReplyTo":"20230907170119.1536694-1-sokcevic@google.com","subject":"Re: [PATCH v2] diff-lib: Fix check_removed when fsmonitor is on","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2023-09-07T17:22:48Z","receivedAt":"2023-09-07T17:24:05Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Josip Sokcevic writes:\n> git-diff-index may return incorrect deleted entries when fsmonitor is used in a\n> repository with git submodules. This can be observed on Mac machines, but it\n> can affect all other supported platforms too.\n> \n> If fsmonitor is used, `stat *st` may not be initialized. Since `lstat` calls\n> aren't not desired when fsmonitor is on, skip the entire gitlink check using\n> the same condition used to initialize `stat *st`.\n\nI think this paragraph is outdated - you'll need to update it to match\nthe code in this version.\n\n> diff --git a/diff-lib.c b/diff-lib.c\n> index d8aa777a73..664613bb1b 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -39,11 +39,22 @@\n> static int check_removed(const struct index_state *istate, const struct cache_entry *ce, struct stat *st)\n> {\n> \tassert(is_fsmonitor_refreshed(istate));\n> -\tif (!(ce->ce_flags & CE_FSMONITOR_VALID) && lstat(ce->name, st) < 0) {\n> -\t\tif (!is_missing_file_error(errno))\n> -\t\t\treturn -1;\n> -\t\treturn 1;\n> +\tif (ce->ce_flags & CE_FSMONITOR_VALID) {\n> +\t\t/*\n> +\t\t * Both check_removed() and its callers expect lstat() to have\n> +\t\t * happened and, in particular, the st_mode field to be set.\n> +\t\t * Simulate this with the contents of ce.\n> +\t\t */\n> +\t\tmemset(st, 0, sizeof(*st));\n> +\t\tst->st_mode = ce->ce_mode;\n> +\t} else {\n> +\t\tif (lstat(ce->name, st) < 0) {\n> +\t\t\tif (!is_missing_file_error(errno))\n> +\t\t\t\treturn -1;\n> +\t\t\treturn 1;\n> +\t\t}\n> \t}\n> +\n> \tif (has_symlink_leading_path(ce->name, ce_namelen(ce)))\n> \t\treturn 1;\n> \tif (S_ISDIR(st->st_mode)) {\n\nI'm on the fence about whether the extra newline is necessary - the \"if\"\ndid get bigger, but the code is clear enough without the newline. I lean\ntowards removing it, to avoid cluttering the diff.\n"},{"id":"481494","messageId":"xmqqa5txluvz.fsf@gitster.g","threadId":"60164","inReplyTo":"20230907170119.1536694-1-sokcevic@google.com","subject":"Re: [PATCH v2] diff-lib: Fix check_removed when fsmonitor is on","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-07T18:07:28Z","receivedAt":"2023-09-07T18:08:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josip Sokcevic writes:\n\n> diff --git a/diff-lib.c b/diff-lib.c\n> index d8aa777a73..664613bb1b 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -39,11 +39,22 @@\n> static int check_removed(const struct index_state *istate, const struct cache_entry *ce, struct stat *st)\n> {\n> \tassert(is_fsmonitor_refreshed(istate));\n\nNot a problem this patch introduces, but doesn't this call path\n\n diff_cache()\n -> unpack_trees()\n -> oneway_diff()\n -> do_oneway_diff()\n -> show_new_file(), show_modified()\n -> get_stat_data()\n -> check_removed()\n\nviolate the assertion? If so, perhaps we should rewrite it into a\nmore explicit \"if (...) BUG(...)\" that is not compiled away.\n\n> -\tif (!(ce->ce_flags & CE_FSMONITOR_VALID) && lstat(ce->name, st) < 0) {\n> -\t\tif (!is_missing_file_error(errno))\n> -\t\t\treturn -1;\n> -\t\treturn 1;\n> +\tif (ce->ce_flags & CE_FSMONITOR_VALID) {\n> +\t\t/*\n> +\t\t * Both check_removed() and its callers expect lstat() to have\n> +\t\t * happened and, in particular, the st_mode field to be set.\n> +\t\t * Simulate this with the contents of ce.\n> +\t\t */\n> +\t\tmemset(st, 0, sizeof(*st));\n\nIt is true that the original, when CE_FSMONITOR_VALID bit is set,\nbypasses lstat() altogether and leaves the contents of st completely\nuninitialized, but this is still way too insufficient, isn't it?\n\nThere are three call sites of the check_removed() function.\n\n * The first one in run_diff_files() only cares about st.st_mode and\n other members of the structure are not looked at. This makes\n readers wonder if the \"st\" parameter to check_removed() should\n become \"mode_t *st_mode\" to clarify this point, but the primary\n thing I want to say is that this caller will not mind if we leave\n other members of st bogus (like 0-bit filled) as long as the mode\n is set correctly.\n\n * The second one in run_diff_files() passes the resulting &st to\n match_stat_with_submodule(), which in turn passes it to\n ie_match_stat(), which cares about \"struct stat\" members that are\n used for quick change detection, like owner, group, mtime.\n Giving it a bogus st will most likely cause it to report a\n change.\n\n * The third one is in get_stat_data(). This also uses the &st to\n call match_stat_with_submodule(), so it is still totally broken\n to give it a bogus st, the same way as the second caller above.\n\n> +\t\tst->st_mode = ce->ce_mode;\n\nDoes this work correctly when the cache entry points at a gitlink,\nwhich uses 0160000 that is not a valid st_mode? I think you'd want\nto use a reverse function of create_ce_mode().\n\n> +\t} else {\n> +\t\tif (lstat(ce->name, st) < 0) {\n> +\t\t\tif (!is_missing_file_error(errno))\n> +\t\t\t\treturn -1;\n> +\t\t\treturn 1;\n> +\t\t}\n> \t}\n\nAt this point, if FSMONITOR_VALID bit is not set, we will always\nperform lstat() and get all the members of st populated properly,\nwhich is a definite improvement.\n\nWhile I think this does not make it worse (it is an existing bug\nthat the code is broken for a ce with the CE_FSMONITOR_VALID bit\nset), we may want to leave a note that we _know_ the code after this\npatch is still broken. \"Simulate this with ...\" -> \"Just setting\nst_mode is still insufficient and will break majority of callers\".\n\nIt may make sense, until we clean it up, to disable the check for\nthe FSMONITOR_VALID bit in this codepath and always perform lstat().\nOptimization matters, but computing quickly in order to return an\nincorrect result is optimizing for a wrong thing. I dunno.\n\nThanks.\n"},{"id":"481544","messageId":"CAJiyOigsH=Pac1g_5ztB01uEaCBjefazUpnH5k3ARZ-COm0uLA@mail.gmail.com","threadId":"60164","inReplyTo":"xmqqa5txluvz.fsf@gitster.g","subject":"Re: [PATCH v2] diff-lib: Fix check_removed when fsmonitor is on","fromName":"Josip Sokcevic","fromEmail":"sokcevic@google.com","sentAt":"2023-09-07T23:08:42Z","receivedAt":"2023-09-07T23:09:21Z","isPatch":true,"sender":{"key":"sokcevic@google.com","avatar":"https://avatars.githubusercontent.com/u/89933621?v=4"},"body":"On Thu, Sep 7, 2023 at 11:07 AM Junio C Hamano wrote:\n>\n> Josip Sokcevic writes:\n>\n> > diff --git a/diff-lib.c b/diff-lib.c\n> > index d8aa777a73..664613bb1b 100644\n> > --- a/diff-lib.c\n> > +++ b/diff-lib.c\n> > @@ -39,11 +39,22 @@\n> > static int check_removed(const struct index_state *istate, const struct cache_entry *ce, struct stat *st)\n> > {\n> > assert(is_fsmonitor_refreshed(istate));\n>\n> Not a problem this patch introduces, but doesn't this call path\n>\n> diff_cache()\n> -> unpack_trees()\n> -> oneway_diff()\n> -> do_oneway_diff()\n> -> show_new_file(), show_modified()\n> -> get_stat_data()\n> -> check_removed()\n>\n> violate the assertion? If so, perhaps we should rewrite it into a\n> more explicit \"if (...) BUG(...)\" that is not compiled away.\n\nTrue, I will update it.\n\n>\n> > - if (!(ce->ce_flags & CE_FSMONITOR_VALID) && lstat(ce->name, st) < 0) {\n> > - if (!is_missing_file_error(errno))\n> > - return -1;\n> > - return 1;\n> > + if (ce->ce_flags & CE_FSMONITOR_VALID) {\n> > + /*\n> > + * Both check_removed() and its callers expect lstat() to have\n> > + * happened and, in particular, the st_mode field to be set.\n> > + * Simulate this with the contents of ce.\n> > + */\n> > + memset(st, 0, sizeof(*st));\n>\n> It is true that the original, when CE_FSMONITOR_VALID bit is set,\n> bypasses lstat() altogether and leaves the contents of st completely\n> uninitialized, but this is still way too insufficient, isn't it?\n>\n> There are three call sites of the check_removed() function.\n>\n> * The first one in run_diff_files() only cares about st.st_mode and\n> other members of the structure are not looked at. This makes\n> readers wonder if the \"st\" parameter to check_removed() should\n> become \"mode_t *st_mode\" to clarify this point, but the primary\n> thing I want to say is that this caller will not mind if we leave\n> other members of st bogus (like 0-bit filled) as long as the mode\n> is set correctly.\n>\n> * The second one in run_diff_files() passes the resulting &st to\n> match_stat_with_submodule(), which in turn passes it to\n> ie_match_stat(), which cares about \"struct stat\" members that are\n> used for quick change detection, like owner, group, mtime.\n> Giving it a bogus st will most likely cause it to report a\n> change.\n>\n> * The third one is in get_stat_data(). This also uses the &st to\n> call match_stat_with_submodule(), so it is still totally broken\n> to give it a bogus st, the same way as the second caller above.\n>\n> > + st->st_mode = ce->ce_mode;\n>\n> Does this work correctly when the cache entry points at a gitlink,\n> which uses 0160000 that is not a valid st_mode? I think you'd want\n> to use a reverse function of create_ce_mode().\n\nI realized this too but after I sent the patch. I don't think we have\na good way to reverse it, so the best we can do is to guess it. But I\ndon't think we should do that. Instead, should we just zero the struct\nand add a TODO? Alternative could be to use a double pointer for stat\nand do more checks in call sites, but I'm not familiar with the\ncodebase to how that branching would need to look like.\n\nAny preference?\n\n-- \nJosip Sokcevic\n"},{"id":"481554","messageId":"xmqqjzt0vhfz.fsf@gitster.g","threadId":"60164","inReplyTo":"CAJiyOigsH=Pac1g_5ztB01uEaCBjefazUpnH5k3ARZ-COm0uLA@mail.gmail.com","subject":"Re: [PATCH v2] diff-lib: Fix check_removed when fsmonitor is on","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-08T15:00:00Z","receivedAt":"2023-09-08T15:00:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josip Sokcevic writes:\n\n>> > @@ -39,11 +39,22 @@\n>> > static int check_removed(const struct index_state *istate, const struct cache_entry *ce, struct stat *st)\n>> > {\n>> > assert(is_fsmonitor_refreshed(istate));\n>>\n>> Not a problem this patch introduces, but doesn't this call path\n>>\n>> diff_cache()\n>> -> unpack_trees()\n>> -> oneway_diff()\n>> -> do_oneway_diff()\n>> -> show_new_file(), show_modified()\n>> -> get_stat_data()\n>> -> check_removed()\n>>\n>> violate the assertion? If so, perhaps we should rewrite it into a\n>> more explicit \"if (...) BUG(...)\" that is not compiled away.\n>\n> True, I will update it.\n\nNot as a part of this series, though.\n\nI think this came from the same series that were merged at 858119f6\n(Merge branch 'nk/diff-index-fsmonitor', 2021-03-24), so if we just\nsimply do a moral \"revert\" of the series, then it should disappear.\n\nMore importantly, you'd want to ask help from folks who are more\nfamiliar with fsmonitor, which would take some time, and fixing this\nassertion can be backburnered while you are fixing the original\nissue that got you started here. I see JeffH is already Cc:ed,\nwhich is good.\n\n>> > - if (!(ce->ce_flags & CE_FSMONITOR_VALID) && lstat(ce->name, st) < 0) {\n>> > - if (!is_missing_file_error(errno))\n>> > - return -1;\n>> > - return 1;\n>> > + if (ce->ce_flags & CE_FSMONITOR_VALID) {\n>> > + /*\n>> > + * Both check_removed() and its callers expect lstat() to have\n>> > + * happened and, in particular, the st_mode field to be set.\n>> > + * Simulate this with the contents of ce.\n>> > + */\n>> > + memset(st, 0, sizeof(*st));\n>>\n>> It is true that the original, when CE_FSMONITOR_VALID bit is set,\n>> bypasses lstat() altogether and leaves the contents of st completely\n>> uninitialized, but this is still way too insufficient, isn't it?\n>>\n>> There are three call sites of the check_removed() function.\n>>\n>> * The first one in run_diff_files() only cares about st.st_mode and\n>> other members of the structure are not looked at. This makes\n>> readers wonder if the \"st\" parameter to check_removed() should\n>> become \"mode_t *st_mode\" to clarify this point, but the primary\n>> thing I want to say is that this caller will not mind if we leave\n>> other members of st bogus (like 0-bit filled) as long as the mode\n>> is set correctly.\n>>\n>> * The second one in run_diff_files() passes the resulting &st to\n>> match_stat_with_submodule(), which in turn passes it to\n>> ie_match_stat(), which cares about \"struct stat\" members that are\n>> used for quick change detection, like owner, group, mtime.\n>> Giving it a bogus st will most likely cause it to report a\n>> change.\n>>\n>> * The third one is in get_stat_data(). This also uses the &st to\n>> call match_stat_with_submodule(), so it is still totally broken\n>> to give it a bogus st, the same way as the second caller above.\n>>\n>> > + st->st_mode = ce->ce_mode;\n>>\n>> Does this work correctly when the cache entry points at a gitlink,\n>> which uses 0160000 that is not a valid st_mode? I think you'd want\n>> to use a reverse function of create_ce_mode().\n>\n> I realized this too but after I sent the patch. I don't think we have\n> a good way to reverse it, so the best we can do is to guess it. But I\n> don't think we should do that. Instead, should we just zero the struct\n> and add a TODO? Alternative could be to use a double pointer for stat\n> and do more checks in call sites, but I'm not familiar with the\n> codebase to how that branching would need to look like.\n>\n> Any preference?\n\nMy preference in the short term actually is to teach this codepath\nto ignore CE_FSMONITOR_VALID bit and always run lstat() instead.\n\nThat will unbreak this \"optimization\" that allowed the code to do a\nwrong thing quickly.\n\nThe solution suitable for a bit longer term becomes rather obvious\nwhen we step back a bit and think about the purpose of this block\ninside \"if ce_flags says CE_FSMONITOR_VALID is true, do this\". The\nfsmonitor has already checked and knows that the path has not been\nupdated, i.e. what we have in the in-core index still matches what\nwe would get from running lstat(). So now what we need to do is to\npretend as if we called lstat(), recreating the data in \"struct\nstat\" from \"struct cache_entry\" well enough to fool ie_match_stat()\ninto saying that the cached stat data in ce still matches what is in\nst.\n\nCan we do that? Of course we can. When we added or refreshed the\ncache entry, we should have had \"struct stat\" taken out of lstat()\nfor the path, and copied some data from the members of \"struct stat\"\ninto \"struct cache_entry\". After all, that is how we can later ask\nie_match_stat() if the cached stat data in the \"struct cache_entry\"\nmatches the current \"struct stat\" data to tell if the path hasn't\nbeen touched since we last refreshed.\n\nWe want to have a helper function that we can ask:\n\n With the data in this 'struct cache_entry', please populate this\n 'struct stat'. Do so well enough that ie_match_stat() says \"ok,\n the path is unchanged\".\n\nYou can write such a function by studying two code paths.\n\n * ie_match_stat() and its helpers to find out which members of\n \"struct stat\" matters (i.e. compared against corresponding\n members in \"struct cache_entry\").\n\n * add_to_index() and its helpers, fill_stat_cache_info() in\n particular, to see which members of \"struct stat\" are used to\n populate ce->ce_stat_data and how (e.g. some members lose\n precision and that is find---the reverse operation will also be\n lossy and the resulting \"struct stat\" you will forge will *not*\n exactly match what you would obtain from real lstat(), but it is\n good enough to fool ie_match_stat() because you'll be comparing\n against an existing ce that was populated in a lossy way).\n\nThen, replace the above partial assignment to st with a call to such\na helper function, which should live in statinfo.c next to\nfill_stat_data() and whose function signature should look like\n\n void fill_stat_from_stat_data(struct stat *st, struct stat_data *sd);\n\nYou'd probably need to define two macros to update nsec part of c/mtime\nmembers that is conditional near where ST_CTIME_NSEC() macro is\ndefined to write that reverse function of fill_stat_data(), perhaps\nalong the lines of ...\n\n\t#ifdef NO_NSEC\n\t#define FILL_ST_CTIME_NSEC(st, nsec) do { ; /* noop */ } while (0)\n\t#else\n\t#ifdef USE_ST_TIMESPEC\n\t#define FILL_ST_CTIME_NSEC(st, nsec) (st)->st_ctimespec.tv_nsec = (nsec)\n\t#else\n\t#define FILL_ST_CTIME_NSEC(st, nsec) (st)->st_ctim.tv_nsec = (nsec)\n\t#endif\n\t#endif\n\n\n"},{"id":"481708","messageId":"20230911170901.49050-2-sokcevic@google.com","threadId":"60164","inReplyTo":"20230906060241.944886-2-sokcevic@google.com","subject":"[PATCH v3] diff-lib: Fix check_removed when fsmonitor is on","fromName":"Josip Sokcevic","fromEmail":"sokcevic@google.com","sentAt":"2023-09-11T17:09:02Z","receivedAt":"2023-09-11T21:39:27Z","isPatch":true,"sender":{"key":"sokcevic@google.com","avatar":"https://avatars.githubusercontent.com/u/89933621?v=4"},"body":"git-diff-index may return incorrect deleted entries when fsmonitor is used in a\nrepository with git submodules. This can be observed on Mac machines, but it\ncan affect all other supported platforms too.\n\nIf fsmonitor is used, `stat *st` is not initialized if cache_entry has\nCE_FSMONITOR_VALID set. But, there are three call sites that rely on stat\nafterwards, which can result in incorrect results.\n\nThis change partially reverts commit 4f3d6d0.\n\nSigned-off-by: Josip Sokcevic \n---\n diff-lib.c | 12 ++++++------\n t/t7527-builtin-fsmonitor.sh | 5 +++++\n 2 files changed, 11 insertions(+), 6 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex d8aa777a73..5848e4f9ca 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -36,14 +36,14 @@\n * exists for ce that is a submodule -- it is a submodule that is not\n * checked out). Return negative for an error.\n */\n-static int check_removed(const struct index_state *istate, const struct cache_entry *ce, struct stat *st)\n+static int check_removed(const struct cache_entry *ce, struct stat *st)\n {\n-\tassert(is_fsmonitor_refreshed(istate));\n-\tif (!(ce->ce_flags & CE_FSMONITOR_VALID) && lstat(ce->name, st) < 0) {\n+\tif (lstat(ce->name, st) < 0) {\n \t\tif (!is_missing_file_error(errno))\n \t\t\treturn -1;\n \t\treturn 1;\n \t}\n+\n \tif (has_symlink_leading_path(ce->name, ce_namelen(ce)))\n \t\treturn 1;\n \tif (S_ISDIR(st->st_mode)) {\n@@ -149,7 +149,7 @@ void run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\tmemset(&(dpath->parent[0]), 0,\n \t\t\t sizeof(struct combine_diff_parent)*5);\n \n-\t\t\tchanged = check_removed(istate, ce, &st);\n+\t\t\tchanged = check_removed(ce, &st);\n \t\t\tif (!changed)\n \t\t\t\twt_mode = ce_mode_from_stat(ce, st.st_mode);\n \t\t\telse {\n@@ -229,7 +229,7 @@ void run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t} else {\n \t\t\tstruct stat st;\n \n-\t\t\tchanged = check_removed(istate, ce, &st);\n+\t\t\tchanged = check_removed(ce, &st);\n \t\t\tif (changed) {\n \t\t\t\tif (changed < 0) {\n \t\t\t\t\tperror(ce->name);\n@@ -303,7 +303,7 @@ static int get_stat_data(const struct index_state *istate,\n \tif (!cached && !ce_uptodate(ce)) {\n \t\tint changed;\n \t\tstruct stat st;\n-\t\tchanged = check_removed(istate, ce, &st);\n+\t\tchanged = check_removed(ce, &st);\n \t\tif (changed < 0)\n \t\t\treturn -1;\n \t\telse if (changed) {\ndiff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh\nindex 0c241d6c14..78503158fd 100755\n--- a/t/t7527-builtin-fsmonitor.sh\n+++ b/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-- \n2.42.0.283.g2d96d420d3-goog\n\n\n"},{"id":"481737","messageId":"xmqqsf7ks4nn.fsf@gitster.g","threadId":"60164","inReplyTo":"20230911170901.49050-2-sokcevic@google.com","subject":"Re: [PATCH v3] diff-lib: Fix check_removed when fsmonitor is on","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-11T22:53:32Z","receivedAt":"2023-09-12T03:20:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josip Sokcevic writes:\n\n> git-diff-index may return incorrect deleted entries when fsmonitor is used in a\n> repository with git submodules. This can be observed on Mac machines, but it\n> can affect all other supported platforms too.\n>\n> If fsmonitor is used, `stat *st` is not initialized if cache_entry has\n> CE_FSMONITOR_VALID set. But, there are three call sites that rely on stat\n> afterwards, which can result in incorrect results.\n>\n> This change partially reverts commit 4f3d6d0.\n>\n> Signed-off-by: Josip Sokcevic \n> ---\n> diff-lib.c | 12 ++++++------\n> t/t7527-builtin-fsmonitor.sh | 5 +++++\n> 2 files changed, 11 insertions(+), 6 deletions(-)\n\nThis certainly is more \"complete\" if simpler than the previous one\n;-)\n\nIn the longer term, we would probably want to enable optimization\nusing what fsmonitor knows, but as we have seen in the review on\nthe previous round, this code needs a bit more work than the\noriginal we are reverting here to get it right, and in the shorter\nterm, hopefully this would do.\n\nThanks.\n\n> diff --git a/diff-lib.c b/diff-lib.c\n> index d8aa777a73..5848e4f9ca 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -36,14 +36,14 @@\n> * exists for ce that is a submodule -- it is a submodule that is not\n> * checked out). Return negative for an error.\n> */\n> -static int check_removed(const struct index_state *istate, const struct cache_entry *ce, struct stat *st)\n> +static int check_removed(const struct cache_entry *ce, struct stat *st)\n> {\n> -\tassert(is_fsmonitor_refreshed(istate));\n> -\tif (!(ce->ce_flags & CE_FSMONITOR_VALID) && lstat(ce->name, st) < 0) {\n> +\tif (lstat(ce->name, st) < 0) {\n> \t\tif (!is_missing_file_error(errno))\n> \t\t\treturn -1;\n> \t\treturn 1;\n> \t}\n> +\n> \tif (has_symlink_leading_path(ce->name, ce_namelen(ce)))\n> \t\treturn 1;\n> \tif (S_ISDIR(st->st_mode)) {\n> @@ -149,7 +149,7 @@ void run_diff_files(struct rev_info *revs, unsigned int option)\n> \t\t\tmemset(&(dpath->parent[0]), 0,\n> \t\t\t sizeof(struct combine_diff_parent)*5);\n> \n> -\t\t\tchanged = check_removed(istate, ce, &st);\n> +\t\t\tchanged = check_removed(ce, &st);\n> \t\t\tif (!changed)\n> \t\t\t\twt_mode = ce_mode_from_stat(ce, st.st_mode);\n> \t\t\telse {\n> @@ -229,7 +229,7 @@ void run_diff_files(struct rev_info *revs, unsigned int option)\n> \t\t} else {\n> \t\t\tstruct stat st;\n> \n> -\t\t\tchanged = check_removed(istate, ce, &st);\n> +\t\t\tchanged = check_removed(ce, &st);\n> \t\t\tif (changed) {\n> \t\t\t\tif (changed < 0) {\n> \t\t\t\t\tperror(ce->name);\n> @@ -303,7 +303,7 @@ static int get_stat_data(const struct index_state *istate,\n> \tif (!cached && !ce_uptodate(ce)) {\n> \t\tint changed;\n> \t\tstruct stat st;\n> -\t\tchanged = check_removed(istate, ce, &st);\n> +\t\tchanged = check_removed(ce, &st);\n> \t\tif (changed < 0)\n> \t\t\treturn -1;\n> \t\telse if (changed) {\n> diff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh\n> index 0c241d6c14..78503158fd 100755\n> --- a/t/t7527-builtin-fsmonitor.sh\n> +++ b/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":"481738","messageId":"CAJiyOijMvqK184wFgoXFyX5kmURkX3k2OmuiBpArikj26iHpMA@mail.gmail.com","threadId":"60164","inReplyTo":"xmqqsf7ks4nn.fsf@gitster.g","subject":"Re: [PATCH v3] diff-lib: Fix check_removed when fsmonitor is on","fromName":"Josip Sokcevic","fromEmail":"sokcevic@google.com","sentAt":"2023-09-12T03:03:47Z","receivedAt":"2023-09-12T03:24:32Z","isPatch":true,"sender":{"key":"sokcevic@google.com","avatar":"https://avatars.githubusercontent.com/u/89933621?v=4"},"body":"On Mon, Sep 11, 2023 at 3:53 PM Junio C Hamano wrote:\n> This certainly is more \"complete\" if simpler than the previous one\n> ;-)\n>\n> In the longer term, we would probably want to enable optimization\n> using what fsmonitor knows, but as we have seen in the review on\n> the previous round, this code needs a bit more work than the\n> original we are reverting here to get it right, and in the shorter\n> term, hopefully this would do.\n\nYes, I agree we should optimize this in a follow up. One thing I'm not\nsure about is if we should try to construct `struct stat` using\n`cache_index`, or we should check for `CE_FSMONITOR_VALID` in a way\nthat `stat` would no longer be needed for those code paths.\n\n\n-- \nJosip Sokcevic\n"},{"id":"481774","messageId":"xmqqfs3jpbg2.fsf@gitster.g","threadId":"60164","inReplyTo":"CAJiyOijMvqK184wFgoXFyX5kmURkX3k2OmuiBpArikj26iHpMA@mail.gmail.com","subject":"Re: [PATCH v3] diff-lib: Fix check_removed when fsmonitor is on","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-12T17:07:25Z","receivedAt":"2023-09-12T17:07:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josip Sokcevic writes:\n\n> Yes, I agree we should optimize this in a follow up. One thing I'm not\n> sure about is if we should try to construct `struct stat` using\n> `cache_index`, or we should check for `CE_FSMONITOR_VALID` in a way\n> that `stat` would no longer be needed for those code paths.\n\nGood point. \n\nIt seems to be entirely doable, even though the stench from the\nabstraction layer violation may be horrible.\n\nie_match_stat(), which is called by match_stat_with_submodule(),\ndoes pay attention to CE_FSMONITOR_VALID bit, so none of the members\nof struct stat matters when the bit is set. But the bit is not set,\nall members that fill_stat_data() and ce_match_stat_basic() care\nabout do matter. Other code that follows callers of check_removed()\ndo care about at least .st_mode member, and I suspect that in the\ncurrent code .st_mode is the only member that gets looked at.\n\nSo after all, I think your original \"fix\" was correct, but it took\nus some time to figure out why it was, which means we would want to\nexplain it in the log message for developers who would want to touch\nthe same area in the future.\n\nThanks.\n"},{"id":"481858","messageId":"CAJiyOiidpD5eNvxVV8rZP7Xej4_8bBfgTOG75T8PVsmvwya3hg@mail.gmail.com","threadId":"60164","inReplyTo":"xmqqfs3jpbg2.fsf@gitster.g","subject":"Re: [PATCH v3] diff-lib: Fix check_removed when fsmonitor is on","fromName":"Josip Sokcevic","fromEmail":"sokcevic@google.com","sentAt":"2023-09-14T22:39:13Z","receivedAt":"2023-09-14T22:39:27Z","isPatch":true,"sender":{"key":"sokcevic@google.com","avatar":"https://avatars.githubusercontent.com/u/89933621?v=4"},"body":"On Tue, Sep 12, 2023 at 10:07 AM Junio C Hamano wrote:\n> It seems to be entirely doable, even though the stench from the\n> abstraction layer violation may be horrible.\n>\n> ie_match_stat(), which is called by match_stat_with_submodule(),\n> does pay attention to CE_FSMONITOR_VALID bit, so none of the members\n> of struct stat matters when the bit is set. But the bit is not set,\n> all members that fill_stat_data() and ce_match_stat_basic() care\n> about do matter. Other code that follows callers of check_removed()\n> do care about at least .st_mode member, and I suspect that in the\n> current code .st_mode is the only member that gets looked at.\n>\n> So after all, I think your original \"fix\" was correct, but it took\n> us some time to figure out why it was, which means we would want to\n> explain it in the log message for developers who would want to touch\n> the same area in the future.\n\nI finished testing this - after my original fix and after running all\ntests, I can confirm that `st_mode` of `struct stat` is indeed not\nconsumed if CE_FSMONITOR_VALID is set. But, it's fragile and likely to\ncause problems in the future. Your approach of constructing `struct\nstat` based on `struct cache_entry` is the way to go.\n\nI see you created a new set of patches in a separate thread, so I'll\nstart those tests and report back there.\n\nThanks!\n\n-- \nJosip Sokcevic\n"},{"id":"481963","messageId":"xmqqy1h3l9qp.fsf@gitster.g","threadId":"60164","inReplyTo":"CAJiyOiidpD5eNvxVV8rZP7Xej4_8bBfgTOG75T8PVsmvwya3hg@mail.gmail.com","subject":"Re: [PATCH v3] diff-lib: Fix check_removed when fsmonitor is on","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-18T16:35:58Z","receivedAt":"2023-09-18T16:40:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josip Sokcevic writes:\n\n> I see you created a new set of patches in a separate thread, so I'll\n> start those tests and report back there.\n\nThanks.\n"}]}