[PATCH 0/5] cache API: always have a "istate->repo"

[PATCH 0/5] cache API: always have a "istate->repo"

[Ævar Arnfjörð Bjarmason]

The "struct index_state" contains a "repo" member, which should be a
pointer to the repository for that index, but which due to us
constructing such structs on an ad-hoc basis in various places wasn't
always available.

We'd thus end up with code like this, in the recent
ds/omit-trailing-hash-in-index topic:

	struct repository *r = istate->repo ? istate->repo : the_repository;

Really we should be able to trust the "istate->repo", but were
carrying those sorts of conditionals because our index might come from
a manually constructed source, so we'd have to fall back to
"the_repository".

This series changes the relvant code so the "repo" field is always
non-NULL, as 5/5 here shows we had various workarounds in place for
that, which can now go away.

See
https://github.com/avar/git/tree/avar/do-not-lazy-populate-istate-repo
for passing CI and a fetchable branch for this topic.

See https://lore.kernel.org/git/xmqqmt6vqo2w.fsf@gitster.g/ for
previous discussion on this topic.

Ævar Arnfjörð Bjarmason (5):
  builtin/difftool.c: { 0 }-initialize rather than using memset()
  sparse-index.c: expand_to_path() can assume non-NULL "istate"
  sparse-index API: fix TODO, BUG() out on NULL ensure_full_index()
  read-cache.c: refactor set_new_index_sparsity() for subsequent commit
  treewide: always have a valid "index_state.repo" member

 apply.c                   |  2 +-
 builtin/difftool.c        |  4 +---
 builtin/sparse-checkout.c |  1 +
 builtin/stash.c           |  8 ++++----
 builtin/worktree.c        |  2 +-
 fsmonitor-settings.c      | 14 --------------
 fsmonitor.c               |  2 +-
 merge-recursive.c         |  2 +-
 read-cache.c              | 23 +++++++++--------------
 repository.c              |  2 ++
 revision.c                |  2 +-
 sparse-index.c            | 15 ++++-----------
 split-index.c             |  1 +
 unpack-trees.c            |  4 +++-
 14 files changed, 30 insertions(+), 52 deletions(-)

-- 
2.39.0.1195.gabc92c078c4

[PATCH 1/5] builtin/difftool.c: { 0 }-initialize rather than using memset()

[Ævar Arnfjörð Bjarmason]

Refactor an initialization of a variable added in
03831ef7b50 (difftool: implement the functionality in the builtin,
2017-01-19). This refactoring makes a subsequent change smaller.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/difftool.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/builtin/difftool.c b/builtin/difftool.c
index d9b76226f6a..1f9d4324df5 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -361,7 +361,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 	struct hashmap symlinks2 = HASHMAP_INIT(pair_cmp, NULL);
 	struct hashmap_iter iter;
 	struct pair_entry *entry;
-	struct index_state wtindex;
+	struct index_state wtindex = { 0 };
 	struct checkout lstate, rstate;
 	int err = 0;
 	struct child_process cmd = CHILD_PROCESS_INIT;
@@ -387,8 +387,6 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 	mkdir(ldir.buf, 0700);
 	mkdir(rdir.buf, 0700);
 
-	memset(&wtindex, 0, sizeof(wtindex));
-
 	memset(&lstate, 0, sizeof(lstate));
 	lstate.base_dir = lbase_dir = xstrdup(ldir.buf);
 	lstate.base_dir_len = ldir.len;
-- 
2.39.0.1195.gabc92c078c4

[PATCH 2/5] sparse-index.c: expand_to_path() can assume non-NULL "istate"

[Ævar Arnfjörð Bjarmason]

This function added in [1] was subsequently used in [2]. All of the
calls to it are in name-hash.c, and come after calls to
lazy_init_name_hash(istate). The first thing that function does is:

	if (istate->name_hash_initialized)
		return;

So we can already assume that we have a non-NULL "istate" here, or
we'd be segfaulting. Let's not confuse matters by making it appear
that's not the case.

1. 71f82d032f3 (sparse-index: expand_to_path(), 2021-04-12)
2. 4589bca829a (name-hash: use expand_to_path(), 2021-04-12)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 sparse-index.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sparse-index.c b/sparse-index.c
index 8c269dab803..65a08d33c73 100644
--- a/sparse-index.c
+++ b/sparse-index.c
@@ -547,7 +547,7 @@ void expand_to_path(struct index_state *istate,
 	if (in_expand_to_path)
 		return;
 
-	if (!istate || !istate->sparse_index)
+	if (!istate->sparse_index)
 		return;
 
 	if (!istate->repo)
-- 
2.39.0.1195.gabc92c078c4

[PATCH 3/5] sparse-index API: fix TODO, BUG() out on NULL ensure_full_index()

[Ævar Arnfjörð Bjarmason]

Make the ensure_full_index() function stricter, and have it only
accept a non-NULL "struct index_state". This function (and this
behavior) was added in [1].

The only reason it needed to be this lax was due to interaction with
repo_index_has_changes(). See the addition of that code in . This
fixes one of the TODO comments added in 0c18c059a15, the other one was
already removed in [3].

The other reason for why this was needed dates back to interaction
with code added in [4]. In [5] we started calling ensure_full_index()
in unpack_trees(), but the caller added in 34110cd4e39 wants to pass
us a NULL "dst_index". Let's instead do the NULL check in
unpack_trees() itself.

1. 4300f8442a2 (sparse-index: implement ensure_full_index(), 2021-03-30)
2. 0c18c059a15 (read-cache: ensure full index, 2021-04-01)
3. d76723ee531 (status: use sparse-index throughout, 2021-07-14).
4. 34110cd4e39 (Make 'unpack_trees()' have a separate source and
   destination index, 2008-03-06)
5. 6863df35503 (unpack-trees: ensure full index, 2021-03-30)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 read-cache.c   | 5 +++--
 sparse-index.c | 4 +++-
 unpack-trees.c | 3 ++-
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index cf87ad70970..9fbff4bc6aa 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2646,8 +2646,7 @@ int repo_index_has_changes(struct repository *repo,
 		}
 		diff_flush(&opt);
 		return opt.flags.has_changes != 0;
-	} else {
-		/* TODO: audit for interaction with sparse-index. */
+	} else if (istate) {
 		ensure_full_index(istate);
 		for (i = 0; sb && i < istate->cache_nr; i++) {
 			if (i)
@@ -2655,6 +2654,8 @@ int repo_index_has_changes(struct repository *repo,
 			strbuf_addstr(sb, istate->cache[i]->name);
 		}
 		return !!istate->cache_nr;
+	} else {
+		return 0;
 	}
 }
 
diff --git a/sparse-index.c b/sparse-index.c
index 65a08d33c73..86e3b99870b 100644
--- a/sparse-index.c
+++ b/sparse-index.c
@@ -299,7 +299,7 @@ void expand_index(struct index_state *istate, struct pattern_list *pl)
 	 * If the index is already full, then keep it full. We will convert
 	 * it to a sparse index on write, if possible.
 	 */
-	if (!istate || istate->sparse_index == INDEX_EXPANDED)
+	if (istate->sparse_index == INDEX_EXPANDED)
 		return;
 
 	/*
@@ -424,6 +424,8 @@ void expand_index(struct index_state *istate, struct pattern_list *pl)
 
 void ensure_full_index(struct index_state *istate)
 {
+	if (!istate)
+		BUG("ensure_full_index() must get an index!");
 	expand_index(istate, NULL);
 }
 
diff --git a/unpack-trees.c b/unpack-trees.c
index ea09023b015..2381cd7cac4 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1880,7 +1880,8 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 	prepare_repo_settings(repo);
 	if (repo->settings.command_requires_full_index) {
 		ensure_full_index(o->src_index);
-		ensure_full_index(o->dst_index);
+		if (o->dst_index)
+			ensure_full_index(o->dst_index);
 	}
 
 	if (o->reset == UNPACK_RESET_OVERWRITE_UNTRACKED &&
-- 
2.39.0.1195.gabc92c078c4

[PATCH 4/5] read-cache.c: refactor set_new_index_sparsity() for subsequent commit

[Ævar Arnfjörð Bjarmason]

Refactor code added to set_new_index_sparsity() in [1] to eliminate
indentation resulting from putting the body of his function within the
"if" block. Let's instead return early if we have no
istate->repo. This trivial change makes the subsequent commit's diff
smaller.

1. 491df5f679f (read-cache: set sparsity when index is new, 2022-05-10)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 read-cache.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 9fbff4bc6aa..78e38b0da28 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2300,12 +2300,12 @@ static void set_new_index_sparsity(struct index_state *istate)
 	 * If the index's repo exists, mark it sparse according to
 	 * repo settings.
 	 */
-	if (istate->repo) {
-		prepare_repo_settings(istate->repo);
-		if (!istate->repo->settings.command_requires_full_index &&
-		    is_sparse_index_allowed(istate, 0))
-			istate->sparse_index = 1;
-	}
+	if (!istate->repo)
+		return;
+	prepare_repo_settings(istate->repo);
+	if (!istate->repo->settings.command_requires_full_index &&
+	    is_sparse_index_allowed(istate, 0))
+		istate->sparse_index = 1;
 }
 
 /* remember to discard_cache() before reading a different cache! */
-- 
2.39.0.1195.gabc92c078c4

[PATCH 5/5] treewide: always have a valid "index_state.repo" member

[Ævar Arnfjörð Bjarmason]

When the "repo" member was added to "the_index" in [1] the
repo_read_index() was made to populate it, but the unpopulated
"the_index" variable didn't get the same treatment.

Let's do that in initialize_the_repository() when we set it up, and
likewise for all of the current callers initialized an empty "struct
index_state".

This simplifies code that needs to deal with "the_index" or a custom
"struct index_state", we no longer need to second-guess this part of
the "index_state" deep in the stack. A recent example of such
second-guessing is the "istate->repo ? istate->repo : the_repository"
code in [2]. We can now simply use "istate->repo".

For "fsmonitor-settings.c" we can remove the initialization of a NULL
"r" argument to "the_repository". This was added back in [3], and was
needed at the time for callers that would pass us the "r" from an
"istate->repo". Before this change such a change to
"fsmonitor-settings.c" would segfault all over the test suite (e.g. in
t0002-gitfile.sh).

