Sparse Index: log why the index is being expanded (#691)

dscho · dscho · commit bd9a7557d1a0 · 2025-01-01T17:12:33.000+01:00
I will intend to send this upstream after the 2.47.0 release cycle, but
this should get to our microsoft/git users for maximum impact.

Customers have been struggling with explaining why the sparse index
expansion advice message is showing up. The advice to run 'git clean'
has not always helped folks, and sometimes it is very unclear why we are
running into trouble.

These changes introduce a way to log a reason for the expansion into the
trace2 logs so it can be found by requesting that a user enable tracing.

While testing this, I created the most standard case that happens, which
is to have an existing directory match a sparse directory in the index.
In this case, it showed that two log messages were required. See the
last commit for this new log message. Together, these two places show
this kind of message in the `GIT_TRACE2_PERF` output (trimmed for
clarity):

```
region_enter | index        | label:clear_skip_worktree_from_present_files_sparse
data         | sparse-index | ..skip-worktree sparsedir:&lt;my-sparse-path&gt;/
data         | index        | ..sparse_path_count:362
data         | index        | ..sparse_lstat_count:732
region_leave | index        | label:clear_skip_worktree_from_present_files_sparse
data         | sparse-index | expansion-reason:failed to clear skip-worktree while sparse
```

I added some tests to demonstrate that these logs are recorded, but it
also seems difficult to hit some of these cases.
diff --git a/Documentation/technical/sparse-index.txt b/Documentation/technical/sparse-index.txt
@@ -206,3 +206,10 @@ Here are some commands that might be useful to update:
 * `git am`
 * `git clean`
 * `git stash`
+
+In order to help identify the cases where remaining index expansion is
+occurring in user machines, calls to `ensure_full_index()` have been
+replaced with `ensure_full_index_with_reason()` or with
+`ensure_full_index_unaudited()`. These versions add tracing that should
+help identify the reason for the index expansion without needing full
+access to someone's repository.
diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
@@ -156,7 +156,8 @@ static int checkout_all(const char *prefix, int prefix_length)
 			 * first entry inside the expanded sparse directory).
 			 */
 			if (ignore_skip_worktree) {
-				ensure_full_index(the_repository->index);
+				ensure_full_index_with_reason(the_repository->index,
+							      "checkout-index");
 				ce = the_repository->index->cache[i];
 			}
 		}
diff --git a/builtin/commit.c b/builtin/commit.c
@@ -385,7 +385,7 @@ static int list_paths(struct string_list *list, const char *with_tree,
 	}
 
 	/* TODO: audit for interaction with sparse-index. */
