patchls-tree: add --sparse-filter-oid argument

> [...]
> To fill this gap, add a new argument to ls-tree '--sparse-filter-oid'
> which takes the object id of a blob containing sparse checkout patterns
> that filters the output of 'ls-tree'. When filtering with given sparsity
> patterns, 'ls-tree' only outputs blobs and commit objects that
> match the given patterns.
> [...]
> Signed-off-by: William Sprent <williams@unity3d.com>

> diff --git a/Documentation/git-ls-tree.txt b/Documentation/git-ls-tree.txt
> @@ -48,6 +48,11 @@ OPTIONS
> +--sparse-filter-oid=<blob-ish>::
> +       Omit showing tree objects and paths that do not match the
> +       sparse-checkout specification contained in the blob
> +       (or blob-expression) <blob-ish>.

> diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
> @@ -329,12 +331,79 @@ static struct ls_tree_cmdmode_to_fmt ls_tree_cmdmode_format[] = {
> +static void sparse_filter_data__init(
> +       struct sparse_filter_data **d,
> +       const char *sparse_oid_name, read_tree_fn_t fn)
> +{
> +       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;
> +
> +       strbuf_init(&(*d)->full_path_buf, 0);
> +
> +

> +static void sparse_filter_data__free(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)
> +{
> +       enum pattern_match_result match = recursively_match_path_with_sparse_patterns(full_path->buf, the_repository->index, dtype, pl);
> +       return match > 0;
> +}
> +
> +

> +static int filter_sparse(const struct object_id *oid, struct strbuf *base,
> +                        const char *pathname, unsigned mode, void *context)
> +{
> +
> +       struct sparse_filter_data *data = context;

> +       int do_recurse = show_recursive(base->buf, base->len, pathname);
> +       if (object_type(mode) == OBJ_TREE)
> +               return do_recurse;
> +
> +       strbuf_reset(&data->full_path_buf);
> +       strbuf_addbuf(&data->full_path_buf, base);
> +       strbuf_addstr(&data->full_path_buf, pathname);
> +
> +       if (!path_matches_sparse_checkout_patterns(&data->full_path_buf, &data->pl, DT_REG))
> +               return 0;
> +
> +       return data->fn(oid, base, pathname, mode, context);
> +}
> +
> +

> diff --git a/dir.c b/dir.c
> @@ -1450,45 +1450,51 @@ int init_sparse_checkout_patterns(struct index_state *istate)
> +static int path_in_sparse_checkout_1(const char *path,
> +                                    struct index_state *istate,
> +                                    int require_cone_mode)
> +{
> +
> +       /*

> @@ -0,0 +1,103 @@
> +check_agrees_with_ls_files () {
> +       REPO=repo
> +       git -C $REPO submodule deinit -f --all
> +       git -C $REPO cat-file -p ${filter_oid} >${REPO}/.git/info/sparse-checkout
> +       git -C $REPO sparse-checkout init --cone 2>err
> +       git -C $REPO submodule init
> +       git -C $REPO ls-files -t| grep -v "^S "|cut -d" " -f2 >ls-files
> +       test_cmp ls-files actual
> +}

> +check_same_result_in_bare_repo () {
> +       FULL=repo
> +       BARE=bare
> +       FILTER=$1
> +       git -C repo cat-file -p ${filter_oid}| git -C bare hash-object -w --stdin
> +       git -C bare ls-tree --name-only --filter-sparse-oid=${filter_oid} -r HEAD >bare-result
> +       test_cmp expect bare-result
> +}

> +test_expect_success 'setup' '
> +       git init submodule &&
> +       (
> +               cd submodule &&
> +               test_commit file
> +       ) &&

> +       git init repo &&
> +       (
> +               cd repo &&
> +               mkdir dir &&
> +               test_commit dir/sub-file &&
> +               test_commit dir/sub-file2 &&
> +               mkdir dir2 &&
> +               test_commit dir2/sub-file1 &&
> +               test_commit dir2/sub-file2 &&
> +               test_commit top-file &&
> +               git clone ../submodule submodule &&
> +               git submodule add ./submodule &&
> +               git submodule absorbgitdirs &&
> +               git commit -m"add submodule" &&
> +               git sparse-checkout init --cone
> +       ) &&

>>     Note that one of the tests only pass when run on top of commit
>>     5842710dc2 (dir: check for single file cone patterns, 2023-01-03), which
>>     was submitted separately and is currently is merged to 'next'.
>
> Thanks for mentioning this. It's exactly the sort of information the
> maintainer needs when applying your patch to his tree. And it can be
> helpful for reviewers too.

> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
>>>      Note that one of the tests only pass when run on top of commit
>>>      5842710dc2 (dir: check for single file cone patterns, 2023-01-03), which
>>>      was submitted separately and is currently is merged to 'next'.
>>
>> Thanks for mentioning this. It's exactly the sort of information the
>> maintainer needs when applying your patch to his tree. And it can be
>> helpful for reviewers too.
> 
> Indeed it is.  Thanks for pointing it out---I just wish other
> contributors see and follow such a wise piece of advice buried in
> review of a patch by others.

> On Wed, Jan 11, 2023 at 12:05 PM William Sprent via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> [...]
>> To fill this gap, add a new argument to ls-tree '--sparse-filter-oid'
>> which takes the object id of a blob containing sparse checkout patterns
>> that filters the output of 'ls-tree'. When filtering with given sparsity
>> patterns, 'ls-tree' only outputs blobs and commit objects that
>> match the given patterns.
>> [...]
>> Signed-off-by: William Sprent <williams@unity3d.com>
> 
> This is not a proper review, but rather just some comments about
> issues I noticed while quickly running my eye over the patch. Many are
> just micro-nits about style; a few regarding the tests are probably
> actionable.
> 
>>      Note that one of the tests only pass when run on top of commit
>>      5842710dc2 (dir: check for single file cone patterns, 2023-01-03), which
>>      was submitted separately and is currently is merged to 'next'.
> 
> Thanks for mentioning this. It's exactly the sort of information the
> maintainer needs when applying your patch to his tree. And it can be
> helpful for reviewers too.
> 
>> diff --git a/Documentation/git-ls-tree.txt b/Documentation/git-ls-tree.txt
>> @@ -48,6 +48,11 @@ OPTIONS
>> +--sparse-filter-oid=<blob-ish>::
>> +       Omit showing tree objects and paths that do not match the
>> +       sparse-checkout specification contained in the blob
>> +       (or blob-expression) <blob-ish>.
> 
> Good to see a documentation update. The SYNOPSIS probably deserves an
> update too.
> 

>> diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
>> @@ -329,12 +331,79 @@ static struct ls_tree_cmdmode_to_fmt ls_tree_cmdmode_format[] = {
>> +static void sparse_filter_data__init(
>> +       struct sparse_filter_data **d,
>> +       const char *sparse_oid_name, read_tree_fn_t fn)
>> +{
>> +       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;
>> +
>> +       strbuf_init(&(*d)->full_path_buf, 0);
>> +
>> +
> 
> Nit: too many blank lines.
> 
>> +       if (get_oid_with_context(the_repository,
>> +                                sparse_oid_name,
>> +                                GET_OID_BLOB, &sparse_oid, &oc))
> 
> Pure nit: somewhat odd choice of wrapping; I'd have expected something
> along the lines of:
> 
>      if (get_oid_with_context(the_repository, sparse_oid_name, GET_OID_BLOB,
>                      &sparse_oid, &oc))
> 
> or:
> 
>      if (get_oid_with_context(the_repository, sparse_oid_name,
>                      GET_OID_BLOB, &sparse_oid, &oc))
> 
>> +static void sparse_filter_data__free(struct sparse_filter_data *d)
>> +{
>> +       clear_pattern_list(&d->pl);
>> +       strbuf_release(&d->full_path_buf);
>> +       free(d);
>> +}
> 
> Is the double-underscore convention in function names imported from
> elsewhere? I don't recall seeing it used in this codebase.
> 

>> +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);
>> +       return match > 0;
>> +}
>> +
>> +
> 
> Nit: too many blank lines
> 
>> +static int filter_sparse(const struct object_id *oid, struct strbuf *base,
>> +                        const char *pathname, unsigned mode, void *context)
>> +{
>> +
>> +       struct sparse_filter_data *data = context;
> 
> Nit: unnecessary blank line after "{"
> 
>> +       int do_recurse = show_recursive(base->buf, base->len, pathname);
>> +       if (object_type(mode) == OBJ_TREE)
>> +               return do_recurse;
>> +
>> +       strbuf_reset(&data->full_path_buf);
>> +       strbuf_addbuf(&data->full_path_buf, base);
>> +       strbuf_addstr(&data->full_path_buf, pathname);
>> +
>> +       if (!path_matches_sparse_checkout_patterns(&data->full_path_buf, &data->pl, DT_REG))
>> +               return 0;
>> +
>> +       return data->fn(oid, base, pathname, mode, context);	
>> +}
>> +
>> +
> 
> Nit: too many blank lines
> 
>> diff --git a/dir.c b/dir.c
>> @@ -1450,45 +1450,51 @@ int init_sparse_checkout_patterns(struct index_state *istate)
>> +static int path_in_sparse_checkout_1(const char *path,
>> +                                    struct index_state *istate,
>> +                                    int require_cone_mode)
>> +{
>> +
>> +       /*
> 
> Nit: unnecessary blank line after "{"
> 
>> diff --git a/t/t3106-ls-tree-sparse-checkout.sh b/t/t3106-ls-tree-sparse-checkout.sh
>> new file mode 100755
> 
> We often try to avoid introducing a new test script if the tests being
> added can fit well into an existing script. If you didn't find any
> existing script where these tests would fit, then creating a new
> script may be fine.
> 

>> @@ -0,0 +1,103 @@
>> +check_agrees_with_ls_files () {
>> +       REPO=repo
>> +       git -C $REPO submodule deinit -f --all
>> +       git -C $REPO cat-file -p ${filter_oid} >${REPO}/.git/info/sparse-checkout
>> +       git -C $REPO sparse-checkout init --cone 2>err
>> +       git -C $REPO submodule init
>> +       git -C $REPO ls-files -t| grep -v "^S "|cut -d" " -f2 >ls-files
>> +       test_cmp ls-files actual
>> +}
> 
> Several comments:
> 
> Since the return code of this function is significant and callers care
> about it, you should &&-chain all of the code in the function itself
> (just like you do within a test) so that failure of any command in the
> function is noticed and causes the calling test to fail. It's a good
> idea to &&-chain the variable assignments at the top of the function
> too (just in case someone later inserts code above the assignments).
> 

> Some shells complain when the pathname following ">" redirection
> operator is not quoted, so:
> 
>      git -C $REPO cat-file -p ${filter_oid} >"$REPO/.git/info/sparse-checkout" &&
> 
> Style: add space around "|" pipe operator
> 
> We usually avoid having a Git command upstream of the pipe since its
> exit code gets swallowed by the pipe, so we usually do this instead:
> 
>      git -C $REPO ls-files -t >out &&
>      grep -v "^S " out | cut -d" " -f2 >ls-files &&
> 

> Minor: The two-command pipeline `grep -v "^S " | cut -d" " -f2
>> ls-files` could be expressed via a single `sed` invocation:
> 
>      sed -n "/^S /!s/^. //p" out &&
> 
> Nit: The first argument to test_cmp() is typically named "expect".
> 
> Error output is captured to file "err" but that file is never consulted.
> 

>> +check_same_result_in_bare_repo () {
>> +       FULL=repo
>> +       BARE=bare
>> +       FILTER=$1
>> +       git -C repo cat-file -p ${filter_oid}| git -C bare hash-object -w --stdin
>> +       git -C bare ls-tree --name-only --filter-sparse-oid=${filter_oid} -r HEAD >bare-result
>> +       test_cmp expect bare-result
>> +}
> 
> Same comments as above, plus:
> 
> What is the purpose of the variables FULL, BARE, FILTER? They don't
> seem to be used in the function.
> 
> I suspect that the function should be using $FILTER internally rather
> than $filter_oid.
> 

>> +test_expect_success 'setup' '
>> +       git init submodule &&
>> +       (
>> +               cd submodule &&
>> +               test_commit file
>> +       ) &&
> 
> Minor: test_commit() accepts a -C option, so this could be done
> without the subshell:
> 
>      test_commit -C submodule file
> 
>> +       git init repo &&
>> +       (
>> +               cd repo &&
>> +               mkdir dir &&
>> +               test_commit dir/sub-file &&
>> +               test_commit dir/sub-file2 &&
>> +               mkdir dir2 &&
>> +               test_commit dir2/sub-file1 &&
>> +               test_commit dir2/sub-file2 &&
>> +               test_commit top-file &&
>> +               git clone ../submodule submodule &&
>> +               git submodule add ./submodule &&
>> +               git submodule absorbgitdirs &&
>> +               git commit -m"add submodule" &&
>> +               git sparse-checkout init --cone
>> +       ) &&
> 
> Here the subshell makes sense since so many commands are run in
> directory "repo." Fine.

> 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'.

> 
> 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.

> 
> 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,

> diff --git a/Documentation/git-ls-tree.txt b/Documentation/git-ls-tree.txt
> index 0240adb8eec..724bb1f9dbd 100644
> --- a/Documentation/git-ls-tree.txt
> +++ b/Documentation/git-ls-tree.txt
> @@ -11,6 +11,7 @@ SYNOPSIS
>  [verse]
>  'git ls-tree' [-d] [-r] [-t] [-l] [-z]
>  	    [--name-only] [--name-status] [--object-only] [--full-name] [--full-tree] [--abbrev[=<n>]] [--format=<format>]
> +	    [--sparse-filter-oid=<blob-ish>]
>  	    <tree-ish> [<path>...]
>  
>  DESCRIPTION
> @@ -48,6 +49,11 @@ OPTIONS
>  	Show tree entries even when going to recurse them. Has no effect
>  	if `-r` was not passed. `-d` implies `-t`.
>  
> +--sparse-filter-oid=<blob-ish>::
> +	Omit showing tree objects and paths that do not match the
> +	sparse-checkout specification contained in the blob
> +	(or blob-expression) <blob-ish>.
> +
>  -l::
>  --long::
>  	Show object size of blob (file) entries.
> diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
> index 72eb70823d5..46a815fc7dc 100644
> --- a/builtin/ls-tree.c
> +++ b/builtin/ls-tree.c
> @@ -13,6 +13,7 @@
>  #include "builtin.h"
>  #include "parse-options.h"
>  #include "pathspec.h"
> +#include "dir.h"
>  
>  static const char * const ls_tree_usage[] = {
>  	N_("git ls-tree [<options>] <tree-ish> [<path>...]"),
> @@ -364,6 +365,65 @@ static struct ls_tree_cmdmode_to_fmt ls_tree_cmdmode_format[] = {
>  	},
>  };

> +	(*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 parse sparse filter data in %s"),
> +		    oid_to_hex(&sparse_oid));
> +}
> +
> +static void free_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)
> +{
> +	enum pattern_match_result match = recursively_match_path_with_sparse_patterns(full_path->buf, the_repository->index, dtype, pl);

> +
> +	if (!path_matches_sparse_checkout_patterns(&data->full_path_buf, &data->pl, DT_REG))
> +			return 0;
> +
> +	return data->fn(oid, base, pathname, mode, data->options);
> +}
> +
>  int cmd_ls_tree(int argc, const char **argv, const char *prefix)
>  {
>  	struct object_id oid;
> @@ -372,7 +432,11 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
>  	read_tree_fn_t fn = NULL;
>  	enum ls_tree_cmdmode cmdmode = MODE_DEFAULT;
>  	int null_termination = 0;
> +

>  	struct ls_tree_options options = { 0 };
> +	char *sparse_oid_name = NULL;
> +	void *read_tree_context = NULL;
> +	struct sparse_filter_data *sparse_filter_data = NULL;
>  	const struct option ls_tree_options[] = {
>  		OPT_BIT('d', NULL, &options.ls_options, N_("only show trees"),
>  			LS_TREE_ONLY),
> @@ -399,6 +463,9 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
>  					 N_("format to use for the output"),
>  					 PARSE_OPT_NONEG),
>  		OPT__ABBREV(&options.abbrev),
> +		OPT_STRING_F(0, "filter-sparse-oid", &sparse_oid_name, N_("filter-sparse-oid"),

> +			   N_("filter output with sparse-checkout specification contained in the blob"),
> +			     PARSE_OPT_NONEG),
>  		OPT_END()
>  	};
>  	struct ls_tree_cmdmode_to_fmt *m2f = ls_tree_cmdmode_format;
> @@ -474,7 +541,17 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
>  		break;
>  	}
>  
> -	ret = !!read_tree(the_repository, tree, &options.pathspec, fn, &options);
> +	if (sparse_oid_name) {
> +		init_sparse_filter_data(&sparse_filter_data, &options, sparse_oid_name, fn);
> +		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);
>  	clear_pathspec(&options.pathspec);
> +	if (sparse_filter_data)
> +		free_sparse_filter_data(sparse_filter_data);

> +
>  	return ret;
>  }
> diff --git a/dir.c b/dir.c
> index 4e99f0c868f..122ebced08e 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1457,45 +1457,50 @@ int init_sparse_checkout_patterns(struct index_state *istate)
>  	return 0;
>  }
>  
> -static int path_in_sparse_checkout_1(const char *path,
> -				     struct index_state *istate,
> -				     int require_cone_mode)
> +int recursively_match_path_with_sparse_patterns(const char *path,
> +						struct index_state *istate,
> +						int dtype,
> +						struct pattern_list *pl)
>  {
> -	int dtype = DT_REG;
>  	enum pattern_match_result match = UNDECIDED;
>  	const char *end, *slash;
> -
> -	/*
> -	 * We default to accepting a path if the path is empty, there are no
> -	 * patterns, or the patterns are of the wrong type.
> -	 */
> -	if (!*path ||
> -	    init_sparse_checkout_patterns(istate) ||
> -	    (require_cone_mode &&
> -	     !istate->sparse_checkout_patterns->use_cone_patterns))
> -		return 1;
> -
>  	/*
>  	 * If UNDECIDED, use the match from the parent dir (recursively), or
>  	 * fall back to NOT_MATCHED at the topmost level. Note that cone mode
>  	 * 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 */
>  
>  		match = path_matches_pattern_list(path, end - path,
>  				slash > path ? slash + 1 : path, &dtype,
> -				istate->sparse_checkout_patterns, istate);
> +				pl, istate);
>  
>  		/* We are going to match the parent dir now */
>  		dtype = DT_DIR;
>  	}
> -	return match > 0;
> +
> +	return match;
> +}
> +
> +static int path_in_sparse_checkout_1(const char *path,
> +				     struct index_state *istate,
> +				     int require_cone_mode)
> +{
> +	/*
> +	 * We default to accepting a path if the path is empty, there are no
> +	 * patterns, or the patterns are of the wrong type.
> +	 */
> +	if (!*path ||
> +	    init_sparse_checkout_patterns(istate) ||
> +	    (require_cone_mode &&
> +	     !istate->sparse_checkout_patterns->use_cone_patterns))
> +		return 1;
> +
> +	return recursively_match_path_with_sparse_patterns(path, istate, DT_REG, istate->sparse_checkout_patterns) > 0;

> +check_agrees_with_ls_files () {
> +	REPO=repo  &&
> +	git -C $REPO submodule deinit -f --all &&
> +	git -C $REPO cat-file -p $filter_oid >"$REPO/.git/info/sparse-checkout" &&
> +	git -C $REPO sparse-checkout init --cone &&
> +	git -C $REPO submodule init &&
> +	git -C $REPO ls-files -t >out &&
> +	sed -n "/^S /!s/^. //p" out >expect &&
> +	test_cmp expect actual

> 
> On Mon, Jan 23 2023, William Sprent via GitGitGadget wrote:
> 
>> From: William Sprent <williams@unity3d.com>
> 
> ...some further inline commentary, in addition to my proposed squash-in...
> 

>> diff --git a/Documentation/git-ls-tree.txt b/Documentation/git-ls-tree.txt
>> index 0240adb8eec..724bb1f9dbd 100644
>> --- a/Documentation/git-ls-tree.txt
>> +++ b/Documentation/git-ls-tree.txt
>> @@ -11,6 +11,7 @@ SYNOPSIS
>>   [verse]
>>   'git ls-tree' [-d] [-r] [-t] [-l] [-z]
>>   	    [--name-only] [--name-status] [--object-only] [--full-name] [--full-tree] [--abbrev[=<n>]] [--format=<format>]
>> +	    [--sparse-filter-oid=<blob-ish>]
>>   	    <tree-ish> [<path>...]
>>   
>>   DESCRIPTION
>> @@ -48,6 +49,11 @@ OPTIONS
>>   	Show tree entries even when going to recurse them. Has no effect
>>   	if `-r` was not passed. `-d` implies `-t`.
>>   
>> +--sparse-filter-oid=<blob-ish>::
>> +	Omit showing tree objects and paths that do not match the
>> +	sparse-checkout specification contained in the blob
>> +	(or blob-expression) <blob-ish>.
>> +
>>   -l::
>>   --long::
>>   	Show object size of blob (file) entries.
>> diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
>> index 72eb70823d5..46a815fc7dc 100644
>> --- a/builtin/ls-tree.c
>> +++ b/builtin/ls-tree.c
>> @@ -13,6 +13,7 @@
>>   #include "builtin.h"
>>   #include "parse-options.h"
>>   #include "pathspec.h"
>> +#include "dir.h"
>>   
>>   static const char * const ls_tree_usage[] = {
>>   	N_("git ls-tree [<options>] <tree-ish> [<path>...]"),
>> @@ -364,6 +365,65 @@ static struct ls_tree_cmdmode_to_fmt ls_tree_cmdmode_format[] = {
>>   	},
>>   };
> 
> Okey, here yo uupdate the *.txt, but not the "-h", but it's one of those
> where way say <options>.
> 
> I for one wouldn't mind some preceding change like my 423be1f83c5 (doc
> txt & -h consistency: make "commit" consistent, 2022-10-13), which in
> turn would make t/t0450-txt-doc-vs-help.sh pass for this command, but
> maybe it's too much of a digression...
> 

>> +	(*d) = xcalloc(1, sizeof(**d));
>> +	(*d)->fn = fn;
>> +	(*d)->pl.use_cone_patterns = core_sparse_checkout_cone;
> 
> I forgot to note in my proposed fix-up that I omitted this, but your
> tests still pass, which suggests it's either not needed, or your tests
> are lacking....
> 

>> +	(*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)
> 
> As noted you can avoid the malloc here, which also sidesteps this (IMO
> at last) ugly &(*v)->m syntax.
> 

>> +		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)
>> +{
>> +	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)
>> +{
>> +	enum pattern_match_result match = recursively_match_path_with_sparse_patterns(full_path->buf, the_repository->index, dtype, pl);
> 
> I for one would welcome e.g. abbreviating "sparse_checkout_patterns" as
> "scp" or whatever throughout, with resulting shortening of very long
> lines & identifiers. E.g. for this one "patch_match_scp" or whatever.
> 
> 

>> +	struct sparse_filter_data *data = context;
>> +
> 
> Style: Don't add a \n\n between variable decls,
> 
>> +	int do_recurse = show_recursive(data->options, base->buf, base->len, pathname);
> 
> Instead add it here, before the code proper.
> 
> 

>> +	if (object_type(mode) == OBJ_TREE)
>> +		return do_recurse;
>> +
>> +	strbuf_reset(&data->full_path_buf);
> 
> I wondered if we need this after, but we don't, I for one would welcome a fix-up like:
> 	
> 	diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
> 	index 68d6ef675f2..74dfa9a4580 100644
> 	--- a/builtin/ls-tree.c
> 	+++ b/builtin/ls-tree.c
> 	@@ -410,19 +410,21 @@ static int filter_sparse(const struct object_id *oid, struct strbuf *base,
> 	 			 const char *pathname, unsigned mode, void *context)
> 	 {
> 	 	struct sparse_filter_data *data = context;
> 	-
> 	 	int do_recurse = show_recursive(data->options, base->buf, base->len, pathname);
> 	+	int ret = 0;
> 	+
> 	 	if (object_type(mode) == OBJ_TREE)
> 	 		return do_recurse;
> 	
> 	-	strbuf_reset(&data->full_path_buf);
> 	 	strbuf_addbuf(&data->full_path_buf, base);
> 	 	strbuf_addstr(&data->full_path_buf, pathname);
> 	
> 	 	if (!path_matches_sparse_checkout_patterns(&data->full_path_buf, &data->pl, DT_REG))
> 	-			return 0;
> 	-
> 	-	return data->fn(oid, base, pathname, mode, data->options);
> 	+		goto cleanup;
> 	+	ret = data->fn(oid, base, pathname, mode, data->options);
> 	+cleanup:
> 	+	strbuf_reset(&data->full_path_buf);
> 	+	return ret;
> 	 }
> 	
> 	 int cmd_ls_tree(int argc, const char **argv, const char *prefix)
> 	
> It's slightly more verbose, and we do end up needlessly reset-ing the
> "last" one, but makes it clear that we don't have need for this after
> this.
> 
> Which to me, also raises the question of why you have this
> "full_path_buf" at all. It looks like a memory optimization, as you're
> trying to avoid a malloc()/free() in the loop.
> 
> That's fair, but you could also do that with:
> 
> 	const size_t oldlen = base->len;
> 	strbuf_addstr(base, pathname);
>          /* do the path_matches_sparse_checkout_patterns() call here */
> 	/* before "cleanup" */
> 	strbuf_setlen(base, oldlen);
> 
> Or whatever, those snippets are untested, but we use similar tricks
> elsewhere, and as it's contained to a few lines here I think it's better
> than carrying another buffer.
> 

>> +	strbuf_addbuf(&data->full_path_buf, base);
>> +	strbuf_addstr(&data->full_path_buf, pathname);
> 
> Nit: Shorter as: strbuf_addf(&sb, "%s%s", base.buf, pathname) (or equivalent);
> 
>> +
>> +	if (!path_matches_sparse_checkout_patterns(&data->full_path_buf, &data->pl, DT_REG))
>> +			return 0;
>> +
>> +	return data->fn(oid, base, pathname, mode, data->options);
>> +}
>> +
>>   int cmd_ls_tree(int argc, const char **argv, const char *prefix)
>>   {
>>   	struct object_id oid;
>> @@ -372,7 +432,11 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
>>   	read_tree_fn_t fn = NULL;
>>   	enum ls_tree_cmdmode cmdmode = MODE_DEFAULT;
>>   	int null_termination = 0;
>> +
> 
> Don't add this stray \n.
> 

>>   	struct ls_tree_options options = { 0 };
>> +	char *sparse_oid_name = NULL;
>> +	void *read_tree_context = NULL;
>> +	struct sparse_filter_data *sparse_filter_data = NULL;
>>   	const struct option ls_tree_options[] = {
>>   		OPT_BIT('d', NULL, &options.ls_options, N_("only show trees"),
>>   			LS_TREE_ONLY),
>> @@ -399,6 +463,9 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
>>   					 N_("format to use for the output"),
>>   					 PARSE_OPT_NONEG),
>>   		OPT__ABBREV(&options.abbrev),
>> +		OPT_STRING_F(0, "filter-sparse-oid", &sparse_oid_name, N_("filter-sparse-oid"),
> 
> Make that N_(...) be "object-id", "oid", "hash" or something?
> 
> I.e. the RHS with the <type> should be <thingy>, not
> <thingy-for-some-purpose>.
> 

>> +			   N_("filter output with sparse-checkout specification contained in the blob"),
>> +			     PARSE_OPT_NONEG),
>>   		OPT_END()
>>   	};
>>   	struct ls_tree_cmdmode_to_fmt *m2f = ls_tree_cmdmode_format;
>> @@ -474,7 +541,17 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
>>   		break;
>>   	}
>>   
>> -	ret = !!read_tree(the_repository, tree, &options.pathspec, fn, &options);
>> +	if (sparse_oid_name) {
>> +		init_sparse_filter_data(&sparse_filter_data, &options, sparse_oid_name, fn);
>> +		read_tree_context = sparse_filter_data;
>> +		fn = filter_sparse;
>> +	} else
>> +		read_tree_context = &options;
> 
> You're missing a {} here for the "else".
> 
> Better yet, delete that "else" and change the decl above to be:
> 
> 	void *context = &options;
> 
> Then just keep the "if" here, where you might clobber the "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);
> 
> I suggested a fixup for this, but as an aside don't make a free_*()
> function that's not happy to accept NULL, it should work like the free()
> itself.
> 

