{"thread":{"id":"57139","subject":"[PATCH] update-index: refresh should rewrite index in case of racy timestamps","startedAt":"2021-12-22T13:56:35Z","lastAt":"2022-01-07T11:17:42Z","messageCount":20,"participants":["Marc Strapetz via GitGitGadget","Junio C Hamano","Marc Strapetz"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"444769","messageId":"pull.1105.git.1640181390841.gitgitgadget@gmail.com","threadId":"57139","inReplyTo":null,"subject":"[PATCH] update-index: refresh should rewrite index in case of racy timestamps","fromName":"Marc Strapetz via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-12-22T13:56:30Z","receivedAt":"2021-12-22T13:56:35Z","isPatch":true,"sender":{"key":"marc.strapetz@syntevo.com","avatar":"https://avatars.githubusercontent.com/u/3380730?v=4"},"body":"From: Marc Strapetz \n\nupdate-index --refresh and --really-refresh should force writing of the\nindex file if racy timestamps have been encountered, as status already\ndoes [1].\n\nNote that calling update-index still does not guarantee that there will\nbe no more racy timestamps afterwards (the same holds true for status):\n\n- calling update-index immediately after touching and adding a file may\n still leave racy timestamps if all three operations occur within the\n racy-tolerance (usually 1 second unless USE_NSEC has been defined)\n\n- calling update-index for timestamps which are set into the future\n will leave them racy\n\nTo guarantee that such racy timestamps will be resolved would require to\nwait until the system clock has passed beyond these timestamps and only\nthen write the index file. Especially for future timestamps, this does\nnot seem feasible because of possibly long delays/hangs.\n\nBoth --refresh and --really-refresh may in theory be used in\ncombination with --unresolve and --again which may reset the\n\"active_cache_changed\" flag. There is no difference of whether we\nwrite the index due to racy timestamps or due to other\nreasons, like if --really-refresh has detected CE_ENTRY_CHANGED in\nrefresh_cache(). Hence, we will set the \"active_cache_changed\" flag\nimmediately after calling refresh_cache().\n\n[1] https://lore.kernel.org/git/d3dd805c-7c1d-30a9-6574-a7bfcb7fc013@syntevo.com/\n\nSigned-off-by: Marc Strapetz \n---\n update-index: refresh should rewrite index in case of racy timestamps\n \n This patch makes update-index --refresh write the index if it contains\n racy timestamps, as discussed at [1].\n \n [1]\n https://lore.kernel.org/git/d3dd805c-7c1d-30a9-6574-a7bfcb7fc013@syntevo.com/\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1105%2Fmstrap%2Ffeature%2Fupdate-index-refresh-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1105/mstrap/feature/update-index-refresh-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1105\n\n builtin/update-index.c | 6 +++\n cache.h | 1 +\n read-cache.c | 2 +-\n t/t2108-update-index-refresh-racy.sh | 58 ++++++++++++++++++++++++++++\n 4 files changed, 66 insertions(+), 1 deletion(-)\n create mode 100755 t/t2108-update-index-refresh-racy.sh\n\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex 187203e8bb5..0a069281e23 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -787,6 +787,12 @@ static int refresh(struct refresh_params *o, unsigned int flag)\n \tsetup_work_tree();\n \tread_cache();\n \t*o->has_errors |= refresh_cache(o->flags | flag);\n+\tif (has_racy_timestamp(&the_index)) {\n+\t\t/* For racy timestamps we should set active_cache_changed immediately:\n+\t\t * other callbacks may follow for which some of them may reset\n+\t\t * active_cache_changed. */\n+\t\tactive_cache_changed |= SOMETHING_CHANGED;\n+\t}\n \treturn 0;\n }\n \ndiff --git a/cache.h b/cache.h\nindex cfba463aa97..dd1932e2d0e 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -891,6 +891,7 @@ void *read_blob_data_from_index(struct index_state *, const char *, unsigned lon\n #define CE_MATCH_IGNORE_FSMONITOR 0X20\n int is_racy_timestamp(const struct index_state *istate,\n \t\t const struct cache_entry *ce);\n+int has_racy_timestamp(struct index_state *istate);\n int ie_match_stat(struct index_state *, const struct cache_entry *, struct stat *, unsigned int);\n int ie_modified(struct index_state *, const struct cache_entry *, struct stat *, unsigned int);\n \ndiff --git a/read-cache.c b/read-cache.c\nindex cbe73f14e5e..ed297635a33 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2775,7 +2775,7 @@ static int repo_verify_index(struct repository *repo)\n \treturn verify_index_from(repo->index, repo->index_file);\n }\n \n-static int has_racy_timestamp(struct index_state *istate)\n+int has_racy_timestamp(struct index_state *istate)\n {\n \tint entries = istate->cache_nr;\n \tint i;\ndiff --git a/t/t2108-update-index-refresh-racy.sh b/t/t2108-update-index-refresh-racy.sh\nnew file mode 100755\nindex 00000000000..ece1151847c\n--- /dev/null\n+++ b/t/t2108-update-index-refresh-racy.sh\n@@ -0,0 +1,58 @@\n+#!/bin/sh\n+\n+test_description='update-index refresh tests related to racy timestamps'\n+\n+. ./test-lib.sh\n+\n+reset_mtime() {\n+\ttest-tool chmtime =$(test-tool chmtime --get .git/fs-tstamp) $1\n+}\n+\n+update_assert_unchanged() {\n+\tlocal ts1=$(test-tool chmtime --get .git/index) &&\n+\tgit update-index $1 &&\n+\tlocal ts2=$(test-tool chmtime --get .git/index) &&\n+\t[ $ts1 -eq $ts2 ]\n+}\n+\n+update_assert_changed() {\n+\tlocal ts1=$(test-tool chmtime --get .git/index) &&\n+\ttest_might_fail git update-index $1 &&\n+\tlocal ts2=$(test-tool chmtime --get .git/index) &&\n+\t[ $ts1 -ne $ts2 ]\n+}\n+\n+test_expect_success 'setup' '\n+\ttouch .git/fs-tstamp &&\n+\ttest-tool chmtime -1 .git/fs-tstamp &&\n+\techo content >file &&\n+\treset_mtime file &&\n+\n+\tgit add file &&\n+\tgit commit -m \"initial import\"\n+'\n+\n+test_expect_success '--refresh has no racy timestamps to fix' '\n+\treset_mtime .git/index &&\n+\ttest-tool chmtime +1 .git/index &&\n+\tupdate_assert_unchanged --refresh\n+'\n+\n+test_expect_success '--refresh should fix racy timestamp' '\n+\treset_mtime .git/index &&\n+\tupdate_assert_changed --refresh\n+'\n+\n+test_expect_success '--really-refresh should fix racy timestamp' '\n+\treset_mtime .git/index &&\n+\tupdate_assert_changed --really-refresh\n+'\n+\n+test_expect_success '--refresh should fix racy timestamp even if needs update' '\n+\techo content2 >file &&\n+\treset_mtime file &&\n+\treset_mtime .git/index &&\n+\tupdate_assert_changed --refresh\n+'\n+\n+test_done\n\nbase-commit: 597af311a2899bfd6640b9b107622c5795d5f998\n-- \ngitgitgadget\n"},{"id":"444844","messageId":"xmqqfsqkdwo4.fsf@gitster.g","threadId":"57139","inReplyTo":"pull.1105.git.1640181390841.gitgitgadget@gmail.com","subject":"Re: [PATCH] update-index: refresh should rewrite index in case of racy timestamps","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-22T23:52:59Z","receivedAt":"2021-12-22T23:53:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Marc Strapetz via GitGitGadget\" writes:\n\n> builtin/update-index.c | 6 +++\n> cache.h | 1 +\n> read-cache.c | 2 +-\n> t/t2108-update-index-refresh-racy.sh | 58 ++++++++++++++++++++++++++++\n> 4 files changed, 66 insertions(+), 1 deletion(-)\n> create mode 100755 t/t2108-update-index-refresh-racy.sh\n>\n> diff --git a/builtin/update-index.c b/builtin/update-index.c\n> index 187203e8bb5..0a069281e23 100644\n> --- a/builtin/update-index.c\n> +++ b/builtin/update-index.c\n> @@ -787,6 +787,12 @@ static int refresh(struct refresh_params *o, unsigned int flag)\n> \tsetup_work_tree();\n> \tread_cache();\n> \t*o->has_errors |= refresh_cache(o->flags | flag);\n> +\tif (has_racy_timestamp(&the_index)) {\n> +\t\t/* For racy timestamps we should set active_cache_changed immediately:\n> +\t\t * other callbacks may follow for which some of them may reset\n> +\t\t * active_cache_changed. */\n> +\t\tactive_cache_changed |= SOMETHING_CHANGED;\n> +\t}\n\nDocumentation/CodingGuidelines says:\n\n - Multi-line comments include their delimiters on separate lines from\n the text. E.g.\n\n\t/*\n\t * A very long\n\t * multi-line comment.\n\t */\n\nThe last half-sentence puzzles me, partly because of the word\n\"callback\", which is an implementation detail of how --refresh and\nother actions are triggered by the update-index command. Calling\nthem \"operation\" or \"action\" might be easier to understand. I dunno.\n\nBut more problematic is the word \"reset\", which at least to me\nimplies that the SOMETHING_CHANGED bit may be cleared by them, which\nsounds just wrong and broken.\n\n ... goes and looks ...\n\nAh, there are cases where we do clear active_cache_changed when we\nnotice that an operation detected an error, to avoid spreading the\nbreakage by writing the index file out, and I think that is the\nright thing to do. Which means that the above patch is not quite\nright. Perhaps taking all of the above together, something like\nthis?\n\n\t*o->has_errors |= refresh_cache(o->flags | flag);\n\tif (*o->has_errors)\n\t\tactive_cache_changed = 0; \n\telse if (has_racy_timestamps(&the_index))\n \t/*\n\t\t * Even if nothing else has changed, updating the file\n\t\t * increases the chance that racy timestamps become\n\t\t * non-racy, helping future run-time performance.\n\t\t */\n\t\tactive_cache_changed |= SOMETHING_CHANGED;\n\n\n> diff --git a/t/t2108-update-index-refresh-racy.sh b/t/t2108-update-index-refresh-racy.sh\n> new file mode 100755\n> index 00000000000..ece1151847c\n> --- /dev/null\n> +++ b/t/t2108-update-index-refresh-racy.sh\n> @@ -0,0 +1,58 @@\n> +#!/bin/sh\n> +\n> +test_description='update-index refresh tests related to racy timestamps'\n> +\n> +. ./test-lib.sh\n> +\n> +reset_mtime() {\n\nDocumentation/CodingGuidelines\n\n - We prefer a space between the function name and the parentheses,\n and no space inside the parentheses. The opening \"{\" should also\n be on the same line.\n\n\t(incorrect)\n\tmy_function(){\n\t\t...\n\n\t(correct)\n\tmy_function () {\n\t\t...\n\n> +\ttest-tool chmtime =$(test-tool chmtime --get .git/fs-tstamp) $1\n\nEven if we know all the existing callers pass a single word argument\nto this function, it would be a good discipline to put double-quotes\naround \"$1\" to assure the readers that we are future-proofed.\n\n> +}\n> +\n> +update_assert_unchanged() {\n> +\tlocal ts1=$(test-tool chmtime --get .git/index) &&\n> +\tgit update-index $1 &&\n> +\tlocal ts2=$(test-tool chmtime --get .git/index) &&\n> +\t[ $ts1 -eq $ts2 ]\n\nDocumentation/CodingGuidelines\n\n - We prefer \"test\" over \"[ ... ]\".\n\n> +}\n> +\n> +update_assert_changed() {\n> +\tlocal ts1=$(test-tool chmtime --get .git/index) &&\n> +\ttest_might_fail git update-index $1 &&\n> +\tlocal ts2=$(test-tool chmtime --get .git/index) &&\n> +\t[ $ts1 -ne $ts2 ]\n> +}\n> +\n> +test_expect_success 'setup' '\n> +\ttouch .git/fs-tstamp &&\n\nNot that it is wrong, but do we need to create such a throw-away\nfile inside the .git directory?\n\nWhen we care only the presence of a path, and not that the path has\nthe current timestamp, we prefer not to use \"touch\".\n\n\t>.git/fs-tstamp\n\nI am debating myself which is more appropriate in this case. A\nmistaken implementation of \"touch\" could call gettimeofday() and use\nthe result to call utimes(), leaving wallclock timestamp in the\nresult, but redirecation to create or truncate the path is a more\nguaranteed way to make sure the timestamp comes from the filesystem,\nso it may be more suitable for our needs here.\n\n> +\ttest-tool chmtime -1 .git/fs-tstamp &&\n> +\techo content >file &&\n> +\treset_mtime file &&\n> +\n> +\tgit add file &&\n> +\tgit commit -m \"initial import\"\n> +'\n> +\n> +test_expect_success '--refresh has no racy timestamps to fix' '\n> +\treset_mtime .git/index &&\n> +\ttest-tool chmtime +1 .git/index &&\n> +\tupdate_assert_unchanged --refresh\n> +'\n> +\n> +test_expect_success '--refresh should fix racy timestamp' '\n> +\treset_mtime .git/index &&\n> +\tupdate_assert_changed --refresh\n> +'\n> +\n> +test_expect_success '--really-refresh should fix racy timestamp' '\n> +\treset_mtime .git/index &&\n> +\tupdate_assert_changed --really-refresh\n> +'\n> +\n> +test_expect_success '--refresh should fix racy timestamp even if needs update' '\n> +\techo content2 >file &&\n> +\treset_mtime file &&\n> +\treset_mtime .git/index &&\n> +\tupdate_assert_changed --refresh\n> +'\n> +\n> +test_done\n>\n> base-commit: 597af311a2899bfd6640b9b107622c5795d5f998\n"},{"id":"444867","messageId":"b97672fa-837f-1e28-f7f2-aee80e52d374@syntevo.com","threadId":"57139","inReplyTo":"xmqqfsqkdwo4.fsf@gitster.g","subject":"Re: [PATCH] update-index: refresh should rewrite index in case of racy timestamps","fromName":"Marc Strapetz","fromEmail":"marc.strapetz@syntevo.com","sentAt":"2021-12-23T18:24:32Z","receivedAt":"2021-12-23T18:24:42Z","isPatch":true,"sender":{"key":"marc.strapetz@syntevo.com","avatar":"https://avatars.githubusercontent.com/u/3380730?v=4"},"body":"On 23/12/2021 00:52, Junio C Hamano wrote:\n> Ah, there are cases where we do clear active_cache_changed when we\n> notice that an operation detected an error, to avoid spreading the\n> breakage by writing the index file out, and I think that is the\n> right thing to do. Which means that the above patch is not quite\n> right. Perhaps taking all of the above together, something like\n> this?\n> \n> \t*o->has_errors |= refresh_cache(o->flags | flag);\n> \tif (*o->has_errors)\n> \t\tactive_cache_changed = 0;\n> \telse if (has_racy_timestamps(&the_index))\n> \t/*\n> \t\t * Even if nothing else has changed, updating the file\n> \t\t * increases the chance that racy timestamps become\n> \t\t * non-racy, helping future run-time performance.\n> \t\t */\n> \t\tactive_cache_changed |= SOMETHING_CHANGED;\n\nI think it's safe to write the index even if refresh_cache() reports an \n\"error\" and we should actually do that:\n\nThe underlying refresh_index() will report an \"error\" only for \"file: \nneeds merge\" and \"file: needs update\". In both cases, the corresponding \nentries will not have been updated. Every entry which has been updated \nis good on its own and writing these updates makes the index a little \nbit better. Subsequent calls to refresh_index() won't have to do the \nsame work again (like invoking the quite expensive LFS filter).\n\nThis is also how cmd_status() currently works: it does not pay attention \nto the return value of refresh_index() and will always write the index \nif racy timestamps are encountered.\n\nOverall, the \"error\" handling in update-index.c might not always do what \none expects. Let's consider your suggested fix. When invoking:\n\nupdate-index --refresh\n\nthis won't fix racy timestamps, however:\n\nupdate-index --refresh --add untracked\n\nwill do. I think this is caused by active_cache_changed being used in \ntwo different ways: to indicate that the cache should be written and to \nindicate that it must not be written. It might be a good idea to take \nthe latter \"block index write\" to a separate static variable in \nupdate-index.c.\n\nCandidate usages of this new \"block index write\" variable will be in the \nexisting callbacks: errors detected in unresolve_callback() should \nprobably continue to block an index write, to ensure that either all or \nnone of the specified files will be unresolved. For the \nreupdate_callback(), the underlying do_reupdate() seems to return 0 \nalways, so there is dead code in the callback (or am I completely \nblind?). To stay on the safe side, we may still continue to block an \nindex write here. The refresh_callback() will never block an index write.\n\nDoes it make sense to clarify error handling in some preceding commit \nand only then address the razy timestamps? It will probably make this \nsecond commit clearer.\n\n>> +}\n>> +\n>> +update_assert_changed() {\n>> +\tlocal ts1=$(test-tool chmtime --get .git/index) &&\n>> +\ttest_might_fail git update-index $1 &&\n>> +\tlocal ts2=$(test-tool chmtime --get .git/index) &&\n>> +\t[ $ts1 -ne $ts2 ]\n>> +}\n>> +\n>> +test_expect_success 'setup' '\n>> +\ttouch .git/fs-tstamp &&\n> \n> Not that it is wrong, but do we need to create such a throw-away\n> file inside the .git directory?\n\nWe actually only need a timestamp for which we know that it is before \nthe timestamp the next file system operation would create. I agree that \nit should be easy to rewrite that using \"test-tool chmtime\". This should \nalso simplify reset_mtime().\n\nRegarding all other comments, thanks, I'll address them as suggested for \nthe next patch. And sorry for not checking CodingGuidelines before (I \nhad completely missed this document).\n\n-Marc\n"},{"id":"445516","messageId":"pull.1105.v2.git.1641388523.gitgitgadget@gmail.com","threadId":"57139","inReplyTo":"pull.1105.git.1640181390841.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] update-index: refresh should rewrite index in case of racy timestamps","fromName":"Marc Strapetz via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-01-05T13:15:21Z","receivedAt":"2022-01-05T13:15:30Z","isPatch":true,"sender":{"key":"marc.strapetz@syntevo.com","avatar":"https://avatars.githubusercontent.com/u/3380730?v=4"},"body":"This patch makes update-index --refresh write the index if it contains racy\ntimestamps, as discussed at [1].\n\nChanges since v1:\n\n * main commit message now uses 'git update-index' and the paragraph was\n dropped\n * t/t7508-status.sh: two tests added which capture status racy handling\n * builtin/update-index.c: comment improved\n * t/t2108-update-index-refresh-racy.sh: major overhaul\n * one test case added\n * mtime-manipulations simplified and aligned to t7508\n * code style fixes, as discussed\n\n[1]\nhttps://lore.kernel.org/git/d3dd805c-7c1d-30a9-6574-a7bfcb7fc013@syntevo.com/\n\nMarc Strapetz (2):\n t7508: add tests capturing racy timestamp handling\n update-index: refresh should rewrite index in case of racy timestamps\n\n builtin/update-index.c | 11 +++++\n cache.h | 1 +\n read-cache.c | 2 +-\n t/t2108-update-index-refresh-racy.sh | 64 ++++++++++++++++++++++++++++\n t/t7508-status.sh | 28 ++++++++++++\n 5 files changed, 105 insertions(+), 1 deletion(-)\n create mode 100755 t/t2108-update-index-refresh-racy.sh\n\n\nbase-commit: dcc0cd074f0c639a0df20461a301af6d45bd582e\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1105%2Fmstrap%2Ffeature%2Fupdate-index-refresh-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1105/mstrap/feature/update-index-refresh-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1105\n\nRange-diff vs v1:\n\n -: ----------- > 1: 7d58f806111 t7508: add tests capturing racy timestamp handling\n 1: 8f9618a44c5 ! 2: dfeabf6af15 update-index: refresh should rewrite index in case of racy timestamps\n @@ Metadata\n ## Commit message ##\n update-index: refresh should rewrite index in case of racy timestamps\n \n - update-index --refresh and --really-refresh should force writing of the\n - index file if racy timestamps have been encountered, as status already\n - does [1].\n + 'git update-index --refresh' and '--really-refresh' should force writing\n + of the index file if racy timestamps have been encountered, as\n + 'git status' already does [1].\n \n - Note that calling update-index still does not guarantee that there will\n - be no more racy timestamps afterwards (the same holds true for status):\n + Note that calling 'git update-index --refresh' still does not guarantee\n + that there will be no more racy timestamps afterwards (the same holds\n + true for 'git status'):\n \n - - calling update-index immediately after touching and adding a file may\n - still leave racy timestamps if all three operations occur within the\n - racy-tolerance (usually 1 second unless USE_NSEC has been defined)\n + - calling 'git update-index --refresh' immediately after touching and\n + adding a file may still leave racy timestamps if all three operations\n + occur within the racy-tolerance (usually 1 second unless USE_NSEC has\n + been defined)\n \n - - calling update-index for timestamps which are set into the future\n - will leave them racy\n + - calling 'git update-index --refresh' for timestamps which are set into\n + the future will leave them racy\n \n To guarantee that such racy timestamps will be resolved would require to\n wait until the system clock has passed beyond these timestamps and only\n then write the index file. Especially for future timestamps, this does\n not seem feasible because of possibly long delays/hangs.\n \n - Both --refresh and --really-refresh may in theory be used in\n - combination with --unresolve and --again which may reset the\n - \"active_cache_changed\" flag. There is no difference of whether we\n - write the index due to racy timestamps or due to other\n - reasons, like if --really-refresh has detected CE_ENTRY_CHANGED in\n - refresh_cache(). Hence, we will set the \"active_cache_changed\" flag\n - immediately after calling refresh_cache().\n -\n [1] https://lore.kernel.org/git/d3dd805c-7c1d-30a9-6574-a7bfcb7fc013@syntevo.com/\n \n Signed-off-by: Marc Strapetz \n @@ builtin/update-index.c: static int refresh(struct refresh_params *o, unsigned in\n \tread_cache();\n \t*o->has_errors |= refresh_cache(o->flags | flag);\n +\tif (has_racy_timestamp(&the_index)) {\n -+\t\t/* For racy timestamps we should set active_cache_changed immediately:\n -+\t\t * other callbacks may follow for which some of them may reset\n -+\t\t * active_cache_changed. */\n ++\t\t/*\n ++\t\t * Even if nothing else has changed, updating the file\n ++\t\t * increases the chance that racy timestamps become\n ++\t\t * non-racy, helping future run-time performance.\n ++\t\t * We do that even in case of \"errors\" returned by\n ++\t\t * refresh_cache() as these are no actual errors.\n ++\t\t * cmd_status() does the same.\n ++\t\t */\n +\t\tactive_cache_changed |= SOMETHING_CHANGED;\n +\t}\n \treturn 0;\n @@ t/t2108-update-index-refresh-racy.sh (new)\n +\n +test_description='update-index refresh tests related to racy timestamps'\n +\n ++TEST_PASSES_SANITIZE_LEAK=true\n +. ./test-lib.sh\n +\n -+reset_mtime() {\n -+\ttest-tool chmtime =$(test-tool chmtime --get .git/fs-tstamp) $1\n -+}\n -+\n -+update_assert_unchanged() {\n -+\tlocal ts1=$(test-tool chmtime --get .git/index) &&\n -+\tgit update-index $1 &&\n -+\tlocal ts2=$(test-tool chmtime --get .git/index) &&\n -+\t[ $ts1 -eq $ts2 ]\n ++reset_files () {\n ++\techo content >file &&\n ++\techo content >other &&\n ++\ttest-tool chmtime =1234567890 file &&\n ++\ttest-tool chmtime =1234567890 other\n +}\n +\n -+update_assert_changed() {\n -+\tlocal ts1=$(test-tool chmtime --get .git/index) &&\n -+\ttest_might_fail git update-index $1 &&\n -+\tlocal ts2=$(test-tool chmtime --get .git/index) &&\n -+\t[ $ts1 -ne $ts2 ]\n ++update_assert_changed () {\n ++\ttest-tool chmtime =1234567890 .git/index &&\n ++\ttest_might_fail git update-index \"$1\" &&\n ++\ttest-tool chmtime --get .git/index >.git/out &&\n ++\t! grep ^1234567890 .git/out\n +}\n +\n +test_expect_success 'setup' '\n -+\ttouch .git/fs-tstamp &&\n -+\ttest-tool chmtime -1 .git/fs-tstamp &&\n -+\techo content >file &&\n -+\treset_mtime file &&\n -+\n -+\tgit add file &&\n ++\treset_files &&\n ++\t# we are calling reset_files() a couple of times during tests;\n ++\t# test-tool chmtime does not change the ctime; to not weaken\n ++\t# or even break our tests, disable ctime-checks entirely\n ++\tgit config core.trustctime false &&\n ++\tgit add file other &&\n +\tgit commit -m \"initial import\"\n +'\n +\n +test_expect_success '--refresh has no racy timestamps to fix' '\n -+\treset_mtime .git/index &&\n -+\ttest-tool chmtime +1 .git/index &&\n -+\tupdate_assert_unchanged --refresh\n ++\treset_files &&\n ++\ttest-tool chmtime =1234567891 .git/index &&\n ++\tgit update-index --refresh &&\n ++\ttest-tool chmtime --get .git/index >.git/out &&\n ++\tgrep ^1234567891 .git/out\n +'\n +\n +test_expect_success '--refresh should fix racy timestamp' '\n -+\treset_mtime .git/index &&\n ++\treset_files &&\n +\tupdate_assert_changed --refresh\n +'\n +\n +test_expect_success '--really-refresh should fix racy timestamp' '\n -+\treset_mtime .git/index &&\n ++\treset_files &&\n +\tupdate_assert_changed --really-refresh\n +'\n +\n -+test_expect_success '--refresh should fix racy timestamp even if needs update' '\n ++test_expect_success '--refresh should fix racy timestamp if other file needs update' '\n ++\treset_files &&\n ++\techo content2 >other &&\n ++\ttest-tool chmtime =1234567890 other &&\n ++\tupdate_assert_changed --refresh\n ++'\n ++\n ++test_expect_success '--refresh should fix racy timestamp if racy file needs update' '\n ++\treset_files &&\n +\techo content2 >file &&\n -+\treset_mtime file &&\n -+\treset_mtime .git/index &&\n ++\ttest-tool chmtime =1234567890 file &&\n +\tupdate_assert_changed --refresh\n +'\n +\n\n-- \ngitgitgadget\n"},{"id":"445517","messageId":"7d58f80611193f8696d99e317fe6b1e53ac740f7.1641388523.git.gitgitgadget@gmail.com","threadId":"57139","inReplyTo":"pull.1105.v2.git.1641388523.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] t7508: add tests capturing racy timestamp handling","fromName":"Marc Strapetz via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-01-05T13:15:22Z","receivedAt":"2022-01-05T13:15:32Z","isPatch":true,"sender":{"key":"marc.strapetz@syntevo.com","avatar":"https://avatars.githubusercontent.com/u/3380730?v=4"},"body":"From: Marc Strapetz \n\n\"git status\" fixes racy timestamps regardless of the worktree being\ndirty or not. The new test cases capture this behavior.\n\nSigned-off-by: Marc Strapetz \n---\n t/t7508-status.sh | 28 ++++++++++++++++++++++++++++\n 1 file changed, 28 insertions(+)\n\ndiff --git a/t/t7508-status.sh b/t/t7508-status.sh\nindex 05c6c02435d..652cbb5ed2e 100755\n--- a/t/t7508-status.sh\n+++ b/t/t7508-status.sh\n@@ -1656,4 +1656,32 @@ test_expect_success '--no-optional-locks prevents index update' '\n \t! grep ^1234567890 out\n '\n \n+test_expect_success 'racy timestamps will be fixed for clean worktree' '\n+\techo content >racy-dirty &&\n+\techo content >racy-racy &&\n+\tgit add racy* &&\n+\tgit commit -m \"racy test files\" &&\n+\t# let status rewrite the index, if necessary; after that we expect\n+\t# no more index writes unless caused by racy timestamps; note that\n+\t# timestamps may already be racy now (depending on previous tests)\n+\tgit status &&\n+\ttest-tool chmtime =1234567890 .git/index &&\n+\ttest-tool chmtime --get .git/index >out &&\n+\tgrep ^1234567890 out &&\n+\tgit status &&\n+\ttest-tool chmtime --get .git/index >out &&\n+\t! grep ^1234567890 out\n+'\n+\n+test_expect_success 'racy timestamps will be fixed for dirty worktree' '\n+\techo content2 >racy-dirty &&\n+\tgit status &&\n+\ttest-tool chmtime =1234567890 .git/index &&\n+\ttest-tool chmtime --get .git/index >out &&\n+\tgrep ^1234567890 out &&\n+\tgit status &&\n+\ttest-tool chmtime --get .git/index >out &&\n+\t! grep ^1234567890 out\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"445518","messageId":"dfeabf6af15dfab06bde1ec7dcc2d0576497be9b.1641388523.git.gitgitgadget@gmail.com","threadId":"57139","inReplyTo":"pull.1105.v2.git.1641388523.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] update-index: refresh should rewrite index in case of racy timestamps","fromName":"Marc Strapetz via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-01-05T13:15:23Z","receivedAt":"2022-01-05T13:15:35Z","isPatch":true,"sender":{"key":"marc.strapetz@syntevo.com","avatar":"https://avatars.githubusercontent.com/u/3380730?v=4"},"body":"From: Marc Strapetz \n\n'git update-index --refresh' and '--really-refresh' should force writing\nof the index file if racy timestamps have been encountered, as\n'git status' already does [1].\n\nNote that calling 'git update-index --refresh' still does not guarantee\nthat there will be no more racy timestamps afterwards (the same holds\ntrue for 'git status'):\n\n- calling 'git update-index --refresh' immediately after touching and\n adding a file may still leave racy timestamps if all three operations\n occur within the racy-tolerance (usually 1 second unless USE_NSEC has\n been defined)\n\n- calling 'git update-index --refresh' for timestamps which are set into\n the future will leave them racy\n\nTo guarantee that such racy timestamps will be resolved would require to\nwait until the system clock has passed beyond these timestamps and only\nthen write the index file. Especially for future timestamps, this does\nnot seem feasible because of possibly long delays/hangs.\n\n[1] https://lore.kernel.org/git/d3dd805c-7c1d-30a9-6574-a7bfcb7fc013@syntevo.com/\n\nSigned-off-by: Marc Strapetz \n---\n builtin/update-index.c | 11 +++++\n cache.h | 1 +\n read-cache.c | 2 +-\n t/t2108-update-index-refresh-racy.sh | 64 ++++++++++++++++++++++++++++\n 4 files changed, 77 insertions(+), 1 deletion(-)\n create mode 100755 t/t2108-update-index-refresh-racy.sh\n\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex 187203e8bb5..7e0a0d9bf80 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -787,6 +787,17 @@ static int refresh(struct refresh_params *o, unsigned int flag)\n \tsetup_work_tree();\n \tread_cache();\n \t*o->has_errors |= refresh_cache(o->flags | flag);\n+\tif (has_racy_timestamp(&the_index)) {\n+\t\t/*\n+\t\t * Even if nothing else has changed, updating the file\n+\t\t * increases the chance that racy timestamps become\n+\t\t * non-racy, helping future run-time performance.\n+\t\t * We do that even in case of \"errors\" returned by\n+\t\t * refresh_cache() as these are no actual errors.\n+\t\t * cmd_status() does the same.\n+\t\t */\n+\t\tactive_cache_changed |= SOMETHING_CHANGED;\n+\t}\n \treturn 0;\n }\n \ndiff --git a/cache.h b/cache.h\nindex cfba463aa97..dd1932e2d0e 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -891,6 +891,7 @@ void *read_blob_data_from_index(struct index_state *, const char *, unsigned lon\n #define CE_MATCH_IGNORE_FSMONITOR 0X20\n int is_racy_timestamp(const struct index_state *istate,\n \t\t const struct cache_entry *ce);\n+int has_racy_timestamp(struct index_state *istate);\n int ie_match_stat(struct index_state *, const struct cache_entry *, struct stat *, unsigned int);\n int ie_modified(struct index_state *, const struct cache_entry *, struct stat *, unsigned int);\n \ndiff --git a/read-cache.c b/read-cache.c\nindex cbe73f14e5e..ed297635a33 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2775,7 +2775,7 @@ static int repo_verify_index(struct repository *repo)\n \treturn verify_index_from(repo->index, repo->index_file);\n }\n \n-static int has_racy_timestamp(struct index_state *istate)\n+int has_racy_timestamp(struct index_state *istate)\n {\n \tint entries = istate->cache_nr;\n \tint i;\ndiff --git a/t/t2108-update-index-refresh-racy.sh b/t/t2108-update-index-refresh-racy.sh\nnew file mode 100755\nindex 00000000000..171c37ebec9\n--- /dev/null\n+++ b/t/t2108-update-index-refresh-racy.sh\n@@ -0,0 +1,64 @@\n+#!/bin/sh\n+\n+test_description='update-index refresh tests related to racy timestamps'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n+. ./test-lib.sh\n+\n+reset_files () {\n+\techo content >file &&\n+\techo content >other &&\n+\ttest-tool chmtime =1234567890 file &&\n+\ttest-tool chmtime =1234567890 other\n+}\n+\n+update_assert_changed () {\n+\ttest-tool chmtime =1234567890 .git/index &&\n+\ttest_might_fail git update-index \"$1\" &&\n+\ttest-tool chmtime --get .git/index >.git/out &&\n+\t! grep ^1234567890 .git/out\n+}\n+\n+test_expect_success 'setup' '\n+\treset_files &&\n+\t# we are calling reset_files() a couple of times during tests;\n+\t# test-tool chmtime does not change the ctime; to not weaken\n+\t# or even break our tests, disable ctime-checks entirely\n+\tgit config core.trustctime false &&\n+\tgit add file other &&\n+\tgit commit -m \"initial import\"\n+'\n+\n+test_expect_success '--refresh has no racy timestamps to fix' '\n+\treset_files &&\n+\ttest-tool chmtime =1234567891 .git/index &&\n+\tgit update-index --refresh &&\n+\ttest-tool chmtime --get .git/index >.git/out &&\n+\tgrep ^1234567891 .git/out\n+'\n+\n+test_expect_success '--refresh should fix racy timestamp' '\n+\treset_files &&\n+\tupdate_assert_changed --refresh\n+'\n+\n+test_expect_success '--really-refresh should fix racy timestamp' '\n+\treset_files &&\n+\tupdate_assert_changed --really-refresh\n+'\n+\n+test_expect_success '--refresh should fix racy timestamp if other file needs update' '\n+\treset_files &&\n+\techo content2 >other &&\n+\ttest-tool chmtime =1234567890 other &&\n+\tupdate_assert_changed --refresh\n+'\n+\n+test_expect_success '--refresh should fix racy timestamp if racy file needs update' '\n+\treset_files &&\n+\techo content2 >file &&\n+\ttest-tool chmtime =1234567890 file &&\n+\tupdate_assert_changed --refresh\n+'\n+\n+test_done\n-- \ngitgitgadget\n"},{"id":"445579","messageId":"xmqqczl5hpaq.fsf@gitster.g","threadId":"57139","inReplyTo":"7d58f80611193f8696d99e317fe6b1e53ac740f7.1641388523.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/2] t7508: add tests capturing racy timestamp handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-05T20:59:25Z","receivedAt":"2022-01-05T20:59:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Marc Strapetz via GitGitGadget\" writes:\n\n> From: Marc Strapetz \n>\n> \"git status\" fixes racy timestamps regardless of the worktree being\n> dirty or not. The new test cases capture this behavior.\n>\n> Signed-off-by: Marc Strapetz \n> ---\n> t/t7508-status.sh | 28 ++++++++++++++++++++++++++++\n> 1 file changed, 28 insertions(+)\n>\n> diff --git a/t/t7508-status.sh b/t/t7508-status.sh\n> index 05c6c02435d..652cbb5ed2e 100755\n> --- a/t/t7508-status.sh\n> +++ b/t/t7508-status.sh\n> @@ -1656,4 +1656,32 @@ test_expect_success '--no-optional-locks prevents index update' '\n> \t! grep ^1234567890 out\n> '\n> \n> +test_expect_success 'racy timestamps will be fixed for clean worktree' '\n> +\techo content >racy-dirty &&\n> +\techo content >racy-racy &&\n> +\tgit add racy* &&\n> +\tgit commit -m \"racy test files\" &&\n> +\t# let status rewrite the index, if necessary; after that we expect\n> +\t# no more index writes unless caused by racy timestamps; note that\n> +\t# timestamps may already be racy now (depending on previous tests)\n> +\tgit status &&\n> +\ttest-tool chmtime =1234567890 .git/index &&\n> +\ttest-tool chmtime --get .git/index >out &&\n> +\tgrep ^1234567890 out &&\n\nIf file contents were 1234567890999, this will still hit, but I do\nnot think that is what you wanted to see. Perhaps\n\n\tgit status &&\n\techo 1234567890 >expect &&\n\ttest-tool chmtime=$(cat expect) .git/index &&\n\ttest-tool chmtime --get .git/index >actual &&\n\ttest_cmp expect actual\n\nor something? But I think you inherited this bogosity from the\nprevious test, so I am OK to add a few more copies of the same\nbogosity to the test.\n\nSomebody later has to step in and clean them all up, though. When\nthat happens, we should document how the magic 1234567890 timestamp\nwas chosen near its first use.\n\nI think it is because it is a timestamp in year 2009, so as long as\nyour filetime clock is reasonably accurate, a write to the file\nwould never get such a low timestamp.\n\n> +\tgit status &&\n> +\ttest-tool chmtime --get .git/index >out &&\n> +\t! grep ^1234567890 out\n\n"},{"id":"445580","messageId":"xmqq7dbdhp36.fsf@gitster.g","threadId":"57139","inReplyTo":"dfeabf6af15dfab06bde1ec7dcc2d0576497be9b.1641388523.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/2] update-index: refresh should rewrite index in case of racy timestamps","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-05T21:03:57Z","receivedAt":"2022-01-05T21:04:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Marc Strapetz via GitGitGadget\" writes:\n\n> +test_expect_success '--refresh has no racy timestamps to fix' '\n> +\treset_files &&\n> +\ttest-tool chmtime =1234567891 .git/index &&\n\nDon't some people use Git on VFAT where the time resolution is 2\nseconds? 1234567890 and 1234567891 differ only by one second, so\nchmtime may not be able to store one of them exactly, so I am not\nsure if this guarantees \"no racy timestamps to fix\".\n\n"},{"id":"445621","messageId":"54fc04b3-1b6d-c8c9-f3cc-8c8bd647f187@syntevo.com","threadId":"57139","inReplyTo":"xmqqczl5hpaq.fsf@gitster.g","subject":"Re: [PATCH v2 1/2] t7508: add tests capturing racy timestamp handling","fromName":"Marc Strapetz","fromEmail":"marc.strapetz@syntevo.com","sentAt":"2022-01-06T10:21:45Z","receivedAt":"2022-01-06T10:21:51Z","isPatch":true,"sender":{"key":"marc.strapetz@syntevo.com","avatar":"https://avatars.githubusercontent.com/u/3380730?v=4"},"body":"On 05/01/2022 21:59, Junio C Hamano wrote:\n>> From: Marc Strapetz \n>> \n>> +test_expect_success 'racy timestamps will be fixed for clean worktree' '\n>> +\techo content >racy-dirty &&\n>> +\techo content >racy-racy &&\n>> +\tgit add racy* &&\n>> +\tgit commit -m \"racy test files\" &&\n>> +\t# let status rewrite the index, if necessary; after that we expect\n>> +\t# no more index writes unless caused by racy timestamps; note that\n>> +\t# timestamps may already be racy now (depending on previous tests)\n>> +\tgit status &&\n>> +\ttest-tool chmtime =1234567890 .git/index &&\n>> +\ttest-tool chmtime --get .git/index >out &&\n>> +\tgrep ^1234567890 out &&\n> \n> If file contents were 1234567890999, this will still hit, but I do\n> not think that is what you wanted to see. Perhaps\n> \n> \tgit status &&\n> \techo 1234567890 >expect &&\n> \ttest-tool chmtime=$(cat expect) .git/index &&\n> \ttest-tool chmtime --get .git/index >actual &&\n> \ttest_cmp expect actual\n> \n> or something? But I think you inherited this bogosity from the\n> previous test, so I am OK to add a few more copies of the same\n> bogosity to the test.\n> \n> Somebody later has to step in and clean them all up, though. When\n> that happens, we should document how the magic 1234567890 timestamp\n> was chosen near its first use.\n\nIt seems like this pattern was used only once before my changes, hence I \nwill extract to test-lib-functions.sh and fix the bogosity for the next \nversion of my patch.\n\n-Marc\n"},{"id":"445664","messageId":"pull.1105.v3.git.1641508499.gitgitgadget@gmail.com","threadId":"57139","inReplyTo":"pull.1105.v2.git.1641388523.gitgitgadget@gmail.com","subject":"[PATCH v3 0/4] update-index: refresh should rewrite index in case of racy timestamps","fromName":"Marc Strapetz via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-01-06T22:34:54Z","receivedAt":"2022-01-06T22:35:03Z","isPatch":true,"sender":{"key":"marc.strapetz@syntevo.com","avatar":"https://avatars.githubusercontent.com/u/3380730?v=4"},"body":"This patch makes update-index --refresh write the index if it contains racy\ntimestamps, as discussed at [1].\n\nChanges since v2:\n\n * new patch: test-lib: introduce API for verifying file mtime\n * new patch: t7508: fix bogus mtime verification for test\n \"--no-optional-locks prevents index update\"\n * change new tests in t2108 and t7508 to use new test-lib mtime API\n * fix \"--refresh has no racy timestamps to fix\" to use +60s mtime to be\n save on VFAT\n\nChanges since v1:\n\n * main commit message now uses 'git update-index' and the paragraph was\n dropped\n * t/t7508-status.sh: two tests added which capture status racy handling\n * builtin/update-index.c: comment improved\n * t/t2108-update-index-refresh-racy.sh: major overhaul\n * one test case added\n * mtime-manipulations simplified and aligned to t7508\n * code style fixes, as discussed\n\n[1]\nhttps://lore.kernel.org/git/d3dd805c-7c1d-30a9-6574-a7bfcb7fc013@syntevo.com/\n\nMarc Strapetz (4):\n test-lib: introduce API for verifying file mtime\n t7508: fix bogus mtime verification\n t7508: add tests capturing racy timestamp handling\n update-index: refresh should rewrite index in case of racy timestamps\n\n builtin/update-index.c | 11 +++++\n cache.h | 1 +\n read-cache.c | 2 +-\n t/t2108-update-index-refresh-racy.sh | 64 ++++++++++++++++++++++++++++\n t/t7508-status.sh | 30 ++++++++++---\n t/test-lib-functions.sh | 28 ++++++++++++\n 6 files changed, 130 insertions(+), 6 deletions(-)\n create mode 100755 t/t2108-update-index-refresh-racy.sh\n\n\nbase-commit: dcc0cd074f0c639a0df20461a301af6d45bd582e\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1105%2Fmstrap%2Ffeature%2Fupdate-index-refresh-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1105/mstrap/feature/update-index-refresh-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1105\n\nRange-diff vs v2:\n\n -: ----------- > 1: e6301e9d770 test-lib: introduce API for verifying file mtime\n -: ----------- > 2: d15a23cc804 t7508: fix bogus mtime verification\n 1: 7d58f806111 ! 3: 3567ef91e7a t7508: add tests capturing racy timestamp handling\n @@ Commit message\n \n ## t/t7508-status.sh ##\n @@ t/t7508-status.sh: test_expect_success '--no-optional-locks prevents index update' '\n - \t! grep ^1234567890 out\n + \t! test_is_magic_mtime .git/index\n '\n \n +test_expect_success 'racy timestamps will be fixed for clean worktree' '\n @@ t/t7508-status.sh: test_expect_success '--no-optional-locks prevents index updat\n +\t# no more index writes unless caused by racy timestamps; note that\n +\t# timestamps may already be racy now (depending on previous tests)\n +\tgit status &&\n -+\ttest-tool chmtime =1234567890 .git/index &&\n -+\ttest-tool chmtime --get .git/index >out &&\n -+\tgrep ^1234567890 out &&\n ++\ttest_set_magic_mtime .git/index &&\n +\tgit status &&\n -+\ttest-tool chmtime --get .git/index >out &&\n -+\t! grep ^1234567890 out\n ++\t! test_is_magic_mtime .git/index\n +'\n +\n +test_expect_success 'racy timestamps will be fixed for dirty worktree' '\n +\techo content2 >racy-dirty &&\n +\tgit status &&\n -+\ttest-tool chmtime =1234567890 .git/index &&\n -+\ttest-tool chmtime --get .git/index >out &&\n -+\tgrep ^1234567890 out &&\n ++\ttest_set_magic_mtime .git/index &&\n +\tgit status &&\n -+\ttest-tool chmtime --get .git/index >out &&\n -+\t! grep ^1234567890 out\n ++\t! test_is_magic_mtime .git/index\n +'\n +\n test_done\n 2: dfeabf6af15 ! 4: 4a6b18fb304 update-index: refresh should rewrite index in case of racy timestamps\n @@ t/t2108-update-index-refresh-racy.sh (new)\n +reset_files () {\n +\techo content >file &&\n +\techo content >other &&\n -+\ttest-tool chmtime =1234567890 file &&\n -+\ttest-tool chmtime =1234567890 other\n ++\ttest_set_magic_mtime file &&\n ++\ttest_set_magic_mtime other\n +}\n +\n +update_assert_changed () {\n -+\ttest-tool chmtime =1234567890 .git/index &&\n ++\ttest_set_magic_mtime .git/index &&\n +\ttest_might_fail git update-index \"$1\" &&\n -+\ttest-tool chmtime --get .git/index >.git/out &&\n -+\t! grep ^1234567890 .git/out\n ++\t! test_is_magic_mtime .git/index\n +}\n +\n +test_expect_success 'setup' '\n @@ t/t2108-update-index-refresh-racy.sh (new)\n +\n +test_expect_success '--refresh has no racy timestamps to fix' '\n +\treset_files &&\n -+\ttest-tool chmtime =1234567891 .git/index &&\n ++\t# set the index time far enough to the future;\n ++\t# it must be at least 3 seconds for VFAT\n ++\ttest_set_magic_mtime .git/index +60 &&\n +\tgit update-index --refresh &&\n -+\ttest-tool chmtime --get .git/index >.git/out &&\n -+\tgrep ^1234567891 .git/out\n ++\ttest_is_magic_mtime .git/index +60\n +'\n +\n +test_expect_success '--refresh should fix racy timestamp' '\n @@ t/t2108-update-index-refresh-racy.sh (new)\n +test_expect_success '--refresh should fix racy timestamp if other file needs update' '\n +\treset_files &&\n +\techo content2 >other &&\n -+\ttest-tool chmtime =1234567890 other &&\n ++\ttest_set_magic_mtime other &&\n +\tupdate_assert_changed --refresh\n +'\n +\n +test_expect_success '--refresh should fix racy timestamp if racy file needs update' '\n +\treset_files &&\n +\techo content2 >file &&\n -+\ttest-tool chmtime =1234567890 file &&\n ++\ttest_set_magic_mtime file &&\n +\tupdate_assert_changed --refresh\n +'\n +\n\n-- \ngitgitgadget\n"},{"id":"445665","messageId":"e6301e9d770bc7b6a2a3eeddcaf4e0123a0b23ab.1641508499.git.gitgitgadget@gmail.com","threadId":"57139","inReplyTo":"pull.1105.v3.git.1641508499.gitgitgadget@gmail.com","subject":"[PATCH v3 1/4] test-lib: introduce API for verifying file mtime","fromName":"Marc Strapetz via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-01-06T22:34:55Z","receivedAt":"2022-01-06T22:35:05Z","isPatch":true,"sender":{"key":"marc.strapetz@syntevo.com","avatar":"https://avatars.githubusercontent.com/u/3380730?v=4"},"body":"From: Marc Strapetz \n\nAdd functions `test_set_magic_mtime` and `test_is_magic_mtime` which can\nbe used to (re)set the mtime of a file to a predefined (\"magic\")\ntimestamp, then perform some operations and finally check for mtime\nchanges of the file.\n\nThe core implementation follows the suggestion from the\nmailing list [1].\n\n[1] https://lore.kernel.org/git/xmqqczl5hpaq.fsf@gitster.g/T/#u\n\nSigned-off-by: Marc Strapetz \n---\n t/test-lib-functions.sh | 28 ++++++++++++++++++++++++++++\n 1 file changed, 28 insertions(+)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 389153e5916..01dd8c01f59 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1806,3 +1806,31 @@ test_region () {\n test_readlink () {\n \tperl -le 'print readlink($_) for @ARGV' \"$@\"\n }\n+\n+# Set a fixed \"magic\" mtime to the given file,\n+# with an optional increment specified as second argument.\n+# Use in combination with test_is_magic_mtime.\n+test_set_magic_mtime () {\n+\t# We are using 1234567890 because it's a common timestamp used in\n+\t# various tests. It represents date 2009-02-13 which should be safe\n+\t# to use as long as the filetime clock is reasonably accurate.\n+\tlocal inc=${2:-0} &&\n+\tlocal mtime=$((1234567890 + $inc)) &&\n+\ttest-tool chmtime =$mtime $1 &&\n+\ttest_is_magic_mtime $1 $inc\n+}\n+\n+# Test whether the given file has the \"magic\" mtime set,\n+# with an optional increment specified as second argument.\n+# Use in combination with test_set_magic_mtime.\n+test_is_magic_mtime () {\n+\tlocal inc=${2:-0} &&\n+\tlocal mtime=$((1234567890 + $inc)) &&\n+\techo $mtime >.git/test-mtime-expect &&\n+\ttest-tool chmtime --get $1 >.git/test-mtime-actual &&\n+\ttest_cmp .git/test-mtime-expect .git/test-mtime-actual\n+\tlocal ret=$?\n+\trm .git/test-mtime-expect\n+\trm .git/test-mtime-actual\n+\treturn $ret\n+}\n-- \ngitgitgadget\n\n"},{"id":"445666","messageId":"d15a23cc8049b1f2f67b089d9edea0ce098065b3.1641508499.git.gitgitgadget@gmail.com","threadId":"57139","inReplyTo":"pull.1105.v3.git.1641508499.gitgitgadget@gmail.com","subject":"[PATCH v3 2/4] t7508: fix bogus mtime verification","fromName":"Marc Strapetz via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-01-06T22:34:56Z","receivedAt":"2022-01-06T22:35:06Z","isPatch":true,"sender":{"key":"marc.strapetz@syntevo.com","avatar":"https://avatars.githubusercontent.com/u/3380730?v=4"},"body":"From: Marc Strapetz \n\nThe current `grep`-approach in \"--no-optional-locks prevents index\nupdate\" may fail e.g. for `out` file contents \"1234567890999\" [1].\nFix this by using test-lib's new mtime-verification API.\n\n[1] https://lore.kernel.org/git/xmqqczl5hpaq.fsf@gitster.g/T/#u\n\nSigned-off-by: Marc Strapetz \n---\n t/t7508-status.sh | 8 +++-----\n 1 file changed, 3 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t7508-status.sh b/t/t7508-status.sh\nindex 05c6c02435d..b9efd2613d0 100755\n--- a/t/t7508-status.sh\n+++ b/t/t7508-status.sh\n@@ -1647,13 +1647,11 @@ test_expect_success '\"Initial commit\" should not be noted in commit template' '\n '\n \n test_expect_success '--no-optional-locks prevents index update' '\n-\ttest-tool chmtime =1234567890 .git/index &&\n+\ttest_set_magic_mtime .git/index &&\n \tgit --no-optional-locks status &&\n-\ttest-tool chmtime --get .git/index >out &&\n-\tgrep ^1234567890 out &&\n+\ttest_is_magic_mtime .git/index &&\n \tgit status &&\n-\ttest-tool chmtime --get .git/index >out &&\n-\t! grep ^1234567890 out\n+\t! test_is_magic_mtime .git/index\n '\n \n test_done\n-- \ngitgitgadget\n\n"},{"id":"445667","messageId":"3567ef91e7a91f4b0fc04884f0748c1f7a918a58.1641508499.git.gitgitgadget@gmail.com","threadId":"57139","inReplyTo":"pull.1105.v3.git.1641508499.gitgitgadget@gmail.com","subject":"[PATCH v3 3/4] t7508: add tests capturing racy timestamp handling","fromName":"Marc Strapetz via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-01-06T22:34:57Z","receivedAt":"2022-01-06T22:35:07Z","isPatch":true,"sender":{"key":"marc.strapetz@syntevo.com","avatar":"https://avatars.githubusercontent.com/u/3380730?v=4"},"body":"From: Marc Strapetz \n\n\"git status\" fixes racy timestamps regardless of the worktree being\ndirty or not. The new test cases capture this behavior.\n\nSigned-off-by: Marc Strapetz \n---\n t/t7508-status.sh | 22 ++++++++++++++++++++++\n 1 file changed, 22 insertions(+)\n\ndiff --git a/t/t7508-status.sh b/t/t7508-status.sh\nindex b9efd2613d0..2b7ef6c41a4 100755\n--- a/t/t7508-status.sh\n+++ b/t/t7508-status.sh\n@@ -1654,4 +1654,26 @@ test_expect_success '--no-optional-locks prevents index update' '\n \t! test_is_magic_mtime .git/index\n '\n \n+test_expect_success 'racy timestamps will be fixed for clean worktree' '\n+\techo content >racy-dirty &&\n+\techo content >racy-racy &&\n+\tgit add racy* &&\n+\tgit commit -m \"racy test files\" &&\n+\t# let status rewrite the index, if necessary; after that we expect\n+\t# no more index writes unless caused by racy timestamps; note that\n+\t# timestamps may already be racy now (depending on previous tests)\n+\tgit status &&\n+\ttest_set_magic_mtime .git/index &&\n+\tgit status &&\n+\t! test_is_magic_mtime .git/index\n+'\n+\n+test_expect_success 'racy timestamps will be fixed for dirty worktree' '\n+\techo content2 >racy-dirty &&\n+\tgit status &&\n+\ttest_set_magic_mtime .git/index &&\n+\tgit status &&\n+\t! test_is_magic_mtime .git/index\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"445668","messageId":"4a6b18fb304cc3adda5cf219be11c29fd953e974.1641508499.git.gitgitgadget@gmail.com","threadId":"57139","inReplyTo":"pull.1105.v3.git.1641508499.gitgitgadget@gmail.com","subject":"[PATCH v3 4/4] update-index: refresh should rewrite index in case of racy timestamps","fromName":"Marc Strapetz via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-01-06T22:34:58Z","receivedAt":"2022-01-06T22:35:08Z","isPatch":true,"sender":{"key":"marc.strapetz@syntevo.com","avatar":"https://avatars.githubusercontent.com/u/3380730?v=4"},"body":"From: Marc Strapetz \n\n'git update-index --refresh' and '--really-refresh' should force writing\nof the index file if racy timestamps have been encountered, as\n'git status' already does [1].\n\nNote that calling 'git update-index --refresh' still does not guarantee\nthat there will be no more racy timestamps afterwards (the same holds\ntrue for 'git status'):\n\n- calling 'git update-index --refresh' immediately after touching and\n adding a file may still leave racy timestamps if all three operations\n occur within the racy-tolerance (usually 1 second unless USE_NSEC has\n been defined)\n\n- calling 'git update-index --refresh' for timestamps which are set into\n the future will leave them racy\n\nTo guarantee that such racy timestamps will be resolved would require to\nwait until the system clock has passed beyond these timestamps and only\nthen write the index file. Especially for future timestamps, this does\nnot seem feasible because of possibly long delays/hangs.\n\n[1] https://lore.kernel.org/git/d3dd805c-7c1d-30a9-6574-a7bfcb7fc013@syntevo.com/\n\nSigned-off-by: Marc Strapetz \n---\n builtin/update-index.c | 11 +++++\n cache.h | 1 +\n read-cache.c | 2 +-\n t/t2108-update-index-refresh-racy.sh | 64 ++++++++++++++++++++++++++++\n 4 files changed, 77 insertions(+), 1 deletion(-)\n create mode 100755 t/t2108-update-index-refresh-racy.sh\n\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex 187203e8bb5..7e0a0d9bf80 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -787,6 +787,17 @@ static int refresh(struct refresh_params *o, unsigned int flag)\n \tsetup_work_tree();\n \tread_cache();\n \t*o->has_errors |= refresh_cache(o->flags | flag);\n+\tif (has_racy_timestamp(&the_index)) {\n+\t\t/*\n+\t\t * Even if nothing else has changed, updating the file\n+\t\t * increases the chance that racy timestamps become\n+\t\t * non-racy, helping future run-time performance.\n+\t\t * We do that even in case of \"errors\" returned by\n+\t\t * refresh_cache() as these are no actual errors.\n+\t\t * cmd_status() does the same.\n+\t\t */\n+\t\tactive_cache_changed |= SOMETHING_CHANGED;\n+\t}\n \treturn 0;\n }\n \ndiff --git a/cache.h b/cache.h\nindex cfba463aa97..dd1932e2d0e 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -891,6 +891,7 @@ void *read_blob_data_from_index(struct index_state *, const char *, unsigned lon\n #define CE_MATCH_IGNORE_FSMONITOR 0X20\n int is_racy_timestamp(const struct index_state *istate,\n \t\t const struct cache_entry *ce);\n+int has_racy_timestamp(struct index_state *istate);\n int ie_match_stat(struct index_state *, const struct cache_entry *, struct stat *, unsigned int);\n int ie_modified(struct index_state *, const struct cache_entry *, struct stat *, unsigned int);\n \ndiff --git a/read-cache.c b/read-cache.c\nindex cbe73f14e5e..ed297635a33 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2775,7 +2775,7 @@ static int repo_verify_index(struct repository *repo)\n \treturn verify_index_from(repo->index, repo->index_file);\n }\n \n-static int has_racy_timestamp(struct index_state *istate)\n+int has_racy_timestamp(struct index_state *istate)\n {\n \tint entries = istate->cache_nr;\n \tint i;\ndiff --git a/t/t2108-update-index-refresh-racy.sh b/t/t2108-update-index-refresh-racy.sh\nnew file mode 100755\nindex 00000000000..bc5f2886faf\n--- /dev/null\n+++ b/t/t2108-update-index-refresh-racy.sh\n@@ -0,0 +1,64 @@\n+#!/bin/sh\n+\n+test_description='update-index refresh tests related to racy timestamps'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n+. ./test-lib.sh\n+\n+reset_files () {\n+\techo content >file &&\n+\techo content >other &&\n+\ttest_set_magic_mtime file &&\n+\ttest_set_magic_mtime other\n+}\n+\n+update_assert_changed () {\n+\ttest_set_magic_mtime .git/index &&\n+\ttest_might_fail git update-index \"$1\" &&\n+\t! test_is_magic_mtime .git/index\n+}\n+\n+test_expect_success 'setup' '\n+\treset_files &&\n+\t# we are calling reset_files() a couple of times during tests;\n+\t# test-tool chmtime does not change the ctime; to not weaken\n+\t# or even break our tests, disable ctime-checks entirely\n+\tgit config core.trustctime false &&\n+\tgit add file other &&\n+\tgit commit -m \"initial import\"\n+'\n+\n+test_expect_success '--refresh has no racy timestamps to fix' '\n+\treset_files &&\n+\t# set the index time far enough to the future;\n+\t# it must be at least 3 seconds for VFAT\n+\ttest_set_magic_mtime .git/index +60 &&\n+\tgit update-index --refresh &&\n+\ttest_is_magic_mtime .git/index +60\n+'\n+\n+test_expect_success '--refresh should fix racy timestamp' '\n+\treset_files &&\n+\tupdate_assert_changed --refresh\n+'\n+\n+test_expect_success '--really-refresh should fix racy timestamp' '\n+\treset_files &&\n+\tupdate_assert_changed --really-refresh\n+'\n+\n+test_expect_success '--refresh should fix racy timestamp if other file needs update' '\n+\treset_files &&\n+\techo content2 >other &&\n+\ttest_set_magic_mtime other &&\n+\tupdate_assert_changed --refresh\n+'\n+\n+test_expect_success '--refresh should fix racy timestamp if racy file needs update' '\n+\treset_files &&\n+\techo content2 >file &&\n+\ttest_set_magic_mtime file &&\n+\tupdate_assert_changed --refresh\n+'\n+\n+test_done\n-- \ngitgitgadget\n"},{"id":"445681","messageId":"xmqqmtk8a083.fsf@gitster.g","threadId":"57139","inReplyTo":"e6301e9d770bc7b6a2a3eeddcaf4e0123a0b23ab.1641508499.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 1/4] test-lib: introduce API for verifying file mtime","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-06T23:55:08Z","receivedAt":"2022-01-06T23:55:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Marc Strapetz via GitGitGadget\" writes:\n\n> +# Set a fixed \"magic\" mtime to the given file,\n> +# with an optional increment specified as second argument.\n> +# Use in combination with test_is_magic_mtime.\n> +test_set_magic_mtime () {\n> +\t# We are using 1234567890 because it's a common timestamp used in\n> +\t# various tests. It represents date 2009-02-13 which should be safe\n> +\t# to use as long as the filetime clock is reasonably accurate.\n\nIn the original context of \"setting an ancient time, and detect\nfilesystem modification by noticing that the timestamp has or has\nnot changed\", such an ancient timestamp \"should be safe to use\", but\nif you expose it to more general audience, the context of their use\nmust be in line with your intended use to be safe.\n\n\t# Set mtime to mid February 2009, before we run an operation\n\t# that may or may not touch the file. If the file was\n\t# touched, its timestamp will not accidentally have such an\n\t# old timestamp, as long as your filesystem clock is\n\t# reasonably correct.\n\nperhaps?\n\n> +\tlocal inc=${2:-0} &&\n> +\tlocal mtime=$((1234567890 + $inc)) &&\n> +\ttest-tool chmtime =$mtime $1 &&\n> +\ttest_is_magic_mtime $1 $inc\n> +}\n\nAlso as a helper function in the library that is (hopefully) useful\nto many other callers, make sure you got your quoting correct.\n\nThere is no rule that you must use filenames without SP in it in\nyour tests, for example, so make sure \"$1\" above are quoted. The\nsame applies to the next function.\n\n> +# Test whether the given file has the \"magic\" mtime set,\n> +# with an optional increment specified as second argument.\n> +# Use in combination with test_set_magic_mtime.\n> +test_is_magic_mtime () {\n> +\tlocal inc=${2:-0} &&\n> +\tlocal mtime=$((1234567890 + $inc)) &&\n> +\techo $mtime >.git/test-mtime-expect &&\n> +\ttest-tool chmtime --get $1 >.git/test-mtime-actual &&\n> +\ttest_cmp .git/test-mtime-expect .git/test-mtime-actual\n> +\tlocal ret=$?\n> +\trm .git/test-mtime-expect\n> +\trm .git/test-mtime-actual\n\nUse \"rm -f\" here? Otherwise, if the main test failed somewhere\nbefore it runs test_cmp, we'd see an error from an attempt to remove\na file that does not exist.\n\n> +\treturn $ret\n> +}\n\nOther than that, quite nicely done (both these two functions and its\nusers).\n\nThanks.\n"},{"id":"445699","messageId":"pull.1105.v4.git.1641554252.gitgitgadget@gmail.com","threadId":"57139","inReplyTo":"pull.1105.v3.git.1641508499.gitgitgadget@gmail.com","subject":"[PATCH v4 0/4] update-index: refresh should rewrite index in case of racy timestamps","fromName":"Marc Strapetz via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-01-07T11:17:27Z","receivedAt":"2022-01-07T11:17:37Z","isPatch":true,"sender":{"key":"marc.strapetz@syntevo.com","avatar":"https://avatars.githubusercontent.com/u/3380730?v=4"},"body":"This patch makes update-index --refresh write the index if it contains racy\ntimestamps, as discussed at [1].\n\nChanges since v3:\n\n * test-lib: improve API for verifying file mtime\n * fix quoting around \"$1\"\n * use \"rm -f\" for cleanup of auxiliary files\n * improve API description comments\n * Note that gitgitgadget's \"freebsd_12\" check is failing since a couple of\n days (unrelated to this pull request); hence, this check hasn't been\n applied to this patch series\n\nChanges since v2:\n\n * new patch: test-lib: introduce API for verifying file mtime\n * new patch: t7508: fix bogus mtime verification for test\n \"--no-optional-locks prevents index update\"\n * change new tests in t2108 and t7508 to use new test-lib mtime API\n * fix \"--refresh has no racy timestamps to fix\" to use +60s mtime to be\n save on VFAT\n\nChanges since v1:\n\n * main commit message now uses 'git update-index' and the paragraph was\n dropped\n * t/t7508-status.sh: two tests added which capture status racy handling\n * builtin/update-index.c: comment improved\n * t/t2108-update-index-refresh-racy.sh: major overhaul\n * one test case added\n * mtime-manipulations simplified and aligned to t7508\n * code style fixes, as discussed\n\n[1]\nhttps://lore.kernel.org/git/d3dd805c-7c1d-30a9-6574-a7bfcb7fc013@syntevo.com/\n\nMarc Strapetz (4):\n test-lib: introduce API for verifying file mtime\n t7508: fix bogus mtime verification\n t7508: add tests capturing racy timestamp handling\n update-index: refresh should rewrite index in case of racy timestamps\n\n builtin/update-index.c | 11 +++++\n cache.h | 1 +\n read-cache.c | 2 +-\n t/t2108-update-index-refresh-racy.sh | 64 ++++++++++++++++++++++++++++\n t/t7508-status.sh | 30 ++++++++++---\n t/test-lib-functions.sh | 33 ++++++++++++++\n 6 files changed, 135 insertions(+), 6 deletions(-)\n create mode 100755 t/t2108-update-index-refresh-racy.sh\n\n\nbase-commit: dcc0cd074f0c639a0df20461a301af6d45bd582e\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1105%2Fmstrap%2Ffeature%2Fupdate-index-refresh-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1105/mstrap/feature/update-index-refresh-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/1105\n\nRange-diff vs v3:\n\n 1: e6301e9d770 ! 1: 37c11bfafc4 test-lib: introduce API for verifying file mtime\n @@ t/test-lib-functions.sh: test_region () {\n \tperl -le 'print readlink($_) for @ARGV' \"$@\"\n }\n +\n -+# Set a fixed \"magic\" mtime to the given file,\n -+# with an optional increment specified as second argument.\n -+# Use in combination with test_is_magic_mtime.\n ++# Set mtime to a fixed \"magic\" timestamp in mid February 2009, before we\n ++# run an operation that may or may not touch the file. If the file was\n ++# touched, its timestamp will not accidentally have such an old timestamp,\n ++# as long as your filesystem clock is reasonably correct. To verify the\n ++# timestamp, follow up with test_is_magic_mtime.\n ++#\n ++# An optional increment to the magic timestamp may be specified as second\n ++# argument.\n +test_set_magic_mtime () {\n -+\t# We are using 1234567890 because it's a common timestamp used in\n -+\t# various tests. It represents date 2009-02-13 which should be safe\n -+\t# to use as long as the filetime clock is reasonably accurate.\n +\tlocal inc=${2:-0} &&\n +\tlocal mtime=$((1234567890 + $inc)) &&\n -+\ttest-tool chmtime =$mtime $1 &&\n -+\ttest_is_magic_mtime $1 $inc\n ++\ttest-tool chmtime =$mtime \"$1\" &&\n ++\ttest_is_magic_mtime \"$1\" $inc\n +}\n +\n -+# Test whether the given file has the \"magic\" mtime set,\n -+# with an optional increment specified as second argument.\n -+# Use in combination with test_set_magic_mtime.\n ++# Test whether the given file has the \"magic\" mtime set. This is meant to\n ++# be used in combination with test_set_magic_mtime.\n ++#\n ++# An optional increment to the magic timestamp may be specified as second\n ++# argument. Usually, this should be the same increment which was used for\n ++# the associated test_set_magic_mtime.\n +test_is_magic_mtime () {\n +\tlocal inc=${2:-0} &&\n +\tlocal mtime=$((1234567890 + $inc)) &&\n +\techo $mtime >.git/test-mtime-expect &&\n -+\ttest-tool chmtime --get $1 >.git/test-mtime-actual &&\n ++\ttest-tool chmtime --get \"$1\" >.git/test-mtime-actual &&\n +\ttest_cmp .git/test-mtime-expect .git/test-mtime-actual\n +\tlocal ret=$?\n -+\trm .git/test-mtime-expect\n -+\trm .git/test-mtime-actual\n ++\trm -f .git/test-mtime-expect\n ++\trm -f .git/test-mtime-actual\n +\treturn $ret\n +}\n 2: d15a23cc804 = 2: c97a41af389 t7508: fix bogus mtime verification\n 3: 3567ef91e7a = 3: 82d0b6ab8d2 t7508: add tests capturing racy timestamp handling\n 4: 4a6b18fb304 = 4: e31edb74e24 update-index: refresh should rewrite index in case of racy timestamps\n\n-- \ngitgitgadget\n"},{"id":"445700","messageId":"37c11bfafc4c5675124db81cdd61fcb3e8c91ab2.1641554252.git.gitgitgadget@gmail.com","threadId":"57139","inReplyTo":"pull.1105.v4.git.1641554252.gitgitgadget@gmail.com","subject":"[PATCH v4 1/4] test-lib: introduce API for verifying file mtime","fromName":"Marc Strapetz via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-01-07T11:17:28Z","receivedAt":"2022-01-07T11:17:38Z","isPatch":true,"sender":{"key":"marc.strapetz@syntevo.com","avatar":"https://avatars.githubusercontent.com/u/3380730?v=4"},"body":"From: Marc Strapetz \n\nAdd functions `test_set_magic_mtime` and `test_is_magic_mtime` which can\nbe used to (re)set the mtime of a file to a predefined (\"magic\")\ntimestamp, then perform some operations and finally check for mtime\nchanges of the file.\n\nThe core implementation follows the suggestion from the\nmailing list [1].\n\n[1] https://lore.kernel.org/git/xmqqczl5hpaq.fsf@gitster.g/T/#u\n\nSigned-off-by: Marc Strapetz \n---\n t/test-lib-functions.sh | 33 +++++++++++++++++++++++++++++++++\n 1 file changed, 33 insertions(+)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 389153e5916..c1afa0884bf 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1806,3 +1806,36 @@ test_region () {\n test_readlink () {\n \tperl -le 'print readlink($_) for @ARGV' \"$@\"\n }\n+\n+# Set mtime to a fixed \"magic\" timestamp in mid February 2009, before we\n+# run an operation that may or may not touch the file. If the file was\n+# touched, its timestamp will not accidentally have such an old timestamp,\n+# as long as your filesystem clock is reasonably correct. To verify the\n+# timestamp, follow up with test_is_magic_mtime.\n+#\n+# An optional increment to the magic timestamp may be specified as second\n+# argument.\n+test_set_magic_mtime () {\n+\tlocal inc=${2:-0} &&\n+\tlocal mtime=$((1234567890 + $inc)) &&\n+\ttest-tool chmtime =$mtime \"$1\" &&\n+\ttest_is_magic_mtime \"$1\" $inc\n+}\n+\n+# Test whether the given file has the \"magic\" mtime set. This is meant to\n+# be used in combination with test_set_magic_mtime.\n+#\n+# An optional increment to the magic timestamp may be specified as second\n+# argument. Usually, this should be the same increment which was used for\n+# the associated test_set_magic_mtime.\n+test_is_magic_mtime () {\n+\tlocal inc=${2:-0} &&\n+\tlocal mtime=$((1234567890 + $inc)) &&\n+\techo $mtime >.git/test-mtime-expect &&\n+\ttest-tool chmtime --get \"$1\" >.git/test-mtime-actual &&\n+\ttest_cmp .git/test-mtime-expect .git/test-mtime-actual\n+\tlocal ret=$?\n+\trm -f .git/test-mtime-expect\n+\trm -f .git/test-mtime-actual\n+\treturn $ret\n+}\n-- \ngitgitgadget\n\n"},{"id":"445701","messageId":"c97a41af38982954b384b68ed7aaf4d9a157043c.1641554252.git.gitgitgadget@gmail.com","threadId":"57139","inReplyTo":"pull.1105.v4.git.1641554252.gitgitgadget@gmail.com","subject":"[PATCH v4 2/4] t7508: fix bogus mtime verification","fromName":"Marc Strapetz via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-01-07T11:17:29Z","receivedAt":"2022-01-07T11:17:39Z","isPatch":true,"sender":{"key":"marc.strapetz@syntevo.com","avatar":"https://avatars.githubusercontent.com/u/3380730?v=4"},"body":"From: Marc Strapetz \n\nThe current `grep`-approach in \"--no-optional-locks prevents index\nupdate\" may fail e.g. for `out` file contents \"1234567890999\" [1].\nFix this by using test-lib's new mtime-verification API.\n\n[1] https://lore.kernel.org/git/xmqqczl5hpaq.fsf@gitster.g/T/#u\n\nSigned-off-by: Marc Strapetz \n---\n t/t7508-status.sh | 8 +++-----\n 1 file changed, 3 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t7508-status.sh b/t/t7508-status.sh\nindex 05c6c02435d..b9efd2613d0 100755\n--- a/t/t7508-status.sh\n+++ b/t/t7508-status.sh\n@@ -1647,13 +1647,11 @@ test_expect_success '\"Initial commit\" should not be noted in commit template' '\n '\n \n test_expect_success '--no-optional-locks prevents index update' '\n-\ttest-tool chmtime =1234567890 .git/index &&\n+\ttest_set_magic_mtime .git/index &&\n \tgit --no-optional-locks status &&\n-\ttest-tool chmtime --get .git/index >out &&\n-\tgrep ^1234567890 out &&\n+\ttest_is_magic_mtime .git/index &&\n \tgit status &&\n-\ttest-tool chmtime --get .git/index >out &&\n-\t! grep ^1234567890 out\n+\t! test_is_magic_mtime .git/index\n '\n \n test_done\n-- \ngitgitgadget\n\n"},{"id":"445702","messageId":"82d0b6ab8d2dc3198216aac9551927dfd6d91cc0.1641554252.git.gitgitgadget@gmail.com","threadId":"57139","inReplyTo":"pull.1105.v4.git.1641554252.gitgitgadget@gmail.com","subject":"[PATCH v4 3/4] t7508: add tests capturing racy timestamp handling","fromName":"Marc Strapetz via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-01-07T11:17:30Z","receivedAt":"2022-01-07T11:17:41Z","isPatch":true,"sender":{"key":"marc.strapetz@syntevo.com","avatar":"https://avatars.githubusercontent.com/u/3380730?v=4"},"body":"From: Marc Strapetz \n\n\"git status\" fixes racy timestamps regardless of the worktree being\ndirty or not. The new test cases capture this behavior.\n\nSigned-off-by: Marc Strapetz \n---\n t/t7508-status.sh | 22 ++++++++++++++++++++++\n 1 file changed, 22 insertions(+)\n\ndiff --git a/t/t7508-status.sh b/t/t7508-status.sh\nindex b9efd2613d0..2b7ef6c41a4 100755\n--- a/t/t7508-status.sh\n+++ b/t/t7508-status.sh\n@@ -1654,4 +1654,26 @@ test_expect_success '--no-optional-locks prevents index update' '\n \t! test_is_magic_mtime .git/index\n '\n \n+test_expect_success 'racy timestamps will be fixed for clean worktree' '\n+\techo content >racy-dirty &&\n+\techo content >racy-racy &&\n+\tgit add racy* &&\n+\tgit commit -m \"racy test files\" &&\n+\t# let status rewrite the index, if necessary; after that we expect\n+\t# no more index writes unless caused by racy timestamps; note that\n+\t# timestamps may already be racy now (depending on previous tests)\n+\tgit status &&\n+\ttest_set_magic_mtime .git/index &&\n+\tgit status &&\n+\t! test_is_magic_mtime .git/index\n+'\n+\n+test_expect_success 'racy timestamps will be fixed for dirty worktree' '\n+\techo content2 >racy-dirty &&\n+\tgit status &&\n+\ttest_set_magic_mtime .git/index &&\n+\tgit status &&\n+\t! test_is_magic_mtime .git/index\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"445703","messageId":"e31edb74e2433d8fa92669a400a1e66076e55a60.1641554252.git.gitgitgadget@gmail.com","threadId":"57139","inReplyTo":"pull.1105.v4.git.1641554252.gitgitgadget@gmail.com","subject":"[PATCH v4 4/4] update-index: refresh should rewrite index in case of racy timestamps","fromName":"Marc Strapetz via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-01-07T11:17:31Z","receivedAt":"2022-01-07T11:17:42Z","isPatch":true,"sender":{"key":"marc.strapetz@syntevo.com","avatar":"https://avatars.githubusercontent.com/u/3380730?v=4"},"body":"From: Marc Strapetz \n\n'git update-index --refresh' and '--really-refresh' should force writing\nof the index file if racy timestamps have been encountered, as\n'git status' already does [1].\n\nNote that calling 'git update-index --refresh' still does not guarantee\nthat there will be no more racy timestamps afterwards (the same holds\ntrue for 'git status'):\n\n- calling 'git update-index --refresh' immediately after touching and\n adding a file may still leave racy timestamps if all three operations\n occur within the racy-tolerance (usually 1 second unless USE_NSEC has\n been defined)\n\n- calling 'git update-index --refresh' for timestamps which are set into\n the future will leave them racy\n\nTo guarantee that such racy timestamps will be resolved would require to\nwait until the system clock has passed beyond these timestamps and only\nthen write the index file. Especially for future timestamps, this does\nnot seem feasible because of possibly long delays/hangs.\n\n[1] https://lore.kernel.org/git/d3dd805c-7c1d-30a9-6574-a7bfcb7fc013@syntevo.com/\n\nSigned-off-by: Marc Strapetz \n---\n builtin/update-index.c | 11 +++++\n cache.h | 1 +\n read-cache.c | 2 +-\n t/t2108-update-index-refresh-racy.sh | 64 ++++++++++++++++++++++++++++\n 4 files changed, 77 insertions(+), 1 deletion(-)\n create mode 100755 t/t2108-update-index-refresh-racy.sh\n\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex 187203e8bb5..7e0a0d9bf80 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -787,6 +787,17 @@ static int refresh(struct refresh_params *o, unsigned int flag)\n \tsetup_work_tree();\n \tread_cache();\n \t*o->has_errors |= refresh_cache(o->flags | flag);\n+\tif (has_racy_timestamp(&the_index)) {\n+\t\t/*\n+\t\t * Even if nothing else has changed, updating the file\n+\t\t * increases the chance that racy timestamps become\n+\t\t * non-racy, helping future run-time performance.\n+\t\t * We do that even in case of \"errors\" returned by\n+\t\t * refresh_cache() as these are no actual errors.\n+\t\t * cmd_status() does the same.\n+\t\t */\n+\t\tactive_cache_changed |= SOMETHING_CHANGED;\n+\t}\n \treturn 0;\n }\n \ndiff --git a/cache.h b/cache.h\nindex cfba463aa97..dd1932e2d0e 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -891,6 +891,7 @@ void *read_blob_data_from_index(struct index_state *, const char *, unsigned lon\n #define CE_MATCH_IGNORE_FSMONITOR 0X20\n int is_racy_timestamp(const struct index_state *istate,\n \t\t const struct cache_entry *ce);\n+int has_racy_timestamp(struct index_state *istate);\n int ie_match_stat(struct index_state *, const struct cache_entry *, struct stat *, unsigned int);\n int ie_modified(struct index_state *, const struct cache_entry *, struct stat *, unsigned int);\n \ndiff --git a/read-cache.c b/read-cache.c\nindex cbe73f14e5e..ed297635a33 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2775,7 +2775,7 @@ static int repo_verify_index(struct repository *repo)\n \treturn verify_index_from(repo->index, repo->index_file);\n }\n \n-static int has_racy_timestamp(struct index_state *istate)\n+int has_racy_timestamp(struct index_state *istate)\n {\n \tint entries = istate->cache_nr;\n \tint i;\ndiff --git a/t/t2108-update-index-refresh-racy.sh b/t/t2108-update-index-refresh-racy.sh\nnew file mode 100755\nindex 00000000000..bc5f2886faf\n--- /dev/null\n+++ b/t/t2108-update-index-refresh-racy.sh\n@@ -0,0 +1,64 @@\n+#!/bin/sh\n+\n+test_description='update-index refresh tests related to racy timestamps'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n+. ./test-lib.sh\n+\n+reset_files () {\n+\techo content >file &&\n+\techo content >other &&\n+\ttest_set_magic_mtime file &&\n+\ttest_set_magic_mtime other\n+}\n+\n+update_assert_changed () {\n+\ttest_set_magic_mtime .git/index &&\n+\ttest_might_fail git update-index \"$1\" &&\n+\t! test_is_magic_mtime .git/index\n+}\n+\n+test_expect_success 'setup' '\n+\treset_files &&\n+\t# we are calling reset_files() a couple of times during tests;\n+\t# test-tool chmtime does not change the ctime; to not weaken\n+\t# or even break our tests, disable ctime-checks entirely\n+\tgit config core.trustctime false &&\n+\tgit add file other &&\n+\tgit commit -m \"initial import\"\n+'\n+\n+test_expect_success '--refresh has no racy timestamps to fix' '\n+\treset_files &&\n+\t# set the index time far enough to the future;\n+\t# it must be at least 3 seconds for VFAT\n+\ttest_set_magic_mtime .git/index +60 &&\n+\tgit update-index --refresh &&\n+\ttest_is_magic_mtime .git/index +60\n+'\n+\n+test_expect_success '--refresh should fix racy timestamp' '\n+\treset_files &&\n+\tupdate_assert_changed --refresh\n+'\n+\n+test_expect_success '--really-refresh should fix racy timestamp' '\n+\treset_files &&\n+\tupdate_assert_changed --really-refresh\n+'\n+\n+test_expect_success '--refresh should fix racy timestamp if other file needs update' '\n+\treset_files &&\n+\techo content2 >other &&\n+\ttest_set_magic_mtime other &&\n+\tupdate_assert_changed --refresh\n+'\n+\n+test_expect_success '--refresh should fix racy timestamp if racy file needs update' '\n+\treset_files &&\n+\techo content2 >file &&\n+\ttest_set_magic_mtime file &&\n+\tupdate_assert_changed --refresh\n+'\n+\n+test_done\n-- \ngitgitgadget\n"}]}