-	ensure_full_index(the_repository->index);
+	ensure_full_index_unaudited(the_repository->index);
 	for (i = 0; i < the_repository->index->cache_nr; i++) {
 		const struct cache_entry *ce = the_repository->index->cache[i];
 		struct string_list_item *item;
@@ -1133,7 +1133,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 			int i, ita_nr = 0;
 
 			/* TODO: audit for interaction with sparse-index. */
-			ensure_full_index(the_repository->index);
+			ensure_full_index_unaudited(the_repository->index);
 			for (i = 0; i < the_repository->index->cache_nr; i++)
 				if (ce_intent_to_add(the_repository->index->cache[i]))
 					ita_nr++;
diff --git a/builtin/difftool.c b/builtin/difftool.c
@@ -592,7 +592,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 	ret = run_command(&cmd);
 
 	/* TODO: audit for interaction with sparse-index. */
-	ensure_full_index(&wtindex);
+	ensure_full_index_unaudited(&wtindex);
 
 	/*
 	 * If the diff includes working copy files and those
diff --git a/builtin/fsck.c b/builtin/fsck.c
@@ -821,7 +821,7 @@ static void fsck_index(struct index_state *istate, const char *index_path,
 	unsigned int i;
 
 	/* TODO: audit for interaction with sparse-index. */
-	ensure_full_index(istate);
+	ensure_full_index_unaudited(istate);
 	for (i = 0; i < istate->cache_nr; i++) {
 		unsigned int mode;
 		struct blob *blob;
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
@@ -413,7 +413,7 @@ static void show_files(struct repository *repo, struct dir_struct *dir)
 		return;
 
 	if (!show_sparse_dirs)
-		ensure_full_index(repo->index);
+		ensure_full_index_with_reason(repo->index, "ls-files");
 
 	for (i = 0; i < repo->index->cache_nr; i++) {
 		const struct cache_entry *ce = repo->index->cache[i];
diff --git a/builtin/merge-index.c b/builtin/merge-index.c
@@ -66,7 +66,7 @@ static void merge_all(void)
 {
 	int i;
 	/* TODO: audit for interaction with sparse-index. */
-	ensure_full_index(the_repository->index);
+	ensure_full_index_unaudited(the_repository->index);
 	for (i = 0; i < the_repository->index->cache_nr; i++) {
 		const struct cache_entry *ce = the_repository->index->cache[i];
 		if (!ce_stage(ce))
@@ -93,7 +93,7 @@ int cmd_merge_index(int argc,
 	repo_read_index(the_repository);
 
 	/* TODO: audit for interaction with sparse-index. */
-	ensure_full_index(the_repository->index);
+	ensure_full_index_unaudited(the_repository->index);
 
 	i = 1;
 	if (!strcmp(argv[i], "-o")) {
diff --git a/builtin/read-tree.c b/builtin/read-tree.c
@@ -226,7 +226,8 @@ int cmd_read_tree(int argc,
 		setup_work_tree();
 
 	if (opts.skip_sparse_checkout)
-		ensure_full_index(the_repository->index);
+		ensure_full_index_with_reason(the_repository->index,
+					      "read-tree");
 
 	if (opts.merge) {
 		switch (stage - 1) {
diff --git a/builtin/reset.c b/builtin/reset.c
@@ -262,7 +262,8 @@ static int read_from_tree(const struct pathspec *pathspec,
 	opt.add_remove = diff_addremove;
 
 	if (pathspec->nr && pathspec_needs_expanded_index(the_repository->index, pathspec))
-		ensure_full_index(the_repository->index);
+		ensure_full_index_with_reason(the_repository->index,
+					      "reset pathspec");
 
 	if (do_diff_cache(tree_oid, &opt))
 		return 1;
diff --git a/builtin/rm.c b/builtin/rm.c
@@ -313,7 +313,8 @@ int cmd_rm(int argc,
 	seen = xcalloc(pathspec.nr, 1);
 
 	if (pathspec_needs_expanded_index(the_repository->index, &pathspec))
-		ensure_full_index(the_repository->index);
+		ensure_full_index_with_reason(the_repository->index,
+					      "rm pathspec");
 
 	for (i = 0; i < the_repository->index->cache_nr; i++) {
 		const struct cache_entry *ce = the_repository->index->cache[i];
diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
@@ -207,7 +207,8 @@ static void clean_tracked_sparse_directories(struct repository *r)
 	strbuf_release(&path);
 
 	if (was_full)
-		ensure_full_index(r->index);
+		ensure_full_index_with_reason(r->index,
+				"sparse-checkout:was full");
 }
 
 static int update_working_directory(struct pattern_list *pl)
@@ -437,7 +438,8 @@ static int update_modes(int *cone_mode, int *sparse_index)
 		the_repository->index->updated_workdir = 1;
 
 		if (!*sparse_index)
-			ensure_full_index(the_repository->index);
+			ensure_full_index_with_reason(the_repository->index,
+				"sparse-checkout:disabling sparse index");
 	}
 
 	return 0;
diff --git a/builtin/stash.c b/builtin/stash.c
@@ -1560,7 +1560,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
 		char *ps_matched = xcalloc(ps->nr, 1);
 
 		/* TODO: audit for interaction with sparse-index. */
-		ensure_full_index(the_repository->index);
+		ensure_full_index_unaudited(the_repository->index);
 		for (size_t i = 0; i < the_repository->index->cache_nr; i++)
 			ce_path_match(the_repository->index, the_repository->index->cache[i], ps,
 				      ps_matched);
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
@@ -3400,7 +3400,7 @@ static void die_on_index_match(const char *path, int force)
 		char *ps_matched = xcalloc(ps.nr, 1);
 
 		/* TODO: audit for interaction with sparse-index. */
-		ensure_full_index(the_repository->index);
+		ensure_full_index_unaudited(the_repository->index);
 
 		/*
 		 * Since there is only one pathspec, we just need to
diff --git a/builtin/update-index.c b/builtin/update-index.c
@@ -714,7 +714,9 @@ static int do_reupdate(const char **paths,
 		 * to process each path individually
 		 */
 		if (S_ISSPARSEDIR(ce->ce_mode)) {
-			ensure_full_index(the_repository->index);
+			const char *fmt = "update-index:modified sparse dir '%s'";
+			ensure_full_index_with_reason(the_repository->index,
+						      fmt, ce->name);
 			goto redo;
 		}
 
diff --git a/entry.c b/entry.c
@@ -453,7 +453,7 @@ static void mark_colliding_entries(const struct checkout *state,
 	ce->ce_flags |= CE_MATCHED;
 
 	/* TODO: audit for interaction with sparse-index. */
-	ensure_full_index(state->istate);
+	ensure_full_index_unaudited(state->istate);
 	for (size_t i = 0; i < state->istate->cache_nr; i++) {
 		struct cache_entry *dup = state->istate->cache[i];
 
diff --git a/merge-ort.c b/merge-ort.c
@@ -4534,7 +4534,8 @@ static int record_conflicted_index_entries(struct merge_options *opt)
 	 */
 	strmap_for_each_entry(&opt->priv->conflicted, &iter, e) {
 		if (!path_in_sparse_checkout(e->key, index)) {
-			ensure_full_index(index);
+			const char *fmt = "merge-ort: path outside sparse checkout (%s)";
+			ensure_full_index_with_reason(index, fmt, e->key);
 			break;
 		}
 	}
diff --git a/merge-recursive.c b/merge-recursive.c
@@ -540,7 +540,7 @@ static struct string_list *get_unmerged(struct index_state *istate)
 	string_list_init_dup(unmerged);
 
 	/* TODO: audit for interaction with sparse-index. */
-	ensure_full_index(istate);
+	ensure_full_index_unaudited(istate);
 	for (i = 0; i < istate->cache_nr; i++) {
 		struct string_list_item *item;
 		struct stage_data *e;
diff --git a/read-cache.c b/read-cache.c
@@ -555,7 +555,9 @@ static int index_name_stage_pos(struct index_state *istate,
 		if (S_ISSPARSEDIR(ce->ce_mode) &&
 		    ce_namelen(ce) < namelen &&
 		    !strncmp(name, ce->name, ce_namelen(ce))) {
-			ensure_full_index(istate);
+			const char *fmt = "searching for '%s' and found parent dir '%s'";
+			ensure_full_index_with_reason(istate, fmt,
+						      name, ce->name);
 			return index_name_stage_pos(istate, name, namelen, stage, search_mode);
 		}
 	}
@@ -2376,7 +2378,7 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
 	 */
 	prepare_repo_settings(istate->repo);
 	if (istate->repo->settings.command_requires_full_index)
-		ensure_full_index(istate);
+		ensure_full_index_with_reason(istate, "incompatible builtin");
 	else
 		ensure_correct_sparsity(istate);
 
@@ -2588,7 +2590,7 @@ int repo_index_has_changes(struct repository *repo,
 		return opt.flags.has_changes != 0;
 	} else {
 		/* TODO: audit for interaction with sparse-index. */
-		ensure_full_index(istate);
+		ensure_full_index_unaudited(istate);
 		for (i = 0; sb && i < istate->cache_nr; i++) {
 			if (i)
 				strbuf_addch(sb, ' ');
@@ -3208,7 +3210,7 @@ static int do_write_locked_index(struct index_state *istate,
 				   "%s", get_lock_file_path(lock));
 
 	if (was_full)
-		ensure_full_index(istate);
+		ensure_full_index_with_reason(istate, "re-expanding after write");
 
 	if (ret)
 		return ret;
@@ -3319,7 +3321,7 @@ static int write_shared_index(struct index_state *istate,
 				   the_repository, "%s", get_tempfile_path(*temp));
 
 	if (was_full)
-		ensure_full_index(istate);
+		ensure_full_index_with_reason(istate, "re-expanding after write");
 
 	if (ret)
 		return ret;
@@ -3870,7 +3872,7 @@ void overlay_tree_on_index(struct index_state *istate,
 
 	/* Hoist the unmerged entries up to stage #3 to make room */
 	/* TODO: audit for interaction with sparse-index. */
-	ensure_full_index(istate);
+	ensure_full_index_unaudited(istate);
 	for (i = 0; i < istate->cache_nr; i++) {
 		struct cache_entry *ce = istate->cache[i];
 		if (!ce_stage(ce))
diff --git a/repository.c b/repository.c
@@ -434,7 +434,7 @@ int repo_read_index(struct repository *repo)
 
 	prepare_repo_settings(repo);
 	if (repo->settings.command_requires_full_index)
-		ensure_full_index(repo->index);
+		ensure_full_index_with_reason(repo->index, "incompatible builtin");
 
 	/*
 	 * If sparse checkouts are in use, check whether paths with the
diff --git a/resolve-undo.c b/resolve-undo.c
@@ -161,7 +161,7 @@ void unmerge_index(struct index_state *istate, const struct pathspec *pathspec,
 		return;
 
 	/* TODO: audit for interaction with sparse-index. */
-	ensure_full_index(istate);
+	ensure_full_index_unaudited(istate);
 
 	for_each_string_list_item(item, istate->resolve_undo) {
 		const char *path = item->string;
diff --git a/revision.c b/revision.c
@@ -1850,7 +1850,7 @@ static void do_add_index_objects_to_pending(struct rev_info *revs,
 	int i;
 
 	/* TODO: audit for interaction with sparse-index. */
-	ensure_full_index(istate);
+	ensure_full_index_unaudited(istate);
 	for (i = 0; i < istate->cache_nr; i++) {
 		struct cache_entry *ce = istate->cache[i];
 		struct blob *blob;
diff --git a/sequencer.c b/sequencer.c
@@ -797,7 +797,7 @@ static int do_recursive_merge(struct repository *r,
 		merge_switch_to_result(&o, head_tree, &result, 1, show_output);
 		clean = result.clean;
 	} else {
-		ensure_full_index(r->index);
+		ensure_full_index_with_reason(r->index, "non-ort merge strategy");
 		clean = merge_trees(&o, head_tree, next_tree, base_tree);
 		if (is_rebase_i(opts) && clean <= 0)
 			fputs(o.obuf.buf, stdout);
@@ -2574,7 +2574,7 @@ static int read_and_refresh_cache(struct repository *r,
 	 * expand the sparse index.
 	 */
 	if (opts->strategy && strcmp(opts->strategy, "ort"))
-		ensure_full_index(r->index);
+		ensure_full_index_with_reason(r->index, "non-ort merge strategy");
 	return 0;
 }
 
diff --git a/sparse-index.c b/sparse-index.c
@@ -463,6 +463,24 @@ void ensure_full_index(struct index_state *istate)
 	expand_index(istate, NULL);
 }
 
+void ensure_full_index_with_reason(struct index_state *istate,
+				   const char *fmt, ...)
+{
+	va_list ap;
+	struct strbuf why = STRBUF_INIT;
+	if (!istate)
+		BUG("ensure_full_index_with_reason() must get an index!");
+	if (istate->sparse_index == INDEX_EXPANDED)
+		return;
+
+	va_start(ap, fmt);
+	strbuf_vaddf(&why, fmt, ap);
+	trace2_data_string("sparse-index", istate->repo, "expansion-reason", why.buf);
+	va_end(ap);
+	strbuf_release(&why);
+	ensure_full_index(istate);
+}
+
 void ensure_correct_sparsity(struct index_state *istate)
 {
 	/*
@@ -472,7 +490,8 @@ void ensure_correct_sparsity(struct index_state *istate)
 	if (is_sparse_index_allowed(istate, 0))
 		convert_to_sparse(istate, 0);
 	else
-		ensure_full_index(istate);
+		ensure_full_index_with_reason(istate,
+					      "sparse index not allowed");
 }
 
 struct path_found_data {
@@ -620,6 +639,8 @@ static int clear_skip_worktree_from_present_files_sparse(struct index_state *ist
 			if (path_found(ce->name, &data)) {
 				if (S_ISSPARSEDIR(ce->ce_mode)) {
 					to_restart = 1;
+					trace2_data_string("sparse-index", istate->repo,
+							   "skip-worktree sparsedir", ce->name);
 					break;
 				}
 				ce->ce_flags &= ~CE_SKIP_WORKTREE;
@@ -675,7 +696,8 @@ void clear_skip_worktree_from_present_files(struct index_state *istate)
 		return;
 
 	if (clear_skip_worktree_from_present_files_sparse(istate)) {
-		ensure_full_index(istate);
+		ensure_full_index_with_reason(istate,
+			"failed to clear skip-worktree while sparse");
 		clear_skip_worktree_from_present_files_full(istate);
 	}
 }
@@ -738,7 +760,9 @@ void expand_to_path(struct index_state *istate,
 			 * in the index, perhaps it exists within this
 			 * sparse-directory.  Expand accordingly.
 			 */
-			ensure_full_index(istate);
+			const char *fmt = "found index entry for '%s'";
+			ensure_full_index_with_reason(istate, fmt,
+						      path_mutable.buf);
 			break;
 		}
 
diff --git a/sparse-index.h b/sparse-index.h
@@ -1,6 +1,8 @@
 #ifndef SPARSE_INDEX_H__
 #define SPARSE_INDEX_H__
 
+#include "strbuf.h"
+
 /*
  * If performing an operation where the index is supposed to expand to a
  * full index, then disable the advice message by setting this global to
@@ -46,4 +48,16 @@ void expand_index(struct index_state *istate, struct pattern_list *pl);
 
 void ensure_full_index(struct index_state *istate);
 
+/**
+ * If there is a clear reason why the sparse index is being expanded, then
+ * trace the information for why the expansion is occurring.
+ */
+void ensure_full_index_with_reason(struct index_state *istate,
+				   const char *fmt,
+				   ...);
+
+#define ensure_full_index_unaudited(i) \
+	ensure_full_index_with_reason((i), \
+		"unaudited call (%s.%d)", __FILE__, __LINE__);
+
 #endif
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
diff --git a/unpack-trees.c b/unpack-trees.c

Original file line number	Diff line number	Diff line change
`@@ -156,7 +156,8 @@ static int checkout_all(const char *prefix, int prefix_length)`
`156`	`156`	`* first entry inside the expanded sparse directory).`
`157`	`157`	`*/`
`158`	`158`	`if (ignore_skip_worktree) {`
`159`		`- ensure_full_index(the_repository->index);`
	`159`	`+ ensure_full_index_with_reason(the_repository->index,`
	`160`	`+ "checkout-index");`
`160`	`161`	`ce = the_repository->index->cache[i];`
`161`	`162`	`}`
`162`	`163`	`}`
Original file line number	Diff line number	Diff line change
`@@ -4534,7 +4534,8 @@ static int record_conflicted_index_entries(struct merge_options *opt)`
`4534`	`4534`	`*/`
`4535`	`4535`	`strmap_for_each_entry(&opt->priv->conflicted, &iter, e) {`
`4536`	`4536`	`if (!path_in_sparse_checkout(e->key, index)) {`
`4537`		`- ensure_full_index(index);`
	`4537`	`+ const char *fmt = "merge-ort: path outside sparse checkout (%s)";`
	`4538`	`+ ensure_full_index_with_reason(index, fmt, e->key);`
`4538`	`4539`	`break;`
`4539`	`4540`	`}`
`4540`	`4541`	`}`