This change has wider eventual implications for
"fsmonitor-settings.c". The reason the other lazy loading behavior in
it is required (starting with "if (!r->settings.fsmonitor) ..." is
because of the previously passed "r" being "NULL".

I have other local changes on top of this which move its configuration
reading to "prepare_repo_settings()" in "repo-settings.c", as we could
now start to rely on it being called for our "r". But let's leave all
of that for now, and narrowly remove this particular part of the
lazy-loading.

1. 1fd9ae517c4 (repository: add repo reference to index_state,
   2021-01-23)
2. ee1f0c242ef (read-cache: add index.skipHash config option,
   2023-01-06)
3. 1e0ea5c4316 (fsmonitor: config settings are repository-specific,
   2022-03-25)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 apply.c                   |  2 +-
 builtin/difftool.c        |  2 +-
 builtin/sparse-checkout.c |  1 +
 builtin/stash.c           |  8 ++++----
 builtin/worktree.c        |  2 +-
 fsmonitor-settings.c      | 14 --------------
 fsmonitor.c               |  2 +-
 merge-recursive.c         |  2 +-
 read-cache.c              | 10 ++--------
 repository.c              |  2 ++
 revision.c                |  2 +-
 sparse-index.c            |  9 ---------
 split-index.c             |  1 +
 unpack-trees.c            |  1 +
 14 files changed, 17 insertions(+), 41 deletions(-)

diff --git a/apply.c b/apply.c
index 85822280476..47bc6598573 100644
--- a/apply.c
+++ b/apply.c
@@ -4105,7 +4105,7 @@ static int preimage_oid_in_gitlink_patch(struct patch *p, struct object_id *oid)
 static int build_fake_ancestor(struct apply_state *state, struct patch *list)
 {
 	struct patch *patch;
-	struct index_state result = { NULL };
+	struct index_state result = { .repo = state->repo };
 	struct lock_file lock = LOCK_INIT;
 	int res;
 
diff --git a/builtin/difftool.c b/builtin/difftool.c
index 1f9d4324df5..ff29906f64a 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -361,7 +361,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 	struct hashmap symlinks2 = HASHMAP_INIT(pair_cmp, NULL);
 	struct hashmap_iter iter;
 	struct pair_entry *entry;
-	struct index_state wtindex = { 0 };
+	struct index_state wtindex = { .repo = the_repository };
 	struct checkout lstate, rstate;
 	int err = 0;
 	struct child_process cmd = CHILD_PROCESS_INIT;
diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 58a22503f04..6f0ea71ebb1 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -217,6 +217,7 @@ static int update_working_directory(struct pattern_list *pl)
 	o.head_idx = -1;
 	o.src_index = r->index;
 	o.dst_index = r->index;
+	o.result.repo = r;
 	o.skip_sparse_checkout = 0;
 	o.pl = pl;
 
diff --git a/builtin/stash.c b/builtin/stash.c
index bb0fd861434..a4ff967ea59 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1137,7 +1137,7 @@ static int save_untracked_files(struct stash_info *info, struct strbuf *msg,
 	int ret = 0;
 	struct strbuf untracked_msg = STRBUF_INIT;
 	struct child_process cp_upd_index = CHILD_PROCESS_INIT;
-	struct index_state istate = { NULL };
+	struct index_state istate = { .repo = the_repository };
 
 	cp_upd_index.git_cmd = 1;
 	strvec_pushl(&cp_upd_index.args, "update-index", "-z", "--add",
@@ -1176,7 +1176,7 @@ static int stash_staged(struct stash_info *info, struct strbuf *out_patch,
 {
 	int ret = 0;
 	struct child_process cp_diff_tree = CHILD_PROCESS_INIT;
-	struct index_state istate = { NULL };
+	struct index_state istate = { .repo = the_repository };
 
 	if (write_index_as_tree(&info->w_tree, &istate, the_repository->index_file,
 				0, NULL)) {
@@ -1209,7 +1209,7 @@ static int stash_patch(struct stash_info *info, const struct pathspec *ps,
 	int ret = 0;
 	struct child_process cp_read_tree = CHILD_PROCESS_INIT;
 	struct child_process cp_diff_tree = CHILD_PROCESS_INIT;
-	struct index_state istate = { NULL };
+	struct index_state istate = { .repo = the_repository };
 	char *old_index_env = NULL, *old_repo_index_file;
 
 	remove_path(stash_index_path.buf);
@@ -1271,7 +1271,7 @@ static int stash_working_tree(struct stash_info *info, const struct pathspec *ps
 	struct rev_info rev;
 	struct child_process cp_upd_index = CHILD_PROCESS_INIT;
 	struct strbuf diff_output = STRBUF_INIT;
-	struct index_state istate = { NULL };
+	struct index_state istate = { .repo = the_repository };
 
 	init_revisions(&rev, NULL);
 	copy_pathspec(&rev.prune_data, ps);
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 591d659faea..fd66fb4c165 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -923,7 +923,7 @@ static int unlock_worktree(int ac, const char **av, const char *prefix)
 
 static void validate_no_submodules(const struct worktree *wt)
 {
-	struct index_state istate = { NULL };
+	struct index_state istate = { .repo = the_repository };
 	struct strbuf path = STRBUF_INIT;
 	int i, found_submodules = 0;
 
diff --git a/fsmonitor-settings.c b/fsmonitor-settings.c
index ee63a97dc51..899bfe9c813 100644
--- a/fsmonitor-settings.c
+++ b/fsmonitor-settings.c
@@ -143,8 +143,6 @@ static void lookup_fsmonitor_settings(struct repository *r)
 
 enum fsmonitor_mode fsm_settings__get_mode(struct repository *r)
 {
-	if (!r)
-		r = the_repository;
 	if (!r->settings.fsmonitor)
 		lookup_fsmonitor_settings(r);
 
@@ -153,8 +151,6 @@ enum fsmonitor_mode fsm_settings__get_mode(struct repository *r)
 
 const char *fsm_settings__get_hook_path(struct repository *r)
 {
-	if (!r)
-		r = the_repository;
 	if (!r->settings.fsmonitor)
 		lookup_fsmonitor_settings(r);
 
@@ -174,8 +170,6 @@ void fsm_settings__set_ipc(struct repository *r)
 	 * Caller requested IPC explicitly, so avoid (possibly
 	 * recursive) config lookup.
 	 */
-	if (!r)
-		r = the_repository;
 	if (!r->settings.fsmonitor)
 		r->settings.fsmonitor = alloc_settings();
 
@@ -197,8 +191,6 @@ void fsm_settings__set_hook(struct repository *r, const char *path)
 	 * Caller requested hook explicitly, so avoid (possibly
 	 * recursive) config lookup.
 	 */
-	if (!r)
-		r = the_repository;
 	if (!r->settings.fsmonitor)
 		r->settings.fsmonitor = alloc_settings();
 
@@ -210,8 +202,6 @@ void fsm_settings__set_hook(struct repository *r, const char *path)
 
 void fsm_settings__set_disabled(struct repository *r)
 {
-	if (!r)
-		r = the_repository;
 	if (!r->settings.fsmonitor)
 		r->settings.fsmonitor = alloc_settings();
 
@@ -223,8 +213,6 @@ void fsm_settings__set_disabled(struct repository *r)
 void fsm_settings__set_incompatible(struct repository *r,
 				    enum fsmonitor_reason reason)
 {
-	if (!r)
-		r = the_repository;
 	if (!r->settings.fsmonitor)
 		r->settings.fsmonitor = alloc_settings();
 
@@ -235,8 +223,6 @@ void fsm_settings__set_incompatible(struct repository *r,
 
 enum fsmonitor_reason fsm_settings__get_reason(struct repository *r)
 {
-	if (!r)
-		r = the_repository;
 	if (!r->settings.fsmonitor)
 		lookup_fsmonitor_settings(r);
 
diff --git a/fsmonitor.c b/fsmonitor.c
index 08af00c7387..a5b9e75437b 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -304,7 +304,7 @@ void refresh_fsmonitor(struct index_state *istate)
 	char *buf;
 	unsigned int i;
 	int is_trivial = 0;
-	struct repository *r = istate->repo ? istate->repo : the_repository;
+	struct repository *r = istate->repo;
 	enum fsmonitor_mode fsm_mode = fsm_settings__get_mode(r);
 	enum fsmonitor_reason reason = fsm_settings__get_reason(r);
 
diff --git a/merge-recursive.c b/merge-recursive.c
index 2fd0aa96875..a63d2e330d5 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -412,7 +412,7 @@ static int unpack_trees_start(struct merge_options *opt,
 {
 	int rc;
 	struct tree_desc t[3];
-	struct index_state tmp_index = { NULL };
+	struct index_state tmp_index = { .repo = opt->repo };
 
 	memset(&opt->priv->unpack_opts, 0, sizeof(opt->priv->unpack_opts));
 	if (opt->priv->call_depth)
diff --git a/read-cache.c b/read-cache.c
index 78e38b0da28..3f018874926 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2300,8 +2300,6 @@ static void set_new_index_sparsity(struct index_state *istate)
 	 * If the index's repo exists, mark it sparse according to
 	 * repo settings.
 	 */
-	if (!istate->repo)
-		return;
 	prepare_repo_settings(istate->repo);
 	if (!istate->repo->settings.command_requires_full_index &&
 	    is_sparse_index_allowed(istate, 0))
@@ -2330,8 +2328,6 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
 	fd = open(path, O_RDONLY);
 	if (fd < 0) {
 		if (!must_exist && errno == ENOENT) {
-			if (!istate->repo)
-				istate->repo = the_repository;
 			set_new_index_sparsity(istate);
 			return 0;
 		}
@@ -2433,9 +2429,6 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
 	trace2_data_intmax("index", the_repository, "read/cache_nr",
 			   istate->cache_nr);
 
-	if (!istate->repo)
-		istate->repo = the_repository;
-
 	/*
 	 * If the command explicitly requires a full index, force it
 	 * to be full. Otherwise, correct the sparsity based on repository
@@ -2501,6 +2494,7 @@ int read_index_from(struct index_state *istate, const char *path,
 		discard_index(split_index->base);
 	else
 		CALLOC_ARRAY(split_index->base, 1);
+	split_index->base->repo = istate->repo;
 
 	base_oid_hex = oid_to_hex(&split_index->base_oid);
 	base_path = xstrfmt("%s/sharedindex.%s", gitdir, base_oid_hex);
@@ -2929,7 +2923,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 	int ieot_entries = 1;
 	struct index_entry_offset_table *ieot = NULL;
 	int nr, nr_threads;
-	struct repository *r = istate->repo ? istate->repo : the_repository;
+	struct repository *r = istate->repo;
 
 	f = hashfd(tempfile->fd, tempfile->filename.buf);
 
diff --git a/repository.c b/repository.c
index 3427085fd6d..fdebc8c597c 100644
--- a/repository.c
+++ b/repository.c
@@ -28,6 +28,8 @@ void initialize_the_repository(void)
 	the_repo.remote_state = remote_state_new();
 	the_repo.parsed_objects = parsed_object_pool_new();
 
+	the_index.repo = the_repository;
+
 	repo_set_hash_algo(&the_repo, GIT_HASH_SHA1);
 }
 
diff --git a/revision.c b/revision.c
index 100e5ad5110..785be1eb3bf 100644
--- a/revision.c
+++ b/revision.c
@@ -1813,7 +1813,7 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)
 	worktrees = get_worktrees();
 	for (p = worktrees; *p; p++) {
 		struct worktree *wt = *p;
-		struct index_state istate = { NULL };
+		struct index_state istate = { .repo = revs->repo };
 
 		if (wt->is_current)
 			continue; /* current index already taken care of */
diff --git a/sparse-index.c b/sparse-index.c
index 86e3b99870b..147a13386a4 100644
--- a/sparse-index.c
+++ b/sparse-index.c
@@ -128,9 +128,6 @@ int is_sparse_index_allowed(struct index_state *istate, int flags)
 	if (!core_apply_sparse_checkout || !core_sparse_checkout_cone)
 		return 0;
 
-	if (!istate->repo)
-		istate->repo = the_repository;
-
 	if (!(flags & SPARSE_INDEX_MEMORY_ONLY)) {
 		int test_env;
 
@@ -327,9 +324,6 @@ void expand_index(struct index_state *istate, struct pattern_list *pl)
 			pl = NULL;
 	}
 
-	if (!istate->repo)
-		istate->repo = the_repository;
-
 	/*
 	 * A NULL pattern set indicates we are expanding a full index, so
 	 * we use a special region name that indicates the full expansion.
@@ -552,9 +546,6 @@ void expand_to_path(struct index_state *istate,
 	if (!istate->sparse_index)
 		return;
 
-	if (!istate->repo)
-		istate->repo = the_repository;
-
 	in_expand_to_path = 1;
 
 	/*
diff --git a/split-index.c b/split-index.c
index 9d0ccc30d00..7815ca9f33e 100644
--- a/split-index.c
+++ b/split-index.c
@@ -91,6 +91,7 @@ void move_cache_to_base_index(struct index_state *istate)
 	}
 
 	CALLOC_ARRAY(si->base, 1);
+	si->base->repo = istate->repo;
 	si->base->version = istate->version;
 	/* zero timestamp disables racy test in ce_write_index() */
 	si->base->timestamp = istate->timestamp;
diff --git a/unpack-trees.c b/unpack-trees.c
index 2381cd7cac4..e8f25bbdce7 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1906,6 +1906,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 	}
 
 	memset(&o->result, 0, sizeof(o->result));
+	o->result.repo = o->src_index->repo;
 	o->result.initialized = 1;
 	o->result.timestamp.sec = o->src_index->timestamp.sec;
 	o->result.timestamp.nsec = o->src_index->timestamp.nsec;
-- 
2.39.0.1195.gabc92c078c4

Re: [PATCH 3/5] sparse-index API: fix TODO, BUG() out on NULL ensure_full_index()

[Philip Oakley]

On 10/01/2023 06:17, Ævar Arnfjörð Bjarmason wrote:
> Make the ensure_full_index() function stricter, and have it only
> accept a non-NULL "struct index_state". This function (and this
> behavior) was added in [1].
>
> The only reason it needed to be this lax was due to interaction with
> repo_index_has_changes(). See the addition of that code in .

Missing reference. Should this be [2]?

> This
> fixes one of the TODO comments added in 0c18c059a15, the other one was
> already removed in [3].
>
> The other reason for why this was needed dates back to interaction
> with code added in [4]. In [5] we started calling ensure_full_index()
> in unpack_trees(), but the caller added in 34110cd4e39 wants to pass
> us a NULL "dst_index". Let's instead do the NULL check in
> unpack_trees() itself.
>
> 1. 4300f8442a2 (sparse-index: implement ensure_full_index(), 2021-03-30)
> 2. 0c18c059a15 (read-cache: ensure full index, 2021-04-01)
> 3. d76723ee531 (status: use sparse-index throughout, 2021-07-14).
> 4. 34110cd4e39 (Make 'unpack_trees()' have a separate source and
>     destination index, 2008-03-06)
> 5. 6863df35503 (unpack-trees: ensure full index, 2021-03-30)
>
> Signed-off-by: Ævar Arnfjörð Bjarmason<avarab@gmail.com>
--
Philip

Re: [PATCH 3/5] sparse-index API: fix TODO, BUG() out on NULL ensure_full_index()

[Ævar Arnfjörð Bjarmason]

On Tue, Jan 10 2023, Philip Oakley wrote:

> On 10/01/2023 06:17, Ævar Arnfjörð Bjarmason wrote:
>> Make the ensure_full_index() function stricter, and have it only
>> accept a non-NULL "struct index_state". This function (and this
>> behavior) was added in [1].
>>
>> The only reason it needed to be this lax was due to interaction with
>> repo_index_has_changes(). See the addition of that code in .
>
> Missing reference. Should this be [2]?

Yes, thanks. Will fix that in a re-roll (pending further comments, will
wait a while).

Re: [PATCH 5/5] treewide: always have a valid "index_state.repo" member

[Ævar Arnfjörð Bjarmason]

On Tue, Jan 10 2023, Ævar Arnfjörð Bjarmason wrote:

> diff --git a/repository.c b/repository.c
> index 3427085fd6d..fdebc8c597c 100644
> --- a/repository.c
> +++ b/repository.c
> @@ -28,6 +28,8 @@ void initialize_the_repository(void)
>  	the_repo.remote_state = remote_state_new();
>  	the_repo.parsed_objects = parsed_object_pool_new();
>  
> +	the_index.repo = the_repository;
> +
>  	repo_set_hash_algo(&the_repo, GIT_HASH_SHA1);
>  }

It's slightly odd to have initialize_the_repository() initialize this
other variable, I did consider adding a initialize_the_variables(), and
have that call an initialize_the_repository() and an
initialize_the_index(), but deemed it not worth it for now. Perhaps
something to revisit once we're initializing more variables here.

Arguably such a split doesn't even conceptually make sense, as we're now
declaring that "the_repository" isn't fully initalized unless the
corresponding "the_index.repo" is also set, i.e. this really is part of
"the_repository" initialization.

Re: [PATCH 5/5] treewide: always have a valid "index_state.repo" member

[Jeff Hostetler]

On 1/10/23 1:17 AM, Ævar Arnfjörð Bjarmason wrote:
> When the "repo" member was added to "the_index" in [1] the
> repo_read_index() was made to populate it, but the unpopulated
> "the_index" variable didn't get the same treatment.
> 
> Let's do that in initialize_the_repository() when we set it up, and
> likewise for all of the current callers initialized an empty "struct
> index_state".
> 
> This simplifies code that needs to deal with "the_index" or a custom
> "struct index_state", we no longer need to second-guess this part of
> the "index_state" deep in the stack. A recent example of such
> second-guessing is the "istate->repo ? istate->repo : the_repository"
> code in [2]. We can now simply use "istate->repo".
> 
> For "fsmonitor-settings.c" we can remove the initialization of a NULL
> "r" argument to "the_repository". This was added back in [3], and was
> needed at the time for callers that would pass us the "r" from an
> "istate->repo". Before this change such a change to
> "fsmonitor-settings.c" would segfault all over the test suite (e.g. in
> t0002-gitfile.sh).
[...]

Thanks for looking at this.  Yes, it'll be nice to be able to
finally depend on istate->repo not being null all over the place.

Jeff

Re: [PATCH 3/5] sparse-index API: fix TODO, BUG() out on NULL ensure_full_index()

[Derrick Stolee]

On 1/10/2023 1:17 AM, Ævar Arnfjörð Bjarmason wrote: 
> The only reason it needed to be this lax was due to interaction with
> repo_index_has_changes(). See the addition of that code in . This
> fixes one of the TODO comments added in 0c18c059a15, the other one was
> already removed in [3].

> 2. 0c18c059a15 (read-cache: ensure full index, 2021-04-01)
> 3. d76723ee531 (status: use sparse-index throughout, 2021-07-14).

> -	} else {
> -		/* TODO: audit for interaction with sparse-index. */

Please don't drop this comment. It was inserted on purpose before the
"ensure_full_index(istate)" as an indicator that the following loop
has not been checked to see if it could be run on a sparse index.

Removing the comment is like saying "this loop was checked and we
_must_ use a full index here". The case of the TODO being removed in
[3] was because the loop was audited as being safe on a sparse index
(and the ensure_full_index() call was removed). I don't believe that
is what you have done here.

> +	} else if (istate) {
>  		ensure_full_index(istate);
>  		for (i = 0; sb && i < istate->cache_nr; i++) {
>  			if (i)

Further, this block has all sorts of direct uses of 'istate'
that would cause a segfault if 'istate' was NULL. Why do we need
to check for non-NULL now?

Looking earlier in the function, 'istate' is initialized to
'repo->index', so the function already assumed the repository had
an initialized index (or "tree || !get_oid_tree("HEAD", &cmp)"
was satisfied, which doesn't seem connected to a NULL index).

So, I'm thinking that we don't actually need any change to
repo_index_has_changes() unless it's being called in new ways
further down in the series.

Thanks,
-Stolee

Re: [PATCH 5/5] treewide: always have a valid "index_state.repo" member

[Derrick Stolee]

On 1/10/2023 1:17 AM, Ævar Arnfjörð Bjarmason wrote:
> When the "repo" member was added to "the_index" in [1] the
> repo_read_index() was made to populate it, but the unpopulated
> "the_index" variable didn't get the same treatment.
> 
> Let's do that in initialize_the_repository() when we set it up, and
> likewise for all of the current callers initialized an empty "struct
> index_state".

> +	struct index_state result = { .repo = state->repo };

> +	struct index_state wtindex = { .repo = the_repository };

> +	o.result.repo = r;

> +	struct index_state istate = { .repo = the_repository };

I think these initialization updates (along with the others I didn't
include) are satisfactory for now. What worries me is that future
consumers that create an index_state will need to remember to manually
initialize like this or risk hitting the BUG() statements in
read-cache.c.

The only alternative I can think about is to create an initialization
method, say "init_index_state(struct index_state *, struct repository *)",
that should be called before doing anything with an index_state. This
includes running a memset() to clear the struct, making these inline
initializers unnecessary.

However, I can't decide if that's actually an improvement. I think
things tip in favor of the init_index_state() method if there ever
becomes another member of struct index_state that _needs_ to be set
before the struct is "valid". I doubt that we would add such a thing
in the near future, so I recommend sticking with this patch as-is.

Thanks,
-Stolee

Re: [PATCH 0/5] cache API: always have a "istate->repo"

[Derrick Stolee]

On 1/10/2023 1:17 AM, Ævar Arnfjörð Bjarmason wrote:
> The "struct index_state" contains a "repo" member, which should be a
> pointer to the repository for that index, but which due to us
> constructing such structs on an ad-hoc basis in various places wasn't
> always available.
> 
> We'd thus end up with code like this, in the recent
> ds/omit-trailing-hash-in-index topic:
> 
> 	struct repository *r = istate->repo ? istate->repo : the_repository;
> 
> Really we should be able to trust the "istate->repo", but were
> carrying those sorts of conditionals because our index might come from
> a manually constructed source, so we'd have to fall back to
> "the_repository".
> 
> This series changes the relvant code so the "repo" field is always
> non-NULL, as 5/5 here shows we had various workarounds in place for
> that, which can now go away.

Thanks for these changes. I only had a recommended change for Patch 3,
along with Philip's recommended commit message fixup.

Glad to wrap up this tech debt.

Thanks,
-Stolee

[PATCH v2 0/6] cache API: always have a "istate->repo"

[Ævar Arnfjörð Bjarmason]

The "struct index_state" contains a "repo" member, which should be a
pointer to the repository for that index, but which due to us
constructing such structs on an ad-hoc basis in various places wasn't
always available.

We'd thus end up with code like this, in the recent
ds/omit-trailing-hash-in-index topic:

			       struct repository *r = istate->repo ? istate->repo : the_repository;

Really we should be able to trust the "istate->repo", but were
carrying those sorts of conditionals because our index might come from
a manually constructed source, so we'd have to fall back to
"the_repository".

This series changes the relvant code so the "repo" field is always
non-NULL, as 6/6 here shows we had various workarounds in place for
that, which can now go away.

For v1 and more general summary, see:
https://lore.kernel.org/git/cover-0.5-00000000000-20230110T060340Z-avarab@gmail.com/

See
https://github.com/avar/git/tree/avar/do-not-lazy-populate-istate-repo-2
for passing CI and a fetchable branch for this topic.

Change since v1:

 * Typo/rewording/correcting of commit messages
 * Kept a "TODO" comment which Derrick notes I shouldn't have removed

 * Derrick suggested in
   https://lore.kernel.org/git/6b92fad2-6b74-fddb-679c-8c8735e7103d@github.com/
   that things might be nicer if we had an explicit initializer, which
   was also the subject of the previous discussion at
   https://lore.kernel.org/git/xmqqmt6vqo2w.fsf@gitster.g/; but
   concluded that it was probably best to leave it for now.

   I tried it out, and I think it's worth just doing that now, which
   is why there's a new 5/6 here: We start by adding an
   INDEX_STATE_INIT macro, and corresponding function.

   There's a bit of churn in 6/6 as all of those now will take a
   "repo" argument, but I think the end result is worth it, because
   even if "repo" remains the only thing that we need to initialize
   we're now able to use ALLOC_ARRAY() instead of CALLOC_ARRAY().

   We'll thus be helped by analysis tools (which would show access to
   un-init'd memory) if we miss properly init-ing not just the "repo"
   field, but anything in the structure, so our test coverage will be
   better.

   It also makes the code easier to follow and change, as it's now
   more obvious where we initialize this, and it'll be easier to
   change it in the future if e.g. we add a new member that has
   mandatory initialization (e.g. a "struct strbuf").

Ævar Arnfjörð Bjarmason (6):
  builtin/difftool.c: { 0 }-initialize rather than using memset()
  sparse-index.c: expand_to_path() can assume non-NULL "istate"
  sparse-index API: BUG() out on NULL ensure_full_index()
  read-cache.c: refactor set_new_index_sparsity() for subsequent commit
  cache API: add a "INDEX_STATE_INIT" macro/function, add
    release_index()
  treewide: always have a valid "index_state.repo" member

 apply.c                   |  2 +-
 builtin/difftool.c        |  4 +---
 builtin/sparse-checkout.c |  1 +
 builtin/stash.c           | 16 ++++++-------
 builtin/worktree.c        |  2 +-
 cache.h                   | 16 +++++++++++++
 fsmonitor-settings.c      | 14 ------------
 fsmonitor.c               |  2 +-
 merge-recursive.c         |  2 +-
 read-cache.c              | 48 +++++++++++++++++++--------------------
 repository.c              |  8 +++++--
 revision.c                |  2 +-
 sparse-index.c            | 15 ++++--------
 split-index.c             |  3 ++-
 unpack-trees.c            |  5 ++--
 15 files changed, 69 insertions(+), 71 deletions(-)

Range-diff against v1:
1:  214fe7d3fc2 = 1:  214fe7d3fc2 builtin/difftool.c: { 0 }-initialize rather than using memset()
2:  6db74fe958f = 2:  6db74fe958f sparse-index.c: expand_to_path() can assume non-NULL "istate"
3:  d96388acef6 ! 3:  25e9cff0e97 sparse-index API: fix TODO, BUG() out on NULL ensure_full_index()
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    sparse-index API: fix TODO, BUG() out on NULL ensure_full_index()
    +    sparse-index API: BUG() out on NULL ensure_full_index()
     
         Make the ensure_full_index() function stricter, and have it only
         accept a non-NULL "struct index_state". This function (and this
         behavior) was added in [1].
     
         The only reason it needed to be this lax was due to interaction with
    -    repo_index_has_changes(). See the addition of that code in . This
    -    fixes one of the TODO comments added in 0c18c059a15, the other one was
    -    already removed in [3].
    +    repo_index_has_changes(). See the addition of that code in [2].
     
         The other reason for why this was needed dates back to interaction
    -    with code added in [4]. In [5] we started calling ensure_full_index()
    +    with code added in [3]. In [4] we started calling ensure_full_index()
         in unpack_trees(), but the caller added in 34110cd4e39 wants to pass
         us a NULL "dst_index". Let's instead do the NULL check in
         unpack_trees() itself.
     
         1. 4300f8442a2 (sparse-index: implement ensure_full_index(), 2021-03-30)
         2. 0c18c059a15 (read-cache: ensure full index, 2021-04-01)
    -    3. d76723ee531 (status: use sparse-index throughout, 2021-07-14).
    -    4. 34110cd4e39 (Make 'unpack_trees()' have a separate source and
    +    3. 34110cd4e39 (Make 'unpack_trees()' have a separate source and
            destination index, 2008-03-06)
    -    5. 6863df35503 (unpack-trees: ensure full index, 2021-03-30)
    +    4. 6863df35503 (unpack-trees: ensure full index, 2021-03-30)
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    - ## read-cache.c ##
    -@@ read-cache.c: int repo_index_has_changes(struct repository *repo,
    - 		}
    - 		diff_flush(&opt);
    - 		return opt.flags.has_changes != 0;
    --	} else {
    --		/* TODO: audit for interaction with sparse-index. */
    -+	} else if (istate) {
    - 		ensure_full_index(istate);
    - 		for (i = 0; sb && i < istate->cache_nr; i++) {
    - 			if (i)
    -@@ read-cache.c: int repo_index_has_changes(struct repository *repo,
    - 			strbuf_addstr(sb, istate->cache[i]->name);
    - 		}
    - 		return !!istate->cache_nr;
    -+	} else {
    -+		return 0;
    - 	}
    - }
    - 
    -
      ## sparse-index.c ##
     @@ sparse-index.c: void expand_index(struct index_state *istate, struct pattern_list *pl)
      	 * If the index is already full, then keep it full. We will convert
4:  e514a881e38 = 4:  a75977bd37b read-cache.c: refactor set_new_index_sparsity() for subsequent commit
-:  ----------- > 5:  ae256efe94a cache API: add a "INDEX_STATE_INIT" macro/function, add release_index()
5:  b3b0e592101 ! 6:  291233b0420 treewide: always have a valid "index_state.repo" member
    @@ Commit message
         second-guessing is the "istate->repo ? istate->repo : the_repository"
         code in [2]. We can now simply use "istate->repo".
     
    +    We're doing this by making use of the INDEX_STATE_INIT() macro (and
    +    corresponding function) added in the preceding commit, which now have
    +    mandatory "repo" arguments. As seen here there are cases where we set
    +    it to NULL, but only if we're about to fill in the correct non-NULL
    +    "repo" right afterwards.
    +
         For "fsmonitor-settings.c" we can remove the initialization of a NULL
         "r" argument to "the_repository". This was added back in [3], and was
         needed at the time for callers that would pass us the "r" from an
    @@ apply.c: static int preimage_oid_in_gitlink_patch(struct patch *p, struct object
      static int build_fake_ancestor(struct apply_state *state, struct patch *list)
      {
      	struct patch *patch;
    --	struct index_state result = { NULL };
    -+	struct index_state result = { .repo = state->repo };
    +-	struct index_state result = INDEX_STATE_INIT;
    ++	struct index_state result = INDEX_STATE_INIT(state->repo);
      	struct lock_file lock = LOCK_INIT;
      	int res;
      
    @@ builtin/difftool.c: static int run_dir_diff(const char *extcmd, int symlinks, co
      	struct hashmap symlinks2 = HASHMAP_INIT(pair_cmp, NULL);
      	struct hashmap_iter iter;
      	struct pair_entry *entry;
    --	struct index_state wtindex = { 0 };
    -+	struct index_state wtindex = { .repo = the_repository };
    +-	struct index_state wtindex = INDEX_STATE_INIT;
    ++	struct index_state wtindex = INDEX_STATE_INIT(the_repository);
      	struct checkout lstate, rstate;
      	int err = 0;
      	struct child_process cmd = CHILD_PROCESS_INIT;
    @@ builtin/sparse-checkout.c: static int update_working_directory(struct pattern_li
      	o.head_idx = -1;
      	o.src_index = r->index;
      	o.dst_index = r->index;
    -+	o.result.repo = r;
    +-	index_state_init(&o.result);
    ++	index_state_init(&o.result, r);
      	o.skip_sparse_checkout = 0;
      	o.pl = pl;
      
    @@ builtin/stash.c: static int save_untracked_files(struct stash_info *info, struct
      	int ret = 0;
      	struct strbuf untracked_msg = STRBUF_INIT;
      	struct child_process cp_upd_index = CHILD_PROCESS_INIT;
    --	struct index_state istate = { NULL };
    -+	struct index_state istate = { .repo = the_repository };
    +-	struct index_state istate = INDEX_STATE_INIT;
    ++	struct index_state istate = INDEX_STATE_INIT(the_repository);
      
      	cp_upd_index.git_cmd = 1;
      	strvec_pushl(&cp_upd_index.args, "update-index", "-z", "--add",
    @@ builtin/stash.c: static int stash_staged(struct stash_info *info, struct strbuf
      {
      	int ret = 0;
      	struct child_process cp_diff_tree = CHILD_PROCESS_INIT;
    --	struct index_state istate = { NULL };
    -+	struct index_state istate = { .repo = the_repository };
    +-	struct index_state istate = INDEX_STATE_INIT;
    ++	struct index_state istate = INDEX_STATE_INIT(the_repository);
      
      	if (write_index_as_tree(&info->w_tree, &istate, the_repository->index_file,
      				0, NULL)) {
    @@ builtin/stash.c: static int stash_patch(struct stash_info *info, const struct pa
      	int ret = 0;
      	struct child_process cp_read_tree = CHILD_PROCESS_INIT;
      	struct child_process cp_diff_tree = CHILD_PROCESS_INIT;
    --	struct index_state istate = { NULL };
    -+	struct index_state istate = { .repo = the_repository };
    +-	struct index_state istate = INDEX_STATE_INIT;
    ++	struct index_state istate = INDEX_STATE_INIT(the_repository);
      	char *old_index_env = NULL, *old_repo_index_file;
      
      	remove_path(stash_index_path.buf);
    @@ builtin/stash.c: static int stash_working_tree(struct stash_info *info, const st
      	struct rev_info rev;
      	struct child_process cp_upd_index = CHILD_PROCESS_INIT;
      	struct strbuf diff_output = STRBUF_INIT;
    --	struct index_state istate = { NULL };
    -+	struct index_state istate = { .repo = the_repository };
    +-	struct index_state istate = INDEX_STATE_INIT;
    ++	struct index_state istate = INDEX_STATE_INIT(the_repository);
      
      	init_revisions(&rev, NULL);
      	copy_pathspec(&rev.prune_data, ps);
    @@ builtin/worktree.c: static int unlock_worktree(int ac, const char **av, const ch
      
      static void validate_no_submodules(const struct worktree *wt)
      {
    --	struct index_state istate = { NULL };
    -+	struct index_state istate = { .repo = the_repository };
    +-	struct index_state istate = INDEX_STATE_INIT;
    ++	struct index_state istate = INDEX_STATE_INIT(the_repository);
      	struct strbuf path = STRBUF_INIT;
      	int i, found_submodules = 0;
      
     
    + ## cache.h ##
    +@@ cache.h: struct index_state {
    +  * If the variable won't be used again, use release_index() to free()
    +  * its resources. If it needs to be used again use discard_index(),
    +  * which does the same thing, but will use use index_state_init() at
    +- * the end.
    ++ * the end. The discard_index() will use its own "istate->repo" as the
    ++ * "r" argument to index_state_init() in that case.
    +  */
    +-#define INDEX_STATE_INIT { 0 }
    +-void index_state_init(struct index_state *istate);
    ++#define INDEX_STATE_INIT(r) { \
    ++	.repo = (r), \
    ++}
    ++void index_state_init(struct index_state *istate, struct repository *r);
    + void release_index(struct index_state *istate);
    + 
    + /* Name hashing */
    +
      ## fsmonitor-settings.c ##
     @@ fsmonitor-settings.c: static void lookup_fsmonitor_settings(struct repository *r)
      
    @@ merge-recursive.c: static int unpack_trees_start(struct merge_options *opt,
      {
      	int rc;
      	struct tree_desc t[3];
    --	struct index_state tmp_index = { NULL };
    -+	struct index_state tmp_index = { .repo = opt->repo };
    +-	struct index_state tmp_index = INDEX_STATE_INIT;
    ++	struct index_state tmp_index = INDEX_STATE_INIT(opt->repo);
      
      	memset(&opt->priv->unpack_opts, 0, sizeof(opt->priv->unpack_opts));
      	if (opt->priv->call_depth)
    @@ read-cache.c: int do_read_index(struct index_state *istate, const char *path, in
      	 * If the command explicitly requires a full index, force it
      	 * to be full. Otherwise, correct the sparsity based on repository
     @@ read-cache.c: int read_index_from(struct index_state *istate, const char *path,
    - 		discard_index(split_index->base);
    + 		release_index(split_index->base);
      	else
    - 		CALLOC_ARRAY(split_index->base, 1);
    -+	split_index->base->repo = istate->repo;
    + 		ALLOC_ARRAY(split_index->base, 1);
    +-	index_state_init(split_index->base);
    ++	index_state_init(split_index->base, istate->repo);
      
      	base_oid_hex = oid_to_hex(&split_index->base_oid);
      	base_path = xstrfmt("%s/sharedindex.%s", gitdir, base_oid_hex);
    +@@ read-cache.c: int is_index_unborn(struct index_state *istate)
    + 	return (!istate->cache_nr && !istate->timestamp.sec);
    + }
    + 
    +-void index_state_init(struct index_state *istate)
    ++void index_state_init(struct index_state *istate, struct repository *r)
    + {
    +-	struct index_state blank = INDEX_STATE_INIT;
    ++	struct index_state blank = INDEX_STATE_INIT(r);
    + 	memcpy(istate, &blank, sizeof(*istate));
    + }
    + 
    +@@ read-cache.c: void release_index(struct index_state *istate)
    + void discard_index(struct index_state *istate)
    + {
    + 	release_index(istate);
    +-	index_state_init(istate);
    ++	index_state_init(istate, istate->repo);
    + }
    + 
    + /*
     @@ read-cache.c: static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
      	int ieot_entries = 1;
      	struct index_entry_offset_table *ieot = NULL;
    @@ repository.c: void initialize_the_repository(void)
      	the_repo.remote_state = remote_state_new();
      	the_repo.parsed_objects = parsed_object_pool_new();
      
    -+	the_index.repo = the_repository;
    ++	index_state_init(&the_index, the_repository);
     +
      	repo_set_hash_algo(&the_repo, GIT_HASH_SHA1);
      }
      
    +@@ repository.c: int repo_read_index(struct repository *repo)
    + 
    + 	if (!repo->index) {
    + 		ALLOC_ARRAY(repo->index, 1);
    +-		index_state_init(repo->index);
    ++		index_state_init(repo->index, NULL /* set below */);
    + 	}
    + 
    + 	/* Complete the double-reference */
     
      ## revision.c ##
     @@ revision.c: void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)
      	worktrees = get_worktrees();
      	for (p = worktrees; *p; p++) {
      		struct worktree *wt = *p;
    --		struct index_state istate = { NULL };
    -+		struct index_state istate = { .repo = revs->repo };
    +-		struct index_state istate = INDEX_STATE_INIT;
    ++		struct index_state istate = INDEX_STATE_INIT(revs->repo);
      
      		if (wt->is_current)
      			continue; /* current index already taken care of */
    @@ split-index.c
     @@ split-index.c: void move_cache_to_base_index(struct index_state *istate)
      	}
      
    - 	CALLOC_ARRAY(si->base, 1);
    -+	si->base->repo = istate->repo;
    + 	ALLOC_ARRAY(si->base, 1);
    +-	index_state_init(si->base);
    ++	index_state_init(si->base, istate->repo);
      	si->base->version = istate->version;
      	/* zero timestamp disables racy test in ce_write_index() */
      	si->base->timestamp = istate->timestamp;
     
      ## unpack-trees.c ##
     @@ unpack-trees.c: int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
    + 		populate_from_existing_patterns(o, &pl);
      	}
      
    - 	memset(&o->result, 0, sizeof(o->result));
    -+	o->result.repo = o->src_index->repo;
    +-	index_state_init(&o->result);
    ++	index_state_init(&o->result, o->src_index->repo);
      	o->result.initialized = 1;
      	o->result.timestamp.sec = o->src_index->timestamp.sec;
      	o->result.timestamp.nsec = o->src_index->timestamp.nsec;
-- 
2.39.0.1205.g2ca064edc27

[PATCH v2 1/6] builtin/difftool.c: { 0 }-initialize rather than using memset()

[Ævar Arnfjörð Bjarmason]

Refactor an initialization of a variable added in
03831ef7b50 (difftool: implement the functionality in the builtin,
2017-01-19). This refactoring makes a subsequent change smaller.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/difftool.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/builtin/difftool.c b/builtin/difftool.c
index d9b76226f6a..1f9d4324df5 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -361,7 +361,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 	struct hashmap symlinks2 = HASHMAP_INIT(pair_cmp, NULL);
 	struct hashmap_iter iter;
 	struct pair_entry *entry;
-	struct index_state wtindex;
+	struct index_state wtindex = { 0 };
 	struct checkout lstate, rstate;
 	int err = 0;
 	struct child_process cmd = CHILD_PROCESS_INIT;
@@ -387,8 +387,6 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 	mkdir(ldir.buf, 0700);
 	mkdir(rdir.buf, 0700);
 
-	memset(&wtindex, 0, sizeof(wtindex));
-
 	memset(&lstate, 0, sizeof(lstate));
 	lstate.base_dir = lbase_dir = xstrdup(ldir.buf);
 	lstate.base_dir_len = ldir.len;
-- 
2.39.0.1205.g2ca064edc27

[PATCH v2 2/6] sparse-index.c: expand_to_path() can assume non-NULL "istate"

[Ævar Arnfjörð Bjarmason]

This function added in [1] was subsequently used in [2]. All of the
calls to it are in name-hash.c, and come after calls to
lazy_init_name_hash(istate). The first thing that function does is:

	if (istate->name_hash_initialized)
		return;

So we can already assume that we have a non-NULL "istate" here, or
we'd be segfaulting. Let's not confuse matters by making it appear
that's not the case.

1. 71f82d032f3 (sparse-index: expand_to_path(), 2021-04-12)
2. 4589bca829a (name-hash: use expand_to_path(), 2021-04-12)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 sparse-index.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sparse-index.c b/sparse-index.c
index 8c269dab803..65a08d33c73 100644
--- a/sparse-index.c
+++ b/sparse-index.c
@@ -547,7 +547,7 @@ void expand_to_path(struct index_state *istate,
 	if (in_expand_to_path)
 		return;
 
-	if (!istate || !istate->sparse_index)
+	if (!istate->sparse_index)
 		return;
 
 	if (!istate->repo)
-- 
2.39.0.1205.g2ca064edc27

[PATCH v2 3/6] sparse-index API: BUG() out on NULL ensure_full_index()

[Ævar Arnfjörð Bjarmason]

Make the ensure_full_index() function stricter, and have it only
accept a non-NULL "struct index_state". This function (and this
behavior) was added in [1].

The only reason it needed to be this lax was due to interaction with
repo_index_has_changes(). See the addition of that code in [2].

The other reason for why this was needed dates back to interaction
with code added in [3]. In [4] we started calling ensure_full_index()
in unpack_trees(), but the caller added in 34110cd4e39 wants to pass
us a NULL "dst_index". Let's instead do the NULL check in
unpack_trees() itself.

1. 4300f8442a2 (sparse-index: implement ensure_full_index(), 2021-03-30)
2. 0c18c059a15 (read-cache: ensure full index, 2021-04-01)
3. 34110cd4e39 (Make 'unpack_trees()' have a separate source and
   destination index, 2008-03-06)
4. 6863df35503 (unpack-trees: ensure full index, 2021-03-30)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 sparse-index.c | 4 +++-
 unpack-trees.c | 3 ++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/sparse-index.c b/sparse-index.c
index 65a08d33c73..86e3b99870b 100644
--- a/sparse-index.c
+++ b/sparse-index.c
@@ -299,7 +299,7 @@ void expand_index(struct index_state *istate, struct pattern_list *pl)
 	 * If the index is already full, then keep it full. We will convert
 	 * it to a sparse index on write, if possible.
 	 */
-	if (!istate || istate->sparse_index == INDEX_EXPANDED)
+	if (istate->sparse_index == INDEX_EXPANDED)
 		return;
 
 	/*
@@ -424,6 +424,8 @@ void expand_index(struct index_state *istate, struct pattern_list *pl)
 
 void ensure_full_index(struct index_state *istate)
 {
+	if (!istate)
+		BUG("ensure_full_index() must get an index!");
 	expand_index(istate, NULL);
 }
 
diff --git a/unpack-trees.c b/unpack-trees.c
index ea09023b015..2381cd7cac4 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1880,7 +1880,8 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 	prepare_repo_settings(repo);
 	if (repo->settings.command_requires_full_index) {
 		ensure_full_index(o->src_index);
-		ensure_full_index(o->dst_index);
+		if (o->dst_index)
+			ensure_full_index(o->dst_index);
 	}
 
 	if (o->reset == UNPACK_RESET_OVERWRITE_UNTRACKED &&
-- 
2.39.0.1205.g2ca064edc27

[PATCH v2 4/6] read-cache.c: refactor set_new_index_sparsity() for subsequent commit

[Ævar Arnfjörð Bjarmason]

Refactor code added to set_new_index_sparsity() in [1] to eliminate
indentation resulting from putting the body of his function within the
"if" block. Let's instead return early if we have no
istate->repo. This trivial change makes the subsequent commit's diff
smaller.

1. 491df5f679f (read-cache: set sparsity when index is new, 2022-05-10)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 read-cache.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index cf87ad70970..25925c8c002 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2300,12 +2300,12 @@ static void set_new_index_sparsity(struct index_state *istate)
 	 * If the index's repo exists, mark it sparse according to
 	 * repo settings.
 	 */
-	if (istate->repo) {
-		prepare_repo_settings(istate->repo);
-		if (!istate->repo->settings.command_requires_full_index &&
-		    is_sparse_index_allowed(istate, 0))
-			istate->sparse_index = 1;
-	}
+	if (!istate->repo)
+		return;
+	prepare_repo_settings(istate->repo);
+	if (!istate->repo->settings.command_requires_full_index &&
+	    is_sparse_index_allowed(istate, 0))
+		istate->sparse_index = 1;
 }
 
 /* remember to discard_cache() before reading a different cache! */
-- 
2.39.0.1205.g2ca064edc27

[PATCH v2 5/6] cache API: add a "INDEX_STATE_INIT" macro/function, add release_index()

[Ævar Arnfjörð Bjarmason]

As we'll see in a subsequent commit we'll get advantages from always
initializing the "repo" member of the "struct index_state". To make
that easier let's introduce an initialization macro & function.

The various ad-hoc initialization of the structure can then be changed
over to it, and we can remove the various "0" assignments in
discard_index() in favor of calling index_state_init() at the end.

While not strictly necessary, let's also change the CALLOC_ARRAY() of
various "struct index_state *" to use an ALLOC_ARRAY() followed by
index_state_init() instead.

We're then adding the release_index() function and converting some
callers (including some of these allocations) over to it if they
either won't need to use their "struct index_state" again, or are just
about to call index_state_init().

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 apply.c                   |  2 +-
 builtin/difftool.c        |  2 +-
 builtin/sparse-checkout.c |  1 +
 builtin/stash.c           | 16 ++++++++--------
 builtin/worktree.c        |  2 +-
 cache.h                   | 13 +++++++++++++
 merge-recursive.c         |  2 +-
 read-cache.c              | 31 ++++++++++++++++++-------------
 repository.c              |  6 ++++--
 revision.c                |  2 +-
 split-index.c             |  3 ++-
 unpack-trees.c            |  2 +-
 12 files changed, 52 insertions(+), 30 deletions(-)

diff --git a/apply.c b/apply.c
index 85822280476..821fe411ec8 100644
--- a/apply.c
+++ b/apply.c
@@ -4105,7 +4105,7 @@ static int preimage_oid_in_gitlink_patch(struct patch *p, struct object_id *oid)
 static int build_fake_ancestor(struct apply_state *state, struct patch *list)
 {
 	struct patch *patch;
-	struct index_state result = { NULL };
+	struct index_state result = INDEX_STATE_INIT;
 	struct lock_file lock = LOCK_INIT;
 	int res;
 
diff --git a/builtin/difftool.c b/builtin/difftool.c
index 1f9d4324df5..758e7bd3b6a 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -361,7 +361,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 	struct hashmap symlinks2 = HASHMAP_INIT(pair_cmp, NULL);
 	struct hashmap_iter iter;
 	struct pair_entry *entry;
-	struct index_state wtindex = { 0 };
+	struct index_state wtindex = INDEX_STATE_INIT;
 	struct checkout lstate, rstate;
 	int err = 0;
 	struct child_process cmd = CHILD_PROCESS_INIT;
diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 58a22503f04..dc332c6d05f 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -217,6 +217,7 @@ static int update_working_directory(struct pattern_list *pl)
 	o.head_idx = -1;
 	o.src_index = r->index;
 	o.dst_index = r->index;
+	index_state_init(&o.result);
 	o.skip_sparse_checkout = 0;
 	o.pl = pl;
 
diff --git a/builtin/stash.c b/builtin/stash.c
index bb0fd861434..a5f13fb1e9f 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1137,7 +1137,7 @@ static int save_untracked_files(struct stash_info *info, struct strbuf *msg,
 	int ret = 0;
 	struct strbuf untracked_msg = STRBUF_INIT;
 	struct child_process cp_upd_index = CHILD_PROCESS_INIT;
-	struct index_state istate = { NULL };
+	struct index_state istate = INDEX_STATE_INIT;
 
 	cp_upd_index.git_cmd = 1;
 	strvec_pushl(&cp_upd_index.args, "update-index", "-z", "--add",
@@ -1165,7 +1165,7 @@ static int save_untracked_files(struct stash_info *info, struct strbuf *msg,
 	}
 
 done:
-	discard_index(&istate);
+	release_index(&istate);
 	strbuf_release(&untracked_msg);
 	remove_path(stash_index_path.buf);
 	return ret;
@@ -1176,7 +1176,7 @@ static int stash_staged(struct stash_info *info, struct strbuf *out_patch,
 {
 	int ret = 0;
 	struct child_process cp_diff_tree = CHILD_PROCESS_INIT;
-	struct index_state istate = { NULL };
+	struct index_state istate = INDEX_STATE_INIT;
 
 	if (write_index_as_tree(&info->w_tree, &istate, the_repository->index_file,
 				0, NULL)) {
@@ -1199,7 +1199,7 @@ static int stash_staged(struct stash_info *info, struct strbuf *out_patch,
 	}
 
 done:
-	discard_index(&istate);
+	release_index(&istate);
 	return ret;
 }
 
@@ -1209,7 +1209,7 @@ static int stash_patch(struct stash_info *info, const struct pathspec *ps,
 	int ret = 0;
 	struct child_process cp_read_tree = CHILD_PROCESS_INIT;
 	struct child_process cp_diff_tree = CHILD_PROCESS_INIT;
-	struct index_state istate = { NULL };
+	struct index_state istate = INDEX_STATE_INIT;
 	char *old_index_env = NULL, *old_repo_index_file;
 
 	remove_path(stash_index_path.buf);
@@ -1260,7 +1260,7 @@ static int stash_patch(struct stash_info *info, const struct pathspec *ps,
 	}
 
 done:
-	discard_index(&istate);
+	release_index(&istate);
 	remove_path(stash_index_path.buf);
 	return ret;
 }
@@ -1271,7 +1271,7 @@ static int stash_working_tree(struct stash_info *info, const struct pathspec *ps
 	struct rev_info rev;
 	struct child_process cp_upd_index = CHILD_PROCESS_INIT;
 	struct strbuf diff_output = STRBUF_INIT;
-	struct index_state istate = { NULL };
+	struct index_state istate = INDEX_STATE_INIT;
 
 	init_revisions(&rev, NULL);
 	copy_pathspec(&rev.prune_data, ps);
@@ -1319,7 +1319,7 @@ static int stash_working_tree(struct stash_info *info, const struct pathspec *ps
 	}
 
 done:
-	discard_index(&istate);
+	release_index(&istate);
 	release_revisions(&rev);
 	strbuf_release(&diff_output);
 	remove_path(stash_index_path.buf);
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 591d659faea..311d6e90750 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -923,7 +923,7 @@ static int unlock_worktree(int ac, const char **av, const char *prefix)
 
 static void validate_no_submodules(const struct worktree *wt)
 {
-	struct index_state istate = { NULL };
+	struct index_state istate = INDEX_STATE_INIT;
 	struct strbuf path = STRBUF_INIT;
 	int i, found_submodules = 0;
 
diff --git a/cache.h b/cache.h
index 21ed9633b2a..be28fce12ac 100644
--- a/cache.h
+++ b/cache.h
@@ -360,6 +360,19 @@ struct index_state {
 	struct pattern_list *sparse_checkout_patterns;
 };
 
+/**
+ * A "struct index_state istate" must be initialized with
+ * INDEX_STATE_INIT or the corresponding index_state_init().
+ *
+ * If the variable won't be used again, use release_index() to free()
+ * its resources. If it needs to be used again use discard_index(),
+ * which does the same thing, but will use use index_state_init() at
+ * the end.
+ */
+#define INDEX_STATE_INIT { 0 }
+void index_state_init(struct index_state *istate);
+void release_index(struct index_state *istate);
+
 /* Name hashing */
 int test_lazy_init_name_hash(struct index_state *istate, int try_threaded);
 void add_name_hash(struct index_state *istate, struct cache_entry *ce);
diff --git a/merge-recursive.c b/merge-recursive.c
index 2fd0aa96875..0948b3ea961 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -412,7 +412,7 @@ static int unpack_trees_start(struct merge_options *opt,
 {
 	int rc;
 	struct tree_desc t[3];
-	struct index_state tmp_index = { NULL };
+	struct index_state tmp_index = INDEX_STATE_INIT;
 
 	memset(&opt->priv->unpack_opts, 0, sizeof(opt->priv->unpack_opts));
 	if (opt->priv->call_depth)
diff --git a/read-cache.c b/read-cache.c
index 25925c8c002..d191741f600 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2498,9 +2498,10 @@ int read_index_from(struct index_state *istate, const char *path,
 
 	trace_performance_enter();
 	if (split_index->base)
-		discard_index(split_index->base);
+		release_index(split_index->base);
 	else
-		CALLOC_ARRAY(split_index->base, 1);
+		ALLOC_ARRAY(split_index->base, 1);
+	index_state_init(split_index->base);
 
 	base_oid_hex = oid_to_hex(&split_index->base_oid);
 	base_path = xstrfmt("%s/sharedindex.%s", gitdir, base_oid_hex);
@@ -2539,7 +2540,13 @@ int is_index_unborn(struct index_state *istate)
 	return (!istate->cache_nr && !istate->timestamp.sec);
 }
 
-void discard_index(struct index_state *istate)
+void index_state_init(struct index_state *istate)
+{
+	struct index_state blank = INDEX_STATE_INIT;
+	memcpy(istate, &blank, sizeof(*istate));
+}
+
+void release_index(struct index_state *istate)
 {
 	/*
 	 * Cache entries in istate->cache[] should have been allocated
@@ -2551,20 +2558,12 @@ void discard_index(struct index_state *istate)
 	validate_cache_entries(istate);
 
 	resolve_undo_clear_index(istate);
-	istate->cache_nr = 0;
-	istate->cache_changed = 0;
-	istate->timestamp.sec = 0;
-	istate->timestamp.nsec = 0;
 	free_name_hash(istate);
 	cache_tree_free(&(istate->cache_tree));
-	istate->initialized = 0;
-	istate->fsmonitor_has_run_once = 0;
-	FREE_AND_NULL(istate->fsmonitor_last_update);
-	FREE_AND_NULL(istate->cache);
-	istate->cache_alloc = 0;
+	free(istate->fsmonitor_last_update);
+	free(istate->cache);
 	discard_split_index(istate);
 	free_untracked_cache(istate->untracked);
-	istate->untracked = NULL;
 
 	if (istate->sparse_checkout_patterns) {
 		clear_pattern_list(istate->sparse_checkout_patterns);
@@ -2577,6 +2576,12 @@ void discard_index(struct index_state *istate)
 	}
 }
 
+void discard_index(struct index_state *istate)
+{
+	release_index(istate);
+	index_state_init(istate);
+}
+
 /*
  * Validate the cache entries of this index.
  * All cache entries associated with this index
diff --git a/repository.c b/repository.c
index 3427085fd6d..a5805fa33b1 100644
--- a/repository.c
+++ b/repository.c
@@ -302,8 +302,10 @@ int repo_read_index(struct repository *repo)
 {
 	int res;
 
-	if (!repo->index)
-		CALLOC_ARRAY(repo->index, 1);
+	if (!repo->index) {
+		ALLOC_ARRAY(repo->index, 1);
+		index_state_init(repo->index);
+	}
 
 	/* Complete the double-reference */
 	if (!repo->index->repo)
diff --git a/revision.c b/revision.c
index 100e5ad5110..fb090886f5b 100644
--- a/revision.c
+++ b/revision.c
@@ -1813,7 +1813,7 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)
 	worktrees = get_worktrees();
 	for (p = worktrees; *p; p++) {
 		struct worktree *wt = *p;
-		struct index_state istate = { NULL };
+		struct index_state istate = INDEX_STATE_INIT;
 
 		if (wt->is_current)
 			continue; /* current index already taken care of */
diff --git a/split-index.c b/split-index.c
index 9d0ccc30d00..a5b56c0c37f 100644
--- a/split-index.c
+++ b/split-index.c
@@ -90,7 +90,8 @@ void move_cache_to_base_index(struct index_state *istate)
 		mem_pool_combine(istate->ce_mem_pool, istate->split_index->base->ce_mem_pool);
 	}
 
-	CALLOC_ARRAY(si->base, 1);
+	ALLOC_ARRAY(si->base, 1);
+	index_state_init(si->base);
 	si->base->version = istate->version;
 	/* zero timestamp disables racy test in ce_write_index() */
 	si->base->timestamp = istate->timestamp;
diff --git a/unpack-trees.c b/unpack-trees.c
index 2381cd7cac4..d1d1b0f319e 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1905,7 +1905,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 		populate_from_existing_patterns(o, &pl);
 	}
 
-	memset(&o->result, 0, sizeof(o->result));
+	index_state_init(&o->result);
 	o->result.initialized = 1;
 	o->result.timestamp.sec = o->src_index->timestamp.sec;
 	o->result.timestamp.nsec = o->src_index->timestamp.nsec;
-- 
2.39.0.1205.g2ca064edc27

[PATCH v2 6/6] treewide: always have a valid "index_state.repo" member

[Ævar Arnfjörð Bjarmason]

When the "repo" member was added to "the_index" in [1] the
repo_read_index() was made to populate it, but the unpopulated
"the_index" variable didn't get the same treatment.

Let's do that in initialize_the_repository() when we set it up, and
likewise for all of the current callers initialized an empty "struct
index_state".

This simplifies code that needs to deal with "the_index" or a custom
"struct index_state", we no longer need to second-guess this part of
the "index_state" deep in the stack. A recent example of such
second-guessing is the "istate->repo ? istate->repo : the_repository"
code in [2]. We can now simply use "istate->repo".

We're doing this by making use of the INDEX_STATE_INIT() macro (and
corresponding function) added in the preceding commit, which now have
mandatory "repo" arguments. As seen here there are cases where we set
it to NULL, but only if we're about to fill in the correct non-NULL
"repo" right afterwards.

For "fsmonitor-settings.c" we can remove the initialization of a NULL
"r" argument to "the_repository". This was added back in [3], and was
needed at the time for callers that would pass us the "r" from an
"istate->repo". Before this change such a change to
"fsmonitor-settings.c" would segfault all over the test suite (e.g. in
t0002-gitfile.sh).

This change has wider eventual implications for
"fsmonitor-settings.c". The reason the other lazy loading behavior in
it is required (starting with "if (!r->settings.fsmonitor) ..." is
because of the previously passed "r" being "NULL".

I have other local changes on top of this which move its configuration
reading to "prepare_repo_settings()" in "repo-settings.c", as we could
now start to rely on it being called for our "r". But let's leave all
of that for now, and narrowly remove this particular part of the
lazy-loading.

1. 1fd9ae517c4 (repository: add repo reference to index_state,
   2021-01-23)
2. ee1f0c242ef (read-cache: add index.skipHash config option,
   2023-01-06)
3. 1e0ea5c4316 (fsmonitor: config settings are repository-specific,
   2022-03-25)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 apply.c                   |  2 +-
 builtin/difftool.c        |  2 +-
 builtin/sparse-checkout.c |  2 +-
 builtin/stash.c           |  8 ++++----
 builtin/worktree.c        |  2 +-
 cache.h                   |  9 ++++++---
 fsmonitor-settings.c      | 14 --------------
 fsmonitor.c               |  2 +-
 merge-recursive.c         |  2 +-
 read-cache.c              | 17 +++++------------
 repository.c              |  4 +++-
 revision.c                |  2 +-
 sparse-index.c            |  9 ---------
 split-index.c             |  2 +-
 unpack-trees.c            |  2 +-
 15 files changed, 27 insertions(+), 52 deletions(-)

diff --git a/apply.c b/apply.c
index 821fe411ec8..5eec433583e 100644
--- a/apply.c
+++ b/apply.c
@@ -4105,7 +4105,7 @@ static int preimage_oid_in_gitlink_patch(struct patch *p, struct object_id *oid)
 static int build_fake_ancestor(struct apply_state *state, struct patch *list)
 {
 	struct patch *patch;
-	struct index_state result = INDEX_STATE_INIT;
+	struct index_state result = INDEX_STATE_INIT(state->repo);
 	struct lock_file lock = LOCK_INIT;
 	int res;
 
diff --git a/builtin/difftool.c b/builtin/difftool.c
index 758e7bd3b6a..dbbfb19f192 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -361,7 +361,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 	struct hashmap symlinks2 = HASHMAP_INIT(pair_cmp, NULL);
 	struct hashmap_iter iter;
 	struct pair_entry *entry;
-	struct index_state wtindex = INDEX_STATE_INIT;
+	struct index_state wtindex = INDEX_STATE_INIT(the_repository);
 	struct checkout lstate, rstate;
 	int err = 0;
 	struct child_process cmd = CHILD_PROCESS_INIT;
diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index dc332c6d05f..c3738154918 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -217,7 +217,7 @@ static int update_working_directory(struct pattern_list *pl)
 	o.head_idx = -1;
 	o.src_index = r->index;
 	o.dst_index = r->index;
-	index_state_init(&o.result);
+	index_state_init(&o.result, r);
 	o.skip_sparse_checkout = 0;
 	o.pl = pl;
 
diff --git a/builtin/stash.c b/builtin/stash.c
index a5f13fb1e9f..839569a9803 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1137,7 +1137,7 @@ static int save_untracked_files(struct stash_info *info, struct strbuf *msg,
 	int ret = 0;
 	struct strbuf untracked_msg = STRBUF_INIT;
 	struct child_process cp_upd_index = CHILD_PROCESS_INIT;
-	struct index_state istate = INDEX_STATE_INIT;
+	struct index_state istate = INDEX_STATE_INIT(the_repository);
 
 	cp_upd_index.git_cmd = 1;
 	strvec_pushl(&cp_upd_index.args, "update-index", "-z", "--add",
@@ -1176,7 +1176,7 @@ static int stash_staged(struct stash_info *info, struct strbuf *out_patch,
 {
 	int ret = 0;
 	struct child_process cp_diff_tree = CHILD_PROCESS_INIT;
-	struct index_state istate = INDEX_STATE_INIT;
+	struct index_state istate = INDEX_STATE_INIT(the_repository);
 
 	if (write_index_as_tree(&info->w_tree, &istate, the_repository->index_file,
 				0, NULL)) {
@@ -1209,7 +1209,7 @@ static int stash_patch(struct stash_info *info, const struct pathspec *ps,
 	int ret = 0;
 	struct child_process cp_read_tree = CHILD_PROCESS_INIT;
 	struct child_process cp_diff_tree = CHILD_PROCESS_INIT;
-	struct index_state istate = INDEX_STATE_INIT;
+	struct index_state istate = INDEX_STATE_INIT(the_repository);
 	char *old_index_env = NULL, *old_repo_index_file;
 
 	remove_path(stash_index_path.buf);
@@ -1271,7 +1271,7 @@ static int stash_working_tree(struct stash_info *info, const struct pathspec *ps
 	struct rev_info rev;
 	struct child_process cp_upd_index = CHILD_PROCESS_INIT;
 	struct strbuf diff_output = STRBUF_INIT;
-	struct index_state istate = INDEX_STATE_INIT;
+	struct index_state istate = INDEX_STATE_INIT(the_repository);
 
 	init_revisions(&rev, NULL);
 	copy_pathspec(&rev.prune_data, ps);
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 311d6e90750..f51c40f1e1e 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -923,7 +923,7 @@ static int unlock_worktree(int ac, const char **av, const char *prefix)
 
 static void validate_no_submodules(const struct worktree *wt)
 {
-	struct index_state istate = INDEX_STATE_INIT;
+	struct index_state istate = INDEX_STATE_INIT(the_repository);
 	struct strbuf path = STRBUF_INIT;
 	int i, found_submodules = 0;
 
diff --git a/cache.h b/cache.h
index be28fce12ac..4bf14e0bd94 100644
--- a/cache.h
+++ b/cache.h
@@ -367,10 +367,13 @@ struct index_state {
  * If the variable won't be used again, use release_index() to free()
  * its resources. If it needs to be used again use discard_index(),
  * which does the same thing, but will use use index_state_init() at
- * the end.
+ * the end. The discard_index() will use its own "istate->repo" as the
+ * "r" argument to index_state_init() in that case.
  */
-#define INDEX_STATE_INIT { 0 }
-void index_state_init(struct index_state *istate);
+#define INDEX_STATE_INIT(r) { \
+	.repo = (r), \
+}
+void index_state_init(struct index_state *istate, struct repository *r);
 void release_index(struct index_state *istate);
 
 /* Name hashing */
diff --git a/fsmonitor-settings.c b/fsmonitor-settings.c
index ee63a97dc51..899bfe9c813 100644
--- a/fsmonitor-settings.c
+++ b/fsmonitor-settings.c
@@ -143,8 +143,6 @@ static void lookup_fsmonitor_settings(struct repository *r)
 
 enum fsmonitor_mode fsm_settings__get_mode(struct repository *r)
 {
-	if (!r)
-		r = the_repository;
 	if (!r->settings.fsmonitor)
 		lookup_fsmonitor_settings(r);
 
@@ -153,8 +151,6 @@ enum fsmonitor_mode fsm_settings__get_mode(struct repository *r)
 
 const char *fsm_settings__get_hook_path(struct repository *r)
 {
-	if (!r)
-		r = the_repository;
 	if (!r->settings.fsmonitor)
 		lookup_fsmonitor_settings(r);
 
@@ -174,8 +170,6 @@ void fsm_settings__set_ipc(struct repository *r)
 	 * Caller requested IPC explicitly, so avoid (possibly
 	 * recursive) config lookup.
 	 */
-	if (!r)
-		r = the_repository;
 	if (!r->settings.fsmonitor)
 		r->settings.fsmonitor = alloc_settings();
 
@@ -197,8 +191,6 @@ void fsm_settings__set_hook(struct repository *r, const char *path)
 	 * Caller requested hook explicitly, so avoid (possibly
 	 * recursive) config lookup.
 	 */
-	if (!r)
-		r = the_repository;
 	if (!r->settings.fsmonitor)
 		r->settings.fsmonitor = alloc_settings();
 
@@ -210,8 +202,6 @@ void fsm_settings__set_hook(struct repository *r, const char *path)
 
 void fsm_settings__set_disabled(struct repository *r)
 {
-	if (!r)
-		r = the_repository;
 	if (!r->settings.fsmonitor)
 		r->settings.fsmonitor = alloc_settings();
 
@@ -223,8 +213,6 @@ void fsm_settings__set_disabled(struct repository *r)
 void fsm_settings__set_incompatible(struct repository *r,
 				    enum fsmonitor_reason reason)
 {
-	if (!r)
-		r = the_repository;
 	if (!r->settings.fsmonitor)
 		r->settings.fsmonitor = alloc_settings();
 
@@ -235,8 +223,6 @@ void fsm_settings__set_incompatible(struct repository *r,
 
 enum fsmonitor_reason fsm_settings__get_reason(struct repository *r)
 {
-	if (!r)
-		r = the_repository;
 	if (!r->settings.fsmonitor)
 		lookup_fsmonitor_settings(r);
 
diff --git a/fsmonitor.c b/fsmonitor.c
index 08af00c7387..a5b9e75437b 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -304,7 +304,7 @@ void refresh_fsmonitor(struct index_state *istate)
 	char *buf;
 	unsigned int i;
 	int is_trivial = 0;
-	struct repository *r = istate->repo ? istate->repo : the_repository;
+	struct repository *r = istate->repo;
 	enum fsmonitor_mode fsm_mode = fsm_settings__get_mode(r);
 	enum fsmonitor_reason reason = fsm_settings__get_reason(r);
 
diff --git a/merge-recursive.c b/merge-recursive.c
index 0948b3ea961..ae469f8cc81 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -412,7 +412,7 @@ static int unpack_trees_start(struct merge_options *opt,
 {
 	int rc;
 	struct tree_desc t[3];
-	struct index_state tmp_index = INDEX_STATE_INIT;
+	struct index_state tmp_index = INDEX_STATE_INIT(opt->repo);
 
 	memset(&opt->priv->unpack_opts, 0, sizeof(opt->priv->unpack_opts));
 	if (opt->priv->call_depth)
diff --git a/read-cache.c b/read-cache.c
index d191741f600..7bd12afb38c 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2300,8 +2300,6 @@ static void set_new_index_sparsity(struct index_state *istate)
 	 * If the index's repo exists, mark it sparse according to
 	 * repo settings.
 	 */
-	if (!istate->repo)
-		return;
 	prepare_repo_settings(istate->repo);
 	if (!istate->repo->settings.command_requires_full_index &&
 	    is_sparse_index_allowed(istate, 0))
@@ -2330,8 +2328,6 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
 	fd = open(path, O_RDONLY);
 	if (fd < 0) {
 		if (!must_exist && errno == ENOENT) {
-			if (!istate->repo)
-				istate->repo = the_repository;
 			set_new_index_sparsity(istate);
 			return 0;
 		}
@@ -2433,9 +2429,6 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
 	trace2_data_intmax("index", the_repository, "read/cache_nr",
 			   istate->cache_nr);
 
-	if (!istate->repo)
-		istate->repo = the_repository;
-
 	/*
 	 * If the command explicitly requires a full index, force it
 	 * to be full. Otherwise, correct the sparsity based on repository
@@ -2501,7 +2494,7 @@ int read_index_from(struct index_state *istate, const char *path,
 		release_index(split_index->base);
 	else
 		ALLOC_ARRAY(split_index->base, 1);
-	index_state_init(split_index->base);
+	index_state_init(split_index->base, istate->repo);
 
 	base_oid_hex = oid_to_hex(&split_index->base_oid);
 	base_path = xstrfmt("%s/sharedindex.%s", gitdir, base_oid_hex);
@@ -2540,9 +2533,9 @@ int is_index_unborn(struct index_state *istate)
 	return (!istate->cache_nr && !istate->timestamp.sec);
 }
 
-void index_state_init(struct index_state *istate)
+void index_state_init(struct index_state *istate, struct repository *r)
 {
-	struct index_state blank = INDEX_STATE_INIT;
+	struct index_state blank = INDEX_STATE_INIT(r);
 	memcpy(istate, &blank, sizeof(*istate));
 }
 
@@ -2579,7 +2572,7 @@ void release_index(struct index_state *istate)
 void discard_index(struct index_state *istate)
 {
 	release_index(istate);
-	index_state_init(istate);
+	index_state_init(istate, istate->repo);
 }
 
 /*
@@ -2933,7 +2926,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 	int ieot_entries = 1;
 	struct index_entry_offset_table *ieot = NULL;
 	int nr, nr_threads;
-	struct repository *r = istate->repo ? istate->repo : the_repository;
+	struct repository *r = istate->repo;
 
 	f = hashfd(tempfile->fd, tempfile->filename.buf);
 
diff --git a/repository.c b/repository.c
index a5805fa33b1..0e15220e6ac 100644
--- a/repository.c
+++ b/repository.c
@@ -28,6 +28,8 @@ void initialize_the_repository(void)
 	the_repo.remote_state = remote_state_new();
 	the_repo.parsed_objects = parsed_object_pool_new();
 
+	index_state_init(&the_index, the_repository);
+
 	repo_set_hash_algo(&the_repo, GIT_HASH_SHA1);
 }
 
@@ -304,7 +306,7 @@ int repo_read_index(struct repository *repo)
 
 	if (!repo->index) {
 		ALLOC_ARRAY(repo->index, 1);
-		index_state_init(repo->index);
+		index_state_init(repo->index, NULL /* set below */);
 	}
 
 	/* Complete the double-reference */
diff --git a/revision.c b/revision.c
index fb090886f5b..21f5f572c22 100644
--- a/revision.c
+++ b/revision.c
@@ -1813,7 +1813,7 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)
 	worktrees = get_worktrees();
 	for (p = worktrees; *p; p++) {
 		struct worktree *wt = *p;
-		struct index_state istate = INDEX_STATE_INIT;
+		struct index_state istate = INDEX_STATE_INIT(revs->repo);
 
 		if (wt->is_current)
 			continue; /* current index already taken care of */
diff --git a/sparse-index.c b/sparse-index.c
index 86e3b99870b..147a13386a4 100644
--- a/sparse-index.c
+++ b/sparse-index.c
@@ -128,9 +128,6 @@ int is_sparse_index_allowed(struct index_state *istate, int flags)
 	if (!core_apply_sparse_checkout || !core_sparse_checkout_cone)
 		return 0;
 
-	if (!istate->repo)
-		istate->repo = the_repository;
-
 	if (!(flags & SPARSE_INDEX_MEMORY_ONLY)) {
 		int test_env;
 
@@ -327,9 +324,6 @@ void expand_index(struct index_state *istate, struct pattern_list *pl)
 			pl = NULL;
 	}
 
-	if (!istate->repo)
-		istate->repo = the_repository;
-
 	/*
 	 * A NULL pattern set indicates we are expanding a full index, so
 	 * we use a special region name that indicates the full expansion.
@@ -552,9 +546,6 @@ void expand_to_path(struct index_state *istate,
 	if (!istate->sparse_index)
 		return;
 
-	if (!istate->repo)
-		istate->repo = the_repository;
-
 	in_expand_to_path = 1;
 
 	/*
diff --git a/split-index.c b/split-index.c
index a5b56c0c37f..5d0f04763ea 100644
--- a/split-index.c
+++ b/split-index.c
@@ -91,7 +91,7 @@ void move_cache_to_base_index(struct index_state *istate)
 	}
 
 	ALLOC_ARRAY(si->base, 1);
-	index_state_init(si->base);
+	index_state_init(si->base, istate->repo);
 	si->base->version = istate->version;
 	/* zero timestamp disables racy test in ce_write_index() */
 	si->base->timestamp = istate->timestamp;
diff --git a/unpack-trees.c b/unpack-trees.c
index d1d1b0f319e..3d05e45a279 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1905,7 +1905,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 		populate_from_existing_patterns(o, &pl);
 	}
 
-	index_state_init(&o->result);
+	index_state_init(&o->result, o->src_index->repo);
 	o->result.initialized = 1;
 	o->result.timestamp.sec = o->src_index->timestamp.sec;
 	o->result.timestamp.nsec = o->src_index->timestamp.nsec;
-- 
2.39.0.1205.g2ca064edc27

Re: [PATCH v2 5/6] cache API: add a "INDEX_STATE_INIT" macro/function, add release_index()

[Derrick Stolee]

On 1/12/2023 7:55 AM, Ævar Arnfjörð Bjarmason wrote:
> As we'll see in a subsequent commit we'll get advantages from always
> initializing the "repo" member of the "struct index_state". To make
> that easier let's introduce an initialization macro & function.
> 
> The various ad-hoc initialization of the structure can then be changed
> over to it, and we can remove the various "0" assignments in
> discard_index() in favor of calling index_state_init() at the end.

> -	memset(&o->result, 0, sizeof(o->result));
> +	index_state_init(&o->result);
>  	o->result.initialized = 1;

It's interesting that 'struct index_state' has an 'initialized'
member that we aren't setting in index_state_init(). Perhaps it's
only being used in special cases like this, and means something
more specific than "index_state_init() was run"? Or maybe we
could add it to INDEX_STATE_INIT and drop this line?

Thanks,
-Stolee

Re: [PATCH v2 6/6] treewide: always have a valid "index_state.repo" member

[Derrick Stolee]

On 1/12/2023 7:55 AM, Ævar Arnfjörð Bjarmason wrote:

> We're doing this by making use of the INDEX_STATE_INIT() macro (and
> corresponding function) added in the preceding commit, which now have
> mandatory "repo" arguments. As seen here there are cases where we set
> it to NULL, but only if we're about to fill in the correct non-NULL
> "repo" right afterwards.

The changes in part 5 to depend on INDEX_STATE_INIT and
index_state_init() make the changes in this patch much more
mechanical and easy to follow.

Thanks,
-Stolee

Re: [PATCH v2 0/6] cache API: always have a "istate->repo"

[Derrick Stolee]

On 1/12/2023 7:55 AM, Ævar Arnfjörð Bjarmason wrote:

>  * Derrick suggested in
>    https://lore.kernel.org/git/6b92fad2-6b74-fddb-679c-8c8735e7103d@github.com/
>    that things might be nicer if we had an explicit initializer, which
>    was also the subject of the previous discussion at
>    https://lore.kernel.org/git/xmqqmt6vqo2w.fsf@gitster.g/; but
>    concluded that it was probably best to leave it for now.
> 
>    I tried it out, and I think it's worth just doing that now, which
>    is why there's a new 5/6 here: We start by adding an
>    INDEX_STATE_INIT macro, and corresponding function.
> 
>    There's a bit of churn in 6/6 as all of those now will take a
>    "repo" argument, but I think the end result is worth it, because
>    even if "repo" remains the only thing that we need to initialize
>    we're now able to use ALLOC_ARRAY() instead of CALLOC_ARRAY().
> 
>    We'll thus be helped by analysis tools (which would show access to
>    un-init'd memory) if we miss properly init-ing not just the "repo"
>    field, but anything in the structure, so our test coverage will be
>    better.
> 
>    It also makes the code easier to follow and change, as it's now
>    more obvious where we initialize this, and it'll be easier to
>    change it in the future if e.g. we add a new member that has
>    mandatory initialization (e.g. a "struct strbuf").

I wasn't expecting the initializer idea to work as well as it has.
Now, Patch 5 does all the heavy lifting and Patch 6 is an easy read.

Outside of one question about the istate->initialized member (which
might not need any change) I'm happy with this version.

Thanks,
-Stolee

Re: [PATCH v2 5/6] cache API: add a "INDEX_STATE_INIT" macro/function, add release_index()

[Ævar Arnfjörð Bjarmason]

On Thu, Jan 12 2023, Derrick Stolee wrote:

> On 1/12/2023 7:55 AM, Ævar Arnfjörð Bjarmason wrote:
>> As we'll see in a subsequent commit we'll get advantages from always
>> initializing the "repo" member of the "struct index_state". To make
>> that easier let's introduce an initialization macro & function.
>> 
>> The various ad-hoc initialization of the structure can then be changed
>> over to it, and we can remove the various "0" assignments in
>> discard_index() in favor of calling index_state_init() at the end.
>
>> -	memset(&o->result, 0, sizeof(o->result));
>> +	index_state_init(&o->result);
>>  	o->result.initialized = 1;
>
> It's interesting that 'struct index_state' has an 'initialized'
> member that we aren't setting in index_state_init(). Perhaps it's
> only being used in special cases like this, and means something
> more specific than "index_state_init() was run"? Or maybe we
> could add it to INDEX_STATE_INIT and drop this line?

It's unrelated, and doing that would be a bug. It's a bit unfortunately
named, a better name might be "read_index_data" or something.

It was added in 913e0e99b6a (unpack_trees(): protect the handcrafted
in-core index from read_cache(), 2008-08-23), which shows the use-case,
i.e. it's for avoiding re-reading the index file itself (or in that
case, to trust our hand-crafted faked-up version of it).

I opted not to mention it in the commit message, after being
sufficiently convinced that it was unrelated, which was probably a
mistake :)

Just as a sanity check, we do have really good test coverage of the
difference, at least 1/2 of the tests I bothered to wait for failed when
I tried this on top:

diff --git a/cache.h b/cache.h
index 4bf14e0bd94..1f8e5f4e823 100644
--- a/cache.h
+++ b/cache.h
@@ -371,6 +371,7 @@ struct index_state {
  * "r" argument to index_state_init() in that case.
  */
 #define INDEX_STATE_INIT(r) { \
+	.initialized = 1, \
 	.repo = (r), \
 }
 void index_state_init(struct index_state *istate, struct repository *r);

Re: [PATCH v2 5/6] cache API: add a "INDEX_STATE_INIT" macro/function, add release_index()

[Derrick Stolee]

On 1/12/2023 11:19 AM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Thu, Jan 12 2023, Derrick Stolee wrote:
>> It's interesting that 'struct index_state' has an 'initialized'
>> member that we aren't setting in index_state_init(). Perhaps it's
>> only being used in special cases like this, and means something
>> more specific than "index_state_init() was run"? Or maybe we
>> could add it to INDEX_STATE_INIT and drop this line?
> 
> It's unrelated, and doing that would be a bug. It's a bit unfortunately
> named, a better name might be "read_index_data" or something.
> 
> It was added in 913e0e99b6a (unpack_trees(): protect the handcrafted
> in-core index from read_cache(), 2008-08-23), which shows the use-case,
> i.e. it's for avoiding re-reading the index file itself (or in that
> case, to trust our hand-crafted faked-up version of it).
> 
> I opted not to mention it in the commit message, after being
> sufficiently convinced that it was unrelated, which was probably a
> mistake :)
> 
> Just as a sanity check, we do have really good test coverage of the
> difference, at least 1/2 of the tests I bothered to wait for failed when
> I tried this on top:
> 
> diff --git a/cache.h b/cache.h
> index 4bf14e0bd94..1f8e5f4e823 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -371,6 +371,7 @@ struct index_state {
>   * "r" argument to index_state_init() in that case.
>   */
>  #define INDEX_STATE_INIT(r) { \
> +	.initialized = 1, \
>  	.repo = (r), \

Thanks for the extra info. The patch is clearly correct with that
information.

Thanks,
-Stolee

Re: [PATCH v2 0/6] cache API: always have a "istate->repo"

[Junio C Hamano]

Derrick Stolee <derrickstolee@github.com> writes:

> I wasn't expecting the initializer idea to work as well as it has.
> Now, Patch 5 does all the heavy lifting and Patch 6 is an easy read.
>
> Outside of one question about the istate->initialized member (which
> might not need any change) I'm happy with this version.

Thanks, both.  And especially for finding 913e0e99 (unpack_trees():
protect the handcrafted in-core index from read_cache(),
2008-08-23).

Will queue.

Re: [PATCH v2 0/6] cache API: always have a "istate->repo"

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> The "struct index_state" contains a "repo" member, which should be a
> pointer to the repository for that index, but which due to us
> constructing such structs on an ad-hoc basis in various places wasn't
> always available.

I'd exclude 6/6 for now, as it seems to depend on some changes only
in 'next'.  Feel free to resend only that step with reduced scope so
that it applies to 'master', and send in incremental updates when
each of these topics that are only in 'next' graduates.

Thanks.

Re: [PATCH v2 0/6] cache API: always have a "istate->repo"

[Ævar Arnfjörð Bjarmason]

On Fri, Jan 13 2023, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> The "struct index_state" contains a "repo" member, which should be a
>> pointer to the repository for that index, but which due to us
>> constructing such structs on an ad-hoc basis in various places wasn't
>> always available.
>
> I'd exclude 6/6 for now, as it seems to depend on some changes only
> in 'next'.  Feel free to resend only that step with reduced scope so
> that it applies to 'master', and send in incremental updates when
> each of these topics that are only in 'next' graduates.

Okey, the 6/6 requires ds/omit-trailing-hash-in-index. As both it and
the 1-5/6 of this are in "next" now I think it's best that I just submit
the 6/6 stand-alone after both of those have graduated.

Re: [PATCH v2 0/6] cache API: always have a "istate->repo"

[Philip Oakley]

On 16/01/2023 13:38, Ævar Arnfjörð Bjarmason wrote:
> On Fri, Jan 13 2023, Junio C Hamano wrote:
>
>> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>>
>>> The "struct index_state" contains a "repo" member, which should be a
>>> pointer to the repository for that index, but which due to us
>>> constructing such structs on an ad-hoc basis in various places wasn't
>>> always available.
>> I'd exclude 6/6 for now, as it seems to depend on some changes only
>> in 'next'.  Feel free to resend only that step with reduced scope so
>> that it applies to 'master', and send in incremental updates when
>> each of these topics that are only in 'next' graduates.
> Okey, the 6/6 requires ds/omit-trailing-hash-in-index. As both it and
> the 1-5/6 of this are in "next" now I think it's best that I just submit
> the 6/6 stand-alone after both of those have graduated.
>
micro-nit: The commit message of 5/6 starts "As we'll see in a
subsequent commit..", which may need a slight tweak if 6/6 becomes 'far
away' in the commit tree.

--
Philip

Re: [PATCH v2 0/6] cache API: always have a "istate->repo"

[Junio C Hamano]

Philip Oakley <philipoakley@iee.email> writes:

> On 16/01/2023 13:38, Ævar Arnfjörð Bjarmason wrote:
>> On Fri, Jan 13 2023, Junio C Hamano wrote:
>>
>>> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>>>
>>>> The "struct index_state" contains a "repo" member, which should be a
>>>> pointer to the repository for that index, but which due to us
>>>> constructing such structs on an ad-hoc basis in various places wasn't
>>>> always available.
>>> I'd exclude 6/6 for now, as it seems to depend on some changes only
>>> in 'next'.  Feel free to resend only that step with reduced scope so
>>> that it applies to 'master', and send in incremental updates when
>>> each of these topics that are only in 'next' graduates.
>> Okey, the 6/6 requires ds/omit-trailing-hash-in-index. As both it and
>> the 1-5/6 of this are in "next" now I think it's best that I just submit
>> the 6/6 stand-alone after both of those have graduated.
>>
> micro-nit: The commit message of 5/6 starts "As we'll see in a
> subsequent commit..", which may need a slight tweak if 6/6 becomes 'far
> away' in the commit tree.

Indeed.  

I'd use 

	Hopefully in some not so distant future, we'll get
	advantages from always initializing the "repo" member ...

for now.

[PATCH] treewide: always have a valid "index_state.repo" member

[Ævar Arnfjörð Bjarmason]

When the "repo" member was added to "the_index" in [1] the
repo_read_index() was made to populate it, but the unpopulated
"the_index" variable didn't get the same treatment.

Let's do that in initialize_the_repository() when we set it up, and
likewise for all of the current callers initialized an empty "struct
index_state".

This simplifies code that needs to deal with "the_index" or a custom
"struct index_state", we no longer need to second-guess this part of
the "index_state" deep in the stack. A recent example of such
second-guessing is the "istate->repo ? istate->repo : the_repository"
code in [2]. We can now simply use "istate->repo".

We're doing this by making use of the INDEX_STATE_INIT() macro (and
corresponding function) added in [3], which now have mandatory "repo"
arguments.

Because we now call index_state_init() in repository.c's
initialize_the_repository() we don't need to handle the case where we
have a "repo->index" whose "repo" member doesn't match the "repo"
we're setting up, i.e. the "Complete the double-reference" code in
repo_read_index() being altered here. That logic was originally added
in [1], and was working around the lack of what we now have in
initialize_the_repository().

For "fsmonitor-settings.c" we can remove the initialization of a NULL
"r" argument to "the_repository". This was added back in [4], and was
needed at the time for callers that would pass us the "r" from an
"istate->repo". Before this change such a change to
"fsmonitor-settings.c" would segfault all over the test suite (e.g. in
t0002-gitfile.sh).

This change has wider eventual implications for
"fsmonitor-settings.c". The reason the other lazy loading behavior in
it is required (starting with "if (!r->settings.fsmonitor) ..." is
because of the previously passed "r" being "NULL".

I have other local changes on top of this which move its configuration
reading to "prepare_repo_settings()" in "repo-settings.c", as we could
now start to rely on it being called for our "r". But let's leave all
of that for now, and narrowly remove this particular part of the
lazy-loading.

1. 1fd9ae517c4 (repository: add repo reference to index_state,
   2021-01-23)
2. ee1f0c242ef (read-cache: add index.skipHash config option,
   2023-01-06)
3. 2f6b1eb794e (cache API: add a "INDEX_STATE_INIT" macro/function,
   add release_index(), 2023-01-12)
4. 1e0ea5c4316 (fsmonitor: config settings are repository-specific,
   2022-03-25)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

This an updated verison of the 6/6 of [A], which per Junio's [B]
wasn't picked up with those patches, which are now in "next".

Junio: Now that Derrick's ds/omit-trailing-hash-in-index has landed on
"master" this can be applied on top a merge of "master" and what you
have in "ab/cache-api-cleanup" (that topic itself being based on a
too-old "master").

Since the v2 I changed the "Complete the double-reference" logic in
repo_read_index() so that we're not working around a state of a
affairs that no longer exists with this change.

A. https://lore.kernel.org/git/cover-v2-0.6-00000000000-20230112T124842Z-avarab@gmail.com/
B. https://lore.kernel.org/git/xmqqtu0u2q9u.fsf@gitster.g/

Range-diff:
1:  291233b0420 ! 1:  b4998652822 treewide: always have a valid "index_state.repo" member
    @@ Commit message
         code in [2]. We can now simply use "istate->repo".
     
         We're doing this by making use of the INDEX_STATE_INIT() macro (and
    -    corresponding function) added in the preceding commit, which now have
    -    mandatory "repo" arguments. As seen here there are cases where we set
    -    it to NULL, but only if we're about to fill in the correct non-NULL
    -    "repo" right afterwards.
    +    corresponding function) added in [3], which now have mandatory "repo"
    +    arguments.
    +
    +    Because we now call index_state_init() in repository.c's
    +    initialize_the_repository() we don't need to handle the case where we
    +    have a "repo->index" whose "repo" member doesn't match the "repo"
    +    we're setting up, i.e. the "Complete the double-reference" code in
    +    repo_read_index() being altered here. That logic was originally added
    +    in [1], and was working around the lack of what we now have in
    +    initialize_the_repository().
     
         For "fsmonitor-settings.c" we can remove the initialization of a NULL
    -    "r" argument to "the_repository". This was added back in [3], and was
    +    "r" argument to "the_repository". This was added back in [4], and was
         needed at the time for callers that would pass us the "r" from an
         "istate->repo". Before this change such a change to
         "fsmonitor-settings.c" would segfault all over the test suite (e.g. in
    @@ Commit message
            2021-01-23)
         2. ee1f0c242ef (read-cache: add index.skipHash config option,
            2023-01-06)
    -    3. 1e0ea5c4316 (fsmonitor: config settings are repository-specific,
    +    3. 2f6b1eb794e (cache API: add a "INDEX_STATE_INIT" macro/function,
    +       add release_index(), 2023-01-12)
    +    4. 1e0ea5c4316 (fsmonitor: config settings are repository-specific,
            2022-03-25)
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
    @@ repository.c: void initialize_the_repository(void)
      }
      
     @@ repository.c: int repo_read_index(struct repository *repo)
    + {
    + 	int res;
      
    ++	/* Complete the double-reference */
      	if (!repo->index) {
      		ALLOC_ARRAY(repo->index, 1);
     -		index_state_init(repo->index);
    -+		index_state_init(repo->index, NULL /* set below */);
    - 	}
    +-	}
    +-
    +-	/* Complete the double-reference */
    +-	if (!repo->index->repo)
    +-		repo->index->repo = repo;
    +-	else if (repo->index->repo != repo)
    ++		index_state_init(repo->index, repo);
    ++	} else if (repo->index->repo != repo) {
    + 		BUG("repo's index should point back at itself");
    ++	}
    + 
    + 	res = read_index_from(repo->index, repo->index_file, repo->gitdir);
      
    - 	/* Complete the double-reference */
     
      ## revision.c ##
     @@ revision.c: void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)

 apply.c                   |  2 +-
 builtin/difftool.c        |  2 +-
 builtin/sparse-checkout.c |  2 +-
 builtin/stash.c           |  8 ++++----
 builtin/worktree.c        |  2 +-
 cache.h                   |  9 ++++++---
 fsmonitor-settings.c      | 14 --------------
 fsmonitor.c               |  2 +-
 merge-recursive.c         |  2 +-
 read-cache.c              | 17 +++++------------
 repository.c              | 13 ++++++-------
 revision.c                |  2 +-
 sparse-index.c            |  9 ---------
 split-index.c             |  2 +-
 unpack-trees.c            |  2 +-
 15 files changed, 30 insertions(+), 58 deletions(-)

diff --git a/apply.c b/apply.c
index 821fe411ec8..5eec433583e 100644
--- a/apply.c
+++ b/apply.c
@@ -4105,7 +4105,7 @@ static int preimage_oid_in_gitlink_patch(struct patch *p, struct object_id *oid)
 static int build_fake_ancestor(struct apply_state *state, struct patch *list)
 {
 	struct patch *patch;
-	struct index_state result = INDEX_STATE_INIT;
+	struct index_state result = INDEX_STATE_INIT(state->repo);
 	struct lock_file lock = LOCK_INIT;
 	int res;
 
diff --git a/builtin/difftool.c b/builtin/difftool.c
index 758e7bd3b6a..dbbfb19f192 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -361,7 +361,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 	struct hashmap symlinks2 = HASHMAP_INIT(pair_cmp, NULL);
 	struct hashmap_iter iter;
 	struct pair_entry *entry;
-	struct index_state wtindex = INDEX_STATE_INIT;
+	struct index_state wtindex = INDEX_STATE_INIT(the_repository);
 	struct checkout lstate, rstate;
 	int err = 0;
 	struct child_process cmd = CHILD_PROCESS_INIT;
diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index dc332c6d05f..c3738154918 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -217,7 +217,7 @@ static int update_working_directory(struct pattern_list *pl)
 	o.head_idx = -1;
 	o.src_index = r->index;
 	o.dst_index = r->index;
-	index_state_init(&o.result);
+	index_state_init(&o.result, r);
 	o.skip_sparse_checkout = 0;
 	o.pl = pl;
 
diff --git a/builtin/stash.c b/builtin/stash.c
index a5f13fb1e9f..839569a9803 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1137,7 +1137,7 @@ static int save_untracked_files(struct stash_info *info, struct strbuf *msg,
 	int ret = 0;
 	struct strbuf untracked_msg = STRBUF_INIT;
 	struct child_process cp_upd_index = CHILD_PROCESS_INIT;
-	struct index_state istate = INDEX_STATE_INIT;
+	struct index_state istate = INDEX_STATE_INIT(the_repository);
 
 	cp_upd_index.git_cmd = 1;
 	strvec_pushl(&cp_upd_index.args, "update-index", "-z", "--add",
@@ -1176,7 +1176,7 @@ static int stash_staged(struct stash_info *info, struct strbuf *out_patch,
 {
 	int ret = 0;
 	struct child_process cp_diff_tree = CHILD_PROCESS_INIT;
-	struct index_state istate = INDEX_STATE_INIT;
+	struct index_state istate = INDEX_STATE_INIT(the_repository);
 
 	if (write_index_as_tree(&info->w_tree, &istate, the_repository->index_file,
 				0, NULL)) {
@@ -1209,7 +1209,7 @@ static int stash_patch(struct stash_info *info, const struct pathspec *ps,
 	int ret = 0;
 	struct child_process cp_read_tree = CHILD_PROCESS_INIT;
 	struct child_process cp_diff_tree = CHILD_PROCESS_INIT;
-	struct index_state istate = INDEX_STATE_INIT;
+	struct index_state istate = INDEX_STATE_INIT(the_repository);
 	char *old_index_env = NULL, *old_repo_index_file;
 
 	remove_path(stash_index_path.buf);
@@ -1271,7 +1271,7 @@ static int stash_working_tree(struct stash_info *info, const struct pathspec *ps
 	struct rev_info rev;
 	struct child_process cp_upd_index = CHILD_PROCESS_INIT;
 	struct strbuf diff_output = STRBUF_INIT;
-	struct index_state istate = INDEX_STATE_INIT;
+	struct index_state istate = INDEX_STATE_INIT(the_repository);
 
 	init_revisions(&rev, NULL);
 	copy_pathspec(&rev.prune_data, ps);
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 311d6e90750..f51c40f1e1e 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -923,7 +923,7 @@ static int unlock_worktree(int ac, const char **av, const char *prefix)
 
 static void validate_no_submodules(const struct worktree *wt)
 {
-	struct index_state istate = INDEX_STATE_INIT;
+	struct index_state istate = INDEX_STATE_INIT(the_repository);
 	struct strbuf path = STRBUF_INIT;
 	int i, found_submodules = 0;
 
diff --git a/cache.h b/cache.h
index be28fce12ac..4bf14e0bd94 100644
--- a/cache.h
+++ b/cache.h
@@ -367,10 +367,13 @@ struct index_state {
  * If the variable won't be used again, use release_index() to free()
  * its resources. If it needs to be used again use discard_index(),
  * which does the same thing, but will use use index_state_init() at
- * the end.
+ * the end. The discard_index() will use its own "istate->repo" as the
+ * "r" argument to index_state_init() in that case.
  */
-#define INDEX_STATE_INIT { 0 }
-void index_state_init(struct index_state *istate);
+#define INDEX_STATE_INIT(r) { \
+	.repo = (r), \
+}
+void index_state_init(struct index_state *istate, struct repository *r);
 void release_index(struct index_state *istate);
 
 /* Name hashing */
diff --git a/fsmonitor-settings.c b/fsmonitor-settings.c
index ee63a97dc51..899bfe9c813 100644
--- a/fsmonitor-settings.c
+++ b/fsmonitor-settings.c
@@ -143,8 +143,6 @@ static void lookup_fsmonitor_settings(struct repository *r)
 
 enum fsmonitor_mode fsm_settings__get_mode(struct repository *r)
 {
-	if (!r)
-		r = the_repository;
 	if (!r->settings.fsmonitor)
 		lookup_fsmonitor_settings(r);
 
@@ -153,8 +151,6 @@ enum fsmonitor_mode fsm_settings__get_mode(struct repository *r)
 
 const char *fsm_settings__get_hook_path(struct repository *r)
 {
-	if (!r)
-		r = the_repository;
 	if (!r->settings.fsmonitor)
 		lookup_fsmonitor_settings(r);
 
@@ -174,8 +170,6 @@ void fsm_settings__set_ipc(struct repository *r)
 	 * Caller requested IPC explicitly, so avoid (possibly
 	 * recursive) config lookup.
 	 */
-	if (!r)
-		r = the_repository;
 	if (!r->settings.fsmonitor)
 		r->settings.fsmonitor = alloc_settings();
 
@@ -197,8 +191,6 @@ void fsm_settings__set_hook(struct repository *r, const char *path)
 	 * Caller requested hook explicitly, so avoid (possibly
 	 * recursive) config lookup.
 	 */
-	if (!r)
-		r = the_repository;
 	if (!r->settings.fsmonitor)
 		r->settings.fsmonitor = alloc_settings();
 
@@ -210,8 +202,6 @@ void fsm_settings__set_hook(struct repository *r, const char *path)
 
 void fsm_settings__set_disabled(struct repository *r)
 {
-	if (!r)
-		r = the_repository;
 	if (!r->settings.fsmonitor)
 		r->settings.fsmonitor = alloc_settings();
 
@@ -223,8 +213,6 @@ void fsm_settings__set_disabled(struct repository *r)
 void fsm_settings__set_incompatible(struct repository *r,
 				    enum fsmonitor_reason reason)
 {
-	if (!r)
-		r = the_repository;
 	if (!r->settings.fsmonitor)
 		r->settings.fsmonitor = alloc_settings();
 
@@ -235,8 +223,6 @@ void fsm_settings__set_incompatible(struct repository *r,
 
 enum fsmonitor_reason fsm_settings__get_reason(struct repository *r)
 {
-	if (!r)
-		r = the_repository;
 	if (!r->settings.fsmonitor)
 		lookup_fsmonitor_settings(r);
 
diff --git a/fsmonitor.c b/fsmonitor.c
index 08af00c7387..a5b9e75437b 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -304,7 +304,7 @@ void refresh_fsmonitor(struct index_state *istate)
 	char *buf;
 	unsigned int i;
 	int is_trivial = 0;
-	struct repository *r = istate->repo ? istate->repo : the_repository;
+	struct repository *r = istate->repo;
 	enum fsmonitor_mode fsm_mode = fsm_settings__get_mode(r);
 	enum fsmonitor_reason reason = fsm_settings__get_reason(r);
 
diff --git a/merge-recursive.c b/merge-recursive.c
index 0948b3ea961..ae469f8cc81 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -412,7 +412,7 @@ static int unpack_trees_start(struct merge_options *opt,
 {
 	int rc;
 	struct tree_desc t[3];
-	struct index_state tmp_index = INDEX_STATE_INIT;
+	struct index_state tmp_index = INDEX_STATE_INIT(opt->repo);
 
 	memset(&opt->priv->unpack_opts, 0, sizeof(opt->priv->unpack_opts));
 	if (opt->priv->call_depth)
diff --git a/read-cache.c b/read-cache.c
index d191741f600..7bd12afb38c 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2300,8 +2300,6 @@ static void set_new_index_sparsity(struct index_state *istate)
 	 * If the index's repo exists, mark it sparse according to
 	 * repo settings.
 	 */
-	if (!istate->repo)
-		return;
 	prepare_repo_settings(istate->repo);
 	if (!istate->repo->settings.command_requires_full_index &&
 	    is_sparse_index_allowed(istate, 0))
@@ -2330,8 +2328,6 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
 	fd = open(path, O_RDONLY);
 	if (fd < 0) {
 		if (!must_exist && errno == ENOENT) {
-			if (!istate->repo)
-				istate->repo = the_repository;
 			set_new_index_sparsity(istate);
 			return 0;
 		}
@@ -2433,9 +2429,6 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
 	trace2_data_intmax("index", the_repository, "read/cache_nr",
 			   istate->cache_nr);
 
-	if (!istate->repo)
-		istate->repo = the_repository;
-
 	/*
 	 * If the command explicitly requires a full index, force it
 	 * to be full. Otherwise, correct the sparsity based on repository
@@ -2501,7 +2494,7 @@ int read_index_from(struct index_state *istate, const char *path,
 		release_index(split_index->base);
 	else
 		ALLOC_ARRAY(split_index->base, 1);
-	index_state_init(split_index->base);
+	index_state_init(split_index->base, istate->repo);
 
 	base_oid_hex = oid_to_hex(&split_index->base_oid);
 	base_path = xstrfmt("%s/sharedindex.%s", gitdir, base_oid_hex);
@@ -2540,9 +2533,9 @@ int is_index_unborn(struct index_state *istate)
 	return (!istate->cache_nr && !istate->timestamp.sec);
 }
 
-void index_state_init(struct index_state *istate)
+void index_state_init(struct index_state *istate, struct repository *r)
 {
-	struct index_state blank = INDEX_STATE_INIT;
+	struct index_state blank = INDEX_STATE_INIT(r);
 	memcpy(istate, &blank, sizeof(*istate));
 }
 
@@ -2579,7 +2572,7 @@ void release_index(struct index_state *istate)
 void discard_index(struct index_state *istate)
 {
 	release_index(istate);
-	index_state_init(istate);
+	index_state_init(istate, istate->repo);
 }
 
 /*
@@ -2933,7 +2926,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 	int ieot_entries = 1;
 	struct index_entry_offset_table *ieot = NULL;
 	int nr, nr_threads;
-	struct repository *r = istate->repo ? istate->repo : the_repository;
+	struct repository *r = istate->repo;
 
 	f = hashfd(tempfile->fd, tempfile->filename.buf);
 
diff --git a/repository.c b/repository.c
index a5805fa33b1..937fa974b38 100644
--- a/repository.c
+++ b/repository.c
@@ -28,6 +28,8 @@ void initialize_the_repository(void)
 	the_repo.remote_state = remote_state_new();
 	the_repo.parsed_objects = parsed_object_pool_new();
 
+	index_state_init(&the_index, the_repository);
+
 	repo_set_hash_algo(&the_repo, GIT_HASH_SHA1);
 }
 
@@ -302,16 +304,13 @@ int repo_read_index(struct repository *repo)
 {
 	int res;
 
+	/* Complete the double-reference */
 	if (!repo->index) {
 		ALLOC_ARRAY(repo->index, 1);
-		index_state_init(repo->index);
-	}
-
-	/* Complete the double-reference */
-	if (!repo->index->repo)
-		repo->index->repo = repo;
-	else if (repo->index->repo != repo)
+		index_state_init(repo->index, repo);
+	} else if (repo->index->repo != repo) {
 		BUG("repo's index should point back at itself");
+	}
 
 	res = read_index_from(repo->index, repo->index_file, repo->gitdir);
 
diff --git a/revision.c b/revision.c
index fb090886f5b..21f5f572c22 100644
--- a/revision.c
+++ b/revision.c
@@ -1813,7 +1813,7 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)
 	worktrees = get_worktrees();
 	for (p = worktrees; *p; p++) {
 		struct worktree *wt = *p;
-		struct index_state istate = INDEX_STATE_INIT;
+		struct index_state istate = INDEX_STATE_INIT(revs->repo);
 
 		if (wt->is_current)
 			continue; /* current index already taken care of */
diff --git a/sparse-index.c b/sparse-index.c
index 86e3b99870b..147a13386a4 100644
--- a/sparse-index.c
+++ b/sparse-index.c
@@ -128,9 +128,6 @@ int is_sparse_index_allowed(struct index_state *istate, int flags)
 	if (!core_apply_sparse_checkout || !core_sparse_checkout_cone)
 		return 0;
 
-	if (!istate->repo)
-		istate->repo = the_repository;
-
 	if (!(flags & SPARSE_INDEX_MEMORY_ONLY)) {
 		int test_env;
 
@@ -327,9 +324,6 @@ void expand_index(struct index_state *istate, struct pattern_list *pl)
 			pl = NULL;
 	}
 
-	if (!istate->repo)
-		istate->repo = the_repository;
-
 	/*
 	 * A NULL pattern set indicates we are expanding a full index, so
 	 * we use a special region name that indicates the full expansion.
@@ -552,9 +546,6 @@ void expand_to_path(struct index_state *istate,
 	if (!istate->sparse_index)
 		return;
 
-	if (!istate->repo)
-		istate->repo = the_repository;
-
 	in_expand_to_path = 1;
 
 	/*
diff --git a/split-index.c b/split-index.c
index a5b56c0c37f..5d0f04763ea 100644
--- a/split-index.c
+++ b/split-index.c
@@ -91,7 +91,7 @@ void move_cache_to_base_index(struct index_state *istate)
 	}
 
 	ALLOC_ARRAY(si->base, 1);
-	index_state_init(si->base);
+	index_state_init(si->base, istate->repo);
 	si->base->version = istate->version;
 	/* zero timestamp disables racy test in ce_write_index() */
 	si->base->timestamp = istate->timestamp;
diff --git a/unpack-trees.c b/unpack-trees.c
index d1d1b0f319e..3d05e45a279 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1905,7 +1905,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 		populate_from_existing_patterns(o, &pl);
 	}
 
-	index_state_init(&o->result);
+	index_state_init(&o->result, o->src_index->repo);
 	o->result.initialized = 1;
 	o->result.timestamp.sec = o->src_index->timestamp.sec;
 	o->result.timestamp.nsec = o->src_index->timestamp.nsec;
-- 
2.39.0.1225.g30a3d88132d

Re: [PATCH] treewide: always have a valid "index_state.repo" member

[Derrick Stolee]

On 1/17/2023 8:57 AM, Ævar Arnfjörð Bjarmason wrote:
 
> This an updated verison of the 6/6 of [A], which per Junio's [B]
> wasn't picked up with those patches, which are now in "next".
> 
> Junio: Now that Derrick's ds/omit-trailing-hash-in-index has landed on
> "master" this can be applied on top a merge of "master" and what you
> have in "ab/cache-api-cleanup" (that topic itself being based on a
> too-old "master").
> 
> Since the v2 I changed the "Complete the double-reference" logic in
> repo_read_index() so that we're not working around a state of a
> affairs that no longer exists with this change.
> 
> A. https://lore.kernel.org/git/cover-v2-0.6-00000000000-20230112T124842Z-avarab@gmail.com/
> B. https://lore.kernel.org/git/xmqqtu0u2q9u.fsf@gitster.g/
> 
> Range-diff:

These changes look good to me. Thanks for dealing with the branch-
hopping.

Thanks,
-Stolee