git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
William Sprent <williams@unity3d.com>
Date
Jan 24, 2023, 15:30 UTC
Message-ID
<996451ae-0d9d-e09c-0fe4-8b037b72ec5c@unity3d.com>
In-Reply-To
<230123.865ycxtn7i.gmgdl@evledraar.gmail.com>
On 23/01/2023 14.00, Ævar Arnfjörð Bjarmason wrote:
Show 15 quoted lines
> 
> 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):

Thanks. The changes all make sense to me. I'll apply them for v3 and shorten the remaining long lines.

Show 7 quoted lines
> 
>   * 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.

Fair point. I do think that having all the initialisation of the struct in one place makes the codes a bit more readable. But I guess it doesn't make a big difference in this case.

>   * 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.
These were oversights. Thanks for catching them.
Show 140 quoted lines
> 
> 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,
Previous: Ævar Arnfjörð BjarmasonNext: Ævar Arnfjörð Bjarmason
Message 8 of 20 in “ls-tree: add --sparse-filter-oid argument”
  1. ls-tree: add --sparse-filter-oid argumentWilliam Sprent via GitGitGadget, Jan 11, 2023
  2. Eric SunshineJan 13, 2023
  3. Junio C HamanoJan 13, 2023
  4. William SprentJan 16, 2023
  5. William SprentJan 16, 2023
  6. ls-tree: add --sparse-filter-oid argumentWilliam Sprent via GitGitGadget, Jan 23, 2023
  7. Ævar Arnfjörð BjarmasonJan 23, 2023
  8. William SprentJan 24, 2023
  9. Ævar Arnfjörð BjarmasonJan 23, 2023
  10. William SprentJan 24, 2023
  11. Victoria DyeJan 24, 2023
  12. William SprentJan 25, 2023
  13. Victoria DyeJan 25, 2023
  14. William SprentJan 26, 2023
  15. Elijah NewrenJan 25, 2023
  16. William SprentJan 25, 2023
  17. Elijah NewrenJan 26, 2023
  18. William SprentJan 27, 2023
  19. Elijah NewrenJan 28, 2023
  20. William SprentJan 30, 2023

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.