diff --git a/builtin/commit.c b/builtin/commit.c index 8ffd42681b..36e47e18ed 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -1543,7 +1543,6 @@ struct repository *repo UNUSED) static const char *rename_score_arg = (const char *)-1; static struct wt_status s; unsigned int progress_flag = 0; - int fd; struct object_id oid; static struct option builtin_status_options[] = { OPT__VERBOSE(&verbose, N_("be verbose")), @@ -1631,11 +1630,6 @@ struct repository *repo UNUSED) REFRESH_QUIET|REFRESH_UNMERGED|progress_flag, &s.pathspec, NULL, NULL); - if (use_optional_locks()) - fd = repo_hold_locked_index(the_repository, &index_lock, 0); - else - fd = -1; - s.is_initial = repo_get_oid(the_repository, s.reference, &oid) ? 1 : 0; if (!s.is_initial) oidcpy(&s.oid_commit, &oid); @@ -1654,8 +1648,12 @@ struct repository *repo UNUSED) wt_status_collect(&s); - if (0 <= fd) - repo_update_index_if_able(the_repository, &index_lock); + /* + * Lock only after collecting (the slow part), so that status + * does not block other commands while it works. + */ + if (use_optional_locks()) + repo_lock_and_update_index_if_able(the_repository); if (s.relative_paths) s.prefix = prefix; diff --git a/read-cache-ll.h b/read-cache-ll.h index 066dd8bc3b..a45aff02ea 100644 --- a/read-cache-ll.h +++ b/read-cache-ll.h @@ -181,6 +181,7 @@ struct index_state { struct hashmap name_hash; struct hashmap dir_hash; struct object_id oid; + struct stat_data file_stat; struct untracked_cache *untracked; char *fsmonitor_last_update; struct ewah_bitmap *fsmonitor_dirty; diff --git a/read-cache.c b/read-cache.c index c4cf08a3a3..1d2b97c429 100644 --- a/read-cache.c +++ b/read-cache.c @@ -2309,6 +2309,7 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist) istate->timestamp.sec = st.st_mtime; istate->timestamp.nsec = ST_MTIME_NSEC(st); + fill_stat_data(&istate->file_stat, &st); /* if we created a thread, join it otherwise load the extensions on the primary thread */ if (extension_offset) { @@ -2699,9 +2700,35 @@ static int ce_write_entry(struct hashfile *f, struct cache_entry *ce, return 0; } +/* + * Whether the index file is still the one whose stat data was recorded + * when the index was read or written. This stands in for the checksum + * of an index written without one, so unlike match_stat_data() it + * always compares the inode and the mtime to the nanosecond, whatever + * core.checkStat and core.useNanosec say: a false "unchanged" would + * discard an update made by another process, while a false "changed" + * only skips an opportunistic write. The ctime is left out because + * renaming the written lockfile into place changes it. + */ +static int index_file_unchanged(const struct stat_data *sd, struct stat *st) +{ + struct stat_data now; + + fill_stat_data(&now, st); + return sd->sd_mtime.sec == now.sd_mtime.sec && + sd->sd_mtime.nsec == now.sd_mtime.nsec && + sd->sd_dev == now.sd_dev && + sd->sd_ino == now.sd_ino && + sd->sd_uid == now.sd_uid && + sd->sd_gid == now.sd_gid && + sd->sd_size == now.sd_size; +} + /* * This function verifies if index_state has the correct sha1 of the - * index file. Don't die if we have any other failure, just return 0. + * index file, or, when the index was written without a checksum + * (index.skipHash), that the file's stat data has not changed. Don't + * die if we have any other failure, just return 0. */ static int verify_index_from(const struct index_state *istate, const char *path) { @@ -2720,6 +2747,13 @@ static int verify_index_from(const struct index_state *istate, const char *path) if (fstat(fd, &st)) goto out; + if (is_null_oid(&istate->oid)) { + if (!index_file_unchanged(&istate->file_stat, &st)) + goto out; + close(fd); + return 1; + } + if (st.st_size < sizeof(struct cache_header) + the_hash_algo->rawsz) goto out; @@ -2756,17 +2790,33 @@ int has_racy_timestamp(struct index_state *istate) return 0; } +static int index_needs_write(struct index_state *istate) +{ + return istate->cache_changed || has_racy_timestamp(istate); +} + void repo_update_index_if_able(struct repository *repo, struct lock_file *lockfile) { - if ((repo->index->cache_changed || - has_racy_timestamp(repo->index)) && - repo_verify_index(repo)) + if (index_needs_write(repo->index) && repo_verify_index(repo)) write_locked_index(repo->index, lockfile, COMMIT_LOCK); else rollback_lock_file(lockfile); } +void repo_lock_and_update_index_if_able(struct repository *repo) +{ + struct lock_file lock = LOCK_INIT; + + if (!index_needs_write(repo->index) || + repo_hold_locked_index(repo, &lock, 0) < 0) + return; + if (repo_verify_index(repo)) + write_locked_index(repo->index, &lock, COMMIT_LOCK); + else + rollback_lock_file(&lock); +} + static int record_eoie(void) { int val; @@ -3098,6 +3148,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, } istate->timestamp.sec = (unsigned int)st.st_mtime; istate->timestamp.nsec = ST_MTIME_NSEC(st); + fill_stat_data(&istate->file_stat, &st); trace_performance_since(start, "write index, changed mask = %x", istate->cache_changed); trace2_data_intmax("index", istate->repo, "write/version", diff --git a/repository.h b/repository.h index 5f99a602b2..7fde28f64e 100644 --- a/repository.h +++ b/repository.h @@ -302,6 +302,13 @@ int repo_read_index_unmerged(struct repository *); */ void repo_update_index_if_able(struct repository *, struct lock_file *); +/* + * Like repo_update_index_if_able(), but take the lock only if the index + * has something to write, so that a caller that read the index without + * the lock holds it only for the write. + */ +void repo_lock_and_update_index_if_able(struct repository *); + /* * Return 1 if upgrade repository format to target_version succeeded, * 0 if no upgrade is necessary, and -1 when upgrade is not possible. diff --git a/t/t7508-status.sh b/t/t7508-status.sh index 0fd7c79911..e10c961268 100755 --- a/t/t7508-status.sh +++ b/t/t7508-status.sh @@ -1681,6 +1681,114 @@ test_expect_success '--no-optional-locks prevents index update' ' ! test_is_magic_mtime .git/index ' +# The fsmonitor hook runs while status reads the index, before status +# takes the lock, and stands in for a user command that updates the +# index at that moment. +for skip_hash in false true +do + test_expect_success "status keeps an index update made after it read the index (index.skipHash=$skip_hash)" ' + test_when_finished "rm -rf update-after-read" && + git init update-after-read && + ( + cd update-after-read && + if test $skip_hash = true + then + # index.skipHash does not work with a split index. + sane_unset GIT_TEST_SPLIT_INDEX + fi && + git config index.skipHash $skip_hash && + test_commit base && + write_script .git/fsmonitor-hook <<-\EOF && + # Act once; the "git add" below runs this hook again. + if test -f to-add && ! test -f .git/hook-ran + then + >.git/hook-ran && + git add to-add + fi + # Report failure so that status checks every path itself. + exit 1 + EOF + git config core.fsmonitorHookVersion 2 && + git config core.fsmonitor .git/fsmonitor-hook && + git status && + test-tool chmtime -60 base.t && + echo content >to-add && + git status && + test_path_is_file .git/hook-ran && + echo to-add >expect && + git diff --cached --name-only >actual && + test_cmp expect actual + ) + ' +done + +# While status collects, it runs "git status" in the submodule, which +# runs the submodule's fsmonitor hook. The hook stands in for a user +# command that updates the superproject's index at that moment. +setup_index_update_during_collection () { + git init "$1" && + ( + cd "$1" && + test_commit base && + git init sub && + test_commit -C sub sub-base && + git -c advice.addEmbeddedRepo=false add sub && + git commit -m "add sub" && + write_script sub/.git/fsmonitor-hook <<-\EOF && + # The hook runs in the submodule with GIT_DIR exported for it; + # clear that before working in the superproject. + unset $(git rev-parse --local-env-vars) + if test -f ../to-add + then + git -C .. add to-add + fi + # Report failure so that status checks every path itself. + exit 1 + EOF + git -C sub config core.fsmonitorHookVersion 2 && + git -C sub config core.fsmonitor .git/fsmonitor-hook + ) +} + +test_expect_success 'status does not block index updates while collecting' ' + test_when_finished "rm -rf update-while-collecting" && + setup_index_update_during_collection update-while-collecting && + ( + cd update-while-collecting && + git status && + echo content >to-add && + git status && + echo to-add >expect && + git diff --cached --name-only >actual && + test_cmp expect actual + ) +' + +for skip_hash in false true +do + test_expect_success "status refresh does not overwrite an index update made while collecting (index.skipHash=$skip_hash)" ' + test_when_finished "rm -rf refresh-while-collecting" && + ( + if test $skip_hash = true + then + # index.skipHash does not work with a split index. + sane_unset GIT_TEST_SPLIT_INDEX + fi && + setup_index_update_during_collection refresh-while-collecting && + cd refresh-while-collecting && + git config index.skipHash $skip_hash && + git update-index --force-write-index && + git status && + test-tool chmtime -60 base.t && + echo content >to-add && + git status && + echo to-add >expect && + git diff --cached --name-only >actual && + test_cmp expect actual + ) + ' +done + test_expect_success 'racy timestamps will be fixed for clean worktree' ' echo content >racy-dirty && echo content >racy-racy && diff --git a/unpack-trees.c b/unpack-trees.c index 1802809ad3..d6a859420f 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -1959,6 +1959,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options init_split_index(&o->internal.result); } oidcpy(&o->internal.result.oid, &o->src_index->oid); + o->internal.result.file_stat = o->src_index->file_stat; o->internal.merge_size = len; mark_all_ce_unused(o->src_index);