>> +
>>   	return ret;
>>   }
>> diff --git a/dir.c b/dir.c
>> index 4e99f0c868f..122ebced08e 100644
>> --- a/dir.c
>> +++ b/dir.c
>> @@ -1457,45 +1457,50 @@ int init_sparse_checkout_patterns(struct index_state *istate)
>>   	return 0;
>>   }
>>   
>> -static int path_in_sparse_checkout_1(const char *path,
>> -				     struct index_state *istate,
>> -				     int require_cone_mode)
>> +int recursively_match_path_with_sparse_patterns(const char *path,
>> +						struct index_state *istate,
>> +						int dtype,
>> +						struct pattern_list *pl)
>>   {
>> -	int dtype = DT_REG;
>>   	enum pattern_match_result match = UNDECIDED;
>>   	const char *end, *slash;
>> -
>> -	/*
>> -	 * We default to accepting a path if the path is empty, there are no
>> -	 * patterns, or the patterns are of the wrong type.
>> -	 */
>> -	if (!*path ||
>> -	    init_sparse_checkout_patterns(istate) ||
>> -	    (require_cone_mode &&
>> -	     !istate->sparse_checkout_patterns->use_cone_patterns))
>> -		return 1;
>> -
>>   	/*
>>   	 * If UNDECIDED, use the match from the parent dir (recursively), or
>>   	 * fall back to NOT_MATCHED at the topmost level. Note that cone mode
>>   	 * 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;
> 
> All good, aside from this whitespace refactoring, as noted.
> 

>>   	     end = slash) {
>> -
>>   		for (slash = end - 1; slash > path && *slash != '/'; slash--)
>>   			; /* do nothing */
>>   
>>   		match = path_matches_pattern_list(path, end - path,
>>   				slash > path ? slash + 1 : path, &dtype,
>> -				istate->sparse_checkout_patterns, istate);
>> +				pl, istate);
>>   
>>   		/* We are going to match the parent dir now */
>>   		dtype = DT_DIR;
>>   	}
>> -	return match > 0;
>> +
>> +	return match;
>> +}
>> +
>> +static int path_in_sparse_checkout_1(const char *path,
>> +				     struct index_state *istate,
>> +				     int require_cone_mode)
>> +{
>> +	/*
>> +	 * We default to accepting a path if the path is empty, there are no
>> +	 * patterns, or the patterns are of the wrong type.
>> +	 */
>> +	if (!*path ||
>> +	    init_sparse_checkout_patterns(istate) ||
>> +	    (require_cone_mode &&
>> +	     !istate->sparse_checkout_patterns->use_cone_patterns))
>> +		return 1;
>> +
>> +	return recursively_match_path_with_sparse_patterns(path, istate, DT_REG, istate->sparse_checkout_patterns) > 0;
> 
> All good, except for the really long line, aside from the previous
> suggestion, maybe pull "istate->sparse_checkout_patterns" into a
> variable, as it's now used twice here in this function?
> 

>> +check_agrees_with_ls_files () {
>> +	REPO=repo  &&
>> +	git -C $REPO submodule deinit -f --all &&
>> +	git -C $REPO cat-file -p $filter_oid >"$REPO/.git/info/sparse-checkout" &&
>> +	git -C $REPO sparse-checkout init --cone &&
>> +	git -C $REPO submodule init &&
>> +	git -C $REPO ls-files -t >out &&
>> +	sed -n "/^S /!s/^. //p" out >expect &&
>> +	test_cmp expect actual
> 
> Instead of this last "sed" munging, why not use the "--format" option to
> "ls-tree" to just make the output the same?

> 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'.

> 
> 'rev-list --objects --filter=sparse:oid' comes close, but as rev-list is
> concerned with objects rather than directory trees, it leaves files out
> when the same blob occurs in at two different paths.
> 
> It is possible to ask git about the sparse status of files currently in
> the index with 'ls-files -t'. However, this does not work well when the
> caller is interested in another commit, intererested in sparsity
> patterns that aren't currently in '.git/info/sparse-checkout', or when
> working in with bare repo.

> 
> To fill this gap, add a new argument to ls-tree '--sparse-filter-oid'
> which takes the object id of a blob containing sparse checkout patterns
> that filters the output of 'ls-tree'. When filtering with given sparsity
> patterns, 'ls-tree' only outputs blobs and commit objects that
> match the given patterns.

> 
> While it may be valid in some situations to output a tree object -- e.g.
> when a cone pattern matches all blobs recursively contained in a tree --
> it is less unclear what should be output if a sparse pattern matches
> parts of a tree.
> 
> To allow for reusing the pattern matching logic found in
> 'path_in_sparse_checkout_1()' in 'dir.c' with arbitrary patterns,
> extract the pattern matching part of the function into its own new
> function 'recursively_match_path_with_sparse_patterns()'.
> 
> Signed-off-by: William Sprent <williams@unity3d.com>
> ---
>     ls-tree: add --sparse-filter-oid argument
>     
>     I'm resubmitting this change as rebased on top of 'master', as it
>     conflicted with the topic 'ls-tree.c: clean-up works' 1
>     [https://public-inbox.org/git/20230112091135.20050-1-tenglong.tl@alibaba-inc.com],
>     which was merged to 'master' recently.
>     
>     This versions also incorporates changes based on the comments made in 2
>     [https://public-inbox.org/git/CAPig+cRgZ0CrkqY7mufuWmhf6BC8yXjXXuOTEQjuz+Y0NA+N7Q@mail.gmail.com/].
>     
>     I'm also looping in contributors that have touched ls-tree and/or
>     sparse-checkouts recently. I hope that's okay.

>     
>     William
> 
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1459%2Fwilliams-unity%2Fls-tree-sparse-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1459/williams-unity/ls-tree-sparse-v2
> Pull-Request: https://github.com/gitgitgadget/git/pull/1459

> 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'.
> 
> These types of use cases align nicely with "Behavior A" as described in
> 'Documentation/technical/sparse-checkout.txt' [1]. I think adding a
> '--scope=(sparse|all)' option to 'ls-trees' would be a good way to make
> progress on the goals of that design document as well as serve the needs
> described above.
> 
> [1] https://lore.kernel.org/git/pull.1367.v4.git.1667714666810.gitgitgadget@gmail.com/
> >>
>> 'rev-list --objects --filter=sparse:oid' comes close, but as rev-list is
>> concerned with objects rather than directory trees, it leaves files out
>> when the same blob occurs in at two different paths.
>>
>> It is possible to ask git about the sparse status of files currently in
>> the index with 'ls-files -t'. However, this does not work well when the
>> caller is interested in another commit, intererested in sparsity
>> patterns that aren't currently in '.git/info/sparse-checkout', or when
>> working in with bare repo.
> 
> I'm a bit confused by your described use cases here, though. Right now,
> 'sparse-checkout' is a local repo-only optimization (for performance and/or
> scoping user workspaces), but you seem to be implying a need for
> "sparse-checkout patterns" as a general-purpose data format (like a config
> file or 'rebase-todo') that can be used to manually configure command
> behavior.
> 
> If that is what you're getting at, it seems like a pretty substantial
> expansion to the scope of what we consider "sparse checkout". That's not to
> say it isn't a good idea - I can definitely imagine tooling where this type
> of functionality is useful - just that it warrants careful consideration so
> we don't over-complicate the typical sparse-checkout user experience.
> 

>>
>> To fill this gap, add a new argument to ls-tree '--sparse-filter-oid'
>> which takes the object id of a blob containing sparse checkout patterns
>> that filters the output of 'ls-tree'. When filtering with given sparsity
>> patterns, 'ls-tree' only outputs blobs and commit objects that
>> match the given patterns.
> 
> I don't think a blob OID is the best way to specify an arbitrary pattern set
> in this case. OIDs will only be created for blobs that are tracked in Git;
> it's possible that your project tracks potential sparse-checkout patterns in
> Git, but it seems overly restrictive. You could instead specify the file
> with a path on-disk (like the '--file' options in various commands, e.g.
> 'git commit'); if you did need to get that file from the object store, you
> could first get its contents with 'git cat-file'.
> 

>>
>> While it may be valid in some situations to output a tree object -- e.g.
>> when a cone pattern matches all blobs recursively contained in a tree --
>> it is less unclear what should be output if a sparse pattern matches
>> parts of a tree.
>>
>> To allow for reusing the pattern matching logic found in
>> 'path_in_sparse_checkout_1()' in 'dir.c' with arbitrary patterns,
>> extract the pattern matching part of the function into its own new
>> function 'recursively_match_path_with_sparse_patterns()'.
>>
>> Signed-off-by: William Sprent <williams@unity3d.com>
>> ---
>>      ls-tree: add --sparse-filter-oid argument
>>      
>>      I'm resubmitting this change as rebased on top of 'master', as it
>>      conflicted with the topic 'ls-tree.c: clean-up works' 1
>>      [https://public-inbox.org/git/20230112091135.20050-1-tenglong.tl@alibaba-inc.com],
>>      which was merged to 'master' recently.
>>      
>>      This versions also incorporates changes based on the comments made in 2
>>      [https://public-inbox.org/git/CAPig+cRgZ0CrkqY7mufuWmhf6BC8yXjXXuOTEQjuz+Y0NA+N7Q@mail.gmail.com/].
>>      
>>      I'm also looping in contributors that have touched ls-tree and/or
>>      sparse-checkouts recently. I hope that's okay.
> 
> Definitely okay, thanks for CC-ing!
> 
> Overall, this is an interesting extension to 'sparse-checkout', and opens up
> some opportunities for expanded tooling. However, at an implementation
> level, I think it's addressing two separate needs ("constrain 'ls-files' to
> display files matching patterns" and "specify sparse patterns not in
> '.git/info/sparse-checkout'") in one option, which makes for a somewhat
> confusing and inflexible user experience. What about instead breaking this
> into two options:
> 
> * --scope=(sparse|all): limits the scope of the files output by ("Behavior
>    A" vs. "Behavior B" from the document linked earlier).
> * --sparse-patterns=<file>: specifies "override" patterns to use instead of
>    those in '.git/info/sparse-checkout' (whether 'sparse-checkout' is enabled
>    for the repository or not).
> 

> I haven't looked at your implementation in detail yet, but I did want to
> offer a recommendation in case you hadn't considered it: if you want to
> allow the use of patterns from a user-specified specific file, it would be
> nice to do it in a way that fully replaces the "default" sparse-checkout
> settings at the lowest level (i.e., override the values of
> 'core_apply_sparse_checkout', 'core_sparse_checkout_cone', and
> 'get_sparse_checkout_filename()'). Doing it that way would both make it
> easier for other commands to add a '--sparse-patterns' option, and avoid the
> separate code path ('path_in_sparse_checkout_1()' vs.
> 'recursively_match_path_with_sparse_patterns()', for example) when dealing
> with '.git/info/sparse-checkout' patterns vs. manually-specified patterns.
> 

>>      
>>      William
>>
>> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1459%2Fwilliams-unity%2Fls-tree-sparse-v2
>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1459/williams-unity/ls-tree-sparse-v2
>> Pull-Request: https://github.com/gitgitgadget/git/pull/1459
> 

> On 24/01/2023 21.11, Victoria Dye wrote>> I haven't looked at your implementation in detail yet, but I did want to
>> offer a recommendation in case you hadn't considered it: if you want to
>> allow the use of patterns from a user-specified specific file, it would be
>> nice to do it in a way that fully replaces the "default" sparse-checkout
>> settings at the lowest level (i.e., override the values of
>> 'core_apply_sparse_checkout', 'core_sparse_checkout_cone', and
>> 'get_sparse_checkout_filename()'). Doing it that way would both make it
>> easier for other commands to add a '--sparse-patterns' option, and avoid the
>> separate code path ('path_in_sparse_checkout_1()' vs.
>> 'recursively_match_path_with_sparse_patterns()', for example) when dealing
>> with '.git/info/sparse-checkout' patterns vs. manually-specified patterns.
>>
> 
> Thanks for the pointers. I'll see what I can do. Do you mean something
> along the line of the following?
> 
>    set_sparse_checkout_file(filename);
>    init_sparse_checkout_patterns(istate);
>    _ = path_in_sparse_checkout_1(some_path, istate, ...);
> 

> William Sprent wrote:
>> On 24/01/2023 21.11, Victoria Dye wrote>> I haven't looked at your implementation in detail yet, but I did want to
>>> offer a recommendation in case you hadn't considered it: if you want to
>>> allow the use of patterns from a user-specified specific file, it would be
>>> nice to do it in a way that fully replaces the "default" sparse-checkout
>>> settings at the lowest level (i.e., override the values of
>>> 'core_apply_sparse_checkout', 'core_sparse_checkout_cone', and
>>> 'get_sparse_checkout_filename()'). Doing it that way would both make it
>>> easier for other commands to add a '--sparse-patterns' option, and avoid the
>>> separate code path ('path_in_sparse_checkout_1()' vs.
>>> 'recursively_match_path_with_sparse_patterns()', for example) when dealing
>>> with '.git/info/sparse-checkout' patterns vs. manually-specified patterns.
>>>
>>
>> Thanks for the pointers. I'll see what I can do. Do you mean something
>> along the line of the following?
>>
>>     set_sparse_checkout_file(filename);
>>     init_sparse_checkout_patterns(istate);
>>     _ = path_in_sparse_checkout_1(some_path, istate, ...);
>>
> 
> Sort of. I mentioned separating the options into "specify the sparse pattern
> file" and "restrict the displayed files to the active pattern set, if there
> is one". For the former, you might add an option like:
> 
> 	OPT_FILENAME(0, "sparse-patterns", &sparse_pattern_file,
> 		     N_("override sparse-checkout behavior using patterns from <file>"))
> 
> Then do something like what you have, right after option parsing:
> 
> 	if (sparse_pattern_file) {
> 		core_apply_sparse_checkout = 1;
> 		core_sparse_checkout_cone = <???>;
> 		set_sparse_checkout_file(filename);
> 	}
> 
> If this option is specified, but the repo already has sparse
> patterns/settings of its own, you'll need to (carefully) override the repo's
> existing configuration:
> 
> * 'core_apply_sparse_checkout' & 'core_sparse_checkout_cone' are set based
>    on the repo config. You'll need to make sure those values are overridden
>    before loading the sparse-checkout patterns, and also that they're set
>    *after* loading the config.

> * Speaking of 'core_sparse_checkout_cone', there are a bunch of ways you
>    might configure "cone-" vs. "non-cone" for your patterns (user-specified
>    with yet another option, always assume one or the other, try deriving from
>    the patterns). My preference would be to always assume "cone mode" - it's
>    the direction Git has been moving with sparse-checkout over the past year,
>    and still automatically "falls back" on non-cone patterns if the patterns
>    can't be used in cone mode (with a warning from
>    'add_pattern_to_hashsets()': "disabling cone pattern matching").

> Fair warning: this probably isn't an exhaustive list of things that would
> need updating, and it'll need thorough testing to make sure there are no
> regressions. But, extending the underlying sparse-checkout infrastructure
> will (ideally) avoid duplicating code and make this behavior reusable across
> other commands.
> 

> For the other desired behavior ("limit the files to the active
> sparse-checkout patterns"), you could add an option:
> 
> 	OPT_CALLBACK_F(0, "scope", &sparse_scope, "(sparse|all)",
> 		       N_("specify the scope of results with respect to the sparse-checkout"),
> 		       PARSE_OPT_NONEG, option_parse_scope),
> 
> ...whose callback parses the string arg into a 'restrict_scope'
> boolean/enum/something. Then, wherever in 'ls-files' a tree or the index are
> iterated over, you can gate the per-file operation on:
> 
> 	if (!restrict_scope || path_in_sparse_checkout(<path>, istate)) {
> 		/* do something with <path> */
> 	}
> 
> Note that you should use 'path_in_sparse_checkout()', *not* the
> internal/private function 'path_in_sparse_checkout_1()'; you also don't need
> to explicitly 'init_sparse_checkout_patterns()'. Regardless of whether you
> specified custom patterns or are using the ones in
> '.git/info/sparse-checkout', 'path_in_sparse_checkout()' will initialize &
> use the appropriate patterns.
> 

>
> 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.

> To fill this gap, add a new argument to ls-tree '--sparse-filter-oid'
> which takes the object id of a blob containing sparse checkout patterns
> that filters the output of 'ls-tree'. When filtering with given sparsity
> patterns, 'ls-tree' only outputs blobs and commit objects that
> match the given patterns.

> While it may be valid in some situations to output a tree object -- e.g.
> when a cone pattern matches all blobs recursively contained in a tree --
> it is less unclear what should be output if a sparse pattern matches
> parts of a tree.
>
> To allow for reusing the pattern matching logic found in
> 'path_in_sparse_checkout_1()' in 'dir.c' with arbitrary patterns,
> extract the pattern matching part of the function into its own new
> function 'recursively_match_path_with_sparse_patterns()'.
>
> Signed-off-by: William Sprent <williams@unity3d.com>
> ---
>     ls-tree: add --sparse-filter-oid argument
>
>     I'm resubmitting this change as rebased on top of 'master', as it
>     conflicted with the topic 'ls-tree.c: clean-up works' 1
>     [https://public-inbox.org/git/20230112091135.20050-1-tenglong.tl@alibaba-inc.com],
>     which was merged to 'master' recently.
>
>     This versions also incorporates changes based on the comments made in 2
>     [https://public-inbox.org/git/CAPig+cRgZ0CrkqY7mufuWmhf6BC8yXjXXuOTEQjuz+Y0NA+N7Q@mail.gmail.com/].
>
>     I'm also looping in contributors that have touched ls-tree and/or
>     sparse-checkouts recently. I hope that's okay.

> +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)
> +{
> +       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;

> diff --git a/dir.c b/dir.c
> index 4e99f0c868f..122ebced08e 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1457,45 +1457,50 @@ int init_sparse_checkout_patterns(struct index_state *istate)
>         return 0;
>  }
>
> -static int path_in_sparse_checkout_1(const char *path,
> -                                    struct index_state *istate,
> -                                    int require_cone_mode)
> +int recursively_match_path_with_sparse_patterns(const char *path,

> +                                               struct index_state *istate,
> +                                               int dtype,
> +                                               struct pattern_list *pl)
>  {
> -       int dtype = DT_REG;
>         enum pattern_match_result match = UNDECIDED;
>         const char *end, *slash;
> -
> -       /*
> -        * We default to accepting a path if the path is empty, there are no
> -        * patterns, or the patterns are of the wrong type.
> -        */
> -       if (!*path ||
> -           init_sparse_checkout_patterns(istate) ||
> -           (require_cone_mode &&
> -            !istate->sparse_checkout_patterns->use_cone_patterns))
> -               return 1;
> -
>         /*
>          * If UNDECIDED, use the match from the parent dir (recursively), or
>          * fall back to NOT_MATCHED at the topmost level. Note that cone mode
>          * 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 */
>
>                 match = path_matches_pattern_list(path, end - path,
>                                 slash > path ? slash + 1 : path, &dtype,
> -                               istate->sparse_checkout_patterns, istate);
> +                               pl, istate);
>
>                 /* We are going to match the parent dir now */
>                 dtype = DT_DIR;
>         }
> -       return match > 0;
> +
> +       return match;

> +}
> +
> +static int path_in_sparse_checkout_1(const char *path,
> +                                    struct index_state *istate,
> +                                    int require_cone_mode)
> +{
> +       /*
> +        * We default to accepting a path if the path is empty, there are no
> +        * patterns, or the patterns are of the wrong type.
> +        */
> +       if (!*path ||
> +           init_sparse_checkout_patterns(istate) ||
> +           (require_cone_mode &&
> +            !istate->sparse_checkout_patterns->use_cone_patterns))
> +               return 1;
> +
> +       return recursively_match_path_with_sparse_patterns(path, istate, DT_REG, istate->sparse_checkout_patterns) > 0;

> It looks like Ævar and Victoria have both given really good reviews
> already, but I think I spotted some additional things to comment on.
> 
> On Mon, Jan 23, 2023 at 3:46 AM William Sprent via GitGitGadget
> <gitgitgadget@gmail.com> 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.
> 
> Could you say more about this usecase?  Why does tooling need or want
> to know this; won't a checkout of the new commit end up being quick
> and simple?  (I'm not saying your usecase is bad, just curious that it
> had never occurred to me, and I'm afraid I'm still not sure what your
> purpose might be.)
> 

>> Another interesting use-case would be for tooling to use in conjunction
>> with 'git update-index --index-info'.
> 
> Sorry, I'm not following.  Could you expound on this a bit?
> 

>> 'rev-list --objects --filter=sparse:oid' comes close, but as rev-list is
>> concerned with objects rather than directory trees, it leaves files out
>> when the same blob occurs in at two different paths.
> 
> s/in at/at/ ?
> >> It is possible to ask git about the sparse status of files currently in
>> the index with 'ls-files -t'. However, this does not work well when the
>> caller is interested in another commit, intererested in sparsity
> 
> s/intererested/interested/
> 
>> patterns that aren't currently in '.git/info/sparse-checkout', or when
>> working in with bare repo.
> 
> s/in with bare/with a bare/ or s/in with bare/in a bare/?
> 

>> To fill this gap, add a new argument to ls-tree '--sparse-filter-oid'
>> which takes the object id of a blob containing sparse checkout patterns
>> that filters the output of 'ls-tree'. When filtering with given sparsity
>> patterns, 'ls-tree' only outputs blobs and commit objects that
>> match the given patterns.
> 
> This seems slightly unfortunate in that it makes things difficult for
> cone mode users to take advantage of.  They will have to figure out
> how to translate their directory list into sparse checkout patterns
> before passing it along, and currently the only way to do that is via
> `git sparse-checkout set <patterns>` and reading the patterns from
> $GIT_DIR/info/sparse-checkout, but that toggles the sparsity of the
> current working tree and avoiding changing the current sparse-checkout
> was something you listed in your commit message as something you
> wanted to avoid.
>  >> While it may be valid in some situations to output a tree object -- e.g.
>> when a cone pattern matches all blobs recursively contained in a tree --
>> it is less unclear what should be output if a sparse pattern matches
>> parts of a tree.
>>
>> To allow for reusing the pattern matching logic found in
>> 'path_in_sparse_checkout_1()' in 'dir.c' with arbitrary patterns,
>> extract the pattern matching part of the function into its own new
>> function 'recursively_match_path_with_sparse_patterns()'.
>>
>> Signed-off-by: William Sprent <williams@unity3d.com>
>> ---
>>      ls-tree: add --sparse-filter-oid argument
>>
>>      I'm resubmitting this change as rebased on top of 'master', as it
>>      conflicted with the topic 'ls-tree.c: clean-up works' 1
>>      [https://public-inbox.org/git/20230112091135.20050-1-tenglong.tl@alibaba-inc.com],
>>      which was merged to 'master' recently.
>>
>>      This versions also incorporates changes based on the comments made in 2
>>      [https://public-inbox.org/git/CAPig+cRgZ0CrkqY7mufuWmhf6BC8yXjXXuOTEQjuz+Y0NA+N7Q@mail.gmail.com/].
>>
>>      I'm also looping in contributors that have touched ls-tree and/or
>>      sparse-checkouts recently. I hope that's okay.
> 
> Of course!  It's encouraged, even.
> 
> [...]
>> diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
> [...]
>> +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)
>> +{
>> +       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;
> 
> Hmm, so the behavior still depends upon the current sparse-checkout
> (or lack thereof), despite the documentation and rationale of your
> feature as being there to check how a different sparse checkout would
> behave?
> 
> I would hate to unconditionally turn cone_patterns off, since that
> would come with a huge performance penalty for the biggest repos.  But
> turning it unconditionally on wouldn't be good for the non-cone users.
> This probably suggests we need something like another flag, or perhaps
> separate flags for each mode.  Separate flags might provide the
> benefit of allowing cone mode users to specify directories rather than
> patterns, which would make it much easier for them to use.
>

> [...]
>> +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);
>> +       return match > 0;
> 
> So your new caller doesn't care about the pattern_match_result, it
> just wants to know if it got MATCHED or MATCHED_RECURSIVELY...
> 
> [...]
>> diff --git a/dir.c b/dir.c
>> index 4e99f0c868f..122ebced08e 100644
>> --- a/dir.c
>> +++ b/dir.c
>> @@ -1457,45 +1457,50 @@ int init_sparse_checkout_patterns(struct index_state *istate)
>>          return 0;
>>   }
>>
>> -static int path_in_sparse_checkout_1(const char *path,
>> -                                    struct index_state *istate,
>> -                                    int require_cone_mode)
>> +int recursively_match_path_with_sparse_patterns(const char *path,
> 
> You claim it returns an int here, but previously you presumed an enum
> pattern_match_result from the new caller.
> 
>> +                                               struct index_state *istate,
>> +                                               int dtype,
>> +                                               struct pattern_list *pl)
>>   {
>> -       int dtype = DT_REG;
>>          enum pattern_match_result match = UNDECIDED;
>>          const char *end, *slash;
>> -
>> -       /*
>> -        * We default to accepting a path if the path is empty, there are no
>> -        * patterns, or the patterns are of the wrong type.
>> -        */
>> -       if (!*path ||
>> -           init_sparse_checkout_patterns(istate) ||
>> -           (require_cone_mode &&
>> -            !istate->sparse_checkout_patterns->use_cone_patterns))
>> -               return 1;
>> -
>>          /*
>>           * If UNDECIDED, use the match from the parent dir (recursively), or
>>           * fall back to NOT_MATCHED at the topmost level. Note that cone mode
>>           * 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 */
>>
>>                  match = path_matches_pattern_list(path, end - path,
>>                                  slash > path ? slash + 1 : path, &dtype,
>> -                               istate->sparse_checkout_patterns, istate);
>> +                               pl, istate);
>>
>>                  /* We are going to match the parent dir now */
>>                  dtype = DT_DIR;
>>          }
>> -       return match > 0;
>> +
>> +       return match;
> 
> Um, this last line seems like a potentially scary change in behavior.
> Why should UNDECIDED return a non-zero value?  Previously, we returned
> a 0 value for both UNDECIDED and NOT_MATCHED, but you've changed that
> here.  If the change in this last line is actually correct, it should
> be split out into its own commit and explained in detail in the commit
> message.
> 
>> +}
>> +
>> +static int path_in_sparse_checkout_1(const char *path,
>> +                                    struct index_state *istate,
>> +                                    int require_cone_mode)
>> +{
>> +       /*
>> +        * We default to accepting a path if the path is empty, there are no
>> +        * patterns, or the patterns are of the wrong type.
>> +        */
>> +       if (!*path ||
>> +           init_sparse_checkout_patterns(istate) ||
>> +           (require_cone_mode &&
>> +            !istate->sparse_checkout_patterns->use_cone_patterns))
>> +               return 1;
>> +
>> +       return recursively_match_path_with_sparse_patterns(path, istate, DT_REG, istate->sparse_checkout_patterns) > 0;
> 
> Oh, you compare to > 0 here...and digging around your only other
> caller just immediately compares to > 0 as well.
> 
> Why not just have recursively_match_path_with_sparse_patterns() do the
>> 0 check?  If it does, returning int is fine.  If it doesn't, it
> should be declared to return enum pattern_match_result.

>
> On 25/01/2023 06.11, Elijah Newren wrote:
> > It looks like Ævar and Victoria have both given really good reviews
> > already, but I think I spotted some additional things to comment on.
> >
> > On Mon, Jan 23, 2023 at 3:46 AM William Sprent via GitGitGadget
> > <gitgitgadget@gmail.com> 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.
> >
> > Could you say more about this usecase?  Why does tooling need or want
> > to know this; won't a checkout of the new commit end up being quick
> > and simple?  (I'm not saying your usecase is bad, just curious that it
> > had never occurred to me, and I'm afraid I'm still not sure what your
> > purpose might be.)
> >
>
> I'm thinking mainly about a monorepo context where there are a number of
> distinct 'units' that can be described with sparse checkout patterns.
> And perhaps there's some tooling that only wants to perform an action if
> the content of a 'unit' changes.

> Depending on the repo, it won't necessarily be quick to check out the
> commit with the given patterns. However, it is more about it being
> inconvenient to have to have a working directory, especially so if you
> want use the tooling in some kind of service or query rapidly about
> different revisions/patterns.
>
> >> Another interesting use-case would be for tooling to use in conjunction
> >> with 'git update-index --index-info'.
> >
> > Sorry, I'm not following.  Could you expound on this a bit?
> >
>
> I was imagining something along the lines of being able to generate new
> tree objects based on what matches the given sparse checkout patterns.
> Not that I have a specific use case for it right now.
>
> I think what I'm trying to evoke with that paragraph is that this
> enables integrations with git that seem interesting and weren't possible
> before.

> >> +       (*d)->pl.use_cone_patterns = core_sparse_checkout_cone;
> >
> > Hmm, so the behavior still depends upon the current sparse-checkout
> > (or lack thereof), despite the documentation and rationale of your
> > feature as being there to check how a different sparse checkout would
> > behave?
> >
> > I would hate to unconditionally turn cone_patterns off, since that
> > would come with a huge performance penalty for the biggest repos.  But
> > turning it unconditionally on wouldn't be good for the non-cone users.
> > This probably suggests we need something like another flag, or perhaps
> > separate flags for each mode.  Separate flags might provide the
> > benefit of allowing cone mode users to specify directories rather than
> > patterns, which would make it much easier for them to use.
> >
> I used 'core_sparse_checkout_cone' because I wanted to allow for the
> cone mode optimisations, but I also figured that I should respect the
> configuration. It doesn't change how the patterns are parsed in this case.
>
> I agree that it is a bit awkward to have to "translate" the directories
> into patterns when wanting to use cone mode. I can try adding
> '--[no]-cone' flags and see how that feels. Together with Victoria's
> suggestions that would result in having the following flags:
>
> * --scope=(sparse|all)
> * --sparse-patterns-file=<path>
> * --[no]-cone: used together with --sparse-patterns-file to tell git
>    whether to interpret the patterns given as directories (cone) or
>    patterns (no-cone).
>
> Which seems like a lot at first glance. But it allows for passing
> directories instead of patterns for cone mode, and is similar to the
> behaviour of 'sparse-checkout set'.
>
> Does that seem like something that would make sense?

> On Wed, Jan 25, 2023 at 8:16 AM William Sprent <williams@unity3d.com> wrote:
>>
>> On 25/01/2023 06.11, Elijah Newren wrote:
>>> It looks like Ævar and Victoria have both given really good reviews
>>> already, but I think I spotted some additional things to comment on.
>>>
>>> On Mon, Jan 23, 2023 at 3:46 AM William Sprent via GitGitGadget
>>> <gitgitgadget@gmail.com> 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.
>>>
>>> Could you say more about this usecase?  Why does tooling need or want
>>> to know this; won't a checkout of the new commit end up being quick
>>> and simple?  (I'm not saying your usecase is bad, just curious that it
>>> had never occurred to me, and I'm afraid I'm still not sure what your
>>> purpose might be.)
>>>
>>
>> I'm thinking mainly about a monorepo context where there are a number of
>> distinct 'units' that can be described with sparse checkout patterns.
>> And perhaps there's some tooling that only wants to perform an action if
>> the content of a 'unit' changes.
> 
> So, you're basically wanting to do something like
>     git ls-tree --paths-matching-sparsity-file=<pattern-file> $COMMIT1
>> sparse-files
>     git ls-tree --paths-matching-sparsity-file=<pattern-file> $COMMIT2
>>> sparse-files
>     sort sparse-files | uniq >relevant-files
>     git diff --name-only $COMMIT1 $COMMIT2 >changed-files
> and then checking whether relevant-files and changed-files have a
> non-empty intersection?

> Would that potentially be better handled by
>     git diff --name-only $COMMIT1 $COMMIT2 | git check-ignore
> --ignore-file=<pattern-file> --stdin
> and seeing whether the output is non-empty?  We'd have to add an
> "--ignore-file" option to check-ignore to override reading of
> .gitignore files and such, and it'd be slightly confusing because the
> documentation talks about "ignored" files rather than "selected"
> files, but that's a confusion point that has been with us ever since
> the gitignore mechanism was repurposed for sparse checkouts.  Or maybe
> we could just add a check-sparsity helper, and then allow it to take
> directories in-lieu of patterns. 

>> Depending on the repo, it won't necessarily be quick to check out the
>> commit with the given patterns. However, it is more about it being
>> inconvenient to have to have a working directory, especially so if you
>> want use the tooling in some kind of service or query rapidly about
>> different revisions/patterns.
>>
>>>> Another interesting use-case would be for tooling to use in conjunction
>>>> with 'git update-index --index-info'.
>>>
>>> Sorry, I'm not following.  Could you expound on this a bit?
>>>
>>
>> I was imagining something along the lines of being able to generate new
>> tree objects based on what matches the given sparse checkout patterns.
>> Not that I have a specific use case for it right now.
>>
>> I think what I'm trying to evoke with that paragraph is that this
>> enables integrations with git that seem interesting and weren't possible
>> before.
> 
> I'm not sure if it's interesting, frightening, or something else.
> Hard to say without better descriptions of usecases, which we can't
> have if we don't even have a usecase.  I think I'd just strike this
> paragraph.
> 
> [...]

>>>> +       (*d)->pl.use_cone_patterns = core_sparse_checkout_cone;
>>>
>>> Hmm, so the behavior still depends upon the current sparse-checkout
>>> (or lack thereof), despite the documentation and rationale of your
>>> feature as being there to check how a different sparse checkout would
>>> behave?
>>>
>>> I would hate to unconditionally turn cone_patterns off, since that
>>> would come with a huge performance penalty for the biggest repos.  But
>>> turning it unconditionally on wouldn't be good for the non-cone users.
>>> This probably suggests we need something like another flag, or perhaps
>>> separate flags for each mode.  Separate flags might provide the
>>> benefit of allowing cone mode users to specify directories rather than
>>> patterns, which would make it much easier for them to use.
>>>
>> I used 'core_sparse_checkout_cone' because I wanted to allow for the
>> cone mode optimisations, but I also figured that I should respect the
>> configuration. It doesn't change how the patterns are parsed in this case.
>>
>> I agree that it is a bit awkward to have to "translate" the directories
>> into patterns when wanting to use cone mode. I can try adding
>> '--[no]-cone' flags and see how that feels. Together with Victoria's
>> suggestions that would result in having the following flags:
>>
>> * --scope=(sparse|all)
>> * --sparse-patterns-file=<path>
>> * --[no]-cone: used together with --sparse-patterns-file to tell git
>>     whether to interpret the patterns given as directories (cone) or
>>     patterns (no-cone).
>>
>> Which seems like a lot at first glance. But it allows for passing
>> directories instead of patterns for cone mode, and is similar to the
>> behaviour of 'sparse-checkout set'.
>>
>> Does that seem like something that would make sense?
> 
> --sparse-patterns-file still implies patterns; I think that would need
> some rewording.

>
> On 26/01/2023 04.25, Elijah Newren wrote:
> > On Wed, Jan 25, 2023 at 8:16 AM William Sprent <williams@unity3d.com> wrote:
> >>
> >> On 25/01/2023 06.11, Elijah Newren wrote:
> >>> It looks like Ævar and Victoria have both given really good reviews
> >>> already, but I think I spotted some additional things to comment on.
> >>>
> >>> On Mon, Jan 23, 2023 at 3:46 AM William Sprent via GitGitGadget
> >>> <gitgitgadget@gmail.com> 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.
> >>>
> >>> Could you say more about this usecase?  Why does tooling need or want
> >>> to know this; won't a checkout of the new commit end up being quick
> >>> and simple?  (I'm not saying your usecase is bad, just curious that it
> >>> had never occurred to me, and I'm afraid I'm still not sure what your
> >>> purpose might be.)
> >>>
> >>
> >> I'm thinking mainly about a monorepo context where there are a number of
> >> distinct 'units' that can be described with sparse checkout patterns.
> >> And perhaps there's some tooling that only wants to perform an action if
> >> the content of a 'unit' changes.
> >
> > So, you're basically wanting to do something like
> >     git ls-tree --paths-matching-sparsity-file=<pattern-file> $COMMIT1
> >> sparse-files
> >     git ls-tree --paths-matching-sparsity-file=<pattern-file> $COMMIT2
> >>> sparse-files
> >     sort sparse-files | uniq >relevant-files
> >     git diff --name-only $COMMIT1 $COMMIT2 >changed-files
> > and then checking whether relevant-files and changed-files have a
> > non-empty intersection?
>
> Well, the concrete use-case I'm exploring is something along the lines
> of using the content hashes of sparse checkouts as cache keys for resource
> heavy jobs (builds/tests/etc).
>
> So, that would be something along the lines of,
>
>      git ls-tree -r --paths-matching-sparsity-file=<pattern-file> \
>      | sha1sum > cache-key
>
> and then performing a lookup before performing an action (which would
> then only be done in the context of the sparse checkout). My thinking
> is that this only would require git and no additional tooling, which in
> turn makes it very easy to reproduce the state where the job took place.
>
>
> > Would that potentially be better handled by
> >     git diff --name-only $COMMIT1 $COMMIT2 | git check-ignore
> > --ignore-file=<pattern-file> --stdin
> > and seeing whether the output is non-empty?  We'd have to add an
> > "--ignore-file" option to check-ignore to override reading of
> > .gitignore files and such, and it'd be slightly confusing because the
> > documentation talks about "ignored" files rather than "selected"
> > files, but that's a confusion point that has been with us ever since
> > the gitignore mechanism was repurposed for sparse checkouts.  Or maybe
> > we could just add a check-sparsity helper, and then allow it to take
> > directories in-lieu of patterns.
>
> I don't think it necessarily would be better handled by that. But it would
> be workable. It would be a matter of collating the output of
>
>    git ls-tree -r <commit>
>
> with
>
>    git ls-tree --name-only -r <commit> | git check-ignore ...
>
> Which is less ergonomic. But it is also a less intrusive change.
>
> Really, the main thing is to expose the sparse filtering logic somehow, and
> allow for building tooling on top of it.
> > This seems nicer than opening a can of worms about letting every git
> > command specify a different set of sparsity rules.
>
> I think you are the better judge of how much of a can of worms that would
> be. I don't think it would be too out of line with how git acts in general
> though, as we have things like the the 'GIT_INDEX_FILE' env-var.

> >> I agree that it is a bit awkward to have to "translate" the directories
> >> into patterns when wanting to use cone mode. I can try adding
> >> '--[no]-cone' flags and see how that feels. Together with Victoria's
> >> suggestions that would result in having the following flags:
> >>
> >> * --scope=(sparse|all)
> >> * --sparse-patterns-file=<path>
> >> * --[no]-cone: used together with --sparse-patterns-file to tell git
> >>     whether to interpret the patterns given as directories (cone) or
> >>     patterns (no-cone).
> >>
> >> Which seems like a lot at first glance. But it allows for passing
> >> directories instead of patterns for cone mode, and is similar to the
> >> behaviour of 'sparse-checkout set'.
> >>
> >> Does that seem like something that would make sense?
> >
> > --sparse-patterns-file still implies patterns; I think that would need
> > some rewording.
>
> Yeah. After sleeping on it, I also think that it becomes a difficult
> interface to work with, and you'll get different results with the same
> patterns whether you pass --cone or --no-cone, which seems error prone
> to me.
>
> For better or for worse, both cone and non-cone uses of sparse-checkouts
> end up producing pattern files. And those pattern files do unambiguously
> describe a filtering of the worktree whether it is in cone-mode or not.

> Given that 'ls-tree' is more of a plumbing command, I think it might still
> make sense to use the patterns. That would also make the interaction
> a bit more logical to me -- e.g. if you want to override the patterns
> you have to pass them in the same format as the ones that would be read
> by default.

> On Fri, Jan 27, 2023 at 3:59 AM William Sprent <williams@unity3d.com> wrote:
>>
>> On 26/01/2023 04.25, Elijah Newren wrote:
>>> On Wed, Jan 25, 2023 at 8:16 AM William Sprent <williams@unity3d.com> wrote:
>>>>
>>>> On 25/01/2023 06.11, Elijah Newren wrote:
>>>>> It looks like Ævar and Victoria have both given really good reviews
>>>>> already, but I think I spotted some additional things to comment on.
>>>>>
>>>>> On Mon, Jan 23, 2023 at 3:46 AM William Sprent via GitGitGadget
>>>>> <gitgitgadget@gmail.com> 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.
>>>>>
>>>>> Could you say more about this usecase?  Why does tooling need or want
>>>>> to know this; won't a checkout of the new commit end up being quick
>>>>> and simple?  (I'm not saying your usecase is bad, just curious that it
>>>>> had never occurred to me, and I'm afraid I'm still not sure what your
>>>>> purpose might be.)
>>>>>
>>>>
>>>> I'm thinking mainly about a monorepo context where there are a number of
>>>> distinct 'units' that can be described with sparse checkout patterns.
>>>> And perhaps there's some tooling that only wants to perform an action if
>>>> the content of a 'unit' changes.
>>>
>>> So, you're basically wanting to do something like
>>>      git ls-tree --paths-matching-sparsity-file=<pattern-file> $COMMIT1
>>>> sparse-files
>>>      git ls-tree --paths-matching-sparsity-file=<pattern-file> $COMMIT2
>>>>> sparse-files
>>>      sort sparse-files | uniq >relevant-files
>>>      git diff --name-only $COMMIT1 $COMMIT2 >changed-files
>>> and then checking whether relevant-files and changed-files have a
>>> non-empty intersection?
>>
>> Well, the concrete use-case I'm exploring is something along the lines
>> of using the content hashes of sparse checkouts as cache keys for resource
>> heavy jobs (builds/tests/etc).
>>
>> So, that would be something along the lines of,
>>
>>       git ls-tree -r --paths-matching-sparsity-file=<pattern-file> \
>>       | sha1sum > cache-key
>>
>> and then performing a lookup before performing an action (which would
>> then only be done in the context of the sparse checkout). My thinking
>> is that this only would require git and no additional tooling, which in
>> turn makes it very easy to reproduce the state where the job took place.
>>
>>
>>> Would that potentially be better handled by
>>>      git diff --name-only $COMMIT1 $COMMIT2 | git check-ignore
>>> --ignore-file=<pattern-file> --stdin
>>> and seeing whether the output is non-empty?  We'd have to add an
>>> "--ignore-file" option to check-ignore to override reading of
>>> .gitignore files and such, and it'd be slightly confusing because the
>>> documentation talks about "ignored" files rather than "selected"
>>> files, but that's a confusion point that has been with us ever since
>>> the gitignore mechanism was repurposed for sparse checkouts.  Or maybe
>>> we could just add a check-sparsity helper, and then allow it to take
>>> directories in-lieu of patterns.
>>
>> I don't think it necessarily would be better handled by that. But it would
>> be workable. It would be a matter of collating the output of
>>
>>     git ls-tree -r <commit>
>>
>> with
>>
>>     git ls-tree --name-only -r <commit> | git check-ignore ...
>>
>> Which is less ergonomic. But it is also a less intrusive change.
>>
>> Really, the main thing is to expose the sparse filtering logic somehow, and
>> allow for building tooling on top of it.
>>> This seems nicer than opening a can of worms about letting every git
>>> command specify a different set of sparsity rules.
>>
>> I think you are the better judge of how much of a can of worms that would
>> be. I don't think it would be too out of line with how git acts in general
>> though, as we have things like the the 'GIT_INDEX_FILE' env-var.
> 
> I agree you've got a reasonable usecase here.  The integration you've
> described sounds like something that could be independently composable
> with several other commands, and you alluded to that in your commit
> message.  But is adding it to ls-tree the best spot?  If we add it
> there, then folks who want to similarly integrate such capabilities
> with other commands (archive, diff, grep, rev-list --objects, etc.),
> seem more likely to do so via direct integration.  We already have a
> very large can of worms to work on to make commands behave in ways
> that are limited to sparse paths (see
> Documentation/techncial/sparse-checkout.txt, namely the "behavior A"
> stuff).  As can be seen in that document, what to do for limiting
> commands to sparse paths is obvious with some commands but has lots of
> interesting edge cases for others (even with years of experience with
> sparse checkouts, we had 3- and 4- way differing opinions on the
> intended behavior for some commands when we started composing that
> document a few months ago).  If we had jumped straight to
> implementation for some commands, we would have likely painted
> ourselves into a corner for other commands.  Adding another layer of
> specifying an alternate set of sparsity rules will likely have
> interesting knock-on effects that we should think through for all the
> commands to ensure we aren't painting ourselves into a similar corner,
> if we go down this route.
> 
> However, in the cases that sparse-checkout.txt document was
> addressing, the behavior fundamentally needs to be integrated into all
> the relevant commands to get user experience right.  In your case, you
> merely need a separate tool to be able to compose the output of
> different commands together.  So, exposing whether sparsity rules
> would select various paths in a single separate command (maybe git
> check-ignore, or a new sparse-checkout subcommand, or maybe just a new
> subcommand similar to check-ignore) would avoid a lot of these issues,
> and give us a single place to extend/improve as we learn about further
> usecases.

> 
> [...]
>>>> I agree that it is a bit awkward to have to "translate" the directories
>>>> into patterns when wanting to use cone mode. I can try adding
>>>> '--[no]-cone' flags and see how that feels. Together with Victoria's
>>>> suggestions that would result in having the following flags:
>>>>
>>>> * --scope=(sparse|all)
>>>> * --sparse-patterns-file=<path>
>>>> * --[no]-cone: used together with --sparse-patterns-file to tell git
>>>>      whether to interpret the patterns given as directories (cone) or
>>>>      patterns (no-cone).
>>>>
>>>> Which seems like a lot at first glance. But it allows for passing
>>>> directories instead of patterns for cone mode, and is similar to the
>>>> behaviour of 'sparse-checkout set'.
>>>>
>>>> Does that seem like something that would make sense?
>>>
>>> --sparse-patterns-file still implies patterns; I think that would need
>>> some rewording.
>>
>> Yeah. After sleeping on it, I also think that it becomes a difficult
>> interface to work with, and you'll get different results with the same
>> patterns whether you pass --cone or --no-cone, which seems error prone
>> to me.
>>
>> For better or for worse, both cone and non-cone uses of sparse-checkouts
>> end up producing pattern files. And those pattern files do unambiguously
>> describe a filtering of the worktree whether it is in cone-mode or not.
> 
> Back when cone mode was introduced, I objected to reusing the existing
> pattern format and/or files...but was assuaged that it was just an
> implementation detail that wouldn't be exposed to users (since people
> would interact with the 'sparse-checkout' command instead of direct
> editing of $GIT_DIR/info/sparse-checkout).  It's still a performance
> penalty, because we waste time both turning directory names into
> patterns when we write them out, and when we read them in by parsing
> the patterns to see if they match cone mode rules and if so discard
> the patterns and just build up our hashes.  The patterns are nothing
> more than an intermediary format we waste time translating to and
> from, though once upon a time they existed so that if someone suddenly
> started using an older (now ancient) version of git on their current
> checkout then it could still hobble along with degraded performance
> instead of providing an error.  (We have introduced other changes to
> the config and git index which would cause older git clients to just
> fail to parse and throw an error, and even have defined mechanisms for
> such erroring out.  We could have taken advantage of that for this
> feature too.)
> 
> Anyway, long story short, if you're going to start exposing users to
> this implementation detail that was meant to be hidden (and do it in a
> way that may be copied into several commands to boot), then I
> definitely object.

>> Given that 'ls-tree' is more of a plumbing command, I think it might still
>> make sense to use the patterns. That would also make the interaction
>> a bit more logical to me -- e.g. if you want to override the patterns
>> you have to pass them in the same format as the ones that would be read
>> by default.
> 
> No, sparsity specification should be provided by users the same way
> they normally specify it (i.e. the way they pass it to
> `sparse-checkout {add,set}`), not the way it's stored via some hidden
> implementation detail.
> 
> I'd make an exception if you ended up using `git check-ignore` because
> that command was specifically written about gitignore-style rules, and
> git-ignore-style rules just happen to have been reused as-is for
> non-cone-mode sparse-checkout rules.  But if you go that route:
>     (1) you have to frame any extensions to check-ignore as things that
> are useful for gitignore checking, and only incidentally useful to
> sparse-checkouts
>     (2) you'd have to forgo any cone-mode optimizations
> Going the check-ignore route seems like the easiest path to providing
> the basic functionality you need right now.  If your usecases remain
> niche, that might be good enough for all time too.  If your usecases
> become popular, though, I expect someone would eventually tire of
> using `check-ignore` and implement similar functionality along with
> supporting cone-mode in a `git check-sparsity` or `git sparse-checkout
> debug-rules` command or something like that.