[Outreachy PATCH] read-cache: do not skip the hash of the shared index

[Tanga Joris <tangajoris@gmail.com>]

With core.splitIndex and index.skipHash both set, index entries are
lost when a new shared index is written:

  $ git init -q r && cd r
  $ git config core.splitIndex true
  $ git config index.skipHash true
  $ touch foo && git add foo
  $ git status --short
  ?? foo

write_shared_index() names the file after the shared index's trailing
checksum. With index.skipHash, that checksum is written as zeros. The
split index then records a null base, and read_index_from() treats a
null base as "no shared index". So the entries stored in the shared
index are lost, and "git status" can die with a BUG().

This has been broken since ee1f0c242e (read-cache: add index.skipHash
config option, 2023-01-06).

Never skip the hash when writing the shared index. $GIT_DIR/index
still honors index.skipHash, because it is written on every index
update. A new shared index is written much less often, so only those
writes pay for the hash.

"git reset" repairs an index that is already broken.

Reported-by: Andy Koppe <andy.koppe@gmail.com>
Signed-off-by: Tanga Joris <tangajoris@gmail.com>
---
A new shared index is only written when more than
splitIndex.maxPercentChange (20% by default) of the entries are not in
it, or by "git update-index --split-index".

Entries that the split index stores as replacements show up with an
empty name, so "git ls-files" prints "./" for them.

 read-cache.c           | 16 +++++++++++-----
 t/t1700-split-index.sh | 41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 5 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index e40f290bb3..c48ea7b1e0 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2818,7 +2818,8 @@ enum write_extensions {
  * rely on it.
  */
 static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
-			  enum write_extensions write_extensions, unsigned flags)
+			  enum write_extensions write_extensions, unsigned flags,
+			  int skip_hash)
 {
 	uint64_t start = getnanotime();
 	struct hashfile *f;
@@ -2841,8 +2842,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 
 	f = hashfd(the_repository->hash_algo, tempfile->fd, tempfile->filename.buf);
 
-	prepare_repo_settings(r);
-	f->skip_hash = r->settings.index_skip_hash;
+	f->skip_hash = skip_hash;
 
 	for (i = removed = extended = 0; i < entries; i++) {
 		if (cache[i]->ce_flags & CE_REMOVE)
@@ -3147,9 +3147,11 @@ static int do_write_locked_index(struct index_state *istate,
 		return ret;
 	}
 
+	prepare_repo_settings(istate->repo);
 	trace2_region_enter_printf("index", "do_write_index", istate->repo,
 				   "%s", get_lock_file_path(lock));
-	ret = do_write_index(istate, lock->tempfile, write_extensions, flags);
+	ret = do_write_index(istate, lock->tempfile, write_extensions, flags,
+			     istate->repo->settings.index_skip_hash);
 	trace2_region_leave_printf("index", "do_write_index", istate->repo,
 				   "%s", get_lock_file_path(lock));
 
@@ -3262,9 +3264,13 @@ static int write_shared_index(struct index_state *istate,
 	move_cache_to_base_index(istate);
 	convert_to_sparse(istate, 0);
 
+	/*
+	 * The shared index is named after its hash, and the split index
+	 * refers to it by that hash, so never skip it here.
+	 */
 	trace2_region_enter_printf("index", "shared/do_write_index",
 				   the_repository, "%s", get_tempfile_path(*temp));
-	ret = do_write_index(si->base, *temp, WRITE_NO_EXTENSION, flags);
+	ret = do_write_index(si->base, *temp, WRITE_NO_EXTENSION, flags, 0);
 	trace2_region_leave_printf("index", "shared/do_write_index",
 				   the_repository, "%s", get_tempfile_path(*temp));
 
diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
index 887e72a5fa..2517ac9eff 100755
--- a/t/t1700-split-index.sh
+++ b/t/t1700-split-index.sh
@@ -544,4 +544,45 @@ test_expect_success 'GIT_TEST_SPLIT_INDEX works' '
 	)
 '
 
+test_expect_success 'index.skipHash does not apply to the shared index' '
+	git init skip-hash-rewrite &&
+	(
+		cd skip-hash-rewrite &&
+		git config core.splitIndex true &&
+		>foo &&
+		>bar &&
+		git add foo bar &&
+		git commit -m initial &&
+		git config index.skipHash true &&
+		git update-index --split-index &&
+		test_path_is_missing .git/sharedindex.$ZERO_OID &&
+		test_trailing_hash .git/index >hash &&
+		echo $ZERO_OID >expect &&
+		test_cmp expect hash &&
+		cat >expect <<-\EOF &&
+		bar
+		foo
+		EOF
+		git ls-files >actual &&
+		test_cmp expect actual &&
+		git status --short --untracked-files=no >actual &&
+		test_must_be_empty actual
+	)
+'
+
+test_expect_success 'index.skipHash does not lose the first entry in split mode' '
+	git init skip-hash-first-add &&
+	(
+		cd skip-hash-first-add &&
+		git config core.splitIndex true &&
+		git config index.skipHash true &&
+		>foo &&
+		git add foo &&
+		test_path_is_missing .git/sharedindex.$ZERO_OID &&
+		echo foo >expect &&
+		git ls-files >actual &&
+		test_cmp expect actual
+	)
+'
+
 test_done
-- 
2.56.0.388.g09a996e1ff