{"thread":{"id":"59071","subject":"[PATCH] ls-tree: add --sparse-filter-oid argument","startedAt":"2023-01-11T17:05:08Z","lastAt":"2023-01-30T15:28:17Z","messageCount":20,"participants":["William Sprent via GitGitGadget","Eric Sunshine","Junio C Hamano","William Sprent","Ævar Arnfjörð Bjarmason","Victoria Dye","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"470094","messageId":"pull.1459.git.1673456518993.gitgitgadget@gmail.com","threadId":"59071","inReplyTo":null,"subject":"[PATCH] ls-tree: add --sparse-filter-oid argument","fromName":"William Sprent via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-11T17:01:58Z","receivedAt":"2023-01-11T17:05:08Z","isPatch":true,"sender":{"key":"williams@unity3d.com","avatar":"https://avatars.githubusercontent.com/u/77490534?v=4"},"body":"From: William Sprent \n\nThere is currently no way to ask git the question \"which files would be\npart of a sparse checkout of commit X with sparse checkout patterns Y\".\nOne use-case would be that tooling may want know whether sparse checkouts\nof two commits contain the same content even if the full trees differ.\nAnother interesting use-case would be for tooling to use in conjunction\nwith 'git update-index --index-info'.\n\n'rev-list --objects --filter=sparse:oid' comes close, but as rev-list is\nconcerned with objects rather than directory trees, it leaves files out\nwhen the same blob occurs in at two different paths.\n\nIt is possible to ask git about the sparse status of files currently in\nthe index with 'ls-files -t'. However, this does not work well when the\ncaller is interested in another commit, intererested in sparsity\npatterns that aren't currently in '.git/info/sparse-checkout', or when\nworking in with bare repo.\n\nTo fill this gap, add a new argument to ls-tree '--sparse-filter-oid'\nwhich takes the object id of a blob containing sparse checkout patterns\nthat filters the output of 'ls-tree'. When filtering with given sparsity\npatterns, 'ls-tree' only outputs blobs and commit objects that\nmatch the given patterns.\n\nWhile it may be valid in some situations to output a tree object -- e.g.\nwhen a cone pattern matches all blobs recursively contained in a tree --\nit is less unclear what should be output if a sparse pattern matches\nparts of a tree.\n\nTo allow for reusing the pattern matching logic found in\n'path_in_sparse_checkout_1()' in 'dir.c' with arbitrary patterns,\nextract the pattern matching part of the function into its own new\nfunction 'recursively_match_path_with_sparse_patterns()'.\n\nSigned-off-by: William Sprent \n---\n [RFC] ls-tree: add --sparse-filter-oid argument\n \n Hi,\n \n I found the need for being able to query git about the contents of\n sparse checkouts for arbitrary commits with arbitrary patterns from a\n bare repository. And it seems to me that ls-tree would be the best fit\n for answering those kinds of questions.\n \n I think it makes sense to have git be able to answer these kinds of\n questions, but I haven't been able to find any previous discussions\n about it. And 'sparse-checkout.txt' is mostly around working with the\n patterns in '.git/info/sparse-checkout'. So I figure this feature maybe\n needs a bit of discussion before landing.\n \n I'm also not very well versed in the code base, so please let me know if\n my approach here is off in general. :)\n \n Note that one of the tests only pass when run on top of commit\n 5842710dc2 (dir: check for single file cone patterns, 2023-01-03), which\n was submitted separately and is currently is merged to 'next'.\n \n Thanks, William\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1459%2Fwilliams-unity%2Fls-tree-sparse-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1459/williams-unity/ls-tree-sparse-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1459\n\n Documentation/git-ls-tree.txt | 5 ++\n builtin/ls-tree.c | 87 +++++++++++++++++++++++-\n dir.c | 46 +++++++------\n dir.h | 5 ++\n t/t3106-ls-tree-sparse-checkout.sh | 103 +++++++++++++++++++++++++++++\n 5 files changed, 224 insertions(+), 22 deletions(-)\n create mode 100755 t/t3106-ls-tree-sparse-checkout.sh\n\ndiff --git a/Documentation/git-ls-tree.txt b/Documentation/git-ls-tree.txt\nindex 0240adb8eec..69cbfb278eb 100644\n--- a/Documentation/git-ls-tree.txt\n+++ b/Documentation/git-ls-tree.txt\n@@ -48,6 +48,11 @@ OPTIONS\n \tShow tree entries even when going to recurse them. Has no effect\n \tif `-r` was not passed. `-d` implies `-t`.\n \n+--sparse-filter-oid=::\n+\tOmit showing tree objects and paths that do not match the\n+\tsparse-checkout specification contained in the blob\n+\t(or blob-expression) .\n+\n -l::\n --long::\n \tShow object size of blob (file) entries.\ndiff --git a/builtin/ls-tree.c b/builtin/ls-tree.c\nindex c3ea09281af..8f1fd5c26c2 100644\n--- a/builtin/ls-tree.c\n+++ b/builtin/ls-tree.c\n@@ -7,12 +7,14 @@\n #include \"config.h\"\n #include \"object-store.h\"\n #include \"blob.h\"\n+#include \"repository.h\"\n #include \"tree.h\"\n #include \"commit.h\"\n #include \"quote.h\"\n #include \"builtin.h\"\n #include \"parse-options.h\"\n #include \"pathspec.h\"\n+#include \"dir.h\"\n \n static int line_termination = '\\n';\n #define LS_RECURSIVE 1\n@@ -329,12 +331,79 @@ static struct ls_tree_cmdmode_to_fmt ls_tree_cmdmode_format[] = {\n \t},\n };\n \n+struct sparse_filter_data {\n+\tread_tree_fn_t fn;\n+\tstruct pattern_list pl;\n+\tstruct strbuf full_path_buf;\n+};\n+\n+static void sparse_filter_data__init(\n+\tstruct sparse_filter_data **d,\n+\tconst char *sparse_oid_name, read_tree_fn_t fn)\n+{\n+\tstruct object_id sparse_oid;\n+\tstruct object_context oc;\n+\n+\t(*d) = xcalloc(1, sizeof(**d));\n+\t(*d)->fn = fn;\n+\t(*d)->pl.use_cone_patterns = core_sparse_checkout_cone;\n+\n+\tstrbuf_init(&(*d)->full_path_buf, 0);\n+\n+\n+\tif (get_oid_with_context(the_repository,\n+\t\t\t\t sparse_oid_name,\n+\t\t\t\t GET_OID_BLOB, &sparse_oid, &oc))\n+\t\tdie(_(\"unable to access sparse blob in '%s'\"), sparse_oid_name);\n+\tif (add_patterns_from_blob_to_list(&sparse_oid, \"\", 0, &(*d)->pl) < 0)\n+\t\tdie(_(\"unable to parse sparse filter data in %s\"),\n+\t\t oid_to_hex(&sparse_oid));\n+}\n+\n+static void sparse_filter_data__free(struct sparse_filter_data *d)\n+{\n+\tclear_pattern_list(&d->pl);\n+\tstrbuf_release(&d->full_path_buf);\n+\tfree(d);\n+}\n+\n+static int path_matches_sparse_checkout_patterns(struct strbuf *full_path, struct pattern_list *pl, int dtype)\n+{\n+\tenum pattern_match_result match = recursively_match_path_with_sparse_patterns(full_path->buf, the_repository->index, dtype, pl);\n+\treturn match > 0;\n+}\n+\n+\n+static int filter_sparse(const struct object_id *oid, struct strbuf *base,\n+\t\t\t const char *pathname, unsigned mode, void *context)\n+{\n+\n+\tstruct sparse_filter_data *data = context;\n+\n+\tint do_recurse = show_recursive(base->buf, base->len, pathname);\n+\tif (object_type(mode) == OBJ_TREE)\n+\t\treturn do_recurse;\n+\n+\tstrbuf_reset(&data->full_path_buf);\n+\tstrbuf_addbuf(&data->full_path_buf, base);\n+\tstrbuf_addstr(&data->full_path_buf, pathname);\n+\n+\tif (!path_matches_sparse_checkout_patterns(&data->full_path_buf, &data->pl, DT_REG))\n+\t\treturn 0;\n+\n+\treturn data->fn(oid, base, pathname, mode, context);\n+}\n+\n+\n int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n {\n \tstruct object_id oid;\n \tstruct tree *tree;\n-\tint i, full_tree = 0;\n+\tint ret, i, full_tree = 0;\n \tread_tree_fn_t fn = NULL;\n+\tchar *sparse_oid_name = NULL;\n+\tvoid *read_tree_context = NULL;\n+\tstruct sparse_filter_data *sparse_filter_data = NULL;\n \tconst struct option ls_tree_options[] = {\n \t\tOPT_BIT('d', NULL, &ls_options, N_(\"only show trees\"),\n \t\t\tLS_TREE_ONLY),\n@@ -360,6 +429,9 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n \t\tOPT_STRING_F(0, \"format\", &format, N_(\"format\"),\n \t\t\t\t\t N_(\"format to use for the output\"),\n \t\t\t\t\t PARSE_OPT_NONEG),\n+\t\tOPT_STRING_F(0, \"filter-sparse-oid\", &sparse_oid_name, N_(\"filter-sparse-oid\"),\n+\t\t\t N_(\"filter output with sparse-checkout specification contained in the blob\"),\n+\t\t\t PARSE_OPT_NONEG),\n \t\tOPT__ABBREV(&abbrev),\n \t\tOPT_END()\n \t};\n@@ -433,5 +505,16 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n \t\tbreak;\n \t}\n \n-\treturn !!read_tree(the_repository, tree, &pathspec, fn, NULL);\n+\tif (sparse_oid_name) {\n+\t\tsparse_filter_data__init(&sparse_filter_data, sparse_oid_name, fn);\n+\t\tread_tree_context = sparse_filter_data;\n+\t\tfn = filter_sparse;\n+\t}\n+\n+\tret = !!read_tree(the_repository, tree, &pathspec, fn, read_tree_context);\n+\n+\tif (sparse_filter_data)\n+\t\tsparse_filter_data__free(sparse_filter_data);\n+\n+\treturn ret;\n }\ndiff --git a/dir.c b/dir.c\nindex fbdb24fc819..cf1f843ceac 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1450,45 +1450,51 @@ int init_sparse_checkout_patterns(struct index_state *istate)\n \treturn 0;\n }\n \n-static int path_in_sparse_checkout_1(const char *path,\n-\t\t\t\t struct index_state *istate,\n-\t\t\t\t int require_cone_mode)\n+int recursively_match_path_with_sparse_patterns(const char *path,\n+\t\t\t\t\t\tstruct index_state *istate,\n+\t\t\t\t\t\tint dtype,\n+\t\t\t\t\t\tstruct pattern_list *pl)\n {\n-\tint dtype = DT_REG;\n \tenum pattern_match_result match = UNDECIDED;\n \tconst char *end, *slash;\n-\n-\t/*\n-\t * We default to accepting a path if the path is empty, there are no\n-\t * patterns, or the patterns are of the wrong type.\n-\t */\n-\tif (!*path ||\n-\t init_sparse_checkout_patterns(istate) ||\n-\t (require_cone_mode &&\n-\t !istate->sparse_checkout_patterns->use_cone_patterns))\n-\t\treturn 1;\n-\n \t/*\n \t * If UNDECIDED, use the match from the parent dir (recursively), or\n \t * fall back to NOT_MATCHED at the topmost level. Note that cone mode\n \t * never returns UNDECIDED, so we will execute only one iteration in\n \t * this case.\n \t */\n-\tfor (end = path + strlen(path);\n-\t end > path && match == UNDECIDED;\n+\tfor (end = path + strlen(path); end > path && match == UNDECIDED;\n \t end = slash) {\n-\n \t\tfor (slash = end - 1; slash > path && *slash != '/'; slash--)\n \t\t\t; /* do nothing */\n \n \t\tmatch = path_matches_pattern_list(path, end - path,\n \t\t\t\tslash > path ? slash + 1 : path, &dtype,\n-\t\t\t\tistate->sparse_checkout_patterns, istate);\n+\t\t\t\tpl, istate);\n \n \t\t/* We are going to match the parent dir now */\n \t\tdtype = DT_DIR;\n \t}\n-\treturn match > 0;\n+\n+\treturn match;\n+}\n+\n+static int path_in_sparse_checkout_1(const char *path,\n+\t\t\t\t struct index_state *istate,\n+\t\t\t\t int require_cone_mode)\n+{\n+\n+\t/*\n+\t * We default to accepting a path if the path is empty, there are no\n+\t * patterns, or the patterns are of the wrong type.\n+\t */\n+\tif (!*path ||\n+\t init_sparse_checkout_patterns(istate) ||\n+\t (require_cone_mode &&\n+\t !istate->sparse_checkout_patterns->use_cone_patterns))\n+\t\treturn 1;\n+\n+\treturn recursively_match_path_with_sparse_patterns(path, istate, DT_REG, istate->sparse_checkout_patterns) > 0;\n }\n \n int path_in_sparse_checkout(const char *path,\ndiff --git a/dir.h b/dir.h\nindex 8acfc044181..d186d271606 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -403,6 +403,11 @@ int path_in_sparse_checkout(const char *path,\n int path_in_cone_mode_sparse_checkout(const char *path,\n \t\t\t\t struct index_state *istate);\n \n+int recursively_match_path_with_sparse_patterns(const char *path,\n+\t\t\t\t\t\tstruct index_state *istate,\n+\t\t\t\t\t\tint dtype,\n+\t\t\t\t\t\tstruct pattern_list *pl);\n+\n struct dir_entry *dir_add_ignored(struct dir_struct *dir,\n \t\t\t\t struct index_state *istate,\n \t\t\t\t const char *pathname, int len);\ndiff --git a/t/t3106-ls-tree-sparse-checkout.sh b/t/t3106-ls-tree-sparse-checkout.sh\nnew file mode 100755\nindex 00000000000..d850950d651\n--- /dev/null\n+++ b/t/t3106-ls-tree-sparse-checkout.sh\n@@ -0,0 +1,103 @@\n+#!/bin/sh\n+\n+test_description='ls-tree with sparse filter patterns'\n+\n+. ./test-lib.sh\n+\n+check_agrees_with_ls_files () {\n+\tREPO=repo\n+\tgit -C $REPO submodule deinit -f --all\n+\tgit -C $REPO cat-file -p ${filter_oid} >${REPO}/.git/info/sparse-checkout\n+\tgit -C $REPO sparse-checkout init --cone 2>err\n+\tgit -C $REPO submodule init\n+\tgit -C $REPO ls-files -t| grep -v \"^S \"|cut -d\" \" -f2 >ls-files\n+\ttest_cmp ls-files actual\n+}\n+\n+check_same_result_in_bare_repo () {\n+\tFULL=repo\n+\tBARE=bare\n+\tFILTER=$1\n+\tgit -C repo cat-file -p ${filter_oid}| git -C bare hash-object -w --stdin\n+\tgit -C bare ls-tree --name-only --filter-sparse-oid=${filter_oid} -r HEAD >bare-result\n+\ttest_cmp expect bare-result\n+}\n+\n+test_expect_success 'setup' '\n+\tgit init submodule &&\n+\t(\n+\t\tcd submodule &&\n+\t\ttest_commit file\n+\t) &&\n+\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tmkdir dir &&\n+\t\ttest_commit dir/sub-file &&\n+\t\ttest_commit dir/sub-file2 &&\n+\t\tmkdir dir2 &&\n+\t\ttest_commit dir2/sub-file1 &&\n+\t\ttest_commit dir2/sub-file2 &&\n+\t\ttest_commit top-file &&\n+\t\tgit clone ../submodule submodule &&\n+\t\tgit submodule add ./submodule &&\n+\t\tgit submodule absorbgitdirs &&\n+\t\tgit commit -m\"add submodule\" &&\n+\t\tgit sparse-checkout init --cone\n+\t) &&\n+\tgit clone --bare ./repo bare\n+'\n+\n+test_expect_success 'toplevel filter only shows toplevel file' '\n+\tfilter_oid=$(git -C repo hash-object -w --stdin <<-\\EOF\n+\t/*\n+\t!/*/\n+\tEOF\n+\t) &&\n+\tcat >expect <<-EOF &&\n+\t.gitmodules\n+\tsubmodule\n+\ttop-file.t\n+\tEOF\n+\tgit -C repo ls-tree --name-only --filter-sparse-oid=${filter_oid} -r HEAD >actual &&\n+\ttest_cmp expect actual &&\n+\tcheck_agrees_with_ls_files &&\n+\tcheck_same_result_in_bare_repo ${filter_oid}\n+'\n+\n+test_expect_success 'non cone single file filter' '\n+\tfilter_oid=$(git -C repo hash-object -w --stdin <<-\\EOF\n+\t/dir/sub-file.t\n+\tEOF\n+\t) &&\n+\tcat >expect <<-EOF &&\n+\tdir/sub-file.t\n+\tEOF\n+\tgit -C repo ls-tree --name-only --filter-sparse-oid=${filter_oid} -r HEAD >actual &&\n+\ttest_cmp expect actual &&\n+\tcheck_agrees_with_ls_files &&\n+\tcheck_same_result_in_bare_repo ${filter_oid}\n+'\n+\n+test_expect_success 'cone filter matching one dir' '\n+\tfilter_oid=$(git -C repo hash-object -w --stdin <<-\\EOF\n+\t/*\n+\t!/*/\n+\t/dir/\n+\tEOF\n+\t) &&\n+\tcat >expect <<-EOF &&\n+\t.gitmodules\n+\tdir/sub-file.t\n+\tdir/sub-file2.t\n+\tsubmodule\n+\ttop-file.t\n+\tEOF\n+\tgit -C repo ls-tree --name-only --filter-sparse-oid=${filter_oid} -r HEAD >actual &&\n+\ttest_cmp expect actual &&\n+\tcheck_agrees_with_ls_files &&\n+\tcheck_same_result_in_bare_repo ${filter_oid}\n+'\n+\n+test_done\n\nbase-commit: a38d39a4c50d1275833aba54c4dbdfce9e2e9ca1\n-- \ngitgitgadget\n"},{"id":"470258","messageId":"CAPig+cRgZ0CrkqY7mufuWmhf6BC8yXjXXuOTEQjuz+Y0NA+N7Q@mail.gmail.com","threadId":"59071","inReplyTo":"pull.1459.git.1673456518993.gitgitgadget@gmail.com","subject":"Re: [PATCH] ls-tree: add --sparse-filter-oid argument","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-01-13T14:17:04Z","receivedAt":"2023-01-13T14:28:20Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jan 11, 2023 at 12:05 PM William Sprent via GitGitGadget\n wrote:\n> [...]\n> To fill this gap, add a new argument to ls-tree '--sparse-filter-oid'\n> which takes the object id of a blob containing sparse checkout patterns\n> that filters the output of 'ls-tree'. When filtering with given sparsity\n> patterns, 'ls-tree' only outputs blobs and commit objects that\n> match the given patterns.\n> [...]\n> Signed-off-by: William Sprent \n\nThis is not a proper review, but rather just some comments about\nissues I noticed while quickly running my eye over the patch. Many are\njust micro-nits about style; a few regarding the tests are probably\nactionable.\n\n> Note that one of the tests only pass when run on top of commit\n> 5842710dc2 (dir: check for single file cone patterns, 2023-01-03), which\n> was submitted separately and is currently is merged to 'next'.\n\nThanks for mentioning this. It's exactly the sort of information the\nmaintainer needs when applying your patch to his tree. And it can be\nhelpful for reviewers too.\n\n> diff --git a/Documentation/git-ls-tree.txt b/Documentation/git-ls-tree.txt\n> @@ -48,6 +48,11 @@ OPTIONS\n> +--sparse-filter-oid=::\n> + Omit showing tree objects and paths that do not match the\n> + sparse-checkout specification contained in the blob\n> + (or blob-expression) .\n\nGood to see a documentation update. The SYNOPSIS probably deserves an\nupdate too.\n\n> diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c\n> @@ -329,12 +331,79 @@ static struct ls_tree_cmdmode_to_fmt ls_tree_cmdmode_format[] = {\n> +static void sparse_filter_data__init(\n> + struct sparse_filter_data **d,\n> + const char *sparse_oid_name, read_tree_fn_t fn)\n> +{\n> + struct object_id sparse_oid;\n> + struct object_context oc;\n> +\n> + (*d) = xcalloc(1, sizeof(**d));\n> + (*d)->fn = fn;\n> + (*d)->pl.use_cone_patterns = core_sparse_checkout_cone;\n> +\n> + strbuf_init(&(*d)->full_path_buf, 0);\n> +\n> +\n\nNit: too many blank lines.\n\n> + if (get_oid_with_context(the_repository,\n> + sparse_oid_name,\n> + GET_OID_BLOB, &sparse_oid, &oc))\n\nPure nit: somewhat odd choice of wrapping; I'd have expected something\nalong the lines of:\n\n if (get_oid_with_context(the_repository, sparse_oid_name, GET_OID_BLOB,\n &sparse_oid, &oc))\n\nor:\n\n if (get_oid_with_context(the_repository, sparse_oid_name,\n GET_OID_BLOB, &sparse_oid, &oc))\n\n> +static void sparse_filter_data__free(struct sparse_filter_data *d)\n> +{\n> + clear_pattern_list(&d->pl);\n> + strbuf_release(&d->full_path_buf);\n> + free(d);\n> +}\n\nIs the double-underscore convention in function names imported from\nelsewhere? I don't recall seeing it used in this codebase.\n\n> +static int path_matches_sparse_checkout_patterns(struct strbuf *full_path, struct pattern_list *pl, int dtype)\n> +{\n> + enum pattern_match_result match = recursively_match_path_with_sparse_patterns(full_path->buf, the_repository->index, dtype, pl);\n> + return match > 0;\n> +}\n> +\n> +\n\nNit: too many blank lines\n\n> +static int filter_sparse(const struct object_id *oid, struct strbuf *base,\n> + const char *pathname, unsigned mode, void *context)\n> +{\n> +\n> + struct sparse_filter_data *data = context;\n\nNit: unnecessary blank line after \"{\"\n\n> + int do_recurse = show_recursive(base->buf, base->len, pathname);\n> + if (object_type(mode) == OBJ_TREE)\n> + return do_recurse;\n> +\n> + strbuf_reset(&data->full_path_buf);\n> + strbuf_addbuf(&data->full_path_buf, base);\n> + strbuf_addstr(&data->full_path_buf, pathname);\n> +\n> + if (!path_matches_sparse_checkout_patterns(&data->full_path_buf, &data->pl, DT_REG))\n> + return 0;\n> +\n> + return data->fn(oid, base, pathname, mode, context);\n> +}\n> +\n> +\n\nNit: too many blank lines\n\n> diff --git a/dir.c b/dir.c\n> @@ -1450,45 +1450,51 @@ int init_sparse_checkout_patterns(struct index_state *istate)\n> +static int path_in_sparse_checkout_1(const char *path,\n> + struct index_state *istate,\n> + int require_cone_mode)\n> +{\n> +\n> + /*\n\nNit: unnecessary blank line after \"{\"\n\n> diff --git a/t/t3106-ls-tree-sparse-checkout.sh b/t/t3106-ls-tree-sparse-checkout.sh\n> new file mode 100755\n\nWe often try to avoid introducing a new test script if the tests being\nadded can fit well into an existing script. If you didn't find any\nexisting script where these tests would fit, then creating a new\nscript may be fine.\n\n> @@ -0,0 +1,103 @@\n> +check_agrees_with_ls_files () {\n> + REPO=repo\n> + git -C $REPO submodule deinit -f --all\n> + git -C $REPO cat-file -p ${filter_oid} >${REPO}/.git/info/sparse-checkout\n> + git -C $REPO sparse-checkout init --cone 2>err\n> + git -C $REPO submodule init\n> + git -C $REPO ls-files -t| grep -v \"^S \"|cut -d\" \" -f2 >ls-files\n> + test_cmp ls-files actual\n> +}\n\nSeveral comments:\n\nSince the return code of this function is significant and callers care\nabout it, you should &&-chain all of the code in the function itself\n(just like you do within a test) so that failure of any command in the\nfunction is noticed and causes the calling test to fail. It's a good\nidea to &&-chain the variable assignments at the top of the function\ntoo (just in case someone later inserts code above the assignments).\n\nIt's odd to see a mixture of $VAR and ${VAR}. It's better to be\nconsistent. We typically use the $VAR form (though it's not exclusive\nby any means).\n\nSome shells complain when the pathname following \">\" redirection\noperator is not quoted, so:\n\n git -C $REPO cat-file -p ${filter_oid} >\"$REPO/.git/info/sparse-checkout\" &&\n\nStyle: add space around \"|\" pipe operator\n\nWe usually avoid having a Git command upstream of the pipe since its\nexit code gets swallowed by the pipe, so we usually do this instead:\n\n git -C $REPO ls-files -t >out &&\n grep -v \"^S \" out | cut -d\" \" -f2 >ls-files &&\n\nMinor: The two-command pipeline `grep -v \"^S \" | cut -d\" \" -f2\n>ls-files` could be expressed via a single `sed` invocation:\n\n sed -n \"/^S /!s/^. //p\" out &&\n\nNit: The first argument to test_cmp() is typically named \"expect\".\n\nError output is captured to file \"err\" but that file is never consulted.\n\n> +check_same_result_in_bare_repo () {\n> + FULL=repo\n> + BARE=bare\n> + FILTER=$1\n> + git -C repo cat-file -p ${filter_oid}| git -C bare hash-object -w --stdin\n> + git -C bare ls-tree --name-only --filter-sparse-oid=${filter_oid} -r HEAD >bare-result\n> + test_cmp expect bare-result\n> +}\n\nSame comments as above, plus:\n\nWhat is the purpose of the variables FULL, BARE, FILTER? They don't\nseem to be used in the function.\n\nI suspect that the function should be using $FILTER internally rather\nthan $filter_oid.\n\n> +test_expect_success 'setup' '\n> + git init submodule &&\n> + (\n> + cd submodule &&\n> + test_commit file\n> + ) &&\n\nMinor: test_commit() accepts a -C option, so this could be done\nwithout the subshell:\n\n test_commit -C submodule file\n\n> + git init repo &&\n> + (\n> + cd repo &&\n> + mkdir dir &&\n> + test_commit dir/sub-file &&\n> + test_commit dir/sub-file2 &&\n> + mkdir dir2 &&\n> + test_commit dir2/sub-file1 &&\n> + test_commit dir2/sub-file2 &&\n> + test_commit top-file &&\n> + git clone ../submodule submodule &&\n> + git submodule add ./submodule &&\n> + git submodule absorbgitdirs &&\n> + git commit -m\"add submodule\" &&\n> + git sparse-checkout init --cone\n> + ) &&\n\nHere the subshell makes sense since so many commands are run in\ndirectory \"repo.\" Fine.\n"},{"id":"470303","messageId":"xmqq8ri62ohd.fsf@gitster.g","threadId":"59071","inReplyTo":"CAPig+cRgZ0CrkqY7mufuWmhf6BC8yXjXXuOTEQjuz+Y0NA+N7Q@mail.gmail.com","subject":"Re: [PATCH] ls-tree: add --sparse-filter-oid argument","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-13T20:01:18Z","receivedAt":"2023-01-13T20:01:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>> Note that one of the tests only pass when run on top of commit\n>> 5842710dc2 (dir: check for single file cone patterns, 2023-01-03), which\n>> was submitted separately and is currently is merged to 'next'.\n>\n> Thanks for mentioning this. It's exactly the sort of information the\n> maintainer needs when applying your patch to his tree. And it can be\n> helpful for reviewers too.\n\nIndeed it is. Thanks for pointing it out---I just wish other\ncontributors see and follow such a wise piece of advice buried in\nreview of a patch by others.\n"},{"id":"470428","messageId":"26f026f6-6c17-7578-e341-d56fe53c95df@unity3d.com","threadId":"59071","inReplyTo":"CAPig+cRgZ0CrkqY7mufuWmhf6BC8yXjXXuOTEQjuz+Y0NA+N7Q@mail.gmail.com","subject":"Re: [PATCH] ls-tree: add --sparse-filter-oid argument","fromName":"William Sprent","fromEmail":"williams@unity3d.com","sentAt":"2023-01-16T12:14:02Z","receivedAt":"2023-01-16T12:14:11Z","isPatch":true,"sender":{"key":"williams@unity3d.com","avatar":"https://avatars.githubusercontent.com/u/77490534?v=4"},"body":"On 13/01/2023 15.17, Eric Sunshine wrote:\n> On Wed, Jan 11, 2023 at 12:05 PM William Sprent via GitGitGadget\n> wrote:\n>> [...]\n>> To fill this gap, add a new argument to ls-tree '--sparse-filter-oid'\n>> which takes the object id of a blob containing sparse checkout patterns\n>> that filters the output of 'ls-tree'. When filtering with given sparsity\n>> patterns, 'ls-tree' only outputs blobs and commit objects that\n>> match the given patterns.\n>> [...]\n>> Signed-off-by: William Sprent \n> \n> This is not a proper review, but rather just some comments about\n> issues I noticed while quickly running my eye over the patch. Many are\n> just micro-nits about style; a few regarding the tests are probably\n> actionable.\n> \n>> Note that one of the tests only pass when run on top of commit\n>> 5842710dc2 (dir: check for single file cone patterns, 2023-01-03), which\n>> was submitted separately and is currently is merged to 'next'.\n> \n> Thanks for mentioning this. It's exactly the sort of information the\n> maintainer needs when applying your patch to his tree. And it can be\n> helpful for reviewers too.\n> \n>> diff --git a/Documentation/git-ls-tree.txt b/Documentation/git-ls-tree.txt\n>> @@ -48,6 +48,11 @@ OPTIONS\n>> +--sparse-filter-oid=::\n>> + Omit showing tree objects and paths that do not match the\n>> + sparse-checkout specification contained in the blob\n>> + (or blob-expression) .\n> \n> Good to see a documentation update. The SYNOPSIS probably deserves an\n> update too.\n> \n\nNice catch. I'll update it.\n\n>> diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c\n>> @@ -329,12 +331,79 @@ static struct ls_tree_cmdmode_to_fmt ls_tree_cmdmode_format[] = {\n>> +static void sparse_filter_data__init(\n>> + struct sparse_filter_data **d,\n>> + const char *sparse_oid_name, read_tree_fn_t fn)\n>> +{\n>> + struct object_id sparse_oid;\n>> + struct object_context oc;\n>> +\n>> + (*d) = xcalloc(1, sizeof(**d));\n>> + (*d)->fn = fn;\n>> + (*d)->pl.use_cone_patterns = core_sparse_checkout_cone;\n>> +\n>> + strbuf_init(&(*d)->full_path_buf, 0);\n>> +\n>> +\n> \n> Nit: too many blank lines.\n> \n>> + if (get_oid_with_context(the_repository,\n>> + sparse_oid_name,\n>> + GET_OID_BLOB, &sparse_oid, &oc))\n> \n> Pure nit: somewhat odd choice of wrapping; I'd have expected something\n> along the lines of:\n> \n> if (get_oid_with_context(the_repository, sparse_oid_name, GET_OID_BLOB,\n> &sparse_oid, &oc))\n> \n> or:\n> \n> if (get_oid_with_context(the_repository, sparse_oid_name,\n> GET_OID_BLOB, &sparse_oid, &oc))\n> \n>> +static void sparse_filter_data__free(struct sparse_filter_data *d)\n>> +{\n>> + clear_pattern_list(&d->pl);\n>> + strbuf_release(&d->full_path_buf);\n>> + free(d);\n>> +}\n> \n> Is the double-underscore convention in function names imported from\n> elsewhere? I don't recall seeing it used in this codebase.\n> \n\nI took inspiration from the similar '--filter:sparse' argument for rev-list\nwhich is implemented in 'list-objects-filter.c'. I've just given it a\nsearch and it looks like it that convention isn't really used outside the\nlist-objects files, which makes it a bit awkward here.\n\nI get a bunch more hits for '(init|free)_', so I will change them to that.\n\n\n>> +static int path_matches_sparse_checkout_patterns(struct strbuf *full_path, struct pattern_list *pl, int dtype)\n>> +{\n>> + enum pattern_match_result match = recursively_match_path_with_sparse_patterns(full_path->buf, the_repository->index, dtype, pl);\n>> + return match > 0;\n>> +}\n>> +\n>> +\n> \n> Nit: too many blank lines\n> \n>> +static int filter_sparse(const struct object_id *oid, struct strbuf *base,\n>> + const char *pathname, unsigned mode, void *context)\n>> +{\n>> +\n>> + struct sparse_filter_data *data = context;\n> \n> Nit: unnecessary blank line after \"{\"\n> \n>> + int do_recurse = show_recursive(base->buf, base->len, pathname);\n>> + if (object_type(mode) == OBJ_TREE)\n>> + return do_recurse;\n>> +\n>> + strbuf_reset(&data->full_path_buf);\n>> + strbuf_addbuf(&data->full_path_buf, base);\n>> + strbuf_addstr(&data->full_path_buf, pathname);\n>> +\n>> + if (!path_matches_sparse_checkout_patterns(&data->full_path_buf, &data->pl, DT_REG))\n>> + return 0;\n>> +\n>> + return data->fn(oid, base, pathname, mode, context);\t\n>> +}\n>> +\n>> +\n> \n> Nit: too many blank lines\n> \n>> diff --git a/dir.c b/dir.c\n>> @@ -1450,45 +1450,51 @@ int init_sparse_checkout_patterns(struct index_state *istate)\n>> +static int path_in_sparse_checkout_1(const char *path,\n>> + struct index_state *istate,\n>> + int require_cone_mode)\n>> +{\n>> +\n>> + /*\n> \n> Nit: unnecessary blank line after \"{\"\n> \n>> diff --git a/t/t3106-ls-tree-sparse-checkout.sh b/t/t3106-ls-tree-sparse-checkout.sh\n>> new file mode 100755\n> \n> We often try to avoid introducing a new test script if the tests being\n> added can fit well into an existing script. If you didn't find any\n> existing script where these tests would fit, then creating a new\n> script may be fine.\n> \n\nWell there's a couple of things. The only ls-tree related test file\nthat isn't too specific is 't3103-ls-tree-misc.sh'. However, the\n'sparse-checkout' command leaks memory, so that would require setting\n'TEST_PASSES_SANITIZE_LEAK' to false.\n\nFor tests related to sparse-checkout, we both have the large\n't1092-sparse-checkout-compatibility.sh' file. But there is also a\ncollection of files like 't3705-add-sparse-checkout.sh'. I guess\nboth could work fine in this case. I think I went with the latter\nbecause I personally find the smaller test files a bit easier to\nparse.\n\nIf someone feels strongly the other way, I'll gladly change it.\n\n>> @@ -0,0 +1,103 @@\n>> +check_agrees_with_ls_files () {\n>> + REPO=repo\n>> + git -C $REPO submodule deinit -f --all\n>> + git -C $REPO cat-file -p ${filter_oid} >${REPO}/.git/info/sparse-checkout\n>> + git -C $REPO sparse-checkout init --cone 2>err\n>> + git -C $REPO submodule init\n>> + git -C $REPO ls-files -t| grep -v \"^S \"|cut -d\" \" -f2 >ls-files\n>> + test_cmp ls-files actual\n>> +}\n> \n> Several comments:\n> \n> Since the return code of this function is significant and callers care\n> about it, you should &&-chain all of the code in the function itself\n> (just like you do within a test) so that failure of any command in the\n> function is noticed and causes the calling test to fail. It's a good\n> idea to &&-chain the variable assignments at the top of the function\n> too (just in case someone later inserts code above the assignments).\n> \n\nThat was an oversight. Thanks for noticing.\n\n> It's odd to see a mixture of $VAR and ${VAR}. It's better to be\n> consistent. We typically use the $VAR form (though it's not exclusive\n> by any means).\n> \n\nI'll remove the braces.\n\n> Some shells complain when the pathname following \">\" redirection\n> operator is not quoted, so:\n> \n> git -C $REPO cat-file -p ${filter_oid} >\"$REPO/.git/info/sparse-checkout\" &&\n> \n> Style: add space around \"|\" pipe operator\n> \n> We usually avoid having a Git command upstream of the pipe since its\n> exit code gets swallowed by the pipe, so we usually do this instead:\n> \n> git -C $REPO ls-files -t >out &&\n> grep -v \"^S \" out | cut -d\" \" -f2 >ls-files &&\n> \n\nGood point. I'll do that.\n\n> Minor: The two-command pipeline `grep -v \"^S \" | cut -d\" \" -f2\n>> ls-files` could be expressed via a single `sed` invocation:\n> \n> sed -n \"/^S /!s/^. //p\" out &&\n> \n> Nit: The first argument to test_cmp() is typically named \"expect\".\n> \n> Error output is captured to file \"err\" but that file is never consulted.\n> \n\nI'll remove the redirection.\n\n>> +check_same_result_in_bare_repo () {\n>> + FULL=repo\n>> + BARE=bare\n>> + FILTER=$1\n>> + git -C repo cat-file -p ${filter_oid}| git -C bare hash-object -w --stdin\n>> + git -C bare ls-tree --name-only --filter-sparse-oid=${filter_oid} -r HEAD >bare-result\n>> + test_cmp expect bare-result\n>> +}\n> \n> Same comments as above, plus:\n> \n> What is the purpose of the variables FULL, BARE, FILTER? They don't\n> seem to be used in the function.\n> \n> I suspect that the function should be using $FILTER internally rather\n> than $filter_oid.\n> \n\nI think I probably had plans with them and then forgot. Thanks for noticing.\nI'll keep (and use) FILTER, but remove the others.\n\n>> +test_expect_success 'setup' '\n>> + git init submodule &&\n>> + (\n>> + cd submodule &&\n>> + test_commit file\n>> + ) &&\n> \n> Minor: test_commit() accepts a -C option, so this could be done\n> without the subshell:\n> \n> test_commit -C submodule file\n> \n>> + git init repo &&\n>> + (\n>> + cd repo &&\n>> + mkdir dir &&\n>> + test_commit dir/sub-file &&\n>> + test_commit dir/sub-file2 &&\n>> + mkdir dir2 &&\n>> + test_commit dir2/sub-file1 &&\n>> + test_commit dir2/sub-file2 &&\n>> + test_commit top-file &&\n>> + git clone ../submodule submodule &&\n>> + git submodule add ./submodule &&\n>> + git submodule absorbgitdirs &&\n>> + git commit -m\"add submodule\" &&\n>> + git sparse-checkout init --cone\n>> + ) &&\n> \n> Here the subshell makes sense since so many commands are run in\n> directory \"repo.\" Fine.\n\nThanks for taking the time to give some comments. I think they all\nmake sense to me. Besides the comments I've explicitly acknowledged above,\nI've also applied all of the minor/nit/style comments to my local tree.\n\n\n"},{"id":"470436","messageId":"d085144f-aa98-a79b-d176-2ab6cc7a7289@unity3d.com","threadId":"59071","inReplyTo":"xmqq8ri62ohd.fsf@gitster.g","subject":"Re: [PATCH] ls-tree: add --sparse-filter-oid argument","fromName":"William Sprent","fromEmail":"williams@unity3d.com","sentAt":"2023-01-16T15:13:31Z","receivedAt":"2023-01-16T15:20:37Z","isPatch":true,"sender":{"key":"williams@unity3d.com","avatar":"https://avatars.githubusercontent.com/u/77490534?v=4"},"body":"On 13/01/2023 21.01, Junio C Hamano wrote:\n> Eric Sunshine writes:\n> \n>>> Note that one of the tests only pass when run on top of commit\n>>> 5842710dc2 (dir: check for single file cone patterns, 2023-01-03), which\n>>> was submitted separately and is currently is merged to 'next'.\n>>\n>> Thanks for mentioning this. It's exactly the sort of information the\n>> maintainer needs when applying your patch to his tree. And it can be\n>> helpful for reviewers too.\n> \n> Indeed it is. Thanks for pointing it out---I just wish other\n> contributors see and follow such a wise piece of advice buried in\n> review of a patch by others.\n\nApropos where to apply the patch: This patch conflicts with the topic\n'ls-tree.c: clean-up works'[1], which refactors parts of 'ls-tree.c'.\nIt has recently been merged to 'next' in [2].\n\nShould I go ahead and rebase my changes on top of 'tl/ls-tree-code-clean-up'\nand then resubmit a new patch, or does it make sure sense to wait?\n\n1: https://public-inbox.org/git/20230112091135.20050-1-tenglong.tl@alibaba-inc.com\n2: Commit f7238037fd (Merge branch 'tl/ls-tree-code-clean-up' into next, 2023-01-14)\n\n"},{"id":"470906","messageId":"pull.1459.v2.git.1674474371817.gitgitgadget@gmail.com","threadId":"59071","inReplyTo":"pull.1459.git.1673456518993.gitgitgadget@gmail.com","subject":"[PATCH v2] ls-tree: add --sparse-filter-oid argument","fromName":"William Sprent via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-23T11:46:11Z","receivedAt":"2023-01-23T11:46:23Z","isPatch":true,"sender":{"key":"williams@unity3d.com","avatar":"https://avatars.githubusercontent.com/u/77490534?v=4"},"body":"From: William Sprent \n\nThere is currently no way to ask git the question \"which files would be\npart of a sparse checkout of commit X with sparse checkout patterns Y\".\nOne use-case would be that tooling may want know whether sparse checkouts\nof two commits contain the same content even if the full trees differ.\nAnother interesting use-case would be for tooling to use in conjunction\nwith 'git update-index --index-info'.\n\n'rev-list --objects --filter=sparse:oid' comes close, but as rev-list is\nconcerned with objects rather than directory trees, it leaves files out\nwhen the same blob occurs in at two different paths.\n\nIt is possible to ask git about the sparse status of files currently in\nthe index with 'ls-files -t'. However, this does not work well when the\ncaller is interested in another commit, intererested in sparsity\npatterns that aren't currently in '.git/info/sparse-checkout', or when\nworking in with bare repo.\n\nTo fill this gap, add a new argument to ls-tree '--sparse-filter-oid'\nwhich takes the object id of a blob containing sparse checkout patterns\nthat filters the output of 'ls-tree'. When filtering with given sparsity\npatterns, 'ls-tree' only outputs blobs and commit objects that\nmatch the given patterns.\n\nWhile it may be valid in some situations to output a tree object -- e.g.\nwhen a cone pattern matches all blobs recursively contained in a tree --\nit is less unclear what should be output if a sparse pattern matches\nparts of a tree.\n\nTo allow for reusing the pattern matching logic found in\n'path_in_sparse_checkout_1()' in 'dir.c' with arbitrary patterns,\nextract the pattern matching part of the function into its own new\nfunction 'recursively_match_path_with_sparse_patterns()'.\n\nSigned-off-by: William Sprent \n---\n ls-tree: add --sparse-filter-oid argument\n \n I'm resubmitting this change as rebased on top of 'master', as it\n conflicted with the topic 'ls-tree.c: clean-up works' 1\n [https://public-inbox.org/git/20230112091135.20050-1-tenglong.tl@alibaba-inc.com],\n which was merged to 'master' recently.\n \n This versions also incorporates changes based on the comments made in 2\n [https://public-inbox.org/git/CAPig+cRgZ0CrkqY7mufuWmhf6BC8yXjXXuOTEQjuz+Y0NA+N7Q@mail.gmail.com/].\n \n I'm also looping in contributors that have touched ls-tree and/or\n sparse-checkouts recently. I hope that's okay.\n \n William\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1459%2Fwilliams-unity%2Fls-tree-sparse-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1459/williams-unity/ls-tree-sparse-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1459\n\nContributor requested no range-diff. You can review it using these commands:\n git fetch https://github.com/gitgitgadget/git a38d39a4 07a4e24e\n git range-diff a38d39a4..8891b110 56c8fb1e..07a4e24e\n\n Documentation/git-ls-tree.txt | 6 ++\n builtin/ls-tree.c | 79 +++++++++++++++++++++++-\n dir.c | 45 ++++++++------\n dir.h | 5 ++\n t/t3106-ls-tree-sparse-checkout.sh | 99 ++++++++++++++++++++++++++++++\n 5 files changed, 213 insertions(+), 21 deletions(-)\n create mode 100755 t/t3106-ls-tree-sparse-checkout.sh\n\ndiff --git a/Documentation/git-ls-tree.txt b/Documentation/git-ls-tree.txt\nindex 0240adb8eec..724bb1f9dbd 100644\n--- a/Documentation/git-ls-tree.txt\n+++ b/Documentation/git-ls-tree.txt\n@@ -11,6 +11,7 @@ SYNOPSIS\n [verse]\n 'git ls-tree' [-d] [-r] [-t] [-l] [-z]\n \t [--name-only] [--name-status] [--object-only] [--full-name] [--full-tree] [--abbrev[=]] [--format=]\n+\t [--sparse-filter-oid=]\n \t [...]\n \n DESCRIPTION\n@@ -48,6 +49,11 @@ OPTIONS\n \tShow tree entries even when going to recurse them. Has no effect\n \tif `-r` was not passed. `-d` implies `-t`.\n \n+--sparse-filter-oid=::\n+\tOmit showing tree objects and paths that do not match the\n+\tsparse-checkout specification contained in the blob\n+\t(or blob-expression) .\n+\n -l::\n --long::\n \tShow object size of blob (file) entries.\ndiff --git a/builtin/ls-tree.c b/builtin/ls-tree.c\nindex 72eb70823d5..46a815fc7dc 100644\n--- a/builtin/ls-tree.c\n+++ b/builtin/ls-tree.c\n@@ -13,6 +13,7 @@\n #include \"builtin.h\"\n #include \"parse-options.h\"\n #include \"pathspec.h\"\n+#include \"dir.h\"\n \n static const char * const ls_tree_usage[] = {\n \tN_(\"git ls-tree [] [...]\"),\n@@ -364,6 +365,65 @@ static struct ls_tree_cmdmode_to_fmt ls_tree_cmdmode_format[] = {\n \t},\n };\n \n+struct sparse_filter_data {\n+\tread_tree_fn_t fn;\n+\tstruct pattern_list pl;\n+\tstruct strbuf full_path_buf;\n+\tstruct ls_tree_options *options;\n+};\n+\n+static void init_sparse_filter_data(struct sparse_filter_data **d, struct ls_tree_options *options,\n+\tconst char *sparse_oid_name, read_tree_fn_t fn)\n+{\n+\tstruct object_id sparse_oid;\n+\tstruct object_context oc;\n+\n+\t(*d) = xcalloc(1, sizeof(**d));\n+\t(*d)->fn = fn;\n+\t(*d)->pl.use_cone_patterns = core_sparse_checkout_cone;\n+\t(*d)->options = options;\n+\tstrbuf_init(&(*d)->full_path_buf, 0);\n+\n+\tif (get_oid_with_context(the_repository, sparse_oid_name, GET_OID_BLOB,\n+\t\t\t&sparse_oid, &oc))\n+\t\tdie(_(\"unable to access sparse blob in '%s'\"), sparse_oid_name);\n+\tif (add_patterns_from_blob_to_list(&sparse_oid, \"\", 0, &(*d)->pl) < 0)\n+\t\tdie(_(\"unable to parse sparse filter data in %s\"),\n+\t\t oid_to_hex(&sparse_oid));\n+}\n+\n+static void free_sparse_filter_data(struct sparse_filter_data *d)\n+{\n+\tclear_pattern_list(&d->pl);\n+\tstrbuf_release(&d->full_path_buf);\n+\tfree(d);\n+}\n+\n+static int path_matches_sparse_checkout_patterns(struct strbuf *full_path, struct pattern_list *pl, int dtype)\n+{\n+\tenum pattern_match_result match = recursively_match_path_with_sparse_patterns(full_path->buf, the_repository->index, dtype, pl);\n+\treturn match > 0;\n+}\n+\n+static int filter_sparse(const struct object_id *oid, struct strbuf *base,\n+\t\t\t const char *pathname, unsigned mode, void *context)\n+{\n+\tstruct sparse_filter_data *data = context;\n+\n+\tint do_recurse = show_recursive(data->options, base->buf, base->len, pathname);\n+\tif (object_type(mode) == OBJ_TREE)\n+\t\treturn do_recurse;\n+\n+\tstrbuf_reset(&data->full_path_buf);\n+\tstrbuf_addbuf(&data->full_path_buf, base);\n+\tstrbuf_addstr(&data->full_path_buf, pathname);\n+\n+\tif (!path_matches_sparse_checkout_patterns(&data->full_path_buf, &data->pl, DT_REG))\n+\t\t\treturn 0;\n+\n+\treturn data->fn(oid, base, pathname, mode, data->options);\n+}\n+\n int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n {\n \tstruct object_id oid;\n@@ -372,7 +432,11 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n \tread_tree_fn_t fn = NULL;\n \tenum ls_tree_cmdmode cmdmode = MODE_DEFAULT;\n \tint null_termination = 0;\n+\n \tstruct ls_tree_options options = { 0 };\n+\tchar *sparse_oid_name = NULL;\n+\tvoid *read_tree_context = NULL;\n+\tstruct sparse_filter_data *sparse_filter_data = NULL;\n \tconst struct option ls_tree_options[] = {\n \t\tOPT_BIT('d', NULL, &options.ls_options, N_(\"only show trees\"),\n \t\t\tLS_TREE_ONLY),\n@@ -399,6 +463,9 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t N_(\"format to use for the output\"),\n \t\t\t\t\t PARSE_OPT_NONEG),\n \t\tOPT__ABBREV(&options.abbrev),\n+\t\tOPT_STRING_F(0, \"filter-sparse-oid\", &sparse_oid_name, N_(\"filter-sparse-oid\"),\n+\t\t\t N_(\"filter output with sparse-checkout specification contained in the blob\"),\n+\t\t\t PARSE_OPT_NONEG),\n \t\tOPT_END()\n \t};\n \tstruct ls_tree_cmdmode_to_fmt *m2f = ls_tree_cmdmode_format;\n@@ -474,7 +541,17 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n \t\tbreak;\n \t}\n \n-\tret = !!read_tree(the_repository, tree, &options.pathspec, fn, &options);\n+\tif (sparse_oid_name) {\n+\t\tinit_sparse_filter_data(&sparse_filter_data, &options, sparse_oid_name, fn);\n+\t\tread_tree_context = sparse_filter_data;\n+\t\tfn = filter_sparse;\n+\t} else\n+\t\tread_tree_context = &options;\n+\n+\tret = !!read_tree(the_repository, tree, &options.pathspec, fn, read_tree_context);\n \tclear_pathspec(&options.pathspec);\n+\tif (sparse_filter_data)\n+\t\tfree_sparse_filter_data(sparse_filter_data);\n+\n \treturn ret;\n }\ndiff --git a/dir.c b/dir.c\nindex 4e99f0c868f..122ebced08e 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1457,45 +1457,50 @@ int init_sparse_checkout_patterns(struct index_state *istate)\n \treturn 0;\n }\n \n-static int path_in_sparse_checkout_1(const char *path,\n-\t\t\t\t struct index_state *istate,\n-\t\t\t\t int require_cone_mode)\n+int recursively_match_path_with_sparse_patterns(const char *path,\n+\t\t\t\t\t\tstruct index_state *istate,\n+\t\t\t\t\t\tint dtype,\n+\t\t\t\t\t\tstruct pattern_list *pl)\n {\n-\tint dtype = DT_REG;\n \tenum pattern_match_result match = UNDECIDED;\n \tconst char *end, *slash;\n-\n-\t/*\n-\t * We default to accepting a path if the path is empty, there are no\n-\t * patterns, or the patterns are of the wrong type.\n-\t */\n-\tif (!*path ||\n-\t init_sparse_checkout_patterns(istate) ||\n-\t (require_cone_mode &&\n-\t !istate->sparse_checkout_patterns->use_cone_patterns))\n-\t\treturn 1;\n-\n \t/*\n \t * If UNDECIDED, use the match from the parent dir (recursively), or\n \t * fall back to NOT_MATCHED at the topmost level. Note that cone mode\n \t * never returns UNDECIDED, so we will execute only one iteration in\n \t * this case.\n \t */\n-\tfor (end = path + strlen(path);\n-\t end > path && match == UNDECIDED;\n+\tfor (end = path + strlen(path); end > path && match == UNDECIDED;\n \t end = slash) {\n-\n \t\tfor (slash = end - 1; slash > path && *slash != '/'; slash--)\n \t\t\t; /* do nothing */\n \n \t\tmatch = path_matches_pattern_list(path, end - path,\n \t\t\t\tslash > path ? slash + 1 : path, &dtype,\n-\t\t\t\tistate->sparse_checkout_patterns, istate);\n+\t\t\t\tpl, istate);\n \n \t\t/* We are going to match the parent dir now */\n \t\tdtype = DT_DIR;\n \t}\n-\treturn match > 0;\n+\n+\treturn match;\n+}\n+\n+static int path_in_sparse_checkout_1(const char *path,\n+\t\t\t\t struct index_state *istate,\n+\t\t\t\t int require_cone_mode)\n+{\n+\t/*\n+\t * We default to accepting a path if the path is empty, there are no\n+\t * patterns, or the patterns are of the wrong type.\n+\t */\n+\tif (!*path ||\n+\t init_sparse_checkout_patterns(istate) ||\n+\t (require_cone_mode &&\n+\t !istate->sparse_checkout_patterns->use_cone_patterns))\n+\t\treturn 1;\n+\n+\treturn recursively_match_path_with_sparse_patterns(path, istate, DT_REG, istate->sparse_checkout_patterns) > 0;\n }\n \n int path_in_sparse_checkout(const char *path,\ndiff --git a/dir.h b/dir.h\nindex 8acfc044181..d186d271606 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -403,6 +403,11 @@ int path_in_sparse_checkout(const char *path,\n int path_in_cone_mode_sparse_checkout(const char *path,\n \t\t\t\t struct index_state *istate);\n \n+int recursively_match_path_with_sparse_patterns(const char *path,\n+\t\t\t\t\t\tstruct index_state *istate,\n+\t\t\t\t\t\tint dtype,\n+\t\t\t\t\t\tstruct pattern_list *pl);\n+\n struct dir_entry *dir_add_ignored(struct dir_struct *dir,\n \t\t\t\t struct index_state *istate,\n \t\t\t\t const char *pathname, int len);\ndiff --git a/t/t3106-ls-tree-sparse-checkout.sh b/t/t3106-ls-tree-sparse-checkout.sh\nnew file mode 100755\nindex 00000000000..945f3ed1888\n--- /dev/null\n+++ b/t/t3106-ls-tree-sparse-checkout.sh\n@@ -0,0 +1,99 @@\n+#!/bin/sh\n+\n+test_description='ls-tree with sparse filter patterns'\n+\n+. ./test-lib.sh\n+\n+check_agrees_with_ls_files () {\n+\tREPO=repo &&\n+\tgit -C $REPO submodule deinit -f --all &&\n+\tgit -C $REPO cat-file -p $filter_oid >\"$REPO/.git/info/sparse-checkout\" &&\n+\tgit -C $REPO sparse-checkout init --cone &&\n+\tgit -C $REPO submodule init &&\n+\tgit -C $REPO ls-files -t >out &&\n+\tsed -n \"/^S /!s/^. //p\" out >expect &&\n+\ttest_cmp expect actual\n+}\n+\n+check_same_result_in_bare_repo () {\n+\tFILTER_OID=$1 &&\n+\tgit -C repo cat-file -p $FILTER_OID >out &&\n+\tgit -C bare hash-object -w --stdin bare-result &&\n+\ttest_cmp expect bare-result\n+}\n+\n+test_expect_success 'setup' '\n+\tgit init submodule &&\n+\ttest_commit -C submodule file &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tmkdir dir &&\n+\t\ttest_commit dir/sub-file &&\n+\t\ttest_commit dir/sub-file2 &&\n+\t\tmkdir dir2 &&\n+\t\ttest_commit dir2/sub-file1 &&\n+\t\ttest_commit dir2/sub-file2 &&\n+\t\ttest_commit top-file &&\n+\t\tgit clone ../submodule submodule &&\n+\t\tgit submodule add ./submodule &&\n+\t\tgit submodule absorbgitdirs &&\n+\t\tgit commit -m\"add submodule\" &&\n+\t\tgit sparse-checkout init --cone\n+\t) &&\n+\tgit clone --bare ./repo bare\n+'\n+\n+test_expect_success 'toplevel filter only shows toplevel file' '\n+\tfilter_oid=$(git -C repo hash-object -w --stdin <<-\\EOF\n+\t/*\n+\t!/*/\n+\tEOF\n+\t) &&\n+\tcat >expect <<-EOF &&\n+\t.gitmodules\n+\tsubmodule\n+\ttop-file.t\n+\tEOF\n+\tgit -C repo ls-tree --name-only --filter-sparse-oid=$filter_oid -r HEAD >actual &&\n+\ttest_cmp expect actual &&\n+\tcheck_agrees_with_ls_files &&\n+\tcheck_same_result_in_bare_repo $filter_oid\n+'\n+\n+test_expect_success 'non cone single file filter' '\n+\tfilter_oid=$(git -C repo hash-object -w --stdin <<-\\EOF\n+\t/dir/sub-file.t\n+\tEOF\n+\t) &&\n+\tcat >expect <<-EOF &&\n+\tdir/sub-file.t\n+\tEOF\n+\tgit -C repo ls-tree --name-only --filter-sparse-oid=$filter_oid -r HEAD >actual &&\n+\ttest_cmp expect actual &&\n+\tcheck_agrees_with_ls_files &&\n+\tcheck_same_result_in_bare_repo $filter_oid\n+'\n+\n+test_expect_success 'cone filter matching one dir' '\n+\tfilter_oid=$(git -C repo hash-object -w --stdin <<-\\EOF\n+\t/*\n+\t!/*/\n+\t/dir/\n+\tEOF\n+\t) &&\n+\tcat >expect <<-EOF &&\n+\t.gitmodules\n+\tdir/sub-file.t\n+\tdir/sub-file2.t\n+\tsubmodule\n+\ttop-file.t\n+\tEOF\n+\tgit -C repo ls-tree --name-only --filter-sparse-oid=$filter_oid -r HEAD >actual &&\n+\ttest_cmp expect actual &&\n+\tcheck_agrees_with_ls_files &&\n+\tcheck_same_result_in_bare_repo $filter_oid\n+'\n+\n+test_done\n\nbase-commit: 56c8fb1e95377900ec9d53c07886022af0a5d3c2\n-- \ngitgitgadget\n"},{"id":"470907","messageId":"230123.865ycxtn7i.gmgdl@evledraar.gmail.com","threadId":"59071","inReplyTo":"pull.1459.v2.git.1674474371817.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] ls-tree: add --sparse-filter-oid argument","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-01-23T13:00:41Z","receivedAt":"2023-01-23T13:06:20Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jan 23 2023, William Sprent via GitGitGadget wrote:\n\n> From: William Sprent \n>\n> There is currently no way to ask git the question \"which files would be\n> part of a sparse checkout of commit X with sparse checkout patterns Y\".\n> One use-case would be that tooling may want know whether sparse checkouts\n> of two commits contain the same content even if the full trees differ.\n> Another interesting use-case would be for tooling to use in conjunction\n> with 'git update-index --index-info'.\n\nRather than commenting on individual points, I checked this out and\nproduced something squashable on-top, it fixes various issues (some\naspects of which still remain):\n\n * You need to wrap your code at 79 chars (and some of that could be\n helped by picking less verbose identifiers & variable names,\n e.g. just use \"context\" rather than \"read_tree_context\" in\n cmd_ls_tree(), it has no other \"context\"...)\n * You're making the memory management aroind init_sparse_filter_data()\n needlessly hard, just put it on the stack instead. That also allows\n for init-ing most of it right away, or via a macro in the assignment.\n * You have a stray refactoring of dir.c in your proposed change, this\n changes it back, let's leave that sort of thing out.\n * You're adding a function that returns an enum, but you declare it as\n returning \"int\", let's retain that type in the header & declaration.\n\n\ndiff --git a/builtin/ls-tree.c b/builtin/ls-tree.c\nindex 46a815fc7dc..68d6ef675f2 100644\n--- a/builtin/ls-tree.c\n+++ b/builtin/ls-tree.c\n@@ -372,36 +372,37 @@ struct sparse_filter_data {\n \tstruct ls_tree_options *options;\n };\n \n-static void init_sparse_filter_data(struct sparse_filter_data **d, struct ls_tree_options *options,\n-\tconst char *sparse_oid_name, read_tree_fn_t fn)\n+static void init_sparse_filter_data(struct sparse_filter_data *d,\n+\t\t\t\t const char *sparse_oid_name)\n {\n \tstruct object_id sparse_oid;\n \tstruct object_context oc;\n \n-\t(*d) = xcalloc(1, sizeof(**d));\n-\t(*d)->fn = fn;\n-\t(*d)->pl.use_cone_patterns = core_sparse_checkout_cone;\n-\t(*d)->options = options;\n-\tstrbuf_init(&(*d)->full_path_buf, 0);\n-\n \tif (get_oid_with_context(the_repository, sparse_oid_name, GET_OID_BLOB,\n \t\t\t&sparse_oid, &oc))\n-\t\tdie(_(\"unable to access sparse blob in '%s'\"), sparse_oid_name);\n-\tif (add_patterns_from_blob_to_list(&sparse_oid, \"\", 0, &(*d)->pl) < 0)\n+\t\tdie(_(\"unable to access sparse blob in '%s'\"),\n+\t\t sparse_oid_name);\n+\tif (add_patterns_from_blob_to_list(&sparse_oid, \"\", 0, &d->pl) < 0)\n \t\tdie(_(\"unable to parse sparse filter data in %s\"),\n \t\t oid_to_hex(&sparse_oid));\n }\n \n-static void free_sparse_filter_data(struct sparse_filter_data *d)\n+static void release_sparse_filter_data(struct sparse_filter_data *d)\n {\n \tclear_pattern_list(&d->pl);\n \tstrbuf_release(&d->full_path_buf);\n-\tfree(d);\n }\n \n-static int path_matches_sparse_checkout_patterns(struct strbuf *full_path, struct pattern_list *pl, int dtype)\n+static int path_matches_sparse_checkout_patterns(struct strbuf *full_path,\n+\t\t\t\t\t\t struct pattern_list *pl,\n+\t\t\t\t\t\t int dtype)\n {\n-\tenum pattern_match_result match = recursively_match_path_with_sparse_patterns(full_path->buf, the_repository->index, dtype, pl);\n+\tenum pattern_match_result match;\n+\n+\tmatch = recursively_match_path_with_sparse_patterns(full_path->buf,\n+\t\t\t\t\t\t\t the_repository->index,\n+\t\t\t\t\t\t\t dtype, pl);\n+\n \treturn match > 0;\n }\n \n@@ -436,7 +437,10 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n \tstruct ls_tree_options options = { 0 };\n \tchar *sparse_oid_name = NULL;\n \tvoid *read_tree_context = NULL;\n-\tstruct sparse_filter_data *sparse_filter_data = NULL;\n+\tstruct sparse_filter_data sparse_filter_data = {\n+\t\t.options = &options,\n+\t\t.full_path_buf = STRBUF_INIT,\n+\t};\n \tconst struct option ls_tree_options[] = {\n \t\tOPT_BIT('d', NULL, &options.ls_options, N_(\"only show trees\"),\n \t\t\tLS_TREE_ONLY),\n@@ -542,16 +546,17 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (sparse_oid_name) {\n-\t\tinit_sparse_filter_data(&sparse_filter_data, &options, sparse_oid_name, fn);\n-\t\tread_tree_context = sparse_filter_data;\n+\t\tsparse_filter_data.fn = fn;\n+\t\tinit_sparse_filter_data(&sparse_filter_data, sparse_oid_name);\n+\t\tread_tree_context = &sparse_filter_data;\n \t\tfn = filter_sparse;\n \t} else\n \t\tread_tree_context = &options;\n \n-\tret = !!read_tree(the_repository, tree, &options.pathspec, fn, read_tree_context);\n+\tret = !!read_tree(the_repository, tree, &options.pathspec, fn,\n+\t\t\t read_tree_context);\n \tclear_pathspec(&options.pathspec);\n-\tif (sparse_filter_data)\n-\t\tfree_sparse_filter_data(sparse_filter_data);\n+\trelease_sparse_filter_data(&sparse_filter_data);\n \n \treturn ret;\n }\ndiff --git a/dir.c b/dir.c\nindex 122ebced08e..57ec6404901 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1457,10 +1457,10 @@ int init_sparse_checkout_patterns(struct index_state *istate)\n \treturn 0;\n }\n \n-int recursively_match_path_with_sparse_patterns(const char *path,\n-\t\t\t\t\t\tstruct index_state *istate,\n-\t\t\t\t\t\tint dtype,\n-\t\t\t\t\t\tstruct pattern_list *pl)\n+enum pattern_match_result recursively_match_path_with_sparse_patterns(const char *path,\n+\t\t\t\t\t\t\t\t struct index_state *istate,\n+\t\t\t\t\t\t\t\t int dtype,\n+\t\t\t\t\t\t\t\t struct pattern_list *pl)\n {\n \tenum pattern_match_result match = UNDECIDED;\n \tconst char *end, *slash;\n@@ -1470,7 +1470,8 @@ int recursively_match_path_with_sparse_patterns(const char *path,\n \t * never returns UNDECIDED, so we will execute only one iteration in\n \t * this case.\n \t */\n-\tfor (end = path + strlen(path); end > path && match == UNDECIDED;\n+\tfor (end = path + strlen(path);\n+\t end > path && match == UNDECIDED;\n \t end = slash) {\n \t\tfor (slash = end - 1; slash > path && *slash != '/'; slash--)\n \t\t\t; /* do nothing */\ndiff --git a/dir.h b/dir.h\nindex d186d271606..3c65e68ca40 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -403,10 +403,10 @@ int path_in_sparse_checkout(const char *path,\n int path_in_cone_mode_sparse_checkout(const char *path,\n \t\t\t\t struct index_state *istate);\n \n-int recursively_match_path_with_sparse_patterns(const char *path,\n-\t\t\t\t\t\tstruct index_state *istate,\n-\t\t\t\t\t\tint dtype,\n-\t\t\t\t\t\tstruct pattern_list *pl);\n+enum pattern_match_result recursively_match_path_with_sparse_patterns(const char *path,\n+\t\t\t\t\t\t\t\t struct index_state *istate,\n+\t\t\t\t\t\t\t\t int dtype,\n+\t\t\t\t\t\t\t\t struct pattern_list *pl);\n \n struct dir_entry *dir_add_ignored(struct dir_struct *dir,\n \t\t\t\t struct index_state *istate,\n"},{"id":"470908","messageId":"230123.861qnltmbp.gmgdl@evledraar.gmail.com","threadId":"59071","inReplyTo":"pull.1459.v2.git.1674474371817.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] ls-tree: add --sparse-filter-oid argument","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-01-23T13:06:10Z","receivedAt":"2023-01-23T13:25:22Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jan 23 2023, William Sprent via GitGitGadget wrote:\n\n> From: William Sprent \n\n...some further inline commentary, in addition to my proposed squash-in...\n\n> diff --git a/Documentation/git-ls-tree.txt b/Documentation/git-ls-tree.txt\n> index 0240adb8eec..724bb1f9dbd 100644\n> --- a/Documentation/git-ls-tree.txt\n> +++ b/Documentation/git-ls-tree.txt\n> @@ -11,6 +11,7 @@ SYNOPSIS\n> [verse]\n> 'git ls-tree' [-d] [-r] [-t] [-l] [-z]\n> \t [--name-only] [--name-status] [--object-only] [--full-name] [--full-tree] [--abbrev[=]] [--format=]\n> +\t [--sparse-filter-oid=]\n> \t [...]\n> \n> DESCRIPTION\n> @@ -48,6 +49,11 @@ OPTIONS\n> \tShow tree entries even when going to recurse them. Has no effect\n> \tif `-r` was not passed. `-d` implies `-t`.\n> \n> +--sparse-filter-oid=::\n> +\tOmit showing tree objects and paths that do not match the\n> +\tsparse-checkout specification contained in the blob\n> +\t(or blob-expression) .\n> +\n> -l::\n> --long::\n> \tShow object size of blob (file) entries.\n> diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c\n> index 72eb70823d5..46a815fc7dc 100644\n> --- a/builtin/ls-tree.c\n> +++ b/builtin/ls-tree.c\n> @@ -13,6 +13,7 @@\n> #include \"builtin.h\"\n> #include \"parse-options.h\"\n> #include \"pathspec.h\"\n> +#include \"dir.h\"\n> \n> static const char * const ls_tree_usage[] = {\n> \tN_(\"git ls-tree [] [...]\"),\n> @@ -364,6 +365,65 @@ static struct ls_tree_cmdmode_to_fmt ls_tree_cmdmode_format[] = {\n> \t},\n> };\n\nOkey, here yo uupdate the *.txt, but not the \"-h\", but it's one of those\nwhere way say .\n\nI for one wouldn't mind some preceding change like my 423be1f83c5 (doc\ntxt & -h consistency: make \"commit\" consistent, 2022-10-13), which in\nturn would make t/t0450-txt-doc-vs-help.sh pass for this command, but\nmaybe it's too much of a digression...\n\n> +\t(*d) = xcalloc(1, sizeof(**d));\n> +\t(*d)->fn = fn;\n> +\t(*d)->pl.use_cone_patterns = core_sparse_checkout_cone;\n\nI forgot to note in my proposed fix-up that I omitted this, but your\ntests still pass, which suggests it's either not needed, or your tests\nare lacking....\n\n> +\t(*d)->options = options;\n> +\tstrbuf_init(&(*d)->full_path_buf, 0);\n> +\n> +\tif (get_oid_with_context(the_repository, sparse_oid_name, GET_OID_BLOB,\n> +\t\t\t&sparse_oid, &oc))\n> +\t\tdie(_(\"unable to access sparse blob in '%s'\"), sparse_oid_name);\n> +\tif (add_patterns_from_blob_to_list(&sparse_oid, \"\", 0, &(*d)->pl) < 0)\n\nAs noted you can avoid the malloc here, which also sidesteps this (IMO\nat last) ugly &(*v)->m syntax.\n\n> +\t\tdie(_(\"unable to parse sparse filter data in %s\"),\n> +\t\t oid_to_hex(&sparse_oid));\n> +}\n> +\n> +static void free_sparse_filter_data(struct sparse_filter_data *d)\n> +{\n> +\tclear_pattern_list(&d->pl);\n> +\tstrbuf_release(&d->full_path_buf);\n> +\tfree(d);\n> +}\n> +\n> +static int path_matches_sparse_checkout_patterns(struct strbuf *full_path, struct pattern_list *pl, int dtype)\n> +{\n> +\tenum pattern_match_result match = recursively_match_path_with_sparse_patterns(full_path->buf, the_repository->index, dtype, pl);\n\nI for one would welcome e.g. abbreviating \"sparse_checkout_patterns\" as\n\"scp\" or whatever throughout, with resulting shortening of very long\nlines & identifiers. E.g. for this one \"patch_match_scp\" or whatever.\n\n\n> +\tstruct sparse_filter_data *data = context;\n> +\n\nStyle: Don't add a \\n\\n between variable decls,\n\n> +\tint do_recurse = show_recursive(data->options, base->buf, base->len, pathname);\n\nInstead add it here, before the code proper.\n\n\n> +\tif (object_type(mode) == OBJ_TREE)\n> +\t\treturn do_recurse;\n> +\n> +\tstrbuf_reset(&data->full_path_buf);\n\nI wondered if we need this after, but we don't, I for one would welcome a fix-up like:\n\t\n\tdiff --git a/builtin/ls-tree.c b/builtin/ls-tree.c\n\tindex 68d6ef675f2..74dfa9a4580 100644\n\t--- a/builtin/ls-tree.c\n\t+++ b/builtin/ls-tree.c\n\t@@ -410,19 +410,21 @@ static int filter_sparse(const struct object_id *oid, struct strbuf *base,\n\t \t\t\t const char *pathname, unsigned mode, void *context)\n\t {\n\t \tstruct sparse_filter_data *data = context;\n\t-\n\t \tint do_recurse = show_recursive(data->options, base->buf, base->len, pathname);\n\t+\tint ret = 0;\n\t+\n\t \tif (object_type(mode) == OBJ_TREE)\n\t \t\treturn do_recurse;\n\t \n\t-\tstrbuf_reset(&data->full_path_buf);\n\t \tstrbuf_addbuf(&data->full_path_buf, base);\n\t \tstrbuf_addstr(&data->full_path_buf, pathname);\n\t \n\t \tif (!path_matches_sparse_checkout_patterns(&data->full_path_buf, &data->pl, DT_REG))\n\t-\t\t\treturn 0;\n\t-\n\t-\treturn data->fn(oid, base, pathname, mode, data->options);\n\t+\t\tgoto cleanup;\n\t+\tret = data->fn(oid, base, pathname, mode, data->options);\n\t+cleanup:\n\t+\tstrbuf_reset(&data->full_path_buf);\n\t+\treturn ret;\n\t }\n\t \n\t int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n\t\nIt's slightly more verbose, and we do end up needlessly reset-ing the\n\"last\" one, but makes it clear that we don't have need for this after\nthis.\n\nWhich to me, also raises the question of why you have this\n\"full_path_buf\" at all. It looks like a memory optimization, as you're\ntrying to avoid a malloc()/free() in the loop.\n\nThat's fair, but you could also do that with:\n\n\tconst size_t oldlen = base->len;\n\tstrbuf_addstr(base, pathname);\n /* do the path_matches_sparse_checkout_patterns() call here */\n\t/* before \"cleanup\" */\n\tstrbuf_setlen(base, oldlen);\n\nOr whatever, those snippets are untested, but we use similar tricks\nelsewhere, and as it's contained to a few lines here I think it's better\nthan carrying another buffer.\n\n> +\tstrbuf_addbuf(&data->full_path_buf, base);\n> +\tstrbuf_addstr(&data->full_path_buf, pathname);\n\nNit: Shorter as: strbuf_addf(&sb, \"%s%s\", base.buf, pathname) (or equivalent);\n\n> +\n> +\tif (!path_matches_sparse_checkout_patterns(&data->full_path_buf, &data->pl, DT_REG))\n> +\t\t\treturn 0;\n> +\n> +\treturn data->fn(oid, base, pathname, mode, data->options);\n> +}\n> +\n> int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n> {\n> \tstruct object_id oid;\n> @@ -372,7 +432,11 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n> \tread_tree_fn_t fn = NULL;\n> \tenum ls_tree_cmdmode cmdmode = MODE_DEFAULT;\n> \tint null_termination = 0;\n> +\n\nDon't add this stray \\n.\n\n> \tstruct ls_tree_options options = { 0 };\n> +\tchar *sparse_oid_name = NULL;\n> +\tvoid *read_tree_context = NULL;\n> +\tstruct sparse_filter_data *sparse_filter_data = NULL;\n> \tconst struct option ls_tree_options[] = {\n> \t\tOPT_BIT('d', NULL, &options.ls_options, N_(\"only show trees\"),\n> \t\t\tLS_TREE_ONLY),\n> @@ -399,6 +463,9 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n> \t\t\t\t\t N_(\"format to use for the output\"),\n> \t\t\t\t\t PARSE_OPT_NONEG),\n> \t\tOPT__ABBREV(&options.abbrev),\n> +\t\tOPT_STRING_F(0, \"filter-sparse-oid\", &sparse_oid_name, N_(\"filter-sparse-oid\"),\n\nMake that N_(...) be \"object-id\", \"oid\", \"hash\" or something?\n\nI.e. the RHS with the should be , not\n.\n\n> +\t\t\t N_(\"filter output with sparse-checkout specification contained in the blob\"),\n> +\t\t\t PARSE_OPT_NONEG),\n> \t\tOPT_END()\n> \t};\n> \tstruct ls_tree_cmdmode_to_fmt *m2f = ls_tree_cmdmode_format;\n> @@ -474,7 +541,17 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n> \t\tbreak;\n> \t}\n> \n> -\tret = !!read_tree(the_repository, tree, &options.pathspec, fn, &options);\n> +\tif (sparse_oid_name) {\n> +\t\tinit_sparse_filter_data(&sparse_filter_data, &options, sparse_oid_name, fn);\n> +\t\tread_tree_context = sparse_filter_data;\n> +\t\tfn = filter_sparse;\n> +\t} else\n> +\t\tread_tree_context = &options;\n\nYou're missing a {} here for the \"else\".\n\nBetter yet, delete that \"else\" and change the decl above to be:\n\n\tvoid *context = &options;\n\nThen just keep the \"if\" here, where you might clobber the \"context\".\n\n> +\n> +\tret = !!read_tree(the_repository, tree, &options.pathspec, fn, read_tree_context);\n> \tclear_pathspec(&options.pathspec);\n> +\tif (sparse_filter_data)\n> +\t\tfree_sparse_filter_data(sparse_filter_data);\n\nI suggested a fixup for this, but as an aside don't make a free_*()\nfunction that's not happy to accept NULL, it should work like the free()\nitself.\n\n> +\n> \treturn ret;\n> }\n> diff --git a/dir.c b/dir.c\n> index 4e99f0c868f..122ebced08e 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -1457,45 +1457,50 @@ int init_sparse_checkout_patterns(struct index_state *istate)\n> \treturn 0;\n> }\n> \n> -static int path_in_sparse_checkout_1(const char *path,\n> -\t\t\t\t struct index_state *istate,\n> -\t\t\t\t int require_cone_mode)\n> +int recursively_match_path_with_sparse_patterns(const char *path,\n> +\t\t\t\t\t\tstruct index_state *istate,\n> +\t\t\t\t\t\tint dtype,\n> +\t\t\t\t\t\tstruct pattern_list *pl)\n> {\n> -\tint dtype = DT_REG;\n> \tenum pattern_match_result match = UNDECIDED;\n> \tconst char *end, *slash;\n> -\n> -\t/*\n> -\t * We default to accepting a path if the path is empty, there are no\n> -\t * patterns, or the patterns are of the wrong type.\n> -\t */\n> -\tif (!*path ||\n> -\t init_sparse_checkout_patterns(istate) ||\n> -\t (require_cone_mode &&\n> -\t !istate->sparse_checkout_patterns->use_cone_patterns))\n> -\t\treturn 1;\n> -\n> \t/*\n> \t * If UNDECIDED, use the match from the parent dir (recursively), or\n> \t * fall back to NOT_MATCHED at the topmost level. Note that cone mode\n> \t * never returns UNDECIDED, so we will execute only one iteration in\n> \t * this case.\n> \t */\n> -\tfor (end = path + strlen(path);\n> -\t end > path && match == UNDECIDED;\n> +\tfor (end = path + strlen(path); end > path && match == UNDECIDED;\n\nAll good, aside from this whitespace refactoring, as noted.\n\n> \t end = slash) {\n> -\n> \t\tfor (slash = end - 1; slash > path && *slash != '/'; slash--)\n> \t\t\t; /* do nothing */\n> \n> \t\tmatch = path_matches_pattern_list(path, end - path,\n> \t\t\t\tslash > path ? slash + 1 : path, &dtype,\n> -\t\t\t\tistate->sparse_checkout_patterns, istate);\n> +\t\t\t\tpl, istate);\n> \n> \t\t/* We are going to match the parent dir now */\n> \t\tdtype = DT_DIR;\n> \t}\n> -\treturn match > 0;\n> +\n> +\treturn match;\n> +}\n> +\n> +static int path_in_sparse_checkout_1(const char *path,\n> +\t\t\t\t struct index_state *istate,\n> +\t\t\t\t int require_cone_mode)\n> +{\n> +\t/*\n> +\t * We default to accepting a path if the path is empty, there are no\n> +\t * patterns, or the patterns are of the wrong type.\n> +\t */\n> +\tif (!*path ||\n> +\t init_sparse_checkout_patterns(istate) ||\n> +\t (require_cone_mode &&\n> +\t !istate->sparse_checkout_patterns->use_cone_patterns))\n> +\t\treturn 1;\n> +\n> +\treturn recursively_match_path_with_sparse_patterns(path, istate, DT_REG, istate->sparse_checkout_patterns) > 0;\n\nAll good, except for the really long line, aside from the previous\nsuggestion, maybe pull \"istate->sparse_checkout_patterns\" into a\nvariable, as it's now used twice here in this function?\n\n> +check_agrees_with_ls_files () {\n> +\tREPO=repo &&\n> +\tgit -C $REPO submodule deinit -f --all &&\n> +\tgit -C $REPO cat-file -p $filter_oid >\"$REPO/.git/info/sparse-checkout\" &&\n> +\tgit -C $REPO sparse-checkout init --cone &&\n> +\tgit -C $REPO submodule init &&\n> +\tgit -C $REPO ls-files -t >out &&\n> +\tsed -n \"/^S /!s/^. //p\" out >expect &&\n> +\ttest_cmp expect actual\n\nInstead of this last \"sed\" munging, why not use the \"--format\" option to\n\"ls-tree\" to just make the output the same?\n"},{"id":"470982","messageId":"996451ae-0d9d-e09c-0fe4-8b037b72ec5c@unity3d.com","threadId":"59071","inReplyTo":"230123.865ycxtn7i.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2] ls-tree: add --sparse-filter-oid argument","fromName":"William Sprent","fromEmail":"williams@unity3d.com","sentAt":"2023-01-24T15:30:02Z","receivedAt":"2023-01-24T15:30:08Z","isPatch":true,"sender":{"key":"williams@unity3d.com","avatar":"https://avatars.githubusercontent.com/u/77490534?v=4"},"body":"On 23/01/2023 14.00, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Mon, Jan 23 2023, William Sprent via GitGitGadget wrote:\n> \n>> From: William Sprent \n>>\n>> There is currently no way to ask git the question \"which files would be\n>> part of a sparse checkout of commit X with sparse checkout patterns Y\".\n>> One use-case would be that tooling may want know whether sparse checkouts\n>> of two commits contain the same content even if the full trees differ.\n>> Another interesting use-case would be for tooling to use in conjunction\n>> with 'git update-index --index-info'.\n> \n> Rather than commenting on individual points, I checked this out and\n> produced something squashable on-top, it fixes various issues (some\n> aspects of which still remain):\n\nThanks. The changes all make sense to me. I'll apply them for v3 and\nshorten the remaining long lines.\n\n> \n> * You need to wrap your code at 79 chars (and some of that could be\n> helped by picking less verbose identifiers & variable names,\n> e.g. just use \"context\" rather than \"read_tree_context\" in\n> cmd_ls_tree(), it has no other \"context\"...)> * You're making the memory management aroind init_sparse_filter_data()\n> needlessly hard, just put it on the stack instead. That also allows\n> for init-ing most of it right away, or via a macro in the assignment.\n\nFair point. I do think that having all the initialisation of the struct\nin one place makes the codes a bit more readable. But I guess it doesn't\nmake a big difference in this case.\n\n> * You have a stray refactoring of dir.c in your proposed change, this\n> changes it back, let's leave that sort of thing out.\n> * You're adding a function that returns an enum, but you declare it as\n> returning \"int\", let's retain that type in the header & declaration.\n\nThese were oversights. Thanks for catching them.\n\n> \n> diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c\n> index 46a815fc7dc..68d6ef675f2 100644\n> --- a/builtin/ls-tree.c\n> +++ b/builtin/ls-tree.c\n> @@ -372,36 +372,37 @@ struct sparse_filter_data {\n> \tstruct ls_tree_options *options;\n> };\n> \n> -static void init_sparse_filter_data(struct sparse_filter_data **d, struct ls_tree_options *options,\n> -\tconst char *sparse_oid_name, read_tree_fn_t fn)\n> +static void init_sparse_filter_data(struct sparse_filter_data *d,\n> +\t\t\t\t const char *sparse_oid_name)\n> {\n> \tstruct object_id sparse_oid;\n> \tstruct object_context oc;\n> \n> -\t(*d) = xcalloc(1, sizeof(**d));\n> -\t(*d)->fn = fn;\n> -\t(*d)->pl.use_cone_patterns = core_sparse_checkout_cone;\n> -\t(*d)->options = options;\n> -\tstrbuf_init(&(*d)->full_path_buf, 0);\n> -\n> \tif (get_oid_with_context(the_repository, sparse_oid_name, GET_OID_BLOB,\n> \t\t\t&sparse_oid, &oc))\n> -\t\tdie(_(\"unable to access sparse blob in '%s'\"), sparse_oid_name);\n> -\tif (add_patterns_from_blob_to_list(&sparse_oid, \"\", 0, &(*d)->pl) < 0)\n> +\t\tdie(_(\"unable to access sparse blob in '%s'\"),\n> +\t\t sparse_oid_name);\n> +\tif (add_patterns_from_blob_to_list(&sparse_oid, \"\", 0, &d->pl) < 0)\n> \t\tdie(_(\"unable to parse sparse filter data in %s\"),\n> \t\t oid_to_hex(&sparse_oid));\n> }\n> \n> -static void free_sparse_filter_data(struct sparse_filter_data *d)\n> +static void release_sparse_filter_data(struct sparse_filter_data *d)\n> {\n> \tclear_pattern_list(&d->pl);\n> \tstrbuf_release(&d->full_path_buf);\n> -\tfree(d);\n> }\n> \n> -static int path_matches_sparse_checkout_patterns(struct strbuf *full_path, struct pattern_list *pl, int dtype)\n> +static int path_matches_sparse_checkout_patterns(struct strbuf *full_path,\n> +\t\t\t\t\t\t struct pattern_list *pl,\n> +\t\t\t\t\t\t int dtype)\n> {\n> -\tenum pattern_match_result match = recursively_match_path_with_sparse_patterns(full_path->buf, the_repository->index, dtype, pl);\n> +\tenum pattern_match_result match;\n> +\n> +\tmatch = recursively_match_path_with_sparse_patterns(full_path->buf,\n> +\t\t\t\t\t\t\t the_repository->index,\n> +\t\t\t\t\t\t\t dtype, pl);\n> +\n> \treturn match > 0;\n> }\n> \n> @@ -436,7 +437,10 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n> \tstruct ls_tree_options options = { 0 };\n> \tchar *sparse_oid_name = NULL;\n> \tvoid *read_tree_context = NULL;\n> -\tstruct sparse_filter_data *sparse_filter_data = NULL;\n> +\tstruct sparse_filter_data sparse_filter_data = {\n> +\t\t.options = &options,\n> +\t\t.full_path_buf = STRBUF_INIT,\n> +\t};\n> \tconst struct option ls_tree_options[] = {\n> \t\tOPT_BIT('d', NULL, &options.ls_options, N_(\"only show trees\"),\n> \t\t\tLS_TREE_ONLY),\n> @@ -542,16 +546,17 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n> \t}\n> \n> \tif (sparse_oid_name) {\n> -\t\tinit_sparse_filter_data(&sparse_filter_data, &options, sparse_oid_name, fn);\n> -\t\tread_tree_context = sparse_filter_data;\n> +\t\tsparse_filter_data.fn = fn;\n> +\t\tinit_sparse_filter_data(&sparse_filter_data, sparse_oid_name);\n> +\t\tread_tree_context = &sparse_filter_data;\n> \t\tfn = filter_sparse;\n> \t} else\n> \t\tread_tree_context = &options;\n> \n> -\tret = !!read_tree(the_repository, tree, &options.pathspec, fn, read_tree_context);\n> +\tret = !!read_tree(the_repository, tree, &options.pathspec, fn,\n> +\t\t\t read_tree_context);\n> \tclear_pathspec(&options.pathspec);\n> -\tif (sparse_filter_data)\n> -\t\tfree_sparse_filter_data(sparse_filter_data);\n> +\trelease_sparse_filter_data(&sparse_filter_data);\n> \n> \treturn ret;\n> }\n> diff --git a/dir.c b/dir.c\n> index 122ebced08e..57ec6404901 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -1457,10 +1457,10 @@ int init_sparse_checkout_patterns(struct index_state *istate)\n> \treturn 0;\n> }\n> \n> -int recursively_match_path_with_sparse_patterns(const char *path,\n> -\t\t\t\t\t\tstruct index_state *istate,\n> -\t\t\t\t\t\tint dtype,\n> -\t\t\t\t\t\tstruct pattern_list *pl)\n> +enum pattern_match_result recursively_match_path_with_sparse_patterns(const char *path,\n> +\t\t\t\t\t\t\t\t struct index_state *istate,\n> +\t\t\t\t\t\t\t\t int dtype,\n> +\t\t\t\t\t\t\t\t struct pattern_list *pl)\n> {\n> \tenum pattern_match_result match = UNDECIDED;\n> \tconst char *end, *slash;\n> @@ -1470,7 +1470,8 @@ int recursively_match_path_with_sparse_patterns(const char *path,\n> \t * never returns UNDECIDED, so we will execute only one iteration in\n> \t * this case.\n> \t */\n> -\tfor (end = path + strlen(path); end > path && match == UNDECIDED;\n> +\tfor (end = path + strlen(path);\n> +\t end > path && match == UNDECIDED;\n> \t end = slash) {\n> \t\tfor (slash = end - 1; slash > path && *slash != '/'; slash--)\n> \t\t\t; /* do nothing */\n> diff --git a/dir.h b/dir.h\n> index d186d271606..3c65e68ca40 100644\n> --- a/dir.h\n> +++ b/dir.h\n> @@ -403,10 +403,10 @@ int path_in_sparse_checkout(const char *path,\n> int path_in_cone_mode_sparse_checkout(const char *path,\n> \t\t\t\t struct index_state *istate);\n> \n> -int recursively_match_path_with_sparse_patterns(const char *path,\n> -\t\t\t\t\t\tstruct index_state *istate,\n> -\t\t\t\t\t\tint dtype,\n> -\t\t\t\t\t\tstruct pattern_list *pl);\n> +enum pattern_match_result recursively_match_path_with_sparse_patterns(const char *path,\n> +\t\t\t\t\t\t\t\t struct index_state *istate,\n> +\t\t\t\t\t\t\t\t int dtype,\n> +\t\t\t\t\t\t\t\t struct pattern_list *pl);\n> \n> struct dir_entry *dir_add_ignored(struct dir_struct *dir,\n> \t\t\t\t struct index_state *istate,\n\n"},{"id":"470983","messageId":"9787a68d-80a3-f554-355c-5015798a8816@unity3d.com","threadId":"59071","inReplyTo":"230123.861qnltmbp.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2] ls-tree: add --sparse-filter-oid argument","fromName":"William Sprent","fromEmail":"williams@unity3d.com","sentAt":"2023-01-24T15:30:29Z","receivedAt":"2023-01-24T15:30:36Z","isPatch":true,"sender":{"key":"williams@unity3d.com","avatar":"https://avatars.githubusercontent.com/u/77490534?v=4"},"body":"On 23/01/2023 14.06, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Mon, Jan 23 2023, William Sprent via GitGitGadget wrote:\n> \n>> From: William Sprent \n> \n> ...some further inline commentary, in addition to my proposed squash-in...\n> \n\nThanks for taking the time. :)\n\n>> diff --git a/Documentation/git-ls-tree.txt b/Documentation/git-ls-tree.txt\n>> index 0240adb8eec..724bb1f9dbd 100644\n>> --- a/Documentation/git-ls-tree.txt\n>> +++ b/Documentation/git-ls-tree.txt\n>> @@ -11,6 +11,7 @@ SYNOPSIS\n>> [verse]\n>> 'git ls-tree' [-d] [-r] [-t] [-l] [-z]\n>> \t [--name-only] [--name-status] [--object-only] [--full-name] [--full-tree] [--abbrev[=]] [--format=]\n>> +\t [--sparse-filter-oid=]\n>> \t [...]\n>> \n>> DESCRIPTION\n>> @@ -48,6 +49,11 @@ OPTIONS\n>> \tShow tree entries even when going to recurse them. Has no effect\n>> \tif `-r` was not passed. `-d` implies `-t`.\n>> \n>> +--sparse-filter-oid=::\n>> +\tOmit showing tree objects and paths that do not match the\n>> +\tsparse-checkout specification contained in the blob\n>> +\t(or blob-expression) .\n>> +\n>> -l::\n>> --long::\n>> \tShow object size of blob (file) entries.\n>> diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c\n>> index 72eb70823d5..46a815fc7dc 100644\n>> --- a/builtin/ls-tree.c\n>> +++ b/builtin/ls-tree.c\n>> @@ -13,6 +13,7 @@\n>> #include \"builtin.h\"\n>> #include \"parse-options.h\"\n>> #include \"pathspec.h\"\n>> +#include \"dir.h\"\n>> \n>> static const char * const ls_tree_usage[] = {\n>> \tN_(\"git ls-tree [] [...]\"),\n>> @@ -364,6 +365,65 @@ static struct ls_tree_cmdmode_to_fmt ls_tree_cmdmode_format[] = {\n>> \t},\n>> };\n> \n> Okey, here yo uupdate the *.txt, but not the \"-h\", but it's one of those\n> where way say .\n> \n> I for one wouldn't mind some preceding change like my 423be1f83c5 (doc\n> txt & -h consistency: make \"commit\" consistent, 2022-10-13), which in\n> turn would make t/t0450-txt-doc-vs-help.sh pass for this command, but\n> maybe it's too much of a digression...\n> \n\nI don't mind doing that.\n\n>> +\t(*d) = xcalloc(1, sizeof(**d));\n>> +\t(*d)->fn = fn;\n>> +\t(*d)->pl.use_cone_patterns = core_sparse_checkout_cone;\n> \n> I forgot to note in my proposed fix-up that I omitted this, but your\n> tests still pass, which suggests it's either not needed, or your tests\n> are lacking....\n> \n\nWell.. The line exists to enable the cone mode optimisations. The only \nobservable differences are performance and that a warning is emitted \nwhen cone mode is enabled in the config but the patterns given aren't in \nthe cone mode pattern set.\n\nI can look into writing a performance test that captures the performance \ndifferences.\n\nI can also test for the latter behaviour. But that is testing for the \nside effect of a main behaviour to ensure that the main behaviour is \nthere. Which isn't the nicest thing I can think of, but it might be \nbetter than nothing.\n\n>> +\t(*d)->options = options;\n>> +\tstrbuf_init(&(*d)->full_path_buf, 0);\n>> +\n>> +\tif (get_oid_with_context(the_repository, sparse_oid_name, GET_OID_BLOB,\n>> +\t\t\t&sparse_oid, &oc))\n>> +\t\tdie(_(\"unable to access sparse blob in '%s'\"), sparse_oid_name);\n>> +\tif (add_patterns_from_blob_to_list(&sparse_oid, \"\", 0, &(*d)->pl) < 0)\n> \n> As noted you can avoid the malloc here, which also sidesteps this (IMO\n> at last) ugly &(*v)->m syntax.\n> \n\nRight.\n\n>> +\t\tdie(_(\"unable to parse sparse filter data in %s\"),\n>> +\t\t oid_to_hex(&sparse_oid));\n>> +}\n>> +\n>> +static void free_sparse_filter_data(struct sparse_filter_data *d)\n>> +{\n>> +\tclear_pattern_list(&d->pl);\n>> +\tstrbuf_release(&d->full_path_buf);\n>> +\tfree(d);\n>> +}\n>> +\n>> +static int path_matches_sparse_checkout_patterns(struct strbuf *full_path, struct pattern_list *pl, int dtype)\n>> +{\n>> +\tenum pattern_match_result match = recursively_match_path_with_sparse_patterns(full_path->buf, the_repository->index, dtype, pl);\n> \n> I for one would welcome e.g. abbreviating \"sparse_checkout_patterns\" as\n> \"scp\" or whatever throughout, with resulting shortening of very long\n> lines & identifiers. E.g. for this one \"patch_match_scp\" or whatever.\n> \n> \n\nI don't mind either way, but a quick search doesn't reveal any of uses \nof an abbreviation like, though.\n\nEither way, I could also drop the '_checkout_' from \n'sparse_checkout_patterns'. I don't think that would make it less clear \nwhat we are talking about.\n\n>> +\tstruct sparse_filter_data *data = context;\n>> +\n> \n> Style: Don't add a \\n\\n between variable decls,\n> \n>> +\tint do_recurse = show_recursive(data->options, base->buf, base->len, pathname);\n> \n> Instead add it here, before the code proper.\n> \n> \n\nAlright. I'll apply that.\n\n>> +\tif (object_type(mode) == OBJ_TREE)\n>> +\t\treturn do_recurse;\n>> +\n>> +\tstrbuf_reset(&data->full_path_buf);\n> \n> I wondered if we need this after, but we don't, I for one would welcome a fix-up like:\n> \t\n> \tdiff --git a/builtin/ls-tree.c b/builtin/ls-tree.c\n> \tindex 68d6ef675f2..74dfa9a4580 100644\n> \t--- a/builtin/ls-tree.c\n> \t+++ b/builtin/ls-tree.c\n> \t@@ -410,19 +410,21 @@ static int filter_sparse(const struct object_id *oid, struct strbuf *base,\n> \t \t\t\t const char *pathname, unsigned mode, void *context)\n> \t {\n> \t \tstruct sparse_filter_data *data = context;\n> \t-\n> \t \tint do_recurse = show_recursive(data->options, base->buf, base->len, pathname);\n> \t+\tint ret = 0;\n> \t+\n> \t \tif (object_type(mode) == OBJ_TREE)\n> \t \t\treturn do_recurse;\n> \t\n> \t-\tstrbuf_reset(&data->full_path_buf);\n> \t \tstrbuf_addbuf(&data->full_path_buf, base);\n> \t \tstrbuf_addstr(&data->full_path_buf, pathname);\n> \t\n> \t \tif (!path_matches_sparse_checkout_patterns(&data->full_path_buf, &data->pl, DT_REG))\n> \t-\t\t\treturn 0;\n> \t-\n> \t-\treturn data->fn(oid, base, pathname, mode, data->options);\n> \t+\t\tgoto cleanup;\n> \t+\tret = data->fn(oid, base, pathname, mode, data->options);\n> \t+cleanup:\n> \t+\tstrbuf_reset(&data->full_path_buf);\n> \t+\treturn ret;\n> \t }\n> \t\n> \t int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n> \t\n> It's slightly more verbose, and we do end up needlessly reset-ing the\n> \"last\" one, but makes it clear that we don't have need for this after\n> this.\n> \n> Which to me, also raises the question of why you have this\n> \"full_path_buf\" at all. It looks like a memory optimization, as you're\n> trying to avoid a malloc()/free() in the loop.\n> \n> That's fair, but you could also do that with:\n> \n> \tconst size_t oldlen = base->len;\n> \tstrbuf_addstr(base, pathname);\n> /* do the path_matches_sparse_checkout_patterns() call here */\n> \t/* before \"cleanup\" */\n> \tstrbuf_setlen(base, oldlen);\n> \n> Or whatever, those snippets are untested, but we use similar tricks\n> elsewhere, and as it's contained to a few lines here I think it's better\n> than carrying another buffer.\n> \n\nOkay. That makes sense. And if it is a common trick then I guess it \nisn't too mysterious.\n\n>> +\tstrbuf_addbuf(&data->full_path_buf, base);\n>> +\tstrbuf_addstr(&data->full_path_buf, pathname);\n> \n> Nit: Shorter as: strbuf_addf(&sb, \"%s%s\", base.buf, pathname) (or equivalent);\n> \n>> +\n>> +\tif (!path_matches_sparse_checkout_patterns(&data->full_path_buf, &data->pl, DT_REG))\n>> +\t\t\treturn 0;\n>> +\n>> +\treturn data->fn(oid, base, pathname, mode, data->options);\n>> +}\n>> +\n>> int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n>> {\n>> \tstruct object_id oid;\n>> @@ -372,7 +432,11 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n>> \tread_tree_fn_t fn = NULL;\n>> \tenum ls_tree_cmdmode cmdmode = MODE_DEFAULT;\n>> \tint null_termination = 0;\n>> +\n> \n> Don't add this stray \\n.\n> \n\nOk.\n\n>> \tstruct ls_tree_options options = { 0 };\n>> +\tchar *sparse_oid_name = NULL;\n>> +\tvoid *read_tree_context = NULL;\n>> +\tstruct sparse_filter_data *sparse_filter_data = NULL;\n>> \tconst struct option ls_tree_options[] = {\n>> \t\tOPT_BIT('d', NULL, &options.ls_options, N_(\"only show trees\"),\n>> \t\t\tLS_TREE_ONLY),\n>> @@ -399,6 +463,9 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n>> \t\t\t\t\t N_(\"format to use for the output\"),\n>> \t\t\t\t\t PARSE_OPT_NONEG),\n>> \t\tOPT__ABBREV(&options.abbrev),\n>> +\t\tOPT_STRING_F(0, \"filter-sparse-oid\", &sparse_oid_name, N_(\"filter-sparse-oid\"),\n> \n> Make that N_(...) be \"object-id\", \"oid\", \"hash\" or something?\n> \n> I.e. the RHS with the should be , not\n> .\n> \n\nRight. I've used the \"blob-ish\" wording in the .txt file, I think it \nmakes sense to reuse it here.\n\n>> +\t\t\t N_(\"filter output with sparse-checkout specification contained in the blob\"),\n>> +\t\t\t PARSE_OPT_NONEG),\n>> \t\tOPT_END()\n>> \t};\n>> \tstruct ls_tree_cmdmode_to_fmt *m2f = ls_tree_cmdmode_format;\n>> @@ -474,7 +541,17 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n>> \t\tbreak;\n>> \t}\n>> \n>> -\tret = !!read_tree(the_repository, tree, &options.pathspec, fn, &options);\n>> +\tif (sparse_oid_name) {\n>> +\t\tinit_sparse_filter_data(&sparse_filter_data, &options, sparse_oid_name, fn);\n>> +\t\tread_tree_context = sparse_filter_data;\n>> +\t\tfn = filter_sparse;\n>> +\t} else\n>> +\t\tread_tree_context = &options;\n> \n> You're missing a {} here for the \"else\".\n> \n> Better yet, delete that \"else\" and change the decl above to be:\n> \n> \tvoid *context = &options;\n> \n> Then just keep the \"if\" here, where you might clobber the \"context\".\n> \n\nI'll delete the else as you suggest.\n\n>> +\n>> +\tret = !!read_tree(the_repository, tree, &options.pathspec, fn, read_tree_context);\n>> \tclear_pathspec(&options.pathspec);\n>> +\tif (sparse_filter_data)\n>> +\t\tfree_sparse_filter_data(sparse_filter_data);\n> \n> I suggested a fixup for this, but as an aside don't make a free_*()\n> function that's not happy to accept NULL, it should work like the free()\n> itself.\n> \n\nAh. That's a fair point. Thanks.\n\n>> +\n>> \treturn ret;\n>> }\n>> diff --git a/dir.c b/dir.c\n>> index 4e99f0c868f..122ebced08e 100644\n>> --- a/dir.c\n>> +++ b/dir.c\n>> @@ -1457,45 +1457,50 @@ int init_sparse_checkout_patterns(struct index_state *istate)\n>> \treturn 0;\n>> }\n>> \n>> -static int path_in_sparse_checkout_1(const char *path,\n>> -\t\t\t\t struct index_state *istate,\n>> -\t\t\t\t int require_cone_mode)\n>> +int recursively_match_path_with_sparse_patterns(const char *path,\n>> +\t\t\t\t\t\tstruct index_state *istate,\n>> +\t\t\t\t\t\tint dtype,\n>> +\t\t\t\t\t\tstruct pattern_list *pl)\n>> {\n>> -\tint dtype = DT_REG;\n>> \tenum pattern_match_result match = UNDECIDED;\n>> \tconst char *end, *slash;\n>> -\n>> -\t/*\n>> -\t * We default to accepting a path if the path is empty, there are no\n>> -\t * patterns, or the patterns are of the wrong type.\n>> -\t */\n>> -\tif (!*path ||\n>> -\t init_sparse_checkout_patterns(istate) ||\n>> -\t (require_cone_mode &&\n>> -\t !istate->sparse_checkout_patterns->use_cone_patterns))\n>> -\t\treturn 1;\n>> -\n>> \t/*\n>> \t * If UNDECIDED, use the match from the parent dir (recursively), or\n>> \t * fall back to NOT_MATCHED at the topmost level. Note that cone mode\n>> \t * never returns UNDECIDED, so we will execute only one iteration in\n>> \t * this case.\n>> \t */\n>> -\tfor (end = path + strlen(path);\n>> -\t end > path && match == UNDECIDED;\n>> +\tfor (end = path + strlen(path); end > path && match == UNDECIDED;\n> \n> All good, aside from this whitespace refactoring, as noted.\n> \n\nOk.\n\n>> \t end = slash) {\n>> -\n>> \t\tfor (slash = end - 1; slash > path && *slash != '/'; slash--)\n>> \t\t\t; /* do nothing */\n>> \n>> \t\tmatch = path_matches_pattern_list(path, end - path,\n>> \t\t\t\tslash > path ? slash + 1 : path, &dtype,\n>> -\t\t\t\tistate->sparse_checkout_patterns, istate);\n>> +\t\t\t\tpl, istate);\n>> \n>> \t\t/* We are going to match the parent dir now */\n>> \t\tdtype = DT_DIR;\n>> \t}\n>> -\treturn match > 0;\n>> +\n>> +\treturn match;\n>> +}\n>> +\n>> +static int path_in_sparse_checkout_1(const char *path,\n>> +\t\t\t\t struct index_state *istate,\n>> +\t\t\t\t int require_cone_mode)\n>> +{\n>> +\t/*\n>> +\t * We default to accepting a path if the path is empty, there are no\n>> +\t * patterns, or the patterns are of the wrong type.\n>> +\t */\n>> +\tif (!*path ||\n>> +\t init_sparse_checkout_patterns(istate) ||\n>> +\t (require_cone_mode &&\n>> +\t !istate->sparse_checkout_patterns->use_cone_patterns))\n>> +\t\treturn 1;\n>> +\n>> +\treturn recursively_match_path_with_sparse_patterns(path, istate, DT_REG, istate->sparse_checkout_patterns) > 0;\n> \n> All good, except for the really long line, aside from the previous\n> suggestion, maybe pull \"istate->sparse_checkout_patterns\" into a\n> variable, as it's now used twice here in this function?\n> \n\nI think that makes sense.\n\n>> +check_agrees_with_ls_files () {\n>> +\tREPO=repo &&\n>> +\tgit -C $REPO submodule deinit -f --all &&\n>> +\tgit -C $REPO cat-file -p $filter_oid >\"$REPO/.git/info/sparse-checkout\" &&\n>> +\tgit -C $REPO sparse-checkout init --cone &&\n>> +\tgit -C $REPO submodule init &&\n>> +\tgit -C $REPO ls-files -t >out &&\n>> +\tsed -n \"/^S /!s/^. //p\" out >expect &&\n>> +\ttest_cmp expect actual\n> \n> Instead of this last \"sed\" munging, why not use the \"--format\" option to\n> \"ls-tree\" to just make the output the same?\n\nI hadn't thought about changing that side of the output. I would have to \ncall ls-tree once more instead of relying on the 'expect' file to be \nthere when the function is called. But that might be nicer anyway.\n\n"},{"id":"470999","messageId":"7ccf7b17-4448-5ef4-63b1-9073a400e486@github.com","threadId":"59071","inReplyTo":"pull.1459.v2.git.1674474371817.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] ls-tree: add --sparse-filter-oid argument","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2023-01-24T20:11:34Z","receivedAt":"2023-01-24T20:11:52Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"William Sprent via GitGitGadget wrote:\n> From: William Sprent \n> \n> There is currently no way to ask git the question \"which files would be\n> part of a sparse checkout of commit X with sparse checkout patterns Y\".\n> One use-case would be that tooling may want know whether sparse checkouts\n> of two commits contain the same content even if the full trees differ.\n> Another interesting use-case would be for tooling to use in conjunction\n> with 'git update-index --index-info'.\n\nThese types of use cases align nicely with \"Behavior A\" as described in\n'Documentation/technical/sparse-checkout.txt' [1]. I think adding a\n'--scope=(sparse|all)' option to 'ls-trees' would be a good way to make\nprogress on the goals of that design document as well as serve the needs\ndescribed above.\n\n[1] https://lore.kernel.org/git/pull.1367.v4.git.1667714666810.gitgitgadget@gmail.com/\n\n> \n> 'rev-list --objects --filter=sparse:oid' comes close, but as rev-list is\n> concerned with objects rather than directory trees, it leaves files out\n> when the same blob occurs in at two different paths.\n> \n> It is possible to ask git about the sparse status of files currently in\n> the index with 'ls-files -t'. However, this does not work well when the\n> caller is interested in another commit, intererested in sparsity\n> patterns that aren't currently in '.git/info/sparse-checkout', or when\n> working in with bare repo.\n\nI'm a bit confused by your described use cases here, though. Right now,\n'sparse-checkout' is a local repo-only optimization (for performance and/or\nscoping user workspaces), but you seem to be implying a need for\n\"sparse-checkout patterns\" as a general-purpose data format (like a config\nfile or 'rebase-todo') that can be used to manually configure command\nbehavior.\n\nIf that is what you're getting at, it seems like a pretty substantial\nexpansion to the scope of what we consider \"sparse checkout\". That's not to\nsay it isn't a good idea - I can definitely imagine tooling where this type\nof functionality is useful - just that it warrants careful consideration so\nwe don't over-complicate the typical sparse-checkout user experience. \n\n> \n> To fill this gap, add a new argument to ls-tree '--sparse-filter-oid'\n> which takes the object id of a blob containing sparse checkout patterns\n> that filters the output of 'ls-tree'. When filtering with given sparsity\n> patterns, 'ls-tree' only outputs blobs and commit objects that\n> match the given patterns.\n\nI don't think a blob OID is the best way to specify an arbitrary pattern set\nin this case. OIDs will only be created for blobs that are tracked in Git;\nit's possible that your project tracks potential sparse-checkout patterns in\nGit, but it seems overly restrictive. You could instead specify the file\nwith a path on-disk (like the '--file' options in various commands, e.g.\n'git commit'); if you did need to get that file from the object store, you\ncould first get its contents with 'git cat-file'. \n\n> \n> While it may be valid in some situations to output a tree object -- e.g.\n> when a cone pattern matches all blobs recursively contained in a tree --\n> it is less unclear what should be output if a sparse pattern matches\n> parts of a tree.\n> \n> To allow for reusing the pattern matching logic found in\n> 'path_in_sparse_checkout_1()' in 'dir.c' with arbitrary patterns,\n> extract the pattern matching part of the function into its own new\n> function 'recursively_match_path_with_sparse_patterns()'.\n> \n> Signed-off-by: William Sprent \n> ---\n> ls-tree: add --sparse-filter-oid argument\n> \n> I'm resubmitting this change as rebased on top of 'master', as it\n> conflicted with the topic 'ls-tree.c: clean-up works' 1\n> [https://public-inbox.org/git/20230112091135.20050-1-tenglong.tl@alibaba-inc.com],\n> which was merged to 'master' recently.\n> \n> This versions also incorporates changes based on the comments made in 2\n> [https://public-inbox.org/git/CAPig+cRgZ0CrkqY7mufuWmhf6BC8yXjXXuOTEQjuz+Y0NA+N7Q@mail.gmail.com/].\n> \n> I'm also looping in contributors that have touched ls-tree and/or\n> sparse-checkouts recently. I hope that's okay.\n\nDefinitely okay, thanks for CC-ing!\n\nOverall, this is an interesting extension to 'sparse-checkout', and opens up\nsome opportunities for expanded tooling. However, at an implementation\nlevel, I think it's addressing two separate needs (\"constrain 'ls-files' to\ndisplay files matching patterns\" and \"specify sparse patterns not in\n'.git/info/sparse-checkout'\") in one option, which makes for a somewhat\nconfusing and inflexible user experience. What about instead breaking this\ninto two options:\n\n* --scope=(sparse|all): limits the scope of the files output by (\"Behavior\n A\" vs. \"Behavior B\" from the document linked earlier). \n* --sparse-patterns=: specifies \"override\" patterns to use instead of\n those in '.git/info/sparse-checkout' (whether 'sparse-checkout' is enabled\n for the repository or not).\n\nI haven't looked at your implementation in detail yet, but I did want to\noffer a recommendation in case you hadn't considered it: if you want to\nallow the use of patterns from a user-specified specific file, it would be\nnice to do it in a way that fully replaces the \"default\" sparse-checkout\nsettings at the lowest level (i.e., override the values of\n'core_apply_sparse_checkout', 'core_sparse_checkout_cone', and\n'get_sparse_checkout_filename()'). Doing it that way would both make it\neasier for other commands to add a '--sparse-patterns' option, and avoid the\nseparate code path ('path_in_sparse_checkout_1()' vs.\n'recursively_match_path_with_sparse_patterns()', for example) when dealing\nwith '.git/info/sparse-checkout' patterns vs. manually-specified patterns.\n\n> \n> William\n> \n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1459%2Fwilliams-unity%2Fls-tree-sparse-v2\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1459/williams-unity/ls-tree-sparse-v2\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1459\n\n"},{"id":"471017","messageId":"CABPp-BExS8UGfGzT+w9R_p0sY+_=A0-nRzU5QTOKwfBSmX6c3A@mail.gmail.com","threadId":"59071","inReplyTo":"pull.1459.v2.git.1674474371817.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] ls-tree: add --sparse-filter-oid argument","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-01-25T05:11:24Z","receivedAt":"2023-01-25T05:12:49Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"It looks like Ævar and Victoria have both given really good reviews\nalready, but I think I spotted some additional things to comment on.\n\nOn Mon, Jan 23, 2023 at 3:46 AM William Sprent via GitGitGadget\n wrote:\n>\n> From: William Sprent \n>\n> There is currently no way to ask git the question \"which files would be\n> part of a sparse checkout of commit X with sparse checkout patterns Y\".\n> One use-case would be that tooling may want know whether sparse checkouts\n> of two commits contain the same content even if the full trees differ.\n\nCould you say more about this usecase? Why does tooling need or want\nto know this; won't a checkout of the new commit end up being quick\nand simple? (I'm not saying your usecase is bad, just curious that it\nhad never occurred to me, and I'm afraid I'm still not sure what your\npurpose might be.)\n\n> Another interesting use-case would be for tooling to use in conjunction\n> with 'git update-index --index-info'.\n\nSorry, I'm not following. Could you expound on this a bit?\n\n> 'rev-list --objects --filter=sparse:oid' comes close, but as rev-list is\n> concerned with objects rather than directory trees, it leaves files out\n> when the same blob occurs in at two different paths.\n\ns/in at/at/ ?\n\n> It is possible to ask git about the sparse status of files currently in\n> the index with 'ls-files -t'. However, this does not work well when the\n> caller is interested in another commit, intererested in sparsity\n\ns/intererested/interested/\n\n> patterns that aren't currently in '.git/info/sparse-checkout', or when\n> working in with bare repo.\n\ns/in with bare/with a bare/ or s/in with bare/in a bare/?\n\n> To fill this gap, add a new argument to ls-tree '--sparse-filter-oid'\n> which takes the object id of a blob containing sparse checkout patterns\n> that filters the output of 'ls-tree'. When filtering with given sparsity\n> patterns, 'ls-tree' only outputs blobs and commit objects that\n> match the given patterns.\n\nThis seems slightly unfortunate in that it makes things difficult for\ncone mode users to take advantage of. They will have to figure out\nhow to translate their directory list into sparse checkout patterns\nbefore passing it along, and currently the only way to do that is via\n`git sparse-checkout set ` and reading the patterns from\n$GIT_DIR/info/sparse-checkout, but that toggles the sparsity of the\ncurrent working tree and avoiding changing the current sparse-checkout\nwas something you listed in your commit message as something you\nwanted to avoid.\n\n> While it may be valid in some situations to output a tree object -- e.g.\n> when a cone pattern matches all blobs recursively contained in a tree --\n> it is less unclear what should be output if a sparse pattern matches\n> parts of a tree.\n>\n> To allow for reusing the pattern matching logic found in\n> 'path_in_sparse_checkout_1()' in 'dir.c' with arbitrary patterns,\n> extract the pattern matching part of the function into its own new\n> function 'recursively_match_path_with_sparse_patterns()'.\n>\n> Signed-off-by: William Sprent \n> ---\n> ls-tree: add --sparse-filter-oid argument\n>\n> I'm resubmitting this change as rebased on top of 'master', as it\n> conflicted with the topic 'ls-tree.c: clean-up works' 1\n> [https://public-inbox.org/git/20230112091135.20050-1-tenglong.tl@alibaba-inc.com],\n> which was merged to 'master' recently.\n>\n> This versions also incorporates changes based on the comments made in 2\n> [https://public-inbox.org/git/CAPig+cRgZ0CrkqY7mufuWmhf6BC8yXjXXuOTEQjuz+Y0NA+N7Q@mail.gmail.com/].\n>\n> I'm also looping in contributors that have touched ls-tree and/or\n> sparse-checkouts recently. I hope that's okay.\n\nOf course! It's encouraged, even.\n\n[...]\n> diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c\n[...]\n> +static void init_sparse_filter_data(struct sparse_filter_data **d, struct ls_tree_options *options,\n> + const char *sparse_oid_name, read_tree_fn_t fn)\n> +{\n> + struct object_id sparse_oid;\n> + struct object_context oc;\n> +\n> + (*d) = xcalloc(1, sizeof(**d));\n> + (*d)->fn = fn;\n> + (*d)->pl.use_cone_patterns = core_sparse_checkout_cone;\n\nHmm, so the behavior still depends upon the current sparse-checkout\n(or lack thereof), despite the documentation and rationale of your\nfeature as being there to check how a different sparse checkout would\nbehave?\n\nI would hate to unconditionally turn cone_patterns off, since that\nwould come with a huge performance penalty for the biggest repos. But\nturning it unconditionally on wouldn't be good for the non-cone users.\nThis probably suggests we need something like another flag, or perhaps\nseparate flags for each mode. Separate flags might provide the\nbenefit of allowing cone mode users to specify directories rather than\npatterns, which would make it much easier for them to use.\n\n[...]\n> +static int path_matches_sparse_checkout_patterns(struct strbuf *full_path, struct pattern_list *pl, int dtype)\n> +{\n> + enum pattern_match_result match = recursively_match_path_with_sparse_patterns(full_path->buf, the_repository->index, dtype, pl);\n> + return match > 0;\n\nSo your new caller doesn't care about the pattern_match_result, it\njust wants to know if it got MATCHED or MATCHED_RECURSIVELY...\n\n[...]\n> diff --git a/dir.c b/dir.c\n> index 4e99f0c868f..122ebced08e 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -1457,45 +1457,50 @@ int init_sparse_checkout_patterns(struct index_state *istate)\n> return 0;\n> }\n>\n> -static int path_in_sparse_checkout_1(const char *path,\n> - struct index_state *istate,\n> - int require_cone_mode)\n> +int recursively_match_path_with_sparse_patterns(const char *path,\n\nYou claim it returns an int here, but previously you presumed an enum\npattern_match_result from the new caller.\n\n> + struct index_state *istate,\n> + int dtype,\n> + struct pattern_list *pl)\n> {\n> - int dtype = DT_REG;\n> enum pattern_match_result match = UNDECIDED;\n> const char *end, *slash;\n> -\n> - /*\n> - * We default to accepting a path if the path is empty, there are no\n> - * patterns, or the patterns are of the wrong type.\n> - */\n> - if (!*path ||\n> - init_sparse_checkout_patterns(istate) ||\n> - (require_cone_mode &&\n> - !istate->sparse_checkout_patterns->use_cone_patterns))\n> - return 1;\n> -\n> /*\n> * If UNDECIDED, use the match from the parent dir (recursively), or\n> * fall back to NOT_MATCHED at the topmost level. Note that cone mode\n> * never returns UNDECIDED, so we will execute only one iteration in\n> * this case.\n> */\n> - for (end = path + strlen(path);\n> - end > path && match == UNDECIDED;\n> + for (end = path + strlen(path); end > path && match == UNDECIDED;\n> end = slash) {\n> -\n> for (slash = end - 1; slash > path && *slash != '/'; slash--)\n> ; /* do nothing */\n>\n> match = path_matches_pattern_list(path, end - path,\n> slash > path ? slash + 1 : path, &dtype,\n> - istate->sparse_checkout_patterns, istate);\n> + pl, istate);\n>\n> /* We are going to match the parent dir now */\n> dtype = DT_DIR;\n> }\n> - return match > 0;\n> +\n> + return match;\n\nUm, this last line seems like a potentially scary change in behavior.\nWhy should UNDECIDED return a non-zero value? Previously, we returned\na 0 value for both UNDECIDED and NOT_MATCHED, but you've changed that\nhere. If the change in this last line is actually correct, it should\nbe split out into its own commit and explained in detail in the commit\nmessage.\n\n> +}\n> +\n> +static int path_in_sparse_checkout_1(const char *path,\n> + struct index_state *istate,\n> + int require_cone_mode)\n> +{\n> + /*\n> + * We default to accepting a path if the path is empty, there are no\n> + * patterns, or the patterns are of the wrong type.\n> + */\n> + if (!*path ||\n> + init_sparse_checkout_patterns(istate) ||\n> + (require_cone_mode &&\n> + !istate->sparse_checkout_patterns->use_cone_patterns))\n> + return 1;\n> +\n> + return recursively_match_path_with_sparse_patterns(path, istate, DT_REG, istate->sparse_checkout_patterns) > 0;\n\nOh, you compare to > 0 here...and digging around your only other\ncaller just immediately compares to > 0 as well.\n\nWhy not just have recursively_match_path_with_sparse_patterns() do the\n> 0 check? If it does, returning int is fine. If it doesn't, it\nshould be declared to return enum pattern_match_result.\n"},{"id":"471020","messageId":"569043fb-9766-037e-c587-1545c2978e7d@unity3d.com","threadId":"59071","inReplyTo":"7ccf7b17-4448-5ef4-63b1-9073a400e486@github.com","subject":"Re: [PATCH v2] ls-tree: add --sparse-filter-oid argument","fromName":"William Sprent","fromEmail":"williams@unity3d.com","sentAt":"2023-01-25T13:47:06Z","receivedAt":"2023-01-25T13:47:14Z","isPatch":true,"sender":{"key":"williams@unity3d.com","avatar":"https://avatars.githubusercontent.com/u/77490534?v=4"},"body":"On 24/01/2023 21.11, Victoria Dye wrote:\n> William Sprent via GitGitGadget wrote:\n>> From: William Sprent \n>>\n>> There is currently no way to ask git the question \"which files would be\n>> part of a sparse checkout of commit X with sparse checkout patterns Y\".\n>> One use-case would be that tooling may want know whether sparse checkouts\n>> of two commits contain the same content even if the full trees differ.\n>> Another interesting use-case would be for tooling to use in conjunction\n>> with 'git update-index --index-info'.\n> \n> These types of use cases align nicely with \"Behavior A\" as described in\n> 'Documentation/technical/sparse-checkout.txt' [1]. I think adding a\n> '--scope=(sparse|all)' option to 'ls-trees' would be a good way to make\n> progress on the goals of that design document as well as serve the needs\n> described above.\n> \n> [1] https://lore.kernel.org/git/pull.1367.v4.git.1667714666810.gitgitgadget@gmail.com/\n> >>\n>> 'rev-list --objects --filter=sparse:oid' comes close, but as rev-list is\n>> concerned with objects rather than directory trees, it leaves files out\n>> when the same blob occurs in at two different paths.\n>>\n>> It is possible to ask git about the sparse status of files currently in\n>> the index with 'ls-files -t'. However, this does not work well when the\n>> caller is interested in another commit, intererested in sparsity\n>> patterns that aren't currently in '.git/info/sparse-checkout', or when\n>> working in with bare repo.\n> \n> I'm a bit confused by your described use cases here, though. Right now,\n> 'sparse-checkout' is a local repo-only optimization (for performance and/or\n> scoping user workspaces), but you seem to be implying a need for\n> \"sparse-checkout patterns\" as a general-purpose data format (like a config\n> file or 'rebase-todo') that can be used to manually configure command\n> behavior.\n> \n> If that is what you're getting at, it seems like a pretty substantial\n> expansion to the scope of what we consider \"sparse checkout\". That's not to\n> say it isn't a good idea - I can definitely imagine tooling where this type\n> of functionality is useful - just that it warrants careful consideration so\n> we don't over-complicate the typical sparse-checkout user experience.\n> \n\nI guess it is sort of what I'm getting at. I want to enable tooling (in \nparticular) to be able to interrogate git about sparse checkouts without \nhaving to checkout a commit and load a set of patterns. There's already \na deterministic relationship between a (commit * sparse checkout \npatterns) and a list of included files, so I think it makes sense to be \nable to ask git about it.\n\nI agree with your concerns about not over complicating things for the \naverage user. I think the cone/non-cone distinctions are already a bit \nhard to grasp (at least it was for me). FWIW, it isn't my intention that \nan average user should use the \"explicitly pass patterns to ls-tree\" \nfunctionality. But of course we should still get it right.\n\n\n>>\n>> To fill this gap, add a new argument to ls-tree '--sparse-filter-oid'\n>> which takes the object id of a blob containing sparse checkout patterns\n>> that filters the output of 'ls-tree'. When filtering with given sparsity\n>> patterns, 'ls-tree' only outputs blobs and commit objects that\n>> match the given patterns.\n> \n> I don't think a blob OID is the best way to specify an arbitrary pattern set\n> in this case. OIDs will only be created for blobs that are tracked in Git;\n> it's possible that your project tracks potential sparse-checkout patterns in\n> Git, but it seems overly restrictive. You could instead specify the file\n> with a path on-disk (like the '--file' options in various commands, e.g.\n> 'git commit'); if you did need to get that file from the object store, you\n> could first get its contents with 'git cat-file'.\n> \n\nI'm fine with changing it to be a file. I picked it to align with\n\n 'git rev-list --filter=sparse:'\n\nwhich I think is the only other place in the code base where you can \nquery about arbitrary filters. But I agree, it is a bit awkward to use.\n\n>>\n>> While it may be valid in some situations to output a tree object -- e.g.\n>> when a cone pattern matches all blobs recursively contained in a tree --\n>> it is less unclear what should be output if a sparse pattern matches\n>> parts of a tree.\n>>\n>> To allow for reusing the pattern matching logic found in\n>> 'path_in_sparse_checkout_1()' in 'dir.c' with arbitrary patterns,\n>> extract the pattern matching part of the function into its own new\n>> function 'recursively_match_path_with_sparse_patterns()'.\n>>\n>> Signed-off-by: William Sprent \n>> ---\n>> ls-tree: add --sparse-filter-oid argument\n>> \n>> I'm resubmitting this change as rebased on top of 'master', as it\n>> conflicted with the topic 'ls-tree.c: clean-up works' 1\n>> [https://public-inbox.org/git/20230112091135.20050-1-tenglong.tl@alibaba-inc.com],\n>> which was merged to 'master' recently.\n>> \n>> This versions also incorporates changes based on the comments made in 2\n>> [https://public-inbox.org/git/CAPig+cRgZ0CrkqY7mufuWmhf6BC8yXjXXuOTEQjuz+Y0NA+N7Q@mail.gmail.com/].\n>> \n>> I'm also looping in contributors that have touched ls-tree and/or\n>> sparse-checkouts recently. I hope that's okay.\n> \n> Definitely okay, thanks for CC-ing!\n> \n> Overall, this is an interesting extension to 'sparse-checkout', and opens up\n> some opportunities for expanded tooling. However, at an implementation\n> level, I think it's addressing two separate needs (\"constrain 'ls-files' to\n> display files matching patterns\" and \"specify sparse patterns not in\n> '.git/info/sparse-checkout'\") in one option, which makes for a somewhat\n> confusing and inflexible user experience. What about instead breaking this\n> into two options:\n> \n> * --scope=(sparse|all): limits the scope of the files output by (\"Behavior\n> A\" vs. \"Behavior B\" from the document linked earlier).\n> * --sparse-patterns=: specifies \"override\" patterns to use instead of\n> those in '.git/info/sparse-checkout' (whether 'sparse-checkout' is enabled\n> for the repository or not).\n> \n\nThat makes sense to me. I'll look into making those changes.\n\n> I haven't looked at your implementation in detail yet, but I did want to\n> offer a recommendation in case you hadn't considered it: if you want to\n> allow the use of patterns from a user-specified specific file, it would be\n> nice to do it in a way that fully replaces the \"default\" sparse-checkout\n> settings at the lowest level (i.e., override the values of\n> 'core_apply_sparse_checkout', 'core_sparse_checkout_cone', and\n> 'get_sparse_checkout_filename()'). Doing it that way would both make it\n> easier for other commands to add a '--sparse-patterns' option, and avoid the\n> separate code path ('path_in_sparse_checkout_1()' vs.\n> 'recursively_match_path_with_sparse_patterns()', for example) when dealing\n> with '.git/info/sparse-checkout' patterns vs. manually-specified patterns.\n> \n\nThanks for the pointers. I'll see what I can do. Do you mean something \nalong the line of the following?\n\n set_sparse_checkout_file(filename);\n init_sparse_checkout_patterns(istate);\n _ = path_in_sparse_checkout_1(some_path, istate, ...);\n\n\nI do I think I like the explicitness of first loading the pattern_list \nand then passing it to the matching function, though. But maybe it is \ntoo cumbersome.\n\n>> \n>> William\n>>\n>> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1459%2Fwilliams-unity%2Fls-tree-sparse-v2\n>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1459/williams-unity/ls-tree-sparse-v2\n>> Pull-Request: https://github.com/gitgitgadget/git/pull/1459\n> \n\n"},{"id":"471024","messageId":"18c94f70-4adf-1b4a-8777-206804c419e6@unity3d.com","threadId":"59071","inReplyTo":"CABPp-BExS8UGfGzT+w9R_p0sY+_=A0-nRzU5QTOKwfBSmX6c3A@mail.gmail.com","subject":"Re: [PATCH v2] ls-tree: add --sparse-filter-oid argument","fromName":"William Sprent","fromEmail":"williams@unity3d.com","sentAt":"2023-01-25T16:16:16Z","receivedAt":"2023-01-25T16:16:24Z","isPatch":true,"sender":{"key":"williams@unity3d.com","avatar":"https://avatars.githubusercontent.com/u/77490534?v=4"},"body":"On 25/01/2023 06.11, Elijah Newren wrote:\n> It looks like Ævar and Victoria have both given really good reviews\n> already, but I think I spotted some additional things to comment on.\n> \n> On Mon, Jan 23, 2023 at 3:46 AM William Sprent via GitGitGadget\n> wrote:\n>>\n>> From: William Sprent \n>>\n>> There is currently no way to ask git the question \"which files would be\n>> part of a sparse checkout of commit X with sparse checkout patterns Y\".\n>> One use-case would be that tooling may want know whether sparse checkouts\n>> of two commits contain the same content even if the full trees differ.\n> \n> Could you say more about this usecase? Why does tooling need or want\n> to know this; won't a checkout of the new commit end up being quick\n> and simple? (I'm not saying your usecase is bad, just curious that it\n> had never occurred to me, and I'm afraid I'm still not sure what your\n> purpose might be.)\n> \n\nI'm thinking mainly about a monorepo context where there are a number of \ndistinct 'units' that can be described with sparse checkout patterns. \nAnd perhaps there's some tooling that only wants to perform an action if \nthe content of a 'unit' changes.\n\nDepending on the repo, it won't necessarily be quick to check out the \ncommit with the given patterns. However, it is more about it being \ninconvenient to have to have a working directory, especially so if you \nwant use the tooling in some kind of service or query rapidly about \ndifferent revisions/patterns.\n\n>> Another interesting use-case would be for tooling to use in conjunction\n>> with 'git update-index --index-info'.\n> \n> Sorry, I'm not following. Could you expound on this a bit?\n> \n\nI was imagining something along the lines of being able to generate new \ntree objects based on what matches the given sparse checkout patterns. \nNot that I have a specific use case for it right now.\n\nI think what I'm trying to evoke with that paragraph is that this \nenables integrations with git that seem interesting and weren't possible \nbefore.\n\n>> 'rev-list --objects --filter=sparse:oid' comes close, but as rev-list is\n>> concerned with objects rather than directory trees, it leaves files out\n>> when the same blob occurs in at two different paths.\n> \n> s/in at/at/ ?\n> >> It is possible to ask git about the sparse status of files currently in\n>> the index with 'ls-files -t'. However, this does not work well when the\n>> caller is interested in another commit, intererested in sparsity\n> \n> s/intererested/interested/\n> \n>> patterns that aren't currently in '.git/info/sparse-checkout', or when\n>> working in with bare repo.\n> \n> s/in with bare/with a bare/ or s/in with bare/in a bare/?\n> \n\nAh. Thanks for catching those.\n\n>> To fill this gap, add a new argument to ls-tree '--sparse-filter-oid'\n>> which takes the object id of a blob containing sparse checkout patterns\n>> that filters the output of 'ls-tree'. When filtering with given sparsity\n>> patterns, 'ls-tree' only outputs blobs and commit objects that\n>> match the given patterns.\n> \n> This seems slightly unfortunate in that it makes things difficult for\n> cone mode users to take advantage of. They will have to figure out\n> how to translate their directory list into sparse checkout patterns\n> before passing it along, and currently the only way to do that is via\n> `git sparse-checkout set ` and reading the patterns from\n> $GIT_DIR/info/sparse-checkout, but that toggles the sparsity of the\n> current working tree and avoiding changing the current sparse-checkout\n> was something you listed in your commit message as something you\n> wanted to avoid.\n> >> While it may be valid in some situations to output a tree object -- e.g.\n>> when a cone pattern matches all blobs recursively contained in a tree --\n>> it is less unclear what should be output if a sparse pattern matches\n>> parts of a tree.\n>>\n>> To allow for reusing the pattern matching logic found in\n>> 'path_in_sparse_checkout_1()' in 'dir.c' with arbitrary patterns,\n>> extract the pattern matching part of the function into its own new\n>> function 'recursively_match_path_with_sparse_patterns()'.\n>>\n>> Signed-off-by: William Sprent \n>> ---\n>> ls-tree: add --sparse-filter-oid argument\n>>\n>> I'm resubmitting this change as rebased on top of 'master', as it\n>> conflicted with the topic 'ls-tree.c: clean-up works' 1\n>> [https://public-inbox.org/git/20230112091135.20050-1-tenglong.tl@alibaba-inc.com],\n>> which was merged to 'master' recently.\n>>\n>> This versions also incorporates changes based on the comments made in 2\n>> [https://public-inbox.org/git/CAPig+cRgZ0CrkqY7mufuWmhf6BC8yXjXXuOTEQjuz+Y0NA+N7Q@mail.gmail.com/].\n>>\n>> I'm also looping in contributors that have touched ls-tree and/or\n>> sparse-checkouts recently. I hope that's okay.\n> \n> Of course! It's encouraged, even.\n> \n> [...]\n>> diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c\n> [...]\n>> +static void init_sparse_filter_data(struct sparse_filter_data **d, struct ls_tree_options *options,\n>> + const char *sparse_oid_name, read_tree_fn_t fn)\n>> +{\n>> + struct object_id sparse_oid;\n>> + struct object_context oc;\n>> +\n>> + (*d) = xcalloc(1, sizeof(**d));\n>> + (*d)->fn = fn;\n>> + (*d)->pl.use_cone_patterns = core_sparse_checkout_cone;\n> \n> Hmm, so the behavior still depends upon the current sparse-checkout\n> (or lack thereof), despite the documentation and rationale of your\n> feature as being there to check how a different sparse checkout would\n> behave?\n> \n> I would hate to unconditionally turn cone_patterns off, since that\n> would come with a huge performance penalty for the biggest repos. But\n> turning it unconditionally on wouldn't be good for the non-cone users.\n> This probably suggests we need something like another flag, or perhaps\n> separate flags for each mode. Separate flags might provide the\n> benefit of allowing cone mode users to specify directories rather than\n> patterns, which would make it much easier for them to use.\n>\nI used 'core_sparse_checkout_cone' because I wanted to allow for the \ncone mode optimisations, but I also figured that I should respect the \nconfiguration. It doesn't change how the patterns are parsed in this case.\n\nI agree that it is a bit awkward to have to \"translate\" the directories \ninto patterns when wanting to use cone mode. I can try adding\n'--[no]-cone' flags and see how that feels. Together with Victoria's \nsuggestions that would result in having the following flags:\n\n* --scope=(sparse|all)\n* --sparse-patterns-file=\n* --[no]-cone: used together with --sparse-patterns-file to tell git\n whether to interpret the patterns given as directories (cone) or\n patterns (no-cone).\n\nWhich seems like a lot at first glance. But it allows for passing \ndirectories instead of patterns for cone mode, and is similar to the \nbehaviour of 'sparse-checkout set'.\n\nDoes that seem like something that would make sense?\n\n> [...]\n>> +static int path_matches_sparse_checkout_patterns(struct strbuf *full_path, struct pattern_list *pl, int dtype)\n>> +{\n>> + enum pattern_match_result match = recursively_match_path_with_sparse_patterns(full_path->buf, the_repository->index, dtype, pl);\n>> + return match > 0;\n> \n> So your new caller doesn't care about the pattern_match_result, it\n> just wants to know if it got MATCHED or MATCHED_RECURSIVELY...\n> \n> [...]\n>> diff --git a/dir.c b/dir.c\n>> index 4e99f0c868f..122ebced08e 100644\n>> --- a/dir.c\n>> +++ b/dir.c\n>> @@ -1457,45 +1457,50 @@ int init_sparse_checkout_patterns(struct index_state *istate)\n>> return 0;\n>> }\n>>\n>> -static int path_in_sparse_checkout_1(const char *path,\n>> - struct index_state *istate,\n>> - int require_cone_mode)\n>> +int recursively_match_path_with_sparse_patterns(const char *path,\n> \n> You claim it returns an int here, but previously you presumed an enum\n> pattern_match_result from the new caller.\n> \n>> + struct index_state *istate,\n>> + int dtype,\n>> + struct pattern_list *pl)\n>> {\n>> - int dtype = DT_REG;\n>> enum pattern_match_result match = UNDECIDED;\n>> const char *end, *slash;\n>> -\n>> - /*\n>> - * We default to accepting a path if the path is empty, there are no\n>> - * patterns, or the patterns are of the wrong type.\n>> - */\n>> - if (!*path ||\n>> - init_sparse_checkout_patterns(istate) ||\n>> - (require_cone_mode &&\n>> - !istate->sparse_checkout_patterns->use_cone_patterns))\n>> - return 1;\n>> -\n>> /*\n>> * If UNDECIDED, use the match from the parent dir (recursively), or\n>> * fall back to NOT_MATCHED at the topmost level. Note that cone mode\n>> * never returns UNDECIDED, so we will execute only one iteration in\n>> * this case.\n>> */\n>> - for (end = path + strlen(path);\n>> - end > path && match == UNDECIDED;\n>> + for (end = path + strlen(path); end > path && match == UNDECIDED;\n>> end = slash) {\n>> -\n>> for (slash = end - 1; slash > path && *slash != '/'; slash--)\n>> ; /* do nothing */\n>>\n>> match = path_matches_pattern_list(path, end - path,\n>> slash > path ? slash + 1 : path, &dtype,\n>> - istate->sparse_checkout_patterns, istate);\n>> + pl, istate);\n>>\n>> /* We are going to match the parent dir now */\n>> dtype = DT_DIR;\n>> }\n>> - return match > 0;\n>> +\n>> + return match;\n> \n> Um, this last line seems like a potentially scary change in behavior.\n> Why should UNDECIDED return a non-zero value? Previously, we returned\n> a 0 value for both UNDECIDED and NOT_MATCHED, but you've changed that\n> here. If the change in this last line is actually correct, it should\n> be split out into its own commit and explained in detail in the commit\n> message.\n> \n>> +}\n>> +\n>> +static int path_in_sparse_checkout_1(const char *path,\n>> + struct index_state *istate,\n>> + int require_cone_mode)\n>> +{\n>> + /*\n>> + * We default to accepting a path if the path is empty, there are no\n>> + * patterns, or the patterns are of the wrong type.\n>> + */\n>> + if (!*path ||\n>> + init_sparse_checkout_patterns(istate) ||\n>> + (require_cone_mode &&\n>> + !istate->sparse_checkout_patterns->use_cone_patterns))\n>> + return 1;\n>> +\n>> + return recursively_match_path_with_sparse_patterns(path, istate, DT_REG, istate->sparse_checkout_patterns) > 0;\n> \n> Oh, you compare to > 0 here...and digging around your only other\n> caller just immediately compares to > 0 as well.\n> \n> Why not just have recursively_match_path_with_sparse_patterns() do the\n>> 0 check? If it does, returning int is fine. If it doesn't, it\n> should be declared to return enum pattern_match_result.\n\nI think my thinking was that it made sense in general for \n'recursively_match_path_with_sparse_patterns()' to expose the match \nresult, but then failed to declare it that way. Thanks for catching it.\n"},{"id":"471031","messageId":"42e14dda-cd2b-09df-dea8-246b3fcfac42@github.com","threadId":"59071","inReplyTo":"569043fb-9766-037e-c587-1545c2978e7d@unity3d.com","subject":"Re: [PATCH v2] ls-tree: add --sparse-filter-oid argument","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2023-01-25T18:32:37Z","receivedAt":"2023-01-25T18:32:44Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"William Sprent wrote:\n> On 24/01/2023 21.11, Victoria Dye wrote>> I haven't looked at your implementation in detail yet, but I did want to\n>> offer a recommendation in case you hadn't considered it: if you want to\n>> allow the use of patterns from a user-specified specific file, it would be\n>> nice to do it in a way that fully replaces the \"default\" sparse-checkout\n>> settings at the lowest level (i.e., override the values of\n>> 'core_apply_sparse_checkout', 'core_sparse_checkout_cone', and\n>> 'get_sparse_checkout_filename()'). Doing it that way would both make it\n>> easier for other commands to add a '--sparse-patterns' option, and avoid the\n>> separate code path ('path_in_sparse_checkout_1()' vs.\n>> 'recursively_match_path_with_sparse_patterns()', for example) when dealing\n>> with '.git/info/sparse-checkout' patterns vs. manually-specified patterns.\n>>\n> \n> Thanks for the pointers. I'll see what I can do. Do you mean something\n> along the line of the following?\n> \n>    set_sparse_checkout_file(filename);\n>    init_sparse_checkout_patterns(istate);\n>    _ = path_in_sparse_checkout_1(some_path, istate, ...);\n> \n\nSort of. I mentioned separating the options into \"specify the sparse pattern\nfile\" and \"restrict the displayed files to the active pattern set, if there\nis one\". For the former, you might add an option like:\n\n\tOPT_FILENAME(0, \"sparse-patterns\", &sparse_pattern_file, \n\t\t N_(\"override sparse-checkout behavior using patterns from \"))\n\nThen do something like what you have, right after option parsing:\n\n\tif (sparse_pattern_file) {\n\t\tcore_apply_sparse_checkout = 1;\n\t\tcore_sparse_checkout_cone = ;\n\t\tset_sparse_checkout_file(filename);\n\t}\n\nIf this option is specified, but the repo already has sparse\npatterns/settings of its own, you'll need to (carefully) override the repo's\nexisting configuration:\n\n* 'core_apply_sparse_checkout' & 'core_sparse_checkout_cone' are set based\n on the repo config. You'll need to make sure those values are overridden\n before loading the sparse-checkout patterns, and also that they're set\n *after* loading the config.\n* Speaking of 'core_sparse_checkout_cone', there are a bunch of ways you\n might configure \"cone-\" vs. \"non-cone\" for your patterns (user-specified\n with yet another option, always assume one or the other, try deriving from\n the patterns). My preference would be to always assume \"cone mode\" - it's\n the direction Git has been moving with sparse-checkout over the past year,\n and still automatically \"falls back\" on non-cone patterns if the patterns\n can't be used in cone mode (with a warning from\n 'add_pattern_to_hashsets()': \"disabling cone pattern matching\").\n* If the repo is using a sparse index, the existing sparse directories may\n not be compatible with the custom patterns. Probably easiest to force use\n of a full index, e.g. with 'command_requires_full_index = 1'.\n\nFair warning: this probably isn't an exhaustive list of things that would\nneed updating, and it'll need thorough testing to make sure there are no\nregressions. But, extending the underlying sparse-checkout infrastructure\nwill (ideally) avoid duplicating code and make this behavior reusable across\nother commands.\n\nFor the other desired behavior (\"limit the files to the active\nsparse-checkout patterns\"), you could add an option:\n\n\tOPT_CALLBACK_F(0, \"scope\", &sparse_scope, \"(sparse|all)\",\n\t\t N_(\"specify the scope of results with respect to the sparse-checkout\"),\n\t\t PARSE_OPT_NONEG, option_parse_scope),\n\n...whose callback parses the string arg into a 'restrict_scope'\nboolean/enum/something. Then, wherever in 'ls-files' a tree or the index are\niterated over, you can gate the per-file operation on:\n\n\tif (!restrict_scope || path_in_sparse_checkout(, istate)) {\n\t\t/* do something with */\n\t}\n\nNote that you should use 'path_in_sparse_checkout()', *not* the\ninternal/private function 'path_in_sparse_checkout_1()'; you also don't need\nto explicitly 'init_sparse_checkout_patterns()'. Regardless of whether you\nspecified custom patterns or are using the ones in\n'.git/info/sparse-checkout', 'path_in_sparse_checkout()' will initialize &\nuse the appropriate patterns.\n\n"},{"id":"471050","messageId":"CABPp-BFtLdRV2zWXn0On0b6mOJgMAatwvUumUxfXfNXo9gc=HA@mail.gmail.com","threadId":"59071","inReplyTo":"18c94f70-4adf-1b4a-8777-206804c419e6@unity3d.com","subject":"Re: [PATCH v2] ls-tree: add --sparse-filter-oid argument","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-01-26T03:25:00Z","receivedAt":"2023-01-26T03:25:56Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Jan 25, 2023 at 8:16 AM William Sprent wrote:\n>\n> On 25/01/2023 06.11, Elijah Newren wrote:\n> > It looks like Ævar and Victoria have both given really good reviews\n> > already, but I think I spotted some additional things to comment on.\n> >\n> > On Mon, Jan 23, 2023 at 3:46 AM William Sprent via GitGitGadget\n> > wrote:\n> >>\n> >> From: William Sprent \n> >>\n> >> There is currently no way to ask git the question \"which files would be\n> >> part of a sparse checkout of commit X with sparse checkout patterns Y\".\n> >> One use-case would be that tooling may want know whether sparse checkouts\n> >> of two commits contain the same content even if the full trees differ.\n> >\n> > Could you say more about this usecase? Why does tooling need or want\n> > to know this; won't a checkout of the new commit end up being quick\n> > and simple? (I'm not saying your usecase is bad, just curious that it\n> > had never occurred to me, and I'm afraid I'm still not sure what your\n> > purpose might be.)\n> >\n>\n> I'm thinking mainly about a monorepo context where there are a number of\n> distinct 'units' that can be described with sparse checkout patterns.\n> And perhaps there's some tooling that only wants to perform an action if\n> the content of a 'unit' changes.\n\nSo, you're basically wanting to do something like\n git ls-tree --paths-matching-sparsity-file= $COMMIT1\n>sparse-files\n git ls-tree --paths-matching-sparsity-file= $COMMIT2\n>>sparse-files\n sort sparse-files | uniq >relevant-files\n git diff --name-only $COMMIT1 $COMMIT2 >changed-files\nand then checking whether relevant-files and changed-files have a\nnon-empty intersection?\n\nWould that potentially be better handled by\n git diff --name-only $COMMIT1 $COMMIT2 | git check-ignore\n--ignore-file= --stdin\nand seeing whether the output is non-empty? We'd have to add an\n\"--ignore-file\" option to check-ignore to override reading of\n.gitignore files and such, and it'd be slightly confusing because the\ndocumentation talks about \"ignored\" files rather than \"selected\"\nfiles, but that's a confusion point that has been with us ever since\nthe gitignore mechanism was repurposed for sparse checkouts. Or maybe\nwe could just add a check-sparsity helper, and then allow it to take\ndirectories in-lieu of patterns.\n\nThis seems nicer than opening a can of worms about letting every git\ncommand specify a different set of sparsity rules.\n\n> Depending on the repo, it won't necessarily be quick to check out the\n> commit with the given patterns. However, it is more about it being\n> inconvenient to have to have a working directory, especially so if you\n> want use the tooling in some kind of service or query rapidly about\n> different revisions/patterns.\n>\n> >> Another interesting use-case would be for tooling to use in conjunction\n> >> with 'git update-index --index-info'.\n> >\n> > Sorry, I'm not following. Could you expound on this a bit?\n> >\n>\n> I was imagining something along the lines of being able to generate new\n> tree objects based on what matches the given sparse checkout patterns.\n> Not that I have a specific use case for it right now.\n>\n> I think what I'm trying to evoke with that paragraph is that this\n> enables integrations with git that seem interesting and weren't possible\n> before.\n\nI'm not sure if it's interesting, frightening, or something else.\nHard to say without better descriptions of usecases, which we can't\nhave if we don't even have a usecase. I think I'd just strike this\nparagraph.\n\n[...]\n> >> + (*d)->pl.use_cone_patterns = core_sparse_checkout_cone;\n> >\n> > Hmm, so the behavior still depends upon the current sparse-checkout\n> > (or lack thereof), despite the documentation and rationale of your\n> > feature as being there to check how a different sparse checkout would\n> > behave?\n> >\n> > I would hate to unconditionally turn cone_patterns off, since that\n> > would come with a huge performance penalty for the biggest repos. But\n> > turning it unconditionally on wouldn't be good for the non-cone users.\n> > This probably suggests we need something like another flag, or perhaps\n> > separate flags for each mode. Separate flags might provide the\n> > benefit of allowing cone mode users to specify directories rather than\n> > patterns, which would make it much easier for them to use.\n> >\n> I used 'core_sparse_checkout_cone' because I wanted to allow for the\n> cone mode optimisations, but I also figured that I should respect the\n> configuration. It doesn't change how the patterns are parsed in this case.\n>\n> I agree that it is a bit awkward to have to \"translate\" the directories\n> into patterns when wanting to use cone mode. I can try adding\n> '--[no]-cone' flags and see how that feels. Together with Victoria's\n> suggestions that would result in having the following flags:\n>\n> * --scope=(sparse|all)\n> * --sparse-patterns-file=\n> * --[no]-cone: used together with --sparse-patterns-file to tell git\n> whether to interpret the patterns given as directories (cone) or\n> patterns (no-cone).\n>\n> Which seems like a lot at first glance. But it allows for passing\n> directories instead of patterns for cone mode, and is similar to the\n> behaviour of 'sparse-checkout set'.\n>\n> Does that seem like something that would make sense?\n\n--sparse-patterns-file still implies patterns; I think that would need\nsome rewording.\n\nMore importantly, though, based on your usecase description, I wonder\nif you might be better served by either extending the check-ignore\nsubcommand or adding a similar helper (\"check-sparsity\"?), rather than\ntweaking ls-tree.\n"},{"id":"471068","messageId":"d96e6ab4-2302-5326-3a0e-bffecc24a295@unity3d.com","threadId":"59071","inReplyTo":"42e14dda-cd2b-09df-dea8-246b3fcfac42@github.com","subject":"Re: [PATCH v2] ls-tree: add --sparse-filter-oid argument","fromName":"William Sprent","fromEmail":"williams@unity3d.com","sentAt":"2023-01-26T14:55:40Z","receivedAt":"2023-01-26T14:55:47Z","isPatch":true,"sender":{"key":"williams@unity3d.com","avatar":"https://avatars.githubusercontent.com/u/77490534?v=4"},"body":"On 25/01/2023 19.32, Victoria Dye wrote:\n> William Sprent wrote:\n>> On 24/01/2023 21.11, Victoria Dye wrote>> I haven't looked at your implementation in detail yet, but I did want to\n>>> offer a recommendation in case you hadn't considered it: if you want to\n>>> allow the use of patterns from a user-specified specific file, it would be\n>>> nice to do it in a way that fully replaces the \"default\" sparse-checkout\n>>> settings at the lowest level (i.e., override the values of\n>>> 'core_apply_sparse_checkout', 'core_sparse_checkout_cone', and\n>>> 'get_sparse_checkout_filename()'). Doing it that way would both make it\n>>> easier for other commands to add a '--sparse-patterns' option, and avoid the\n>>> separate code path ('path_in_sparse_checkout_1()' vs.\n>>> 'recursively_match_path_with_sparse_patterns()', for example) when dealing\n>>> with '.git/info/sparse-checkout' patterns vs. manually-specified patterns.\n>>>\n>>\n>> Thanks for the pointers. I'll see what I can do. Do you mean something\n>> along the line of the following?\n>>\n>>    set_sparse_checkout_file(filename);\n>>    init_sparse_checkout_patterns(istate);\n>>    _ = path_in_sparse_checkout_1(some_path, istate, ...);\n>>\n> \n> Sort of. I mentioned separating the options into \"specify the sparse pattern\n> file\" and \"restrict the displayed files to the active pattern set, if there\n> is one\". For the former, you might add an option like:\n> \n> \tOPT_FILENAME(0, \"sparse-patterns\", &sparse_pattern_file,\n> \t\t N_(\"override sparse-checkout behavior using patterns from \"))\n> \n> Then do something like what you have, right after option parsing:\n> \n> \tif (sparse_pattern_file) {\n> \t\tcore_apply_sparse_checkout = 1;\n> \t\tcore_sparse_checkout_cone = ;\n> \t\tset_sparse_checkout_file(filename);\n> \t}\n> \n> If this option is specified, but the repo already has sparse\n> patterns/settings of its own, you'll need to (carefully) override the repo's\n> existing configuration:\n> \n> * 'core_apply_sparse_checkout' & 'core_sparse_checkout_cone' are set based\n> on the repo config. You'll need to make sure those values are overridden\n> before loading the sparse-checkout patterns, and also that they're set\n> *after* loading the config.\n\nThis sounds a bit easy to get wrong to me, but I assume I can trust that the\nconfig has been loaded by the time 'cmd_ls_tree()' is invoked.\n\n> * Speaking of 'core_sparse_checkout_cone', there are a bunch of ways you\n> might configure \"cone-\" vs. \"non-cone\" for your patterns (user-specified\n> with yet another option, always assume one or the other, try deriving from\n> the patterns). My preference would be to always assume \"cone mode\" - it's\n> the direction Git has been moving with sparse-checkout over the past year,\n> and still automatically \"falls back\" on non-cone patterns if the patterns\n> can't be used in cone mode (with a warning from\n> 'add_pattern_to_hashsets()': \"disabling cone pattern matching\").\n\nAlright. I've had similar thoughts. But I ended up deciding to respect the\nconfig value since there wouldn't be any way for the user to silence the warning\nwhen passing non-cone mode patterns to the command. I don't feel too strongly about\nit, though.\n\n> * If the repo is using a sparse index, the existing sparse directories may\n> not be compatible with the custom patterns. Probably easiest to force use\n> of a full index, e.g. with 'command_requires_full_index = 1'.\n> \n\nOK.\n\n> Fair warning: this probably isn't an exhaustive list of things that would\n> need updating, and it'll need thorough testing to make sure there are no\n> regressions. But, extending the underlying sparse-checkout infrastructure\n> will (ideally) avoid duplicating code and make this behavior reusable across\n> other commands.\n> \n\nAlright. I'll give it a shot.\n\n> For the other desired behavior (\"limit the files to the active\n> sparse-checkout patterns\"), you could add an option:\n> \n> \tOPT_CALLBACK_F(0, \"scope\", &sparse_scope, \"(sparse|all)\",\n> \t\t N_(\"specify the scope of results with respect to the sparse-checkout\"),\n> \t\t PARSE_OPT_NONEG, option_parse_scope),\n> \n> ...whose callback parses the string arg into a 'restrict_scope'\n> boolean/enum/something. Then, wherever in 'ls-files' a tree or the index are\n> iterated over, you can gate the per-file operation on:\n> \n> \tif (!restrict_scope || path_in_sparse_checkout(, istate)) {\n> \t\t/* do something with */\n> \t}\n> \n> Note that you should use 'path_in_sparse_checkout()', *not* the\n> internal/private function 'path_in_sparse_checkout_1()'; you also don't need\n> to explicitly 'init_sparse_checkout_patterns()'. Regardless of whether you\n> specified custom patterns or are using the ones in\n> '.git/info/sparse-checkout', 'path_in_sparse_checkout()' will initialize &\n> use the appropriate patterns.\n> \n\nYeah. And I wouldn't need the refactor of 'path_in_sparse_checkout_1()'.\n\nThanks a lot (again) for the pointers.\n"},{"id":"471086","messageId":"9dda8cde-7c96-a5f1-f271-951f8b348b80@unity3d.com","threadId":"59071","inReplyTo":"CABPp-BFtLdRV2zWXn0On0b6mOJgMAatwvUumUxfXfNXo9gc=HA@mail.gmail.com","subject":"Re: [PATCH v2] ls-tree: add --sparse-filter-oid argument","fromName":"William Sprent","fromEmail":"williams@unity3d.com","sentAt":"2023-01-27T11:58:57Z","receivedAt":"2023-01-27T12:11:24Z","isPatch":true,"sender":{"key":"williams@unity3d.com","avatar":"https://avatars.githubusercontent.com/u/77490534?v=4"},"body":"On 26/01/2023 04.25, Elijah Newren wrote:\n> On Wed, Jan 25, 2023 at 8:16 AM William Sprent wrote:\n>>\n>> On 25/01/2023 06.11, Elijah Newren wrote:\n>>> It looks like Ævar and Victoria have both given really good reviews\n>>> already, but I think I spotted some additional things to comment on.\n>>>\n>>> On Mon, Jan 23, 2023 at 3:46 AM William Sprent via GitGitGadget\n>>> wrote:\n>>>>\n>>>> From: William Sprent \n>>>>\n>>>> There is currently no way to ask git the question \"which files would be\n>>>> part of a sparse checkout of commit X with sparse checkout patterns Y\".\n>>>> One use-case would be that tooling may want know whether sparse checkouts\n>>>> of two commits contain the same content even if the full trees differ.\n>>>\n>>> Could you say more about this usecase? Why does tooling need or want\n>>> to know this; won't a checkout of the new commit end up being quick\n>>> and simple? (I'm not saying your usecase is bad, just curious that it\n>>> had never occurred to me, and I'm afraid I'm still not sure what your\n>>> purpose might be.)\n>>>\n>>\n>> I'm thinking mainly about a monorepo context where there are a number of\n>> distinct 'units' that can be described with sparse checkout patterns.\n>> And perhaps there's some tooling that only wants to perform an action if\n>> the content of a 'unit' changes.\n> \n> So, you're basically wanting to do something like\n> git ls-tree --paths-matching-sparsity-file= $COMMIT1\n>> sparse-files\n> git ls-tree --paths-matching-sparsity-file= $COMMIT2\n>>> sparse-files\n> sort sparse-files | uniq >relevant-files\n> git diff --name-only $COMMIT1 $COMMIT2 >changed-files\n> and then checking whether relevant-files and changed-files have a\n> non-empty intersection?\n\nWell, the concrete use-case I'm exploring is something along the lines\nof using the content hashes of sparse checkouts as cache keys for resource\nheavy jobs (builds/tests/etc).\n\nSo, that would be something along the lines of,\n\n git ls-tree -r --paths-matching-sparsity-file= \\\n | sha1sum > cache-key\n\nand then performing a lookup before performing an action (which would\nthen only be done in the context of the sparse checkout). My thinking\nis that this only would require git and no additional tooling, which in\nturn makes it very easy to reproduce the state where the job took place.\n\n\n> Would that potentially be better handled by\n> git diff --name-only $COMMIT1 $COMMIT2 | git check-ignore\n> --ignore-file= --stdin\n> and seeing whether the output is non-empty? We'd have to add an\n> \"--ignore-file\" option to check-ignore to override reading of\n> .gitignore files and such, and it'd be slightly confusing because the\n> documentation talks about \"ignored\" files rather than \"selected\"\n> files, but that's a confusion point that has been with us ever since\n> the gitignore mechanism was repurposed for sparse checkouts. Or maybe\n> we could just add a check-sparsity helper, and then allow it to take\n> directories in-lieu of patterns. \n\nI don't think it necessarily would be better handled by that. But it would\nbe workable. It would be a matter of collating the output of\n\n git ls-tree -r \n\nwith\n\n git ls-tree --name-only -r | git check-ignore ...\n\nWhich is less ergonomic. But it is also a less intrusive change.\n\nReally, the main thing is to expose the sparse filtering logic somehow, and\nallow for building tooling on top of it.\n\n> This seems nicer than opening a can of worms about letting every git\n> command specify a different set of sparsity rules.\n\nI think you are the better judge of how much of a can of worms that would\nbe. I don't think it would be too out of line with how git acts in general\nthough, as we have things like the the 'GIT_INDEX_FILE' env-var.\n\n>> Depending on the repo, it won't necessarily be quick to check out the\n>> commit with the given patterns. However, it is more about it being\n>> inconvenient to have to have a working directory, especially so if you\n>> want use the tooling in some kind of service or query rapidly about\n>> different revisions/patterns.\n>>\n>>>> Another interesting use-case would be for tooling to use in conjunction\n>>>> with 'git update-index --index-info'.\n>>>\n>>> Sorry, I'm not following. Could you expound on this a bit?\n>>>\n>>\n>> I was imagining something along the lines of being able to generate new\n>> tree objects based on what matches the given sparse checkout patterns.\n>> Not that I have a specific use case for it right now.\n>>\n>> I think what I'm trying to evoke with that paragraph is that this\n>> enables integrations with git that seem interesting and weren't possible\n>> before.\n> \n> I'm not sure if it's interesting, frightening, or something else.\n> Hard to say without better descriptions of usecases, which we can't\n> have if we don't even have a usecase. I think I'd just strike this\n> paragraph.\n> \n> [...]\n\nFair. Will do.\n\n>>>> + (*d)->pl.use_cone_patterns = core_sparse_checkout_cone;\n>>>\n>>> Hmm, so the behavior still depends upon the current sparse-checkout\n>>> (or lack thereof), despite the documentation and rationale of your\n>>> feature as being there to check how a different sparse checkout would\n>>> behave?\n>>>\n>>> I would hate to unconditionally turn cone_patterns off, since that\n>>> would come with a huge performance penalty for the biggest repos. But\n>>> turning it unconditionally on wouldn't be good for the non-cone users.\n>>> This probably suggests we need something like another flag, or perhaps\n>>> separate flags for each mode. Separate flags might provide the\n>>> benefit of allowing cone mode users to specify directories rather than\n>>> patterns, which would make it much easier for them to use.\n>>>\n>> I used 'core_sparse_checkout_cone' because I wanted to allow for the\n>> cone mode optimisations, but I also figured that I should respect the\n>> configuration. It doesn't change how the patterns are parsed in this case.\n>>\n>> I agree that it is a bit awkward to have to \"translate\" the directories\n>> into patterns when wanting to use cone mode. I can try adding\n>> '--[no]-cone' flags and see how that feels. Together with Victoria's\n>> suggestions that would result in having the following flags:\n>>\n>> * --scope=(sparse|all)\n>> * --sparse-patterns-file=\n>> * --[no]-cone: used together with --sparse-patterns-file to tell git\n>> whether to interpret the patterns given as directories (cone) or\n>> patterns (no-cone).\n>>\n>> Which seems like a lot at first glance. But it allows for passing\n>> directories instead of patterns for cone mode, and is similar to the\n>> behaviour of 'sparse-checkout set'.\n>>\n>> Does that seem like something that would make sense?\n> \n> --sparse-patterns-file still implies patterns; I think that would need\n> some rewording.\n\nYeah. After sleeping on it, I also think that it becomes a difficult\ninterface to work with, and you'll get different results with the same\npatterns whether you pass --cone or --no-cone, which seems error prone\nto me.\n\nFor better or for worse, both cone and non-cone uses of sparse-checkouts\nend up producing pattern files. And those pattern files do unambiguously\ndescribe a filtering of the worktree whether it is in cone-mode or not.\n\nGiven that 'ls-tree' is more of a plumbing command, I think it might still\nmake sense to use the patterns. That would also make the interaction\na bit more logical to me -- e.g. if you want to override the patterns\nyou have to pass them in the same format as the ones that would be read\nby default.\n\nThen maybe it could eventually make sense to expose the translation of\ncone-mode patterns as well, e.g.\n\n git sparse-checkout set --cone --std-out dir1 dir2 dir3\n\nor similar.\n\n> More importantly, though, based on your usecase description, I wonder\n> if you might be better served by either extending the check-ignore\n> subcommand or adding a similar helper (\"check-sparsity\"?), rather than\n> tweaking ls-tree.\n"},{"id":"471117","messageId":"CABPp-BGs2wG6a3oR8dmT9dkeakoiZ+w-Tf=4A-GXeDVkJ9QNMA@mail.gmail.com","threadId":"59071","inReplyTo":"9dda8cde-7c96-a5f1-f271-951f8b348b80@unity3d.com","subject":"Re: [PATCH v2] ls-tree: add --sparse-filter-oid argument","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-01-28T16:45:31Z","receivedAt":"2023-01-28T16:45:51Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Jan 27, 2023 at 3:59 AM William Sprent wrote:\n>\n> On 26/01/2023 04.25, Elijah Newren wrote:\n> > On Wed, Jan 25, 2023 at 8:16 AM William Sprent wrote:\n> >>\n> >> On 25/01/2023 06.11, Elijah Newren wrote:\n> >>> It looks like Ævar and Victoria have both given really good reviews\n> >>> already, but I think I spotted some additional things to comment on.\n> >>>\n> >>> On Mon, Jan 23, 2023 at 3:46 AM William Sprent via GitGitGadget\n> >>> wrote:\n> >>>>\n> >>>> From: William Sprent \n> >>>>\n> >>>> There is currently no way to ask git the question \"which files would be\n> >>>> part of a sparse checkout of commit X with sparse checkout patterns Y\".\n> >>>> One use-case would be that tooling may want know whether sparse checkouts\n> >>>> of two commits contain the same content even if the full trees differ.\n> >>>\n> >>> Could you say more about this usecase? Why does tooling need or want\n> >>> to know this; won't a checkout of the new commit end up being quick\n> >>> and simple? (I'm not saying your usecase is bad, just curious that it\n> >>> had never occurred to me, and I'm afraid I'm still not sure what your\n> >>> purpose might be.)\n> >>>\n> >>\n> >> I'm thinking mainly about a monorepo context where there are a number of\n> >> distinct 'units' that can be described with sparse checkout patterns.\n> >> And perhaps there's some tooling that only wants to perform an action if\n> >> the content of a 'unit' changes.\n> >\n> > So, you're basically wanting to do something like\n> > git ls-tree --paths-matching-sparsity-file= $COMMIT1\n> >> sparse-files\n> > git ls-tree --paths-matching-sparsity-file= $COMMIT2\n> >>> sparse-files\n> > sort sparse-files | uniq >relevant-files\n> > git diff --name-only $COMMIT1 $COMMIT2 >changed-files\n> > and then checking whether relevant-files and changed-files have a\n> > non-empty intersection?\n>\n> Well, the concrete use-case I'm exploring is something along the lines\n> of using the content hashes of sparse checkouts as cache keys for resource\n> heavy jobs (builds/tests/etc).\n>\n> So, that would be something along the lines of,\n>\n> git ls-tree -r --paths-matching-sparsity-file= \\\n> | sha1sum > cache-key\n>\n> and then performing a lookup before performing an action (which would\n> then only be done in the context of the sparse checkout). My thinking\n> is that this only would require git and no additional tooling, which in\n> turn makes it very easy to reproduce the state where the job took place.\n>\n>\n> > Would that potentially be better handled by\n> > git diff --name-only $COMMIT1 $COMMIT2 | git check-ignore\n> > --ignore-file= --stdin\n> > and seeing whether the output is non-empty? We'd have to add an\n> > \"--ignore-file\" option to check-ignore to override reading of\n> > .gitignore files and such, and it'd be slightly confusing because the\n> > documentation talks about \"ignored\" files rather than \"selected\"\n> > files, but that's a confusion point that has been with us ever since\n> > the gitignore mechanism was repurposed for sparse checkouts. Or maybe\n> > we could just add a check-sparsity helper, and then allow it to take\n> > directories in-lieu of patterns.\n>\n> I don't think it necessarily would be better handled by that. But it would\n> be workable. It would be a matter of collating the output of\n>\n> git ls-tree -r \n>\n> with\n>\n> git ls-tree --name-only -r | git check-ignore ...\n>\n> Which is less ergonomic. But it is also a less intrusive change.\n>\n> Really, the main thing is to expose the sparse filtering logic somehow, and\n> allow for building tooling on top of it.\n> > This seems nicer than opening a can of worms about letting every git\n> > command specify a different set of sparsity rules.\n>\n> I think you are the better judge of how much of a can of worms that would\n> be. I don't think it would be too out of line with how git acts in general\n> though, as we have things like the the 'GIT_INDEX_FILE' env-var.\n\nI agree you've got a reasonable usecase here. The integration you've\ndescribed sounds like something that could be independently composable\nwith several other commands, and you alluded to that in your commit\nmessage. But is adding it to ls-tree the best spot? If we add it\nthere, then folks who want to similarly integrate such capabilities\nwith other commands (archive, diff, grep, rev-list --objects, etc.),\nseem more likely to do so via direct integration. We already have a\nvery large can of worms to work on to make commands behave in ways\nthat are limited to sparse paths (see\nDocumentation/techncial/sparse-checkout.txt, namely the \"behavior A\"\nstuff). As can be seen in that document, what to do for limiting\ncommands to sparse paths is obvious with some commands but has lots of\ninteresting edge cases for others (even with years of experience with\nsparse checkouts, we had 3- and 4- way differing opinions on the\nintended behavior for some commands when we started composing that\ndocument a few months ago). If we had jumped straight to\nimplementation for some commands, we would have likely painted\nourselves into a corner for other commands. Adding another layer of\nspecifying an alternate set of sparsity rules will likely have\ninteresting knock-on effects that we should think through for all the\ncommands to ensure we aren't painting ourselves into a similar corner,\nif we go down this route.\n\nHowever, in the cases that sparse-checkout.txt document was\naddressing, the behavior fundamentally needs to be integrated into all\nthe relevant commands to get user experience right. In your case, you\nmerely need a separate tool to be able to compose the output of\ndifferent commands together. So, exposing whether sparsity rules\nwould select various paths in a single separate command (maybe git\ncheck-ignore, or a new sparse-checkout subcommand, or maybe just a new\nsubcommand similar to check-ignore) would avoid a lot of these issues,\nand give us a single place to extend/improve as we learn about further\nusecases.\n\n[...]\n> >> I agree that it is a bit awkward to have to \"translate\" the directories\n> >> into patterns when wanting to use cone mode. I can try adding\n> >> '--[no]-cone' flags and see how that feels. Together with Victoria's\n> >> suggestions that would result in having the following flags:\n> >>\n> >> * --scope=(sparse|all)\n> >> * --sparse-patterns-file=\n> >> * --[no]-cone: used together with --sparse-patterns-file to tell git\n> >> whether to interpret the patterns given as directories (cone) or\n> >> patterns (no-cone).\n> >>\n> >> Which seems like a lot at first glance. But it allows for passing\n> >> directories instead of patterns for cone mode, and is similar to the\n> >> behaviour of 'sparse-checkout set'.\n> >>\n> >> Does that seem like something that would make sense?\n> >\n> > --sparse-patterns-file still implies patterns; I think that would need\n> > some rewording.\n>\n> Yeah. After sleeping on it, I also think that it becomes a difficult\n> interface to work with, and you'll get different results with the same\n> patterns whether you pass --cone or --no-cone, which seems error prone\n> to me.\n>\n> For better or for worse, both cone and non-cone uses of sparse-checkouts\n> end up producing pattern files. And those pattern files do unambiguously\n> describe a filtering of the worktree whether it is in cone-mode or not.\n\nBack when cone mode was introduced, I objected to reusing the existing\npattern format and/or files...but was assuaged that it was just an\nimplementation detail that wouldn't be exposed to users (since people\nwould interact with the 'sparse-checkout' command instead of direct\nediting of $GIT_DIR/info/sparse-checkout). It's still a performance\npenalty, because we waste time both turning directory names into\npatterns when we write them out, and when we read them in by parsing\nthe patterns to see if they match cone mode rules and if so discard\nthe patterns and just build up our hashes. The patterns are nothing\nmore than an intermediary format we waste time translating to and\nfrom, though once upon a time they existed so that if someone suddenly\nstarted using an older (now ancient) version of git on their current\ncheckout then it could still hobble along with degraded performance\ninstead of providing an error. (We have introduced other changes to\nthe config and git index which would cause older git clients to just\nfail to parse and throw an error, and even have defined mechanisms for\nsuch erroring out. We could have taken advantage of that for this\nfeature too.)\n\nAnyway, long story short, if you're going to start exposing users to\nthis implementation detail that was meant to be hidden (and do it in a\nway that may be copied into several commands to boot), then I\ndefinitely object.\n\n> Given that 'ls-tree' is more of a plumbing command, I think it might still\n> make sense to use the patterns. That would also make the interaction\n> a bit more logical to me -- e.g. if you want to override the patterns\n> you have to pass them in the same format as the ones that would be read\n> by default.\n\nNo, sparsity specification should be provided by users the same way\nthey normally specify it (i.e. the way they pass it to\n`sparse-checkout {add,set}`), not the way it's stored via some hidden\nimplementation detail.\n\nI'd make an exception if you ended up using `git check-ignore` because\nthat command was specifically written about gitignore-style rules, and\ngit-ignore-style rules just happen to have been reused as-is for\nnon-cone-mode sparse-checkout rules. But if you go that route:\n (1) you have to frame any extensions to check-ignore as things that\nare useful for gitignore checking, and only incidentally useful to\nsparse-checkouts\n (2) you'd have to forgo any cone-mode optimizations\nGoing the check-ignore route seems like the easiest path to providing\nthe basic functionality you need right now. If your usecases remain\nniche, that might be good enough for all time too. If your usecases\nbecome popular, though, I expect someone would eventually tire of\nusing `check-ignore` and implement similar functionality along with\nsupporting cone-mode in a `git check-sparsity` or `git sparse-checkout\ndebug-rules` command or something like that.\n"},{"id":"471140","messageId":"2fd448af-cdd8-92ca-f714-472aac9c6db1@unity3d.com","threadId":"59071","inReplyTo":"CABPp-BGs2wG6a3oR8dmT9dkeakoiZ+w-Tf=4A-GXeDVkJ9QNMA@mail.gmail.com","subject":"Re: [PATCH v2] ls-tree: add --sparse-filter-oid argument","fromName":"William Sprent","fromEmail":"williams@unity3d.com","sentAt":"2023-01-30T15:28:09Z","receivedAt":"2023-01-30T15:28:17Z","isPatch":true,"sender":{"key":"williams@unity3d.com","avatar":"https://avatars.githubusercontent.com/u/77490534?v=4"},"body":"On 28/01/2023 17.45, Elijah Newren wrote:\n> On Fri, Jan 27, 2023 at 3:59 AM William Sprent wrote:\n>>\n>> On 26/01/2023 04.25, Elijah Newren wrote:\n>>> On Wed, Jan 25, 2023 at 8:16 AM William Sprent wrote:\n>>>>\n>>>> On 25/01/2023 06.11, Elijah Newren wrote:\n>>>>> It looks like Ævar and Victoria have both given really good reviews\n>>>>> already, but I think I spotted some additional things to comment on.\n>>>>>\n>>>>> On Mon, Jan 23, 2023 at 3:46 AM William Sprent via GitGitGadget\n>>>>> wrote:\n>>>>>>\n>>>>>> From: William Sprent \n>>>>>>\n>>>>>> There is currently no way to ask git the question \"which files would be\n>>>>>> part of a sparse checkout of commit X with sparse checkout patterns Y\".\n>>>>>> One use-case would be that tooling may want know whether sparse checkouts\n>>>>>> of two commits contain the same content even if the full trees differ.\n>>>>>\n>>>>> Could you say more about this usecase? Why does tooling need or want\n>>>>> to know this; won't a checkout of the new commit end up being quick\n>>>>> and simple? (I'm not saying your usecase is bad, just curious that it\n>>>>> had never occurred to me, and I'm afraid I'm still not sure what your\n>>>>> purpose might be.)\n>>>>>\n>>>>\n>>>> I'm thinking mainly about a monorepo context where there are a number of\n>>>> distinct 'units' that can be described with sparse checkout patterns.\n>>>> And perhaps there's some tooling that only wants to perform an action if\n>>>> the content of a 'unit' changes.\n>>>\n>>> So, you're basically wanting to do something like\n>>> git ls-tree --paths-matching-sparsity-file= $COMMIT1\n>>>> sparse-files\n>>> git ls-tree --paths-matching-sparsity-file= $COMMIT2\n>>>>> sparse-files\n>>> sort sparse-files | uniq >relevant-files\n>>> git diff --name-only $COMMIT1 $COMMIT2 >changed-files\n>>> and then checking whether relevant-files and changed-files have a\n>>> non-empty intersection?\n>>\n>> Well, the concrete use-case I'm exploring is something along the lines\n>> of using the content hashes of sparse checkouts as cache keys for resource\n>> heavy jobs (builds/tests/etc).\n>>\n>> So, that would be something along the lines of,\n>>\n>> git ls-tree -r --paths-matching-sparsity-file= \\\n>> | sha1sum > cache-key\n>>\n>> and then performing a lookup before performing an action (which would\n>> then only be done in the context of the sparse checkout). My thinking\n>> is that this only would require git and no additional tooling, which in\n>> turn makes it very easy to reproduce the state where the job took place.\n>>\n>>\n>>> Would that potentially be better handled by\n>>> git diff --name-only $COMMIT1 $COMMIT2 | git check-ignore\n>>> --ignore-file= --stdin\n>>> and seeing whether the output is non-empty? We'd have to add an\n>>> \"--ignore-file\" option to check-ignore to override reading of\n>>> .gitignore files and such, and it'd be slightly confusing because the\n>>> documentation talks about \"ignored\" files rather than \"selected\"\n>>> files, but that's a confusion point that has been with us ever since\n>>> the gitignore mechanism was repurposed for sparse checkouts. Or maybe\n>>> we could just add a check-sparsity helper, and then allow it to take\n>>> directories in-lieu of patterns.\n>>\n>> I don't think it necessarily would be better handled by that. But it would\n>> be workable. It would be a matter of collating the output of\n>>\n>> git ls-tree -r \n>>\n>> with\n>>\n>> git ls-tree --name-only -r | git check-ignore ...\n>>\n>> Which is less ergonomic. But it is also a less intrusive change.\n>>\n>> Really, the main thing is to expose the sparse filtering logic somehow, and\n>> allow for building tooling on top of it.\n>>> This seems nicer than opening a can of worms about letting every git\n>>> command specify a different set of sparsity rules.\n>>\n>> I think you are the better judge of how much of a can of worms that would\n>> be. I don't think it would be too out of line with how git acts in general\n>> though, as we have things like the the 'GIT_INDEX_FILE' env-var.\n> \n> I agree you've got a reasonable usecase here. The integration you've\n> described sounds like something that could be independently composable\n> with several other commands, and you alluded to that in your commit\n> message. But is adding it to ls-tree the best spot? If we add it\n> there, then folks who want to similarly integrate such capabilities\n> with other commands (archive, diff, grep, rev-list --objects, etc.),\n> seem more likely to do so via direct integration. We already have a\n> very large can of worms to work on to make commands behave in ways\n> that are limited to sparse paths (see\n> Documentation/techncial/sparse-checkout.txt, namely the \"behavior A\"\n> stuff). As can be seen in that document, what to do for limiting\n> commands to sparse paths is obvious with some commands but has lots of\n> interesting edge cases for others (even with years of experience with\n> sparse checkouts, we had 3- and 4- way differing opinions on the\n> intended behavior for some commands when we started composing that\n> document a few months ago). If we had jumped straight to\n> implementation for some commands, we would have likely painted\n> ourselves into a corner for other commands. Adding another layer of\n> specifying an alternate set of sparsity rules will likely have\n> interesting knock-on effects that we should think through for all the\n> commands to ensure we aren't painting ourselves into a similar corner,\n> if we go down this route.\n> \n> However, in the cases that sparse-checkout.txt document was\n> addressing, the behavior fundamentally needs to be integrated into all\n> the relevant commands to get user experience right. In your case, you\n> merely need a separate tool to be able to compose the output of\n> different commands together. So, exposing whether sparsity rules\n> would select various paths in a single separate command (maybe git\n> check-ignore, or a new sparse-checkout subcommand, or maybe just a new\n> subcommand similar to check-ignore) would avoid a lot of these issues,\n> and give us a single place to extend/improve as we learn about further\n> usecases.\n\nI do think that 'ls-tree' is ultimately (eventually?) the right place for\nsomething like this. But you do make some good points about it perhaps being a\nbit early.\n\n> \n> [...]\n>>>> I agree that it is a bit awkward to have to \"translate\" the directories\n>>>> into patterns when wanting to use cone mode. I can try adding\n>>>> '--[no]-cone' flags and see how that feels. Together with Victoria's\n>>>> suggestions that would result in having the following flags:\n>>>>\n>>>> * --scope=(sparse|all)\n>>>> * --sparse-patterns-file=\n>>>> * --[no]-cone: used together with --sparse-patterns-file to tell git\n>>>> whether to interpret the patterns given as directories (cone) or\n>>>> patterns (no-cone).\n>>>>\n>>>> Which seems like a lot at first glance. But it allows for passing\n>>>> directories instead of patterns for cone mode, and is similar to the\n>>>> behaviour of 'sparse-checkout set'.\n>>>>\n>>>> Does that seem like something that would make sense?\n>>>\n>>> --sparse-patterns-file still implies patterns; I think that would need\n>>> some rewording.\n>>\n>> Yeah. After sleeping on it, I also think that it becomes a difficult\n>> interface to work with, and you'll get different results with the same\n>> patterns whether you pass --cone or --no-cone, which seems error prone\n>> to me.\n>>\n>> For better or for worse, both cone and non-cone uses of sparse-checkouts\n>> end up producing pattern files. And those pattern files do unambiguously\n>> describe a filtering of the worktree whether it is in cone-mode or not.\n> \n> Back when cone mode was introduced, I objected to reusing the existing\n> pattern format and/or files...but was assuaged that it was just an\n> implementation detail that wouldn't be exposed to users (since people\n> would interact with the 'sparse-checkout' command instead of direct\n> editing of $GIT_DIR/info/sparse-checkout). It's still a performance\n> penalty, because we waste time both turning directory names into\n> patterns when we write them out, and when we read them in by parsing\n> the patterns to see if they match cone mode rules and if so discard\n> the patterns and just build up our hashes. The patterns are nothing\n> more than an intermediary format we waste time translating to and\n> from, though once upon a time they existed so that if someone suddenly\n> started using an older (now ancient) version of git on their current\n> checkout then it could still hobble along with degraded performance\n> instead of providing an error. (We have introduced other changes to\n> the config and git index which would cause older git clients to just\n> fail to parse and throw an error, and even have defined mechanisms for\n> such erroring out. We could have taken advantage of that for this\n> feature too.)\n> \n> Anyway, long story short, if you're going to start exposing users to\n> this implementation detail that was meant to be hidden (and do it in a\n> way that may be copied into several commands to boot), then I\n> definitely object.\n\nAlright. Fair. I think with that additional context, I agree. I was coming from\na \"this is a common format to both cone and non-cone modes\" not a \"this is a\nleftover implementation detail from now-deprecated use cases\" which is the vibe\nI'm getting here.\n\nI still think it would be unfortunate to have a single input parameter that\ntakes both kinds of specifications and interprets them either as cone or non-\ncone depending on config and/or a flag. With cone mode specifications like\n'a/b/c' being valid gitignore patterns there's no way of knowing whether the\nuser actually is supplying a pattern or accidentally supplied a directory in\nnon-cone mode.\n\nWith that in mind, I'd probably suggest something along the lines of having\n'ls-tree' only accept cone-mode specifications (e.g. --cone-rules-file or so) to\navoid having the non-cone/cone distinction spread outside the 'sparse-checkout'\ncommand. But that would be leaving non-cone users behind, and I guess we are not\nquite there yet.\n \n>> Given that 'ls-tree' is more of a plumbing command, I think it might still\n>> make sense to use the patterns. That would also make the interaction\n>> a bit more logical to me -- e.g. if you want to override the patterns\n>> you have to pass them in the same format as the ones that would be read\n>> by default.\n> \n> No, sparsity specification should be provided by users the same way\n> they normally specify it (i.e. the way they pass it to\n> `sparse-checkout {add,set}`), not the way it's stored via some hidden\n> implementation detail.\n> \n> I'd make an exception if you ended up using `git check-ignore` because\n> that command was specifically written about gitignore-style rules, and\n> git-ignore-style rules just happen to have been reused as-is for\n> non-cone-mode sparse-checkout rules. But if you go that route:\n> (1) you have to frame any extensions to check-ignore as things that\n> are useful for gitignore checking, and only incidentally useful to\n> sparse-checkouts\n> (2) you'd have to forgo any cone-mode optimizations\n> Going the check-ignore route seems like the easiest path to providing\n> the basic functionality you need right now. If your usecases remain\n> niche, that might be good enough for all time too. If your usecases\n> become popular, though, I expect someone would eventually tire of\n> using `check-ignore` and implement similar functionality along with\n> supporting cone-mode in a `git check-sparsity` or `git sparse-checkout\n> debug-rules` command or something like that.\n\nI guess that would be workable. But I would only want to use the command with\ncone-mode patterns, so losing the cone-mode optimisations would be suboptimal.\n\nIf the 'ls-tree' path isn't the best way forward right now, do you think it\nwould be viable to add a subcommand along the lines of 'debug-rules/check-rules'\nto sparse-checkout?\n\nThen\n\n git ls-tree -r HEAD | git sparse-checkout debug-rules\n\nwould output only the path that match the current patterns. And\n\n <...> | git sparse-checkout debug-rules --rules-file --[no]-cone\n\nwould be equivalent to setting the patterns with\n\n cat | git sparse-checkout set --stdin --[no]-cone\n\nbefore running the command above.\n\nThen all the cone/no-cone pattern specification stuff is still contained within\nthe 'sparse-checkout' command and the documentation for the sub-command would\nreside in the sparse-checkout.tx file next to all the caveats and discussion about\nsparse-checkout.\n\n\n"}]}