From ab6245bdeed46e1ff3bd378690066abd2ed0f2ef Mon Sep 17 00:00:00 2001 From: Marc Strapetz Date: Fri, 7 Jan 2022 11:17:28 +0000 Subject: test-lib: introduce API for verifying file mtime Add functions `test_set_magic_mtime` and `test_is_magic_mtime` which can be used to (re)set the mtime of a file to a predefined ("magic") timestamp, then perform some operations and finally check for mtime changes of the file. The core implementation follows the suggestion from the mailing list [1]. [1] https://lore.kernel.org/git/xmqqczl5hpaq.fsf@gitster.g/T/#u Signed-off-by: Marc Strapetz Signed-off-by: Junio C Hamano --- t/test-lib-functions.sh | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index 389153e591..c1afa0884b 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -1806,3 +1806,36 @@ test_region () { test_readlink () { perl -le 'print readlink($_) for @ARGV' "$@" } + +# Set mtime to a fixed "magic" timestamp in mid February 2009, before we +# run an operation that may or may not touch the file. If the file was +# touched, its timestamp will not accidentally have such an old timestamp, +# as long as your filesystem clock is reasonably correct. To verify the +# timestamp, follow up with test_is_magic_mtime. +# +# An optional increment to the magic timestamp may be specified as second +# argument. +test_set_magic_mtime () { + local inc=${2:-0} && + local mtime=$((1234567890 + $inc)) && + test-tool chmtime =$mtime "$1" && + test_is_magic_mtime "$1" $inc +} + +# Test whether the given file has the "magic" mtime set. This is meant to +# be used in combination with test_set_magic_mtime. +# +# An optional increment to the magic timestamp may be specified as second +# argument. Usually, this should be the same increment which was used for +# the associated test_set_magic_mtime. +test_is_magic_mtime () { + local inc=${2:-0} && + local mtime=$((1234567890 + $inc)) && + echo $mtime >.git/test-mtime-expect && + test-tool chmtime --get "$1" >.git/test-mtime-actual && + test_cmp .git/test-mtime-expect .git/test-mtime-actual + local ret=$? + rm -f .git/test-mtime-expect + rm -f .git/test-mtime-actual + return $ret +} -- cgit v1.2.3 From 0275e4daabed330c4d27cc9a482c2d23d7544aca Mon Sep 17 00:00:00 2001 From: Marc Strapetz Date: Fri, 7 Jan 2022 11:17:29 +0000 Subject: t7508: fix bogus mtime verification The current `grep`-approach in "--no-optional-locks prevents index update" may fail e.g. for `out` file contents "1234567890999" [1]. Fix this by using test-lib's new mtime-verification API. [1] https://lore.kernel.org/git/xmqqczl5hpaq.fsf@gitster.g/T/#u Signed-off-by: Marc Strapetz Signed-off-by: Junio C Hamano --- t/t7508-status.sh | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/t/t7508-status.sh b/t/t7508-status.sh index 05c6c02435..b9efd2613d 100755 --- a/t/t7508-status.sh +++ b/t/t7508-status.sh @@ -1647,13 +1647,11 @@ test_expect_success '"Initial commit" should not be noted in commit template' ' ' test_expect_success '--no-optional-locks prevents index update' ' - test-tool chmtime =1234567890 .git/index && + test_set_magic_mtime .git/index && git --no-optional-locks status && - test-tool chmtime --get .git/index >out && - grep ^1234567890 out && + test_is_magic_mtime .git/index && git status && - test-tool chmtime --get .git/index >out && - ! grep ^1234567890 out + ! test_is_magic_mtime .git/index ' test_done -- cgit v1.2.3 From 9b71efd01489d6c93436740e4c1b3eadeb0c719b Mon Sep 17 00:00:00 2001 From: Marc Strapetz Date: Fri, 7 Jan 2022 11:17:30 +0000 Subject: t7508: add tests capturing racy timestamp handling "git status" fixes racy timestamps regardless of the worktree being dirty or not. The new test cases capture this behavior. Signed-off-by: Marc Strapetz Signed-off-by: Junio C Hamano --- t/t7508-status.sh | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/t/t7508-status.sh b/t/t7508-status.sh index b9efd2613d..2b7ef6c41a 100755 --- a/t/t7508-status.sh +++ b/t/t7508-status.sh @@ -1654,4 +1654,26 @@ test_expect_success '--no-optional-locks prevents index update' ' ! test_is_magic_mtime .git/index ' +test_expect_success 'racy timestamps will be fixed for clean worktree' ' + echo content >racy-dirty && + echo content >racy-racy && + git add racy* && + git commit -m "racy test files" && + # let status rewrite the index, if necessary; after that we expect + # no more index writes unless caused by racy timestamps; note that + # timestamps may already be racy now (depending on previous tests) + git status && + test_set_magic_mtime .git/index && + git status && + ! test_is_magic_mtime .git/index +' + +test_expect_success 'racy timestamps will be fixed for dirty worktree' ' + echo content2 >racy-dirty && + git status && + test_set_magic_mtime .git/index && + git status && + ! test_is_magic_mtime .git/index +' + test_done -- cgit v1.2.3 From 2ede073fd2287eb91af26484c2e7dafc15ecb7b7 Mon Sep 17 00:00:00 2001 From: Marc Strapetz Date: Fri, 7 Jan 2022 11:17:31 +0000 Subject: update-index: refresh should rewrite index in case of racy timestamps 'git update-index --refresh' and '--really-refresh' should force writing of the index file if racy timestamps have been encountered, as 'git status' already does [1]. Note that calling 'git update-index --refresh' still does not guarantee that there will be no more racy timestamps afterwards (the same holds true for 'git status'): - calling 'git update-index --refresh' immediately after touching and adding a file may still leave racy timestamps if all three operations occur within the racy-tolerance (usually 1 second unless USE_NSEC has been defined) - calling 'git update-index --refresh' for timestamps which are set into the future will leave them racy To guarantee that such racy timestamps will be resolved would require to wait until the system clock has passed beyond these timestamps and only then write the index file. Especially for future timestamps, this does not seem feasible because of possibly long delays/hangs. [1] https://lore.kernel.org/git/d3dd805c-7c1d-30a9-6574-a7bfcb7fc013@syntevo.com/ Signed-off-by: Marc Strapetz Signed-off-by: Junio C Hamano --- builtin/update-index.c | 11 +++++++ cache.h | 1 + read-cache.c | 2 +- t/t2108-update-index-refresh-racy.sh | 64 ++++++++++++++++++++++++++++++++++++ 4 files changed, 77 insertions(+), 1 deletion(-) create mode 100755 t/t2108-update-index-refresh-racy.sh diff --git a/builtin/update-index.c b/builtin/update-index.c index 187203e8bb..7e0a0d9bf8 100644 --- a/builtin/update-index.c +++ b/builtin/update-index.c @@ -787,6 +787,17 @@ static int refresh(struct refresh_params *o, unsigned int flag) setup_work_tree(); read_cache(); *o->has_errors |= refresh_cache(o->flags | flag); + if (has_racy_timestamp(&the_index)) { + /* + * Even if nothing else has changed, updating the file + * increases the chance that racy timestamps become + * non-racy, helping future run-time performance. + * We do that even in case of "errors" returned by + * refresh_cache() as these are no actual errors. + * cmd_status() does the same. + */ + active_cache_changed |= SOMETHING_CHANGED; + } return 0; } diff --git a/cache.h b/cache.h index cfba463aa9..dd1932e2d0 100644 --- a/cache.h +++ b/cache.h @@ -891,6 +891,7 @@ void *read_blob_data_from_index(struct index_state *, const char *, unsigned lon #define CE_MATCH_IGNORE_FSMONITOR 0X20 int is_racy_timestamp(const struct index_state *istate, const struct cache_entry *ce); +int has_racy_timestamp(struct index_state *istate); int ie_match_stat(struct index_state *, const struct cache_entry *, struct stat *, unsigned int); int ie_modified(struct index_state *, const struct cache_entry *, struct stat *, unsigned int); diff --git a/read-cache.c b/read-cache.c index cbe73f14e5..ed297635a3 100644 --- a/read-cache.c +++ b/read-cache.c @@ -2775,7 +2775,7 @@ static int repo_verify_index(struct repository *repo) return verify_index_from(repo->index, repo->index_file); } -static int has_racy_timestamp(struct index_state *istate) +int has_racy_timestamp(struct index_state *istate) { int entries = istate->cache_nr; int i; diff --git a/t/t2108-update-index-refresh-racy.sh b/t/t2108-update-index-refresh-racy.sh new file mode 100755 index 0000000000..bc5f2886fa --- /dev/null +++ b/t/t2108-update-index-refresh-racy.sh @@ -0,0 +1,64 @@ +#!/bin/sh + +test_description='update-index refresh tests related to racy timestamps' + +TEST_PASSES_SANITIZE_LEAK=true +. ./test-lib.sh + +reset_files () { + echo content >file && + echo content >other && + test_set_magic_mtime file && + test_set_magic_mtime other +} + +update_assert_changed () { + test_set_magic_mtime .git/index && + test_might_fail git update-index "$1" && + ! test_is_magic_mtime .git/index +} + +test_expect_success 'setup' ' + reset_files && + # we are calling reset_files() a couple of times during tests; + # test-tool chmtime does not change the ctime; to not weaken + # or even break our tests, disable ctime-checks entirely + git config core.trustctime false && + git add file other && + git commit -m "initial import" +' + +test_expect_success '--refresh has no racy timestamps to fix' ' + reset_files && + # set the index time far enough to the future; + # it must be at least 3 seconds for VFAT + test_set_magic_mtime .git/index +60 && + git update-index --refresh && + test_is_magic_mtime .git/index +60 +' + +test_expect_success '--refresh should fix racy timestamp' ' + reset_files && + update_assert_changed --refresh +' + +test_expect_success '--really-refresh should fix racy timestamp' ' + reset_files && + update_assert_changed --really-refresh +' + +test_expect_success '--refresh should fix racy timestamp if other file needs update' ' + reset_files && + echo content2 >other && + test_set_magic_mtime other && + update_assert_changed --refresh +' + +test_expect_success '--refresh should fix racy timestamp if racy file needs update' ' + reset_files && + echo content2 >file && + test_set_magic_mtime file && + update_assert_changed --refresh +' + +test_done -- cgit v1.2.3