[PATCH v2 3/4] commit: fix write race

[Jun Wu <quark.zju@gmail.com>]

From: Jun Wu <quark@meta.com>

"commit" today does "read (index) -> lock -> write" and it does not verify
index is unchanged after locking, therefore is racy (an update made by another
process between "read" and "lock" gets overwritten). Fix the race by reordering
to "lock -> read -> write". This is more correct, but at the cost of more work
(e.g. lstat() the whole working tree) while holding the index lock.

The other purpose of fixing the write race is to make repo_hold_locked_index()
(see previous commit) believe that "commit" is now race-free, so "commit"
qualifies the new "waiting for index lock" behavior, which could be more
friendly to end-users (that expect "commit" to not fail).

Context checked: history shows no particular reason for reading before locking:
- f5bbc3225c (Port git commit to C., 2007-11-08): lock before reading
- 2888605c64 (builtin-commit: fix partial-commit support, 2007-11-18):
  seems to accidentally move reading to before locking

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Jun Wu <quark@meta.com>
---
 builtin/commit.c              | 9 ++-------
 t/t0032-index-lock-timeout.sh | 5 ++---
 2 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index a808c0ba39..bf41f04da7 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -389,13 +389,13 @@ static const char *prepare_index(const char **argv, const char *prefix,
 	    (!amend || (fixup_message && strcmp(fixup_prefix, "amend"))))))
 		die(_("No paths with --include/--only does not make sense."));
 
+	repo_hold_locked_index(the_repository, &index_lock, LOCK_DIE_ON_ERROR);
+
 	if (repo_read_index_preload(the_repository, &pathspec, 0) < 0)
 		die(_("index file corrupt"));
 
 	if (interactive) {
 		char *old_index_env = NULL, *old_repo_index_file;
-		repo_hold_locked_index(the_repository, &index_lock,
-				       LOCK_DIE_ON_ERROR);
 
 		refresh_cache_or_die(refresh_flags);
 
@@ -453,8 +453,6 @@ static const char *prepare_index(const char **argv, const char *prefix,
 	 */
 	if (all || (also && pathspec.nr)) {
 		char *ps_matched = xcalloc(pathspec.nr, 1);
-		repo_hold_locked_index(the_repository, &index_lock,
-				       LOCK_DIE_ON_ERROR);
 		add_files_to_cache(the_repository, also ? prefix : NULL,
 				   &pathspec, ps_matched, 0, 0, 0 );
 		if (!all && report_path_error(ps_matched, &pathspec))
@@ -480,8 +478,6 @@ static const char *prepare_index(const char **argv, const char *prefix,
 	 * We still need to refresh the index here.
 	 */
 	if (!only && !pathspec.nr) {
-		repo_hold_locked_index(the_repository, &index_lock,
-				       LOCK_DIE_ON_ERROR);
 		refresh_cache_or_die(refresh_flags);
 		if (the_repository->index->cache_changed
 		    || !cache_tree_fully_valid(the_repository->index))
@@ -531,7 +527,6 @@ static const char *prepare_index(const char **argv, const char *prefix,
 	if (repo_read_index(the_repository) < 0)
 		die(_("cannot read the index"));
 
-	repo_hold_locked_index(the_repository, &index_lock, LOCK_DIE_ON_ERROR);
 	add_remove_files(&partial);
 	refresh_index(the_repository->index, REFRESH_QUIET, NULL, NULL, NULL);
 	cache_tree_update(the_repository->index, WRITE_TREE_SILENT);
diff --git a/t/t0032-index-lock-timeout.sh b/t/t0032-index-lock-timeout.sh
index cde6f1892c..18caa182d7 100755
--- a/t/t0032-index-lock-timeout.sh
+++ b/t/t0032-index-lock-timeout.sh
@@ -29,13 +29,12 @@ test_expect_success 'index locked, add waits with indexLockTimeout>0' '
 	git -c core.indexLockTimeout=5000 add foo
 '
 
-test_expect_success 'index locked, commit fails even with indexLockTimeout>0 (read index before lock)' '
+test_expect_success 'index locked, commit waits with indexLockTimeout>0' '
 	prepare_index_lock &&
-	test_must_fail_with_index_lock git -c core.indexLockTimeout=5000 commit -m init
+	git -c core.indexLockTimeout=5000 commit -m init
 '
 
 test_expect_success 'index locked, checkout waits with indexLockTimeout>0' '
-	git commit -m init &&
 	prepare_index_lock &&
 	git -c core.indexLockTimeout=5000 checkout HEAD
 '
-- 
2.56.0