Re: [PATCH v2] ls-tree: add --sparse-filter-oid argument

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Mon, Jan 23 2023, William Sprent via GitGitGadget wrote:

> From: William Sprent <williams@unity3d.com>
>
> There is currently no way to ask git the question "which files would be
> part of a sparse checkout of commit X with sparse checkout patterns Y".
> One use-case would be that tooling may want know whether sparse checkouts
> of two commits contain the same content even if the full trees differ.
> Another interesting use-case would be for tooling to use in conjunction
> with 'git update-index --index-info'.

Rather than commenting on individual points, I checked this out and
produced something squashable on-top, it fixes various issues (some
aspects of which still remain):

 * You need to wrap your code at 79 chars (and some of that could be
   helped by picking less verbose identifiers & variable names,
   e.g. just use "context" rather than "read_tree_context" in
   cmd_ls_tree(), it has no other "context"...)
 * You're making the memory management aroind init_sparse_filter_data()
   needlessly hard, just put it on the stack instead. That also allows
   for init-ing most of it right away, or via a macro in the assignment.
 * You have a stray refactoring of dir.c in your proposed change, this
   changes it back, let's leave that sort of thing out.
 * You're adding a function that returns an enum, but you declare it as
   returning "int", let's retain that type in the header & declaration.


diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index 46a815fc7dc..68d6ef675f2 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -372,36 +372,37 @@ struct sparse_filter_data {
 	struct ls_tree_options *options;
 };
 
-static void init_sparse_filter_data(struct sparse_filter_data **d, struct ls_tree_options *options,
-	const char *sparse_oid_name, read_tree_fn_t fn)
+static void init_sparse_filter_data(struct sparse_filter_data *d,
+				    const char *sparse_oid_name)
 {
 	struct object_id sparse_oid;
 	struct object_context oc;
 
-	(*d) = xcalloc(1, sizeof(**d));
-	(*d)->fn = fn;
-	(*d)->pl.use_cone_patterns = core_sparse_checkout_cone;
-	(*d)->options = options;
-	strbuf_init(&(*d)->full_path_buf, 0);
-
 	if (get_oid_with_context(the_repository, sparse_oid_name, GET_OID_BLOB,
 			&sparse_oid, &oc))
-		die(_("unable to access sparse blob in '%s'"), sparse_oid_name);
-	if (add_patterns_from_blob_to_list(&sparse_oid, "", 0, &(*d)->pl) < 0)
+		die(_("unable to access sparse blob in '%s'"),
+		    sparse_oid_name);
+	if (add_patterns_from_blob_to_list(&sparse_oid, "", 0, &d->pl) < 0)
 		die(_("unable to parse sparse filter data in %s"),
 		    oid_to_hex(&sparse_oid));
 }
 
-static void free_sparse_filter_data(struct sparse_filter_data *d)
+static void release_sparse_filter_data(struct sparse_filter_data *d)
 {
 	clear_pattern_list(&d->pl);
 	strbuf_release(&d->full_path_buf);
-	free(d);
 }
 
-static int path_matches_sparse_checkout_patterns(struct strbuf *full_path, struct pattern_list *pl, int dtype)
+static int path_matches_sparse_checkout_patterns(struct strbuf *full_path,
+						 struct pattern_list *pl,
+						 int dtype)
 {
-	enum pattern_match_result match = recursively_match_path_with_sparse_patterns(full_path->buf, the_repository->index, dtype, pl);
+	enum pattern_match_result match;
+
+	match = recursively_match_path_with_sparse_patterns(full_path->buf,
+							    the_repository->index,
+							    dtype, pl);
+
 	return match > 0;
 }
 
@@ -436,7 +437,10 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 	struct ls_tree_options options = { 0 };
 	char *sparse_oid_name = NULL;
 	void *read_tree_context = NULL;
-	struct sparse_filter_data *sparse_filter_data = NULL;
+	struct sparse_filter_data sparse_filter_data = {
+		.options = &options,
+		.full_path_buf = STRBUF_INIT,
+	};
 	const struct option ls_tree_options[] = {
 		OPT_BIT('d', NULL, &options.ls_options, N_("only show trees"),
 			LS_TREE_ONLY),
@@ -542,16 +546,17 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 	}
 
 	if (sparse_oid_name) {
-		init_sparse_filter_data(&sparse_filter_data, &options, sparse_oid_name, fn);
-		read_tree_context = sparse_filter_data;
+		sparse_filter_data.fn = fn;
+		init_sparse_filter_data(&sparse_filter_data, sparse_oid_name);
+		read_tree_context = &sparse_filter_data;
 		fn = filter_sparse;
 	} else
 		read_tree_context = &options;
 
-	ret = !!read_tree(the_repository, tree, &options.pathspec, fn, read_tree_context);
+	ret = !!read_tree(the_repository, tree, &options.pathspec, fn,
+			  read_tree_context);
 	clear_pathspec(&options.pathspec);
-	if (sparse_filter_data)
-		free_sparse_filter_data(sparse_filter_data);
+	release_sparse_filter_data(&sparse_filter_data);
 
 	return ret;
 }
diff --git a/dir.c b/dir.c
index 122ebced08e..57ec6404901 100644
--- a/dir.c
+++ b/dir.c
@@ -1457,10 +1457,10 @@ int init_sparse_checkout_patterns(struct index_state *istate)
 	return 0;
 }
 
-int recursively_match_path_with_sparse_patterns(const char *path,
-						struct index_state *istate,
-						int dtype,
-						struct pattern_list *pl)
+enum pattern_match_result recursively_match_path_with_sparse_patterns(const char *path,
+								      struct index_state *istate,
+								      int dtype,
+								      struct pattern_list *pl)
 {
 	enum pattern_match_result match = UNDECIDED;
 	const char *end, *slash;
@@ -1470,7 +1470,8 @@ int recursively_match_path_with_sparse_patterns(const char *path,
 	 * never returns UNDECIDED, so we will execute only one iteration in
 	 * this case.
 	 */
-	for (end = path + strlen(path); end > path && match == UNDECIDED;
+	for (end = path + strlen(path);
+	     end > path && match == UNDECIDED;
 	     end = slash) {
 		for (slash = end - 1; slash > path && *slash != '/'; slash--)
 			; /* do nothing */
diff --git a/dir.h b/dir.h
index d186d271606..3c65e68ca40 100644
--- a/dir.h
+++ b/dir.h
@@ -403,10 +403,10 @@ int path_in_sparse_checkout(const char *path,
 int path_in_cone_mode_sparse_checkout(const char *path,
 				      struct index_state *istate);
 
-int recursively_match_path_with_sparse_patterns(const char *path,
-						struct index_state *istate,
-						int dtype,
-						struct pattern_list *pl);
+enum pattern_match_result recursively_match_path_with_sparse_patterns(const char *path,
+								      struct index_state *istate,
+								      int dtype,
+								      struct pattern_list *pl);
 
 struct dir_entry *dir_add_ignored(struct dir_struct *dir,
 				  struct index_state *istate,