{"thread":{"id":"58267","subject":"[PATCH 0/4] reset/checkout: fix miscellaneous sparse index bugs","startedAt":"2022-08-04T20:46:15Z","lastAt":"2022-08-09T13:20:25Z","messageCount":23,"participants":["Victoria Dye via GitGitGadget","Junio C Hamano","Victoria Dye","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"460664","messageId":"pull.1312.git.1659645967.gitgitgadget@gmail.com","threadId":"58267","inReplyTo":null,"subject":"[PATCH 0/4] reset/checkout: fix miscellaneous sparse index bugs","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-04T20:46:03Z","receivedAt":"2022-08-04T20:46:15Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"While working on sparse index integration for 'git rm' [1], Shaoxuan found\nthat removed sparse directories, when reset, would no longer be sparse. This\nwas due to how 'unpack_trees()' determined whether a traversed directory was\na sparse directory or not; it would only unpack an entry as a sparse\ndirectory if it existed in the index. However, if the sparse directory was\nremoved, it would be treated like a non-sparse directory and its contents\nwould be individually unpacked.\n\nTo avoid this unnecessary traversal and keep the results of 'reset' as\nsparse as possible, the decision logic for whether a directory is sparse is\nchanged to:\n\n * If the directory is a sparse directory in the index, unpack it.\n * If not, is the directory inside the sparse cone? If so, do not unpack it.\n * If the directory is outside the sparse cone, does it have any child\n   entries in the index? If so, do not unpack it.\n * Otherwise, unpack the entry as a sparse directory.\n\nIn the process of updating 'reset', a separate issue was found in 'checkout'\nwhere collapsed sparse directories did not have modified contents reported\nfile-by-file. A similar bug was found with 'status' in 2c521b0e49 (status:\nfix nested sparse directory diff in sparse index, 2022-03-01), and\n'checkout' was corrected the same way (setting the diff flag 'recursive' to\n1).\n\nThanks!\n\n * Victoria\n\n[1]\nhttps://lore.kernel.org/git/20220803045118.1243087-1-shaoxuan.yuan02@gmail.com/\n\nVictoria Dye (4):\n  checkout: fix nested sparse directory diff in sparse index\n  oneway_diff: handle removed sparse directories\n  cache.h: replace 'index_entry_exists()' with 'index_name_pos_sparse()'\n  unpack-trees: handle missing sparse directories\n\n builtin/checkout.c                       |  1 +\n cache-tree.c                             |  2 +-\n cache.h                                  | 15 ++--\n diff-lib.c                               |  5 ++\n read-cache.c                             |  4 +-\n t/t1092-sparse-checkout-compatibility.sh | 25 +++++++\n unpack-trees.c                           | 88 +++++++++++++++++++++---\n 7 files changed, 119 insertions(+), 21 deletions(-)\n\n\nbase-commit: 4af7188bc97f70277d0f10d56d5373022b1fa385\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1312%2Fvdye%2Freset%2Fhandle-missing-dirs-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1312/vdye/reset/handle-missing-dirs-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1312\n-- \ngitgitgadget\n"},{"id":"460665","messageId":"255318f4dc61daef3e892a4dd5c571463985f340.1659645967.git.gitgitgadget@gmail.com","threadId":"58267","inReplyTo":"pull.1312.git.1659645967.gitgitgadget@gmail.com","subject":"[PATCH 1/4] checkout: fix nested sparse directory diff in sparse index","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-04T20:46:04Z","receivedAt":"2022-08-04T20:46:18Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye <vdye@github.com>\n\nAdd the 'recursive' diff flag to the local changes reporting done by 'git\ncheckout' in 'show_local_changes()'. Without the flag enabled, unexpanded\nsparse directories will not be recursed into to report the diff of each\nfile's contents, resulting in the reported local changes including\n\"modified\" sparse directories.\n\nThe same issue was found and fixed for 'git status' in 2c521b0e49 (status:\nfix nested sparse directory diff in sparse index, 2022-03-01)\n\nSigned-off-by: Victoria Dye <vdye@github.com>\n---\n builtin/checkout.c                       | 1 +\n t/t1092-sparse-checkout-compatibility.sh | 8 ++++++++\n 2 files changed, 9 insertions(+)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 29c74f898bf..f9d63d80b92 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -626,6 +626,7 @@ static void show_local_changes(struct object *head,\n \trepo_init_revisions(the_repository, &rev, NULL);\n \trev.diffopt.flags = opts->flags;\n \trev.diffopt.output_format |= DIFF_FORMAT_NAME_STATUS;\n+\trev.diffopt.flags.recursive = 1;\n \tdiff_setup_done(&rev.diffopt);\n \tadd_pending_object(&rev, head, NULL);\n \trun_diff_index(&rev, 0);\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 763c6cc6846..99a1425a929 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -372,6 +372,14 @@ test_expect_success 'deep changes during checkout' '\n \ttest_all_match git checkout base\n '\n \n+test_expect_success 'checkout with modified sparse directory' '\n+\tinit_repos &&\n+\n+\ttest_all_match git checkout rename-in-to-out -- . &&\n+\ttest_sparse_match git sparse-checkout reapply &&\n+\ttest_all_match git checkout base\n+'\n+\n test_expect_success 'add outside sparse cone' '\n \tinit_repos &&\n \n-- \ngitgitgadget\n\n"},{"id":"460666","messageId":"f7978d223fe86b6d0d43e905ee3abdc02fef8b7d.1659645967.git.gitgitgadget@gmail.com","threadId":"58267","inReplyTo":"pull.1312.git.1659645967.gitgitgadget@gmail.com","subject":"[PATCH 3/4] cache.h: replace 'index_entry_exists()' with 'index_name_pos_sparse()'","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-04T20:46:06Z","receivedAt":"2022-08-04T20:46:20Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye <vdye@github.com>\n\nReplace 'index_entry_exists()' (which returns a binary '1' or '0' depending\non whether a specified entry exists in the index) with\n'index_name_pos_sparse()' (which behaves the same as 'index_name_pos()',\nexcept that it does not expand a sparse index to search for an entry inside\na sparse directory).\n\n'index_entry_exists()' was original implemented in 20ec2d034c (reset: make\nsparse-aware (except --mixed), 2021-11-29) to allow callers to search for an\nindex entry without expanding a sparse index. That particular case only\nrequired knowing whether the requested entry existed. This patch expands the\namount of information returned by indicating both 1) whether the entry\nexists, and 2) its position (or potential position) in the index.\n\nSigned-off-by: Victoria Dye <vdye@github.com>\n---\n cache-tree.c |  2 +-\n cache.h      | 15 +++++++--------\n read-cache.c |  4 ++--\n 3 files changed, 10 insertions(+), 11 deletions(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 56db0b5026b..ff0c1733356 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -798,7 +798,7 @@ static void prime_cache_tree_rec(struct repository *r,\n \t\t\t * as normal.\n \t\t\t */\n \t\t\tif (r->index->sparse_index &&\n-\t\t\t    index_entry_exists(r->index, tree_path->buf, tree_path->len))\n+\t\t\t    index_name_pos_sparse(r->index, tree_path->buf, tree_path->len) >= 0)\n \t\t\t\tprime_cache_tree_sparse_dir(sub->cache_tree, subtree);\n \t\t\telse\n \t\t\t\tprime_cache_tree_rec(r, sub->cache_tree, subtree, tree_path);\ndiff --git a/cache.h b/cache.h\nindex 4aa1bd079d5..6bfd1d80b24 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -831,14 +831,13 @@ struct cache_entry *index_file_exists(struct index_state *istate, const char *na\n int index_name_pos(struct index_state *, const char *name, int namelen);\n \n /*\n- * Determines whether an entry with the given name exists within the\n- * given index. The return value is 1 if an exact match is found, otherwise\n- * it is 0. Note that, unlike index_name_pos, this function does not expand\n- * the index if it is sparse. If an item exists within the full index but it\n- * is contained within a sparse directory (and not in the sparse index), 0 is\n- * returned.\n- */\n-int index_entry_exists(struct index_state *, const char *name, int namelen);\n+ * Like index_name_pos, returns the position of an entry of the given name in\n+ * the index if one exists, otherwise returns a negative value where the negated\n+ * value minus 1 is the position where the index entry would be inserted. Unlike\n+ * index_name_pos, however, a sparse index is not expanded to find an entry\n+ * inside a sparse directory.\n+ */\n+int index_name_pos_sparse(struct index_state *, const char *name, int namelen);\n \n /*\n  * Some functions return the negative complement of an insert position when a\ndiff --git a/read-cache.c b/read-cache.c\nindex 4de207752dc..a85b922a3bc 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -620,9 +620,9 @@ int index_name_pos(struct index_state *istate, const char *name, int namelen)\n \treturn index_name_stage_pos(istate, name, namelen, 0, EXPAND_SPARSE);\n }\n \n-int index_entry_exists(struct index_state *istate, const char *name, int namelen)\n+int index_name_pos_sparse(struct index_state *istate, const char *name, int namelen)\n {\n-\treturn index_name_stage_pos(istate, name, namelen, 0, NO_EXPAND_SPARSE) >= 0;\n+\treturn index_name_stage_pos(istate, name, namelen, 0, NO_EXPAND_SPARSE);\n }\n \n int remove_index_entry_at(struct index_state *istate, int pos)\n-- \ngitgitgadget\n\n"},{"id":"460667","messageId":"55c77ba4b291fffe3031fbef67dcee9aa91a105c.1659645967.git.gitgitgadget@gmail.com","threadId":"58267","inReplyTo":"pull.1312.git.1659645967.gitgitgadget@gmail.com","subject":"[PATCH 2/4] oneway_diff: handle removed sparse directories","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-04T20:46:05Z","receivedAt":"2022-08-04T20:46:22Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye <vdye@github.com>\n\nUpdate 'do_oneway_diff()' to perform a 'diff_tree_oid()' on removed sparse\ndirectories, as it does for added or modified sparse directories (see\n9eb00af562 (diff-lib: handle index diffs with sparse dirs, 2021-07-14)).\n\nAt the moment, this update is unreachable code because 'unpack_trees()'\n(currently the only way 'oneway_diff()' can be called, via 'diff_cache()')\nwill always traverse trees down to the individual removed files of a deleted\nsparse directory. A subsequent patch will change this to better preserve a\nsparse index in other uses of 'unpack_tree()', e.g. 'git reset --hard'.\nHowever, making that change without this patch would result in (among other\nissues) 'git status' printing only the name of a deleted sparse directory,\nnot its contents. To avoid introducing that bug, 'do_oneway_diff()' is\nupdated before modifying 'unpack_trees()'.\n\nSigned-off-by: Victoria Dye <vdye@github.com>\n---\n diff-lib.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 7eb66a417aa..2edea41a234 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -466,6 +466,11 @@ static void do_oneway_diff(struct unpack_trees_options *o,\n \t * Something removed from the tree?\n \t */\n \tif (!idx) {\n+\t\tif (S_ISSPARSEDIR(tree->ce_mode)) {\n+\t\t\tdiff_tree_oid(&tree->oid, NULL, tree->name, &revs->diffopt);\n+\t\t\treturn;\n+\t\t}\n+\n \t\tdiff_index_show_file(revs, \"-\", tree, &tree->oid, 1,\n \t\t\t\t     tree->ce_mode, 0);\n \t\treturn;\n-- \ngitgitgadget\n\n"},{"id":"460668","messageId":"016971a67112efe2d15fe7908e86c5d2631f8e66.1659645967.git.gitgitgadget@gmail.com","threadId":"58267","inReplyTo":"pull.1312.git.1659645967.gitgitgadget@gmail.com","subject":"[PATCH 4/4] unpack-trees: handle missing sparse directories","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-04T20:46:07Z","receivedAt":"2022-08-04T20:46:30Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye <vdye@github.com>\n\nIf a sparse directory does not exist in the index, unpack it at the\ndirectory level rather than recursing into it an unpacking its contents\nfile-by-file. This helps keep the sparse index as collapsed as possible in\ncases such as 'git reset --hard' restoring a sparse directory.\n\nA directory is determined to be truly non-existent in the index (rather than\nthe parent of existing index entries), if 1) its path is outside the sparse\ncone and 2) there are no children of the directory in the index. This check\nis performed by 'missing_dir_is_sparse()' in 'unpack_single_entry()'. If the\ndirectory is a missing sparse dir, 'unpack_single_entry()'  will proceed\nwith unpacking it. This determination is also propagated back up to\n'unpack_callback()' via 'is_missing_sparse_dir' to prevent further tree\ntraversal into the unpacked directory.\n\nReported-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>\nSigned-off-by: Victoria Dye <vdye@github.com>\n---\n t/t1092-sparse-checkout-compatibility.sh | 17 +++++\n unpack-trees.c                           | 88 +++++++++++++++++++++---\n 2 files changed, 95 insertions(+), 10 deletions(-)\n\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 99a1425a929..eb5edebfa5d 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -695,6 +695,23 @@ test_expect_success 'reset with wildcard pathspec' '\n \ttest_all_match git ls-files -s -- folder1\n '\n \n+test_expect_success 'reset hard with removed sparse dir' '\n+\tinit_repos &&\n+\n+\ttest_all_match git rm -r --sparse folder1 &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\ttest_all_match git reset --hard &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\tcat >expect <<-\\EOF &&\n+\tfolder1/\n+\tEOF\n+\n+\tgit -C sparse-index ls-files --sparse folder1 >out &&\n+\ttest_cmp expect out\n+'\n+\n test_expect_success 'update-index modify outside sparse definition' '\n \tinit_repos &&\n \ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 8a454e03bff..aa62cef20fe 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1069,6 +1069,53 @@ static struct cache_entry *create_ce_entry(const struct traverse_info *info,\n \treturn ce;\n }\n \n+/*\n+ * Determine whether the path specified corresponds to a sparse directory\n+ * completely missing from the index. This function is assumed to only be\n+ * called when the named path isn't already in the index.\n+ */\n+static int missing_dir_is_sparse(const struct traverse_info *info,\n+\t\t\t\t const struct name_entry *p)\n+{\n+\tint res, pos;\n+\tstruct strbuf dirpath = STRBUF_INIT;\n+\tstruct unpack_trees_options *o = info->data;\n+\n+\t/*\n+\t * First, check whether the path is in the sparse cone. If it is,\n+\t * then this directory shouldn't be sparse.\n+\t */\n+\tstrbuf_add(&dirpath, info->traverse_path, info->pathlen);\n+\tstrbuf_add(&dirpath, p->path, p->pathlen);\n+\tstrbuf_addch(&dirpath, '/');\n+\tif (path_in_cone_mode_sparse_checkout(dirpath.buf, o->src_index)) {\n+\t\tres = 0;\n+\t\tgoto cleanup;\n+\t}\n+\n+\t/*\n+\t * Given that the directory is not inside the sparse cone, it could be\n+\t * (partially) expanded in the index. If child entries exist, the path\n+\t * is not a missing sparse directory.\n+\t */\n+\tpos = index_name_pos_sparse(o->src_index, dirpath.buf, dirpath.len);\n+\tif (pos >= 0)\n+\t\tBUG(\"cache entry '%s%s' shouldn't exist in the index\",\n+\t\t    info->traverse_path, p->path);\n+\n+\tpos = -pos - 1;\n+\tif (pos >= o->src_index->cache_nr) {\n+\t\tres = 1;\n+\t\tgoto cleanup;\n+\t}\n+\n+\tres = strncmp(o->src_index->cache[pos]->name, dirpath.buf, dirpath.len);\n+\n+cleanup:\n+\tstrbuf_release(&dirpath);\n+\treturn res;\n+}\n+\n /*\n  * Note that traverse_by_cache_tree() duplicates some logic in this function\n  * without actually calling it. If you change the logic here you may need to\n@@ -1078,21 +1125,40 @@ static int unpack_single_entry(int n, unsigned long mask,\n \t\t\t       unsigned long dirmask,\n \t\t\t       struct cache_entry **src,\n \t\t\t       const struct name_entry *names,\n-\t\t\t       const struct traverse_info *info)\n+\t\t\t       const struct traverse_info *info,\n+\t\t\t       int *is_missing_sparse_dir)\n {\n \tint i;\n \tstruct unpack_trees_options *o = info->data;\n \tunsigned long conflicts = info->df_conflicts | dirmask;\n+\tconst struct name_entry *p = names;\n \n-\tif (mask == dirmask && !src[0])\n-\t\treturn 0;\n+\t*is_missing_sparse_dir = 0;\n+\tif (mask == dirmask && !src[0]) {\n+\t\t/*\n+\t\t * If the directory is completely missing from the index but\n+\t\t * would otherwise be a sparse directory, we should unpack it.\n+\t\t * If not, we'll return and continue recursively traversing the\n+\t\t * tree.\n+\t\t */\n+\t\tif (!o->src_index->sparse_index)\n+\t\t\treturn 0;\n+\n+\t\t/* Find first entry with a real name (we could use \"mask\" too) */\n+\t\twhile (!p->mode)\n+\t\t\tp++;\n+\n+\t\t*is_missing_sparse_dir = missing_dir_is_sparse(info, p);\n+\t\tif (!*is_missing_sparse_dir)\n+\t\t\treturn 0;\n+\t}\n \n \t/*\n-\t * When we have a sparse directory entry for src[0],\n-\t * then this isn't necessarily a directory-file conflict.\n+\t * When we are unpacking a sparse directory, then this isn't necessarily\n+\t * a directory-file conflict.\n \t */\n-\tif (mask == dirmask && src[0] &&\n-\t    S_ISSPARSEDIR(src[0]->ce_mode))\n+\tif (mask == dirmask &&\n+\t    (*is_missing_sparse_dir || (src[0] && S_ISSPARSEDIR(src[0]->ce_mode))))\n \t\tconflicts = 0;\n \n \t/*\n@@ -1352,7 +1418,7 @@ static int unpack_sparse_callback(int n, unsigned long mask, unsigned long dirma\n {\n \tstruct cache_entry *src[MAX_UNPACK_TREES + 1] = { NULL, };\n \tstruct unpack_trees_options *o = info->data;\n-\tint ret;\n+\tint ret, is_missing_sparse_dir;\n \n \tassert(o->merge);\n \n@@ -1376,7 +1442,7 @@ static int unpack_sparse_callback(int n, unsigned long mask, unsigned long dirma\n \t * \"index\" tree (i.e., names[0]) and adjust 'names', 'n', 'mask', and\n \t * 'dirmask' accordingly.\n \t */\n-\tret = unpack_single_entry(n - 1, mask >> 1, dirmask >> 1, src, names + 1, info);\n+\tret = unpack_single_entry(n - 1, mask >> 1, dirmask >> 1, src, names + 1, info, &is_missing_sparse_dir);\n \n \tif (src[0])\n \t\tdiscard_cache_entry(src[0]);\n@@ -1394,6 +1460,7 @@ static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, str\n \tstruct cache_entry *src[MAX_UNPACK_TREES + 1] = { NULL, };\n \tstruct unpack_trees_options *o = info->data;\n \tconst struct name_entry *p = names;\n+\tint is_missing_sparse_dir;\n \n \t/* Find first entry with a real name (we could use \"mask\" too) */\n \twhile (!p->mode)\n@@ -1440,7 +1507,7 @@ static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, str\n \t\t}\n \t}\n \n-\tif (unpack_single_entry(n, mask, dirmask, src, names, info) < 0)\n+\tif (unpack_single_entry(n, mask, dirmask, src, names, info, &is_missing_sparse_dir))\n \t\treturn -1;\n \n \tif (o->merge && src[0]) {\n@@ -1478,6 +1545,7 @@ static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, str\n \t\t}\n \n \t\tif (!is_sparse_directory_entry(src[0], names, info) &&\n+\t\t    !is_missing_sparse_dir &&\n \t\t    traverse_trees_recursive(n, dirmask, mask & ~dirmask,\n \t\t\t\t\t\t    names, info) < 0) {\n \t\t\treturn -1;\n-- \ngitgitgadget\n"},{"id":"460678","messageId":"xmqqpmhf1wpc.fsf@gitster.g","threadId":"58267","inReplyTo":"f7978d223fe86b6d0d43e905ee3abdc02fef8b7d.1659645967.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/4] cache.h: replace 'index_entry_exists()' with 'index_name_pos_sparse()'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-04T22:16:15Z","receivedAt":"2022-08-04T22:16:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Victoria Dye via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Victoria Dye <vdye@github.com>\n>\n> Replace 'index_entry_exists()' (which returns a binary '1' or '0' depending\n> on whether a specified entry exists in the index) with\n> 'index_name_pos_sparse()' (which behaves the same as 'index_name_pos()',\n> except that it does not expand a sparse index to search for an entry inside\n> a sparse directory).\n>\n> 'index_entry_exists()' was original implemented in 20ec2d034c (reset: make\n\n\"original\" -> \"originally\"?\n\n> sparse-aware (except --mixed), 2021-11-29) to allow callers to search for an\n> index entry without expanding a sparse index. That particular case only\n> required knowing whether the requested entry existed. This patch expands the\n> amount of information returned by indicating both 1) whether the entry\n> exists, and 2) its position (or potential position) in the index.\n\n\"position or potential position\" is good, but the latter stalled my\nreading briefly, wondering if \"potential\" refers to \"the position\nthat the entry would be at when the sparse entries are all\nexpanded\", which is not what the patch and the description intend to\nsay.  I am not sure how to rephrase the proposed log message to\nalleviate the confusion, though.\n\n> diff --git a/cache.h b/cache.h\n> index 4aa1bd079d5..6bfd1d80b24 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -831,14 +831,13 @@ struct cache_entry *index_file_exists(struct index_state *istate, const char *na\n>  int index_name_pos(struct index_state *, const char *name, int namelen);\n>  \n>  /*\n> + * Like index_name_pos, returns the position of an entry of the given name in\n> + * the index if one exists, otherwise returns a negative value where the negated\n> + * value minus 1 is the position where the index entry would be inserted. Unlike\n> + * index_name_pos, however, a sparse index is not expanded to find an entry\n> + * inside a sparse directory.\n> + */\n\nThis description is perfectly clear.\n\n> +int index_name_pos_sparse(struct index_state *, const char *name, int namelen);\n>  \n>  /*\n>   * Some functions return the negative complement of an insert position when a\n> diff --git a/read-cache.c b/read-cache.c\n> index 4de207752dc..a85b922a3bc 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -620,9 +620,9 @@ int index_name_pos(struct index_state *istate, const char *name, int namelen)\n>  \treturn index_name_stage_pos(istate, name, namelen, 0, EXPAND_SPARSE);\n>  }\n>  \n> -int index_entry_exists(struct index_state *istate, const char *name, int namelen)\n> +int index_name_pos_sparse(struct index_state *istate, const char *name, int namelen)\n>  {\n> -\treturn index_name_stage_pos(istate, name, namelen, 0, NO_EXPAND_SPARSE) >= 0;\n> +\treturn index_name_stage_pos(istate, name, namelen, 0, NO_EXPAND_SPARSE);\n>  }\n\nNice.\n\nThanks.\n"},{"id":"460683","messageId":"xmqqa68j1tlr.fsf@gitster.g","threadId":"58267","inReplyTo":"016971a67112efe2d15fe7908e86c5d2631f8e66.1659645967.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/4] unpack-trees: handle missing sparse directories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-04T23:23:12Z","receivedAt":"2022-08-04T23:23:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Victoria Dye via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Victoria Dye <vdye@github.com>\n>\n> If a sparse directory does not exist in the index, unpack it at the\n> directory level rather than recursing into it an unpacking its contents\n> file-by-file. This helps keep the sparse index as collapsed as possible in\n> cases such as 'git reset --hard' restoring a sparse directory.\n\nMy reading hiccuped at \"does not exist in\".  After reading the above\ntwice, I think the situation is that there is a directory A/B in\nwhich a file C sits, and A/ is marked as sparse and the paragraph is\ntalking about directory A/B.  Because the index has A/ as a tree in\nindex, A/B does not exist in the index.\n\nWhen we _somehow_ need to ensure that A/B exists in the index for\n_unknown_ reason, we could flatten the index fully and have A/B/C as\na blob (without A or A/B in the index proper, even though they may\nappear in cache-tree), but if we stop at removing A and adding A/B\nstill as a tree without showing A/B/C in the index, we may gain\nefficiency, under the assumption that we do not have to access A/B/C\nand its siblings.\n\nDid I read the intention correctly?  I suspect future readers of the\ncommit that would result from this patch would also wonder what the\n\"somehow\" and \"unknown\" above are.\n\n> A directory is determined to be truly non-existent in the index (rather than\n> the parent of existing index entries), if 1) its path is outside the sparse\n> cone and 2) there are no children of the directory in the index. This check\n> is performed by 'missing_dir_is_sparse()' in 'unpack_single_entry()'. If the\n> directory is a missing sparse dir, 'unpack_single_entry()'  will proceed\n> with unpacking it. This determination is also propagated back up to\n> 'unpack_callback()' via 'is_missing_sparse_dir' to prevent further tree\n> traversal into the unpacked directory.\n\nMakes sense.\n\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index 8a454e03bff..aa62cef20fe 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -1069,6 +1069,53 @@ static struct cache_entry *create_ce_entry(const struct traverse_info *info,\n>  \treturn ce;\n>  }\n>  \n> +/*\n> + * Determine whether the path specified corresponds to a sparse directory\n> + * completely missing from the index. This function is assumed to only be\n> + * called when the named path isn't already in the index.\n> + */\n> +static int missing_dir_is_sparse(const struct traverse_info *info,\n> +\t\t\t\t const struct name_entry *p)\n> +{\n> +\tint res, pos;\n> +\tstruct strbuf dirpath = STRBUF_INIT;\n> +\tstruct unpack_trees_options *o = info->data;\n> +\n> +\t/*\n> +\t * First, check whether the path is in the sparse cone. If it is,\n> +\t * then this directory shouldn't be sparse.\n> +\t */\n> +\tstrbuf_add(&dirpath, info->traverse_path, info->pathlen);\n> +\tstrbuf_add(&dirpath, p->path, p->pathlen);\n> +\tstrbuf_addch(&dirpath, '/');\n> +\tif (path_in_cone_mode_sparse_checkout(dirpath.buf, o->src_index)) {\n> +\t\tres = 0;\n> +\t\tgoto cleanup;\n> +\t}\n\nOK.\n\n> +\t/*\n> +\t * Given that the directory is not inside the sparse cone, it could be\n> +\t * (partially) expanded in the index. If child entries exist, the path\n> +\t * is not a missing sparse directory.\n> +\t */\n> +\tpos = index_name_pos_sparse(o->src_index, dirpath.buf, dirpath.len);\n> +\tif (pos >= 0)\n> +\t\tBUG(\"cache entry '%s%s' shouldn't exist in the index\",\n> +\t\t    info->traverse_path, p->path);\n\nSo, we fed 'p' to this function, and we didn't expect it to exist in\nthe index if it is outside the sparse cone.\n\n> +\tpos = -pos - 1;\n\nThis is the location that a cache entry for the dirpath.buf would\nsit in the index if it were a sparse entry.\n\n> +\tif (pos >= o->src_index->cache_nr) {\n> +\t\tres = 1;\n> +\t\tgoto cleanup;\n> +\t}\n> +\tres = strncmp(o->src_index->cache[pos]->name, dirpath.buf, dirpath.len);\n\nSo, we found where dirpath.buf would be inserted.  If we (can) look\nat the cache entry that is currently sitting at that position, and\nfind that it is a path inside it, then res becomes zero.  IOW, we\nfound that the sparse directory is missing in the index, but there\nis a path that is in the directory recorded in the index.\n\nThe current entry, on the other hand, is outside the dirpath.buf,\nthen res becomes non-zero.  We are asked to yield true (i.e.\nnonzero) if and only if the given directory and all paths in that\ndirectory are missing from the index, so that is what happens here.\nSounds OK.\n\nAnd the \"out of bounds\" check just means that the entries that sit\nnear the end of the index sort strictly before our dirpath.buf, so\nthey cannot be inside our directory, which is also the case where we\nare expected to yield \"true\".\n\nOK.\n\n> +\n> +cleanup:\n> +\tstrbuf_release(&dirpath);\n> +\treturn res;\n> +}\n> +\n>  /*\n>   * Note that traverse_by_cache_tree() duplicates some logic in this function\n>   * without actually calling it. If you change the logic here you may need to\n> @@ -1078,21 +1125,40 @@ static int unpack_single_entry(int n, unsigned long mask,\n>  \t\t\t       unsigned long dirmask,\n>  \t\t\t       struct cache_entry **src,\n>  \t\t\t       const struct name_entry *names,\n> -\t\t\t       const struct traverse_info *info)\n> +\t\t\t       const struct traverse_info *info,\n> +\t\t\t       int *is_missing_sparse_dir)\n>  {\n>  \tint i;\n>  \tstruct unpack_trees_options *o = info->data;\n>  \tunsigned long conflicts = info->df_conflicts | dirmask;\n> +\tconst struct name_entry *p = names;\n>  \n> -\tif (mask == dirmask && !src[0])\n> -\t\treturn 0;\n> +\t*is_missing_sparse_dir = 0;\n> +\tif (mask == dirmask && !src[0]) {\n> +\t\t/*\n> +\t\t * If the directory is completely missing from the index but\n> +\t\t * would otherwise be a sparse directory, we should unpack it.\n> +\t\t * If not, we'll return and continue recursively traversing the\n> +\t\t * tree.\n> +\t\t */\n> +\t\tif (!o->src_index->sparse_index)\n> +\t\t\treturn 0;\n> +\n> +\t\t/* Find first entry with a real name (we could use \"mask\" too) */\n> +\t\twhile (!p->mode)\n> +\t\t\tp++;\n> +\n> +\t\t*is_missing_sparse_dir = missing_dir_is_sparse(info, p);\n> +\t\tif (!*is_missing_sparse_dir)\n> +\t\t\treturn 0;\n\nInteresting.  Nobody checked if the name in 'p' is a directory up to\nthis point, but missing_dir_is_sparse() does not care?  The only\nthing we know is that !src[0], so the name represented by 'p' does\nnot exist in the index.  IIRC, the function only checked if p names\na path inside or outside the sparse cone and if there are or are not\npaths that would be inside the path if p _were_ a directory but I\ndidn't read it carefully what it did when the caller fed a path to a\nnon-directory to the function.  As long as it will say \"no\" to such\na situation, I won't complain, but I have to wonder how the logic\naround here tells apart a case where a sparse directory is\ncompletely hidden (which missing_dir_is_sparse() gives us) from a\ncase where there is absolutely nothing, not a directory and not a\nblob, at the path in the index.  Or perhaps it works correctly\nwithout having to tell them apart?  I dunno.\n\nBy the way, there is a comment before the function that reminds us\nthat traverse_by_cache_tree() may need a matching change whenever a\nchange is made to this function.  Does this require such a matching\nchange?\n"},{"id":"460720","messageId":"3825ef9a-4c71-21ed-6452-bbd322ca839c@github.com","threadId":"58267","inReplyTo":"xmqqa68j1tlr.fsf@gitster.g","subject":"Re: [PATCH 4/4] unpack-trees: handle missing sparse directories","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-08-05T16:36:47Z","receivedAt":"2022-08-05T16:36:54Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Junio C Hamano wrote:\n> \"Victoria Dye via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n>> From: Victoria Dye <vdye@github.com>\n>>\n>> If a sparse directory does not exist in the index, unpack it at the\n>> directory level rather than recursing into it an unpacking its contents\n>> file-by-file. This helps keep the sparse index as collapsed as possible in\n>> cases such as 'git reset --hard' restoring a sparse directory.\n> \n> My reading hiccuped at \"does not exist in\".  After reading the above\n> twice, I think the situation is that there is a directory A/B in\n> which a file C sits, and A/ is marked as sparse and the paragraph is\n> talking about directory A/B.  Because the index has A/ as a tree in\n> index, A/B does not exist in the index.\n> > When we _somehow_ need to ensure that A/B exists in the index for\n> _unknown_ reason, we could flatten the index fully and have A/B/C as\n> a blob (without A or A/B in the index proper, even though they may\n> appear in cache-tree), but if we stop at removing A and adding A/B\n> still as a tree without showing A/B/C in the index, we may gain\n> efficiency, under the assumption that we do not have to access A/B/C\n> and its siblings.\n> \n> Did I read the intention correctly?  I suspect future readers of the\n> commit that would result from this patch would also wonder what the\n> \"somehow\" and \"unknown\" above are.\n> \nIf I'm reading this correctly, it's not quite what I meant - the situation\nthis patch addresses is when _nothing_ in the tree rooted at 'A/' (files,\nsubdirectories) exists in the index, but 'unpack_trees()' is merging tree(s)\nwhere 'A/' *does* exist. I originally wanted to write \"If a sparse directory\n*is deleted from* the index\", but that doesn't cover the case where 'A/'\nnever existed in the index and we're merging in tree(s) that introduce it.\n\nMaybe it would be clearer to describe it with a different perspective: \"If\n'unpack_callback()' is merging a new tree into a sparse index, merge the\ntree as a sparse directory rather than traversing its contents\" or something\nlike that? I'll try to come up with a better way of explaining this and\nupdate in V2. \n\n>> A directory is determined to be truly non-existent in the index (rather than\n>> the parent of existing index entries), if 1) its path is outside the sparse\n>> cone and 2) there are no children of the directory in the index. This check\n>> is performed by 'missing_dir_is_sparse()' in 'unpack_single_entry()'. If the\n>> directory is a missing sparse dir, 'unpack_single_entry()'  will proceed\n>> with unpacking it. This determination is also propagated back up to\n>> 'unpack_callback()' via 'is_missing_sparse_dir' to prevent further tree\n>> traversal into the unpacked directory.\n> \n> Makes sense.\n> \n>> diff --git a/unpack-trees.c b/unpack-trees.c\n>> index 8a454e03bff..aa62cef20fe 100644\n>> --- a/unpack-trees.c\n>> +++ b/unpack-trees.c\n>> @@ -1069,6 +1069,53 @@ static struct cache_entry *create_ce_entry(const struct traverse_info *info,\n>>  \treturn ce;\n>>  }\n>>  \n>> +/*\n>> + * Determine whether the path specified corresponds to a sparse directory\n>> + * completely missing from the index. This function is assumed to only be\n>> + * called when the named path isn't already in the index.\n>> + */\n>> +static int missing_dir_is_sparse(const struct traverse_info *info,\n>> +\t\t\t\t const struct name_entry *p)\n>> +{\n>> +\tint res, pos;\n>> +\tstruct strbuf dirpath = STRBUF_INIT;\n>> +\tstruct unpack_trees_options *o = info->data;\n>> +\n>> +\t/*\n>> +\t * First, check whether the path is in the sparse cone. If it is,\n>> +\t * then this directory shouldn't be sparse.\n>> +\t */\n>> +\tstrbuf_add(&dirpath, info->traverse_path, info->pathlen);\n>> +\tstrbuf_add(&dirpath, p->path, p->pathlen);\n>> +\tstrbuf_addch(&dirpath, '/');\n>> +\tif (path_in_cone_mode_sparse_checkout(dirpath.buf, o->src_index)) {\n>> +\t\tres = 0;\n>> +\t\tgoto cleanup;\n>> +\t}\n> \n> OK.\n> \n>> +\t/*\n>> +\t * Given that the directory is not inside the sparse cone, it could be\n>> +\t * (partially) expanded in the index. If child entries exist, the path\n>> +\t * is not a missing sparse directory.\n>> +\t */\n>> +\tpos = index_name_pos_sparse(o->src_index, dirpath.buf, dirpath.len);\n>> +\tif (pos >= 0)\n>> +\t\tBUG(\"cache entry '%s%s' shouldn't exist in the index\",\n>> +\t\t    info->traverse_path, p->path);\n> \n> So, we fed 'p' to this function, and we didn't expect it to exist in\n> the index if it is outside the sparse cone.\n> \n\nCorrect; if an index entry with the name 'p' existed, 'src[0]' would not be\nNULL in 'unpack_callback()'/'unpack_single_entry()' and this function\nwouldn't have been called.\n\n>> +\tpos = -pos - 1;\n> \n> This is the location that a cache entry for the dirpath.buf would\n> sit in the index if it were a sparse entry.\n> \n>> +\tif (pos >= o->src_index->cache_nr) {\n>> +\t\tres = 1;\n>> +\t\tgoto cleanup;\n>> +\t}\n>> +\tres = strncmp(o->src_index->cache[pos]->name, dirpath.buf, dirpath.len);\n> \n> So, we found where dirpath.buf would be inserted.  If we (can) look\n> at the cache entry that is currently sitting at that position, and\n> find that it is a path inside it, then res becomes zero.  IOW, we\n> found that the sparse directory is missing in the index, but there\n> is a path that is in the directory recorded in the index.\n> \n> The current entry, on the other hand, is outside the dirpath.buf,\n> then res becomes non-zero.  We are asked to yield true (i.e.\n> nonzero) if and only if the given directory and all paths in that\n> directory are missing from the index, so that is what happens here.\n> Sounds OK.\n> \n> And the \"out of bounds\" check just means that the entries that sit\n> near the end of the index sort strictly before our dirpath.buf, so\n> they cannot be inside our directory, which is also the case where we\n> are expected to yield \"true\".\n> \n> OK.\n> \n>> +\n>> +cleanup:\n>> +\tstrbuf_release(&dirpath);\n>> +\treturn res;\n>> +}\n>> +\n>>  /*\n>>   * Note that traverse_by_cache_tree() duplicates some logic in this function\n>>   * without actually calling it. If you change the logic here you may need to\n>> @@ -1078,21 +1125,40 @@ static int unpack_single_entry(int n, unsigned long mask,\n>>  \t\t\t       unsigned long dirmask,\n>>  \t\t\t       struct cache_entry **src,\n>>  \t\t\t       const struct name_entry *names,\n>> -\t\t\t       const struct traverse_info *info)\n>> +\t\t\t       const struct traverse_info *info,\n>> +\t\t\t       int *is_missing_sparse_dir)\n>>  {\n>>  \tint i;\n>>  \tstruct unpack_trees_options *o = info->data;\n>>  \tunsigned long conflicts = info->df_conflicts | dirmask;\n>> +\tconst struct name_entry *p = names;\n>>  \n>> -\tif (mask == dirmask && !src[0])\n>> -\t\treturn 0;\n>> +\t*is_missing_sparse_dir = 0;\n>> +\tif (mask == dirmask && !src[0]) {\n>> +\t\t/*\n>> +\t\t * If the directory is completely missing from the index but\n>> +\t\t * would otherwise be a sparse directory, we should unpack it.\n>> +\t\t * If not, we'll return and continue recursively traversing the\n>> +\t\t * tree.\n>> +\t\t */\n>> +\t\tif (!o->src_index->sparse_index)\n>> +\t\t\treturn 0;\n>> +\n>> +\t\t/* Find first entry with a real name (we could use \"mask\" too) */\n>> +\t\twhile (!p->mode)\n>> +\t\t\tp++;\n>> +\n>> +\t\t*is_missing_sparse_dir = missing_dir_is_sparse(info, p);\n>> +\t\tif (!*is_missing_sparse_dir)\n>> +\t\t\treturn 0;\n> \n> Interesting.  Nobody checked if the name in 'p' is a directory up to\n> this point, but missing_dir_is_sparse() does not care?  The only\n> thing we know is that !src[0], so the name represented by 'p' does\n> not exist in the index.  IIRC, the function only checked if p names\n> a path inside or outside the sparse cone and if there are or are not\n> paths that would be inside the path if p _were_ a directory but I\n> didn't read it carefully what it did when the caller fed a path to a\n> non-directory to the function.  As long as it will say \"no\" to such\n> a situation, I won't complain, but I have to wonder how the logic\n> around here tells apart a case where a sparse directory is\n> completely hidden (which missing_dir_is_sparse() gives us) from a\n> case where there is absolutely nothing, not a directory and not a\n> blob, at the path in the index.  Or perhaps it works correctly\n> without having to tell them apart?  I dunno.\n> \n\nI wrote 'missing_dir_is_sparse()' in an attempt keep some complex logic out\nof the already-complicated 'unpack_single_entry()', but as part of that it\nrelies on information already established by its caller.\n\nWe know 'p' is a directory because 'missing_dir_is_sparse()' is called\ninside a 'mask == dirmask' condition. 'mask' is a representation of which\ntrees being traversed have an entry with the given name and 'dirmask' is a\nrepresentation of which of those entries are directories, so the only way\n'mask == dirmask' and 'p' is *not* a directory is if the currently-traversed\nentries in all of the trees do not exist. *That* won't happen because\n'unpack_callback()' won't be invoked at all by 'traverse_trees()' if 'mask'\nis 0.\n\nGiven that it requires jumping through multiple function invocations and\ncallbacks to figure that out, I can add some assertions or 'return 0'\nconditions at the beginning of 'missing_dir_is_sparse()' to codify its\nassumptions. Even if the assertions are slightly redundant now, they'll make\nthe code clearer and make the function safer for reuse.\n\n> By the way, there is a comment before the function that reminds us\n> that traverse_by_cache_tree() may need a matching change whenever a\n> change is made to this function.  Does this require such a matching\n> change?\n\nIn this case, I *think* a parallel change there is unnecessary.\n'traverse_by_cache_tree()' iterates on the contents of the index, so its\nequivalent of 'src[0]' always exists when it merges individual entries. And,\nto trigger that function, the cache tree needs to identically match the\ntree(s) being merged; if a subtree doesn't exist in either the index or any\nof the merging trees, there's nothing to merge.\n\nIn any case, thanks for the reminder - I didn't notice the comment while\nimplementing this, so it could have just as easily needed fixing.\n"},{"id":"460730","messageId":"c93dc6d7-ab49-667f-ef4e-544a764fb737@github.com","threadId":"58267","inReplyTo":"255318f4dc61daef3e892a4dd5c571463985f340.1659645967.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/4] checkout: fix nested sparse directory diff in sparse index","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-08-05T17:59:01Z","receivedAt":"2022-08-05T17:59:33Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/4/2022 4:46 PM, Victoria Dye via GitGitGadget wrote:\n> From: Victoria Dye <vdye@github.com>\n> \n> Add the 'recursive' diff flag to the local changes reporting done by 'git\n> checkout' in 'show_local_changes()'. Without the flag enabled, unexpanded\n> sparse directories will not be recursed into to report the diff of each\n> file's contents, resulting in the reported local changes including\n> \"modified\" sparse directories.\n\nNice find!\n\n> +\trev.diffopt.flags.recursive = 1;\n\nSimple fix.\n\n> +test_expect_success 'checkout with modified sparse directory' '\n> +\tinit_repos &&\n> +\n> +\ttest_all_match git checkout rename-in-to-out -- . &&\n> +\ttest_sparse_match git sparse-checkout reapply &&\n> +\ttest_all_match git checkout base\n> +'\n\nSimple test.\n\nExcellent.\n-Stolee\n"},{"id":"460737","messageId":"xmqqiln6ze64.fsf@gitster.g","threadId":"58267","inReplyTo":"3825ef9a-4c71-21ed-6452-bbd322ca839c@github.com","subject":"Re: [PATCH 4/4] unpack-trees: handle missing sparse directories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-05T19:24:51Z","receivedAt":"2022-08-05T19:25:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Victoria Dye <vdye@github.com> writes:\n\n> If I'm reading this correctly, it's not quite what I meant - the situation\n> this patch addresses is when _nothing_ in the tree rooted at 'A/' (files,\n> subdirectories) exists in the index, but 'unpack_trees()' is merging tree(s)\n> where 'A/' *does* exist.\n\nOK, that is very different from my flawed readign.\n\n> Maybe it would be clearer to describe it with a different perspective: \"If\n> 'unpack_callback()' is merging a new tree into a sparse index, merge the\n> tree as a sparse directory rather than traversing its contents\" or something\n> like that? I'll try to come up with a better way of explaining this and\n> update in V2. \n\nYeah, that explains a typical scenario where you want to do this\nkind of thing better.  Right now, do we just merge in the contents\nof that whole tree whether the root of that new tree is or is not in\nthe sparse cone(s)?  Noticing that the new tree is outside the cones\nof our interest and populating the index minimally, just enough to\nbe able to write the resulting index as a tree, does make sense.\n\n> I wrote 'missing_dir_is_sparse()' in an attempt keep some complex logic out\n> of the already-complicated 'unpack_single_entry()', but as part of that it\n> relies on information already established by its caller.\n>\n> We know 'p' is a directory because 'missing_dir_is_sparse()' is called\n> inside a 'mask == dirmask' condition.\n\nAhh, thanks---that is exactly what I missed, and led to my questions.\n\n> 'mask' is a representation of which\n> trees being traversed have an entry with the given name and 'dirmask' is a\n> representation of which of those entries are directories, so the only way\n> 'mask == dirmask' and 'p' is *not* a directory is if the currently-traversed\n> entries in all of the trees do not exist. *That* won't happen because\n> 'unpack_callback()' won't be invoked at all by 'traverse_trees()' if 'mask'\n> is 0.\n>\n> Given that it requires jumping through multiple function invocations and\n> callbacks to figure that out, I can add some assertions or 'return 0'\n> conditions at the beginning of 'missing_dir_is_sparse()' to codify its\n> assumptions. Even if the assertions are slightly redundant now, they'll make\n> the code clearer and make the function safer for reuse.\n\nSounds good.\n\nThanks.\n"},{"id":"460744","messageId":"xmqqfsiaxmfa.fsf@gitster.g","threadId":"58267","inReplyTo":"f7978d223fe86b6d0d43e905ee3abdc02fef8b7d.1659645967.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/4] cache.h: replace 'index_entry_exists()' with 'index_name_pos_sparse()'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-06T00:09:29Z","receivedAt":"2022-08-06T00:09:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Victoria Dye via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> 'index_entry_exists()' was original implemented in 20ec2d034c (reset: make\n> sparse-aware (except --mixed), 2021-11-29) to allow callers to search for an\n> index entry without expanding a sparse index. That particular case only\n> required knowing whether the requested entry existed. This patch expands the\n> amount of information returned by indicating both 1) whether the entry\n> exists, and 2) its position (or potential position) in the index.\n\nThis patch probably should keep index_entry_exists() to potential\nnew callers that may be introduced by contemporary topics in flight,\nby doing something like the following squashed in.  We know that\nShaoxuan's series does add a new callsite, which is of a lessor\nconcern as that series may want to be rebased on top of these fixes\nanyway.  But there may be other topics that may want to add new\ncalls for this helper that is more trivially and obviously correct\nthan these four patches, in which case such topics want to proceed\nindependent of these four patches.\n\n cache.h | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git c/cache.h w/cache.h\nindex ba85435fee..039a32a317 100644\n--- c/cache.h\n+++ w/cache.h\n@@ -839,6 +839,17 @@ int index_name_pos(struct index_state *, const char *name, int namelen);\n  */\n int index_name_pos_sparse(struct index_state *, const char *name, int namelen);\n \n+/*\n+ * This helper function is only kept to help possible\n+ * contemporary topics in flight.  Do not use it in new\n+ * code; use index_name_pos_sparse() instead.\n+ */\n+static inline int index_entry_exists(struct index_state *istate,\n+\t\t\t\t     const char *name, int namelen)\n+{\n+\treturn 0 <= index_name_pos_sparse(istate, name, namelen);\n+}\n+\n /*\n  * Some functions return the negative complement of an insert position when a\n  * precise match was not found but a position was found where the entry would\n"},{"id":"460772","messageId":"pull.1312.v2.git.1659841030.gitgitgadget@gmail.com","threadId":"58267","inReplyTo":"pull.1312.git.1659645967.gitgitgadget@gmail.com","subject":"[PATCH v2 0/4] reset/checkout: fix miscellaneous sparse index bugs","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-07T02:57:05Z","receivedAt":"2022-08-07T02:57:19Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"While working on sparse index integration for 'git rm' [1], Shaoxuan found\nthat removed sparse directories, when reset, would no longer be sparse. This\nwas due to how 'unpack_trees()' determined whether a traversed directory was\na sparse directory or not; it would only unpack an entry as a sparse\ndirectory if it existed in the index. However, if the sparse directory was\nremoved, it would be treated like a non-sparse directory and its contents\nwould be individually unpacked.\n\nTo avoid this unnecessary traversal and keep the results of 'reset' as\nsparse as possible, the decision logic for whether a directory is sparse is\nchanged to:\n\n * If the directory is a sparse directory in the index, unpack it.\n * If not, is the directory inside the sparse cone? If so, do not unpack it.\n * If the directory is outside the sparse cone, does it have any child\n   entries in the index? If so, do not unpack it.\n * Otherwise, unpack the entry as a sparse directory.\n\nIn the process of updating 'reset', a separate issue was found in 'checkout'\nwhere collapsed sparse directories did not have modified contents reported\nfile-by-file. A similar bug was found with 'status' in 2c521b0e49 (status:\nfix nested sparse directory diff in sparse index, 2022-03-01), and\n'checkout' was corrected the same way (setting the diff flag 'recursive' to\n1).\n\n\nChanges since V1\n================\n\n * Reverted the removal of 'index_entry_exists()' to avoid breaking other\n   in-flight series.\n * Renamed 'is_missing_sparse_dir()' to 'is_new_sparse_dir()'; revised\n   comments and commit messages to clarify what that function is doing and\n   why.\n * Handled \"unexpected\" inputs to 'is_new_sparse_dir()' more gently,\n   returning 0 if 'p' is not a directory or the directory already exists in\n   the index (rather than exiting with 'BUG()'). This is intended to make\n   'is_new_sparse_dir()' less reliant on information about the index\n   established by 'unpack_callback()' & 'unpack_single_entry()', resulting\n   in easier-to-read and more reusable code.\n\nThanks!\n\n * Victoria\n\n[1]\nhttps://lore.kernel.org/git/20220803045118.1243087-1-shaoxuan.yuan02@gmail.com/\n\nVictoria Dye (4):\n  checkout: fix nested sparse directory diff in sparse index\n  oneway_diff: handle removed sparse directories\n  cache.h: create 'index_name_pos_sparse()'\n  unpack-trees: unpack new trees as sparse directories\n\n builtin/checkout.c                       |   1 +\n cache.h                                  |   9 ++\n diff-lib.c                               |   5 ++\n read-cache.c                             |   5 ++\n t/t1092-sparse-checkout-compatibility.sh |  25 ++++++\n unpack-trees.c                           | 106 ++++++++++++++++++++---\n 6 files changed, 141 insertions(+), 10 deletions(-)\n\n\nbase-commit: 4af7188bc97f70277d0f10d56d5373022b1fa385\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1312%2Fvdye%2Freset%2Fhandle-missing-dirs-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1312/vdye/reset/handle-missing-dirs-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1312\n\nRange-diff vs v1:\n\n 1:  255318f4dc6 = 1:  255318f4dc6 checkout: fix nested sparse directory diff in sparse index\n 2:  55c77ba4b29 = 2:  55c77ba4b29 oneway_diff: handle removed sparse directories\n 3:  f7978d223fe ! 3:  d0bdec63286 cache.h: replace 'index_entry_exists()' with 'index_name_pos_sparse()'\n     @@ Metadata\n      Author: Victoria Dye <vdye@github.com>\n      \n       ## Commit message ##\n     -    cache.h: replace 'index_entry_exists()' with 'index_name_pos_sparse()'\n     +    cache.h: create 'index_name_pos_sparse()'\n      \n     -    Replace 'index_entry_exists()' (which returns a binary '1' or '0' depending\n     -    on whether a specified entry exists in the index) with\n     -    'index_name_pos_sparse()' (which behaves the same as 'index_name_pos()',\n     +    Add 'index_name_pos_sparse()', which behaves the same as 'index_name_pos()',\n          except that it does not expand a sparse index to search for an entry inside\n     -    a sparse directory).\n     +    a sparse directory.\n      \n     -    'index_entry_exists()' was original implemented in 20ec2d034c (reset: make\n     -    sparse-aware (except --mixed), 2021-11-29) to allow callers to search for an\n     -    index entry without expanding a sparse index. That particular case only\n     -    required knowing whether the requested entry existed. This patch expands the\n     -    amount of information returned by indicating both 1) whether the entry\n     -    exists, and 2) its position (or potential position) in the index.\n     +    'index_entry_exists()' was originally implemented in 20ec2d034c (reset: make\n     +    sparse-aware (except --mixed), 2021-11-29) as an alternative to\n     +    'index_name_pos()' to allow callers to search for an index entry without\n     +    expanding a sparse index. However, that particular use case only required\n     +    knowing whether the requested entry existed, so 'index_entry_exists()' does\n     +    not return the index positioning information provided by 'index_name_pos()'.\n      \n     -    Signed-off-by: Victoria Dye <vdye@github.com>\n     +    This patch implements 'index_name_pos_sparse()' to accommodate callers that\n     +    need the positioning information of 'index_name_pos()', but do not want to\n     +    expand the index.\n      \n     - ## cache-tree.c ##\n     -@@ cache-tree.c: static void prime_cache_tree_rec(struct repository *r,\n     - \t\t\t * as normal.\n     - \t\t\t */\n     - \t\t\tif (r->index->sparse_index &&\n     --\t\t\t    index_entry_exists(r->index, tree_path->buf, tree_path->len))\n     -+\t\t\t    index_name_pos_sparse(r->index, tree_path->buf, tree_path->len) >= 0)\n     - \t\t\t\tprime_cache_tree_sparse_dir(sub->cache_tree, subtree);\n     - \t\t\telse\n     - \t\t\t\tprime_cache_tree_rec(r, sub->cache_tree, subtree, tree_path);\n     +    Signed-off-by: Victoria Dye <vdye@github.com>\n      \n       ## cache.h ##\n      @@ cache.h: struct cache_entry *index_file_exists(struct index_state *istate, const char *na\n     +  */\n       int index_name_pos(struct index_state *, const char *name, int namelen);\n       \n     - /*\n     -- * Determines whether an entry with the given name exists within the\n     -- * given index. The return value is 1 if an exact match is found, otherwise\n     -- * it is 0. Note that, unlike index_name_pos, this function does not expand\n     -- * the index if it is sparse. If an item exists within the full index but it\n     -- * is contained within a sparse directory (and not in the sparse index), 0 is\n     -- * returned.\n     -- */\n     --int index_entry_exists(struct index_state *, const char *name, int namelen);\n     ++/*\n      + * Like index_name_pos, returns the position of an entry of the given name in\n      + * the index if one exists, otherwise returns a negative value where the negated\n      + * value minus 1 is the position where the index entry would be inserted. Unlike\n     @@ cache.h: struct cache_entry *index_file_exists(struct index_state *istate, const\n      + * inside a sparse directory.\n      + */\n      +int index_name_pos_sparse(struct index_state *, const char *name, int namelen);\n     - \n     ++\n       /*\n     -  * Some functions return the negative complement of an insert position when a\n     +  * Determines whether an entry with the given name exists within the\n     +  * given index. The return value is 1 if an exact match is found, otherwise\n      \n       ## read-cache.c ##\n      @@ read-cache.c: int index_name_pos(struct index_state *istate, const char *name, int namelen)\n       \treturn index_name_stage_pos(istate, name, namelen, 0, EXPAND_SPARSE);\n       }\n       \n     --int index_entry_exists(struct index_state *istate, const char *name, int namelen)\n      +int index_name_pos_sparse(struct index_state *istate, const char *name, int namelen)\n     - {\n     --\treturn index_name_stage_pos(istate, name, namelen, 0, NO_EXPAND_SPARSE) >= 0;\n     ++{\n      +\treturn index_name_stage_pos(istate, name, namelen, 0, NO_EXPAND_SPARSE);\n     - }\n     - \n     - int remove_index_entry_at(struct index_state *istate, int pos)\n     ++}\n     ++\n     + int index_entry_exists(struct index_state *istate, const char *name, int namelen)\n     + {\n     + \treturn index_name_stage_pos(istate, name, namelen, 0, NO_EXPAND_SPARSE) >= 0;\n 4:  016971a6711 ! 4:  97ca668102c unpack-trees: handle missing sparse directories\n     @@ Metadata\n      Author: Victoria Dye <vdye@github.com>\n      \n       ## Commit message ##\n     -    unpack-trees: handle missing sparse directories\n     +    unpack-trees: unpack new trees as sparse directories\n      \n     -    If a sparse directory does not exist in the index, unpack it at the\n     -    directory level rather than recursing into it an unpacking its contents\n     -    file-by-file. This helps keep the sparse index as collapsed as possible in\n     -    cases such as 'git reset --hard' restoring a sparse directory.\n     +    If 'unpack_single_entry()' is unpacking a new directory tree (that is, one\n     +    not already present in the index) into a sparse index, unpack the tree as a\n     +    sparse directory rather than traversing its contents and unpacking each file\n     +    individually. This helps keep the sparse index as collapsed as possible in\n     +    cases such as 'git reset --hard' restoring a outside-of-cone directory\n     +    removed with 'git rm -r --sparse'.\n      \n     -    A directory is determined to be truly non-existent in the index (rather than\n     -    the parent of existing index entries), if 1) its path is outside the sparse\n     -    cone and 2) there are no children of the directory in the index. This check\n     -    is performed by 'missing_dir_is_sparse()' in 'unpack_single_entry()'. If the\n     -    directory is a missing sparse dir, 'unpack_single_entry()'  will proceed\n     -    with unpacking it. This determination is also propagated back up to\n     -    'unpack_callback()' via 'is_missing_sparse_dir' to prevent further tree\n     -    traversal into the unpacked directory.\n     +    Without this patch, 'unpack_single_entry()' will only unpack a directory\n     +    into the index as a sparse directory (rather than traversing into it and\n     +    unpacking its files one-by-one) if an entry with the same name already\n     +    exists in the index. This patch allows sparse directory unpacking without a\n     +    matching index entry when the following conditions are met:\n     +\n     +    1. the directory's path is outside the sparse cone, and\n     +    2. there are no children of the directory in the index\n     +\n     +    If a directory meets these requirements (as determined by\n     +    'is_new_sparse_dir()'), 'unpack_single_entry()' unpacks the sparse directory\n     +    index entry and propagates the decision back up to 'unpack_callback()' to\n     +    prevent unnecessary tree traversal into the unpacked directory.\n      \n          Reported-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>\n          Signed-off-by: Victoria Dye <vdye@github.com>\n     @@ unpack-trees.c: static struct cache_entry *create_ce_entry(const struct traverse\n       }\n       \n      +/*\n     -+ * Determine whether the path specified corresponds to a sparse directory\n     -+ * completely missing from the index. This function is assumed to only be\n     -+ * called when the named path isn't already in the index.\n     ++ * Determine whether the path specified by 'p' should be unpacked as a new\n     ++ * sparse directory in a sparse index. A new sparse directory 'A/':\n     ++ * - must be outside the sparse cone.\n     ++ * - must not already be in the index (i.e., no index entry with name 'A/'\n     ++ *   exists).\n     ++ * - must not have any child entries in the index (i.e., no index entry\n     ++ *   'A/<something>' exists).\n     ++ * If 'p' meets the above requirements, return 1; otherwise, return 0.\n      + */\n     -+static int missing_dir_is_sparse(const struct traverse_info *info,\n     -+\t\t\t\t const struct name_entry *p)\n     ++static int entry_is_new_sparse_dir(const struct traverse_info *info,\n     ++\t\t\t\t   const struct name_entry *p)\n      +{\n      +\tint res, pos;\n      +\tstruct strbuf dirpath = STRBUF_INIT;\n      +\tstruct unpack_trees_options *o = info->data;\n      +\n     ++\tif (!S_ISDIR(p->mode))\n     ++\t\treturn 0;\n     ++\n      +\t/*\n     -+\t * First, check whether the path is in the sparse cone. If it is,\n     -+\t * then this directory shouldn't be sparse.\n     ++\t * If the path is inside the sparse cone, it can't be a sparse directory.\n      +\t */\n      +\tstrbuf_add(&dirpath, info->traverse_path, info->pathlen);\n      +\tstrbuf_add(&dirpath, p->path, p->pathlen);\n     @@ unpack-trees.c: static struct cache_entry *create_ce_entry(const struct traverse\n      +\t\tgoto cleanup;\n      +\t}\n      +\n     -+\t/*\n     -+\t * Given that the directory is not inside the sparse cone, it could be\n     -+\t * (partially) expanded in the index. If child entries exist, the path\n     -+\t * is not a missing sparse directory.\n     -+\t */\n      +\tpos = index_name_pos_sparse(o->src_index, dirpath.buf, dirpath.len);\n     -+\tif (pos >= 0)\n     -+\t\tBUG(\"cache entry '%s%s' shouldn't exist in the index\",\n     -+\t\t    info->traverse_path, p->path);\n     ++\tif (pos >= 0) {\n     ++\t\t/* Path is already in the index, not a new sparse dir */\n     ++\t\tres = 0;\n     ++\t\tgoto cleanup;\n     ++\t}\n      +\n     ++\t/* Where would this sparse dir be inserted into the index? */\n      +\tpos = -pos - 1;\n      +\tif (pos >= o->src_index->cache_nr) {\n     ++\t\t/*\n     ++\t\t * Sparse dir would be inserted at the end of the index, so we\n     ++\t\t * know it has no child entries.\n     ++\t\t */\n      +\t\tres = 1;\n      +\t\tgoto cleanup;\n      +\t}\n      +\n     ++\t/*\n     ++\t * If the dir has child entries in the index, the first would be at the\n     ++\t * position the sparse directory would be inserted. If the entry at this\n     ++\t * position is inside the dir, not a new sparse dir.\n     ++\t */\n      +\tres = strncmp(o->src_index->cache[pos]->name, dirpath.buf, dirpath.len);\n      +\n      +cleanup:\n     @@ unpack-trees.c: static int unpack_single_entry(int n, unsigned long mask,\n       \t\t\t       const struct name_entry *names,\n      -\t\t\t       const struct traverse_info *info)\n      +\t\t\t       const struct traverse_info *info,\n     -+\t\t\t       int *is_missing_sparse_dir)\n     ++\t\t\t       int *is_new_sparse_dir)\n       {\n       \tint i;\n       \tstruct unpack_trees_options *o = info->data;\n     @@ unpack-trees.c: static int unpack_single_entry(int n, unsigned long mask,\n       \n      -\tif (mask == dirmask && !src[0])\n      -\t\treturn 0;\n     -+\t*is_missing_sparse_dir = 0;\n     ++\t*is_new_sparse_dir = 0;\n      +\tif (mask == dirmask && !src[0]) {\n      +\t\t/*\n     -+\t\t * If the directory is completely missing from the index but\n     -+\t\t * would otherwise be a sparse directory, we should unpack it.\n     -+\t\t * If not, we'll return and continue recursively traversing the\n     -+\t\t * tree.\n     ++\t\t * If we're not in a sparse index, we can't unpack a directory\n     ++\t\t * without recursing into it, so we return.\n      +\t\t */\n      +\t\tif (!o->src_index->sparse_index)\n      +\t\t\treturn 0;\n     @@ unpack-trees.c: static int unpack_single_entry(int n, unsigned long mask,\n      +\t\twhile (!p->mode)\n      +\t\t\tp++;\n      +\n     -+\t\t*is_missing_sparse_dir = missing_dir_is_sparse(info, p);\n     -+\t\tif (!*is_missing_sparse_dir)\n     ++\t\t/*\n     ++\t\t * If the directory is completely missing from the index but\n     ++\t\t * would otherwise be a sparse directory, we should unpack it.\n     ++\t\t * If not, we'll return and continue recursively traversing the\n     ++\t\t * tree.\n     ++\t\t */\n     ++\t\t*is_new_sparse_dir = entry_is_new_sparse_dir(info, p);\n     ++\t\tif (!*is_new_sparse_dir)\n      +\t\t\treturn 0;\n      +\t}\n       \n     @@ unpack-trees.c: static int unpack_single_entry(int n, unsigned long mask,\n      -\tif (mask == dirmask && src[0] &&\n      -\t    S_ISSPARSEDIR(src[0]->ce_mode))\n      +\tif (mask == dirmask &&\n     -+\t    (*is_missing_sparse_dir || (src[0] && S_ISSPARSEDIR(src[0]->ce_mode))))\n     ++\t    (*is_new_sparse_dir || (src[0] && S_ISSPARSEDIR(src[0]->ce_mode))))\n       \t\tconflicts = 0;\n       \n       \t/*\n     @@ unpack-trees.c: static int unpack_sparse_callback(int n, unsigned long mask, uns\n       \tstruct cache_entry *src[MAX_UNPACK_TREES + 1] = { NULL, };\n       \tstruct unpack_trees_options *o = info->data;\n      -\tint ret;\n     -+\tint ret, is_missing_sparse_dir;\n     ++\tint ret, is_new_sparse_dir;\n       \n       \tassert(o->merge);\n       \n     @@ unpack-trees.c: static int unpack_sparse_callback(int n, unsigned long mask, uns\n       \t * 'dirmask' accordingly.\n       \t */\n      -\tret = unpack_single_entry(n - 1, mask >> 1, dirmask >> 1, src, names + 1, info);\n     -+\tret = unpack_single_entry(n - 1, mask >> 1, dirmask >> 1, src, names + 1, info, &is_missing_sparse_dir);\n     ++\tret = unpack_single_entry(n - 1, mask >> 1, dirmask >> 1, src, names + 1, info, &is_new_sparse_dir);\n       \n       \tif (src[0])\n       \t\tdiscard_cache_entry(src[0]);\n     @@ unpack-trees.c: static int unpack_callback(int n, unsigned long mask, unsigned l\n       \tstruct cache_entry *src[MAX_UNPACK_TREES + 1] = { NULL, };\n       \tstruct unpack_trees_options *o = info->data;\n       \tconst struct name_entry *p = names;\n     -+\tint is_missing_sparse_dir;\n     ++\tint is_new_sparse_dir;\n       \n       \t/* Find first entry with a real name (we could use \"mask\" too) */\n       \twhile (!p->mode)\n     @@ unpack-trees.c: static int unpack_callback(int n, unsigned long mask, unsigned l\n       \t}\n       \n      -\tif (unpack_single_entry(n, mask, dirmask, src, names, info) < 0)\n     -+\tif (unpack_single_entry(n, mask, dirmask, src, names, info, &is_missing_sparse_dir))\n     ++\tif (unpack_single_entry(n, mask, dirmask, src, names, info, &is_new_sparse_dir))\n       \t\treturn -1;\n       \n       \tif (o->merge && src[0]) {\n     @@ unpack-trees.c: static int unpack_callback(int n, unsigned long mask, unsigned l\n       \t\t}\n       \n       \t\tif (!is_sparse_directory_entry(src[0], names, info) &&\n     -+\t\t    !is_missing_sparse_dir &&\n     ++\t\t    !is_new_sparse_dir &&\n       \t\t    traverse_trees_recursive(n, dirmask, mask & ~dirmask,\n       \t\t\t\t\t\t    names, info) < 0) {\n       \t\t\treturn -1;\n\n-- \ngitgitgadget\n"},{"id":"460773","messageId":"55c77ba4b291fffe3031fbef67dcee9aa91a105c.1659841030.git.gitgitgadget@gmail.com","threadId":"58267","inReplyTo":"pull.1312.v2.git.1659841030.gitgitgadget@gmail.com","subject":"[PATCH v2 2/4] oneway_diff: handle removed sparse directories","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-07T02:57:07Z","receivedAt":"2022-08-07T02:57:23Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye <vdye@github.com>\n\nUpdate 'do_oneway_diff()' to perform a 'diff_tree_oid()' on removed sparse\ndirectories, as it does for added or modified sparse directories (see\n9eb00af562 (diff-lib: handle index diffs with sparse dirs, 2021-07-14)).\n\nAt the moment, this update is unreachable code because 'unpack_trees()'\n(currently the only way 'oneway_diff()' can be called, via 'diff_cache()')\nwill always traverse trees down to the individual removed files of a deleted\nsparse directory. A subsequent patch will change this to better preserve a\nsparse index in other uses of 'unpack_tree()', e.g. 'git reset --hard'.\nHowever, making that change without this patch would result in (among other\nissues) 'git status' printing only the name of a deleted sparse directory,\nnot its contents. To avoid introducing that bug, 'do_oneway_diff()' is\nupdated before modifying 'unpack_trees()'.\n\nSigned-off-by: Victoria Dye <vdye@github.com>\n---\n diff-lib.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 7eb66a417aa..2edea41a234 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -466,6 +466,11 @@ static void do_oneway_diff(struct unpack_trees_options *o,\n \t * Something removed from the tree?\n \t */\n \tif (!idx) {\n+\t\tif (S_ISSPARSEDIR(tree->ce_mode)) {\n+\t\t\tdiff_tree_oid(&tree->oid, NULL, tree->name, &revs->diffopt);\n+\t\t\treturn;\n+\t\t}\n+\n \t\tdiff_index_show_file(revs, \"-\", tree, &tree->oid, 1,\n \t\t\t\t     tree->ce_mode, 0);\n \t\treturn;\n-- \ngitgitgadget\n\n"},{"id":"460774","messageId":"255318f4dc61daef3e892a4dd5c571463985f340.1659841030.git.gitgitgadget@gmail.com","threadId":"58267","inReplyTo":"pull.1312.v2.git.1659841030.gitgitgadget@gmail.com","subject":"[PATCH v2 1/4] checkout: fix nested sparse directory diff in sparse index","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-07T02:57:06Z","receivedAt":"2022-08-07T02:57:23Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye <vdye@github.com>\n\nAdd the 'recursive' diff flag to the local changes reporting done by 'git\ncheckout' in 'show_local_changes()'. Without the flag enabled, unexpanded\nsparse directories will not be recursed into to report the diff of each\nfile's contents, resulting in the reported local changes including\n\"modified\" sparse directories.\n\nThe same issue was found and fixed for 'git status' in 2c521b0e49 (status:\nfix nested sparse directory diff in sparse index, 2022-03-01)\n\nSigned-off-by: Victoria Dye <vdye@github.com>\n---\n builtin/checkout.c                       | 1 +\n t/t1092-sparse-checkout-compatibility.sh | 8 ++++++++\n 2 files changed, 9 insertions(+)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 29c74f898bf..f9d63d80b92 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -626,6 +626,7 @@ static void show_local_changes(struct object *head,\n \trepo_init_revisions(the_repository, &rev, NULL);\n \trev.diffopt.flags = opts->flags;\n \trev.diffopt.output_format |= DIFF_FORMAT_NAME_STATUS;\n+\trev.diffopt.flags.recursive = 1;\n \tdiff_setup_done(&rev.diffopt);\n \tadd_pending_object(&rev, head, NULL);\n \trun_diff_index(&rev, 0);\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 763c6cc6846..99a1425a929 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -372,6 +372,14 @@ test_expect_success 'deep changes during checkout' '\n \ttest_all_match git checkout base\n '\n \n+test_expect_success 'checkout with modified sparse directory' '\n+\tinit_repos &&\n+\n+\ttest_all_match git checkout rename-in-to-out -- . &&\n+\ttest_sparse_match git sparse-checkout reapply &&\n+\ttest_all_match git checkout base\n+'\n+\n test_expect_success 'add outside sparse cone' '\n \tinit_repos &&\n \n-- \ngitgitgadget\n\n"},{"id":"460775","messageId":"d0bdec63286e320a12cca3f0bb8a780ae451caf2.1659841030.git.gitgitgadget@gmail.com","threadId":"58267","inReplyTo":"pull.1312.v2.git.1659841030.gitgitgadget@gmail.com","subject":"[PATCH v2 3/4] cache.h: create 'index_name_pos_sparse()'","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-07T02:57:08Z","receivedAt":"2022-08-07T02:57:31Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye <vdye@github.com>\n\nAdd 'index_name_pos_sparse()', which behaves the same as 'index_name_pos()',\nexcept that it does not expand a sparse index to search for an entry inside\na sparse directory.\n\n'index_entry_exists()' was originally implemented in 20ec2d034c (reset: make\nsparse-aware (except --mixed), 2021-11-29) as an alternative to\n'index_name_pos()' to allow callers to search for an index entry without\nexpanding a sparse index. However, that particular use case only required\nknowing whether the requested entry existed, so 'index_entry_exists()' does\nnot return the index positioning information provided by 'index_name_pos()'.\n\nThis patch implements 'index_name_pos_sparse()' to accommodate callers that\nneed the positioning information of 'index_name_pos()', but do not want to\nexpand the index.\n\nSigned-off-by: Victoria Dye <vdye@github.com>\n---\n cache.h      | 9 +++++++++\n read-cache.c | 5 +++++\n 2 files changed, 14 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex 4aa1bd079d5..4fa3be26393 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -830,6 +830,15 @@ struct cache_entry *index_file_exists(struct index_state *istate, const char *na\n  */\n int index_name_pos(struct index_state *, const char *name, int namelen);\n \n+/*\n+ * Like index_name_pos, returns the position of an entry of the given name in\n+ * the index if one exists, otherwise returns a negative value where the negated\n+ * value minus 1 is the position where the index entry would be inserted. Unlike\n+ * index_name_pos, however, a sparse index is not expanded to find an entry\n+ * inside a sparse directory.\n+ */\n+int index_name_pos_sparse(struct index_state *, const char *name, int namelen);\n+\n /*\n  * Determines whether an entry with the given name exists within the\n  * given index. The return value is 1 if an exact match is found, otherwise\ndiff --git a/read-cache.c b/read-cache.c\nindex 4de207752dc..b09128b1884 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -620,6 +620,11 @@ int index_name_pos(struct index_state *istate, const char *name, int namelen)\n \treturn index_name_stage_pos(istate, name, namelen, 0, EXPAND_SPARSE);\n }\n \n+int index_name_pos_sparse(struct index_state *istate, const char *name, int namelen)\n+{\n+\treturn index_name_stage_pos(istate, name, namelen, 0, NO_EXPAND_SPARSE);\n+}\n+\n int index_entry_exists(struct index_state *istate, const char *name, int namelen)\n {\n \treturn index_name_stage_pos(istate, name, namelen, 0, NO_EXPAND_SPARSE) >= 0;\n-- \ngitgitgadget\n\n"},{"id":"460776","messageId":"97ca668102cdade9501996af4d31e38dc7420a29.1659841030.git.gitgitgadget@gmail.com","threadId":"58267","inReplyTo":"pull.1312.v2.git.1659841030.gitgitgadget@gmail.com","subject":"[PATCH v2 4/4] unpack-trees: unpack new trees as sparse directories","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-07T02:57:09Z","receivedAt":"2022-08-07T02:57:33Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye <vdye@github.com>\n\nIf 'unpack_single_entry()' is unpacking a new directory tree (that is, one\nnot already present in the index) into a sparse index, unpack the tree as a\nsparse directory rather than traversing its contents and unpacking each file\nindividually. This helps keep the sparse index as collapsed as possible in\ncases such as 'git reset --hard' restoring a outside-of-cone directory\nremoved with 'git rm -r --sparse'.\n\nWithout this patch, 'unpack_single_entry()' will only unpack a directory\ninto the index as a sparse directory (rather than traversing into it and\nunpacking its files one-by-one) if an entry with the same name already\nexists in the index. This patch allows sparse directory unpacking without a\nmatching index entry when the following conditions are met:\n\n1. the directory's path is outside the sparse cone, and\n2. there are no children of the directory in the index\n\nIf a directory meets these requirements (as determined by\n'is_new_sparse_dir()'), 'unpack_single_entry()' unpacks the sparse directory\nindex entry and propagates the decision back up to 'unpack_callback()' to\nprevent unnecessary tree traversal into the unpacked directory.\n\nReported-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>\nSigned-off-by: Victoria Dye <vdye@github.com>\n---\n t/t1092-sparse-checkout-compatibility.sh |  17 ++++\n unpack-trees.c                           | 106 ++++++++++++++++++++---\n 2 files changed, 113 insertions(+), 10 deletions(-)\n\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 99a1425a929..eb5edebfa5d 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -695,6 +695,23 @@ test_expect_success 'reset with wildcard pathspec' '\n \ttest_all_match git ls-files -s -- folder1\n '\n \n+test_expect_success 'reset hard with removed sparse dir' '\n+\tinit_repos &&\n+\n+\ttest_all_match git rm -r --sparse folder1 &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\ttest_all_match git reset --hard &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\tcat >expect <<-\\EOF &&\n+\tfolder1/\n+\tEOF\n+\n+\tgit -C sparse-index ls-files --sparse folder1 >out &&\n+\ttest_cmp expect out\n+'\n+\n test_expect_success 'update-index modify outside sparse definition' '\n \tinit_repos &&\n \ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 8a454e03bff..90b92114be8 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1069,6 +1069,67 @@ static struct cache_entry *create_ce_entry(const struct traverse_info *info,\n \treturn ce;\n }\n \n+/*\n+ * Determine whether the path specified by 'p' should be unpacked as a new\n+ * sparse directory in a sparse index. A new sparse directory 'A/':\n+ * - must be outside the sparse cone.\n+ * - must not already be in the index (i.e., no index entry with name 'A/'\n+ *   exists).\n+ * - must not have any child entries in the index (i.e., no index entry\n+ *   'A/<something>' exists).\n+ * If 'p' meets the above requirements, return 1; otherwise, return 0.\n+ */\n+static int entry_is_new_sparse_dir(const struct traverse_info *info,\n+\t\t\t\t   const struct name_entry *p)\n+{\n+\tint res, pos;\n+\tstruct strbuf dirpath = STRBUF_INIT;\n+\tstruct unpack_trees_options *o = info->data;\n+\n+\tif (!S_ISDIR(p->mode))\n+\t\treturn 0;\n+\n+\t/*\n+\t * If the path is inside the sparse cone, it can't be a sparse directory.\n+\t */\n+\tstrbuf_add(&dirpath, info->traverse_path, info->pathlen);\n+\tstrbuf_add(&dirpath, p->path, p->pathlen);\n+\tstrbuf_addch(&dirpath, '/');\n+\tif (path_in_cone_mode_sparse_checkout(dirpath.buf, o->src_index)) {\n+\t\tres = 0;\n+\t\tgoto cleanup;\n+\t}\n+\n+\tpos = index_name_pos_sparse(o->src_index, dirpath.buf, dirpath.len);\n+\tif (pos >= 0) {\n+\t\t/* Path is already in the index, not a new sparse dir */\n+\t\tres = 0;\n+\t\tgoto cleanup;\n+\t}\n+\n+\t/* Where would this sparse dir be inserted into the index? */\n+\tpos = -pos - 1;\n+\tif (pos >= o->src_index->cache_nr) {\n+\t\t/*\n+\t\t * Sparse dir would be inserted at the end of the index, so we\n+\t\t * know it has no child entries.\n+\t\t */\n+\t\tres = 1;\n+\t\tgoto cleanup;\n+\t}\n+\n+\t/*\n+\t * If the dir has child entries in the index, the first would be at the\n+\t * position the sparse directory would be inserted. If the entry at this\n+\t * position is inside the dir, not a new sparse dir.\n+\t */\n+\tres = strncmp(o->src_index->cache[pos]->name, dirpath.buf, dirpath.len);\n+\n+cleanup:\n+\tstrbuf_release(&dirpath);\n+\treturn res;\n+}\n+\n /*\n  * Note that traverse_by_cache_tree() duplicates some logic in this function\n  * without actually calling it. If you change the logic here you may need to\n@@ -1078,21 +1139,44 @@ static int unpack_single_entry(int n, unsigned long mask,\n \t\t\t       unsigned long dirmask,\n \t\t\t       struct cache_entry **src,\n \t\t\t       const struct name_entry *names,\n-\t\t\t       const struct traverse_info *info)\n+\t\t\t       const struct traverse_info *info,\n+\t\t\t       int *is_new_sparse_dir)\n {\n \tint i;\n \tstruct unpack_trees_options *o = info->data;\n \tunsigned long conflicts = info->df_conflicts | dirmask;\n+\tconst struct name_entry *p = names;\n \n-\tif (mask == dirmask && !src[0])\n-\t\treturn 0;\n+\t*is_new_sparse_dir = 0;\n+\tif (mask == dirmask && !src[0]) {\n+\t\t/*\n+\t\t * If we're not in a sparse index, we can't unpack a directory\n+\t\t * without recursing into it, so we return.\n+\t\t */\n+\t\tif (!o->src_index->sparse_index)\n+\t\t\treturn 0;\n+\n+\t\t/* Find first entry with a real name (we could use \"mask\" too) */\n+\t\twhile (!p->mode)\n+\t\t\tp++;\n+\n+\t\t/*\n+\t\t * If the directory is completely missing from the index but\n+\t\t * would otherwise be a sparse directory, we should unpack it.\n+\t\t * If not, we'll return and continue recursively traversing the\n+\t\t * tree.\n+\t\t */\n+\t\t*is_new_sparse_dir = entry_is_new_sparse_dir(info, p);\n+\t\tif (!*is_new_sparse_dir)\n+\t\t\treturn 0;\n+\t}\n \n \t/*\n-\t * When we have a sparse directory entry for src[0],\n-\t * then this isn't necessarily a directory-file conflict.\n+\t * When we are unpacking a sparse directory, then this isn't necessarily\n+\t * a directory-file conflict.\n \t */\n-\tif (mask == dirmask && src[0] &&\n-\t    S_ISSPARSEDIR(src[0]->ce_mode))\n+\tif (mask == dirmask &&\n+\t    (*is_new_sparse_dir || (src[0] && S_ISSPARSEDIR(src[0]->ce_mode))))\n \t\tconflicts = 0;\n \n \t/*\n@@ -1352,7 +1436,7 @@ static int unpack_sparse_callback(int n, unsigned long mask, unsigned long dirma\n {\n \tstruct cache_entry *src[MAX_UNPACK_TREES + 1] = { NULL, };\n \tstruct unpack_trees_options *o = info->data;\n-\tint ret;\n+\tint ret, is_new_sparse_dir;\n \n \tassert(o->merge);\n \n@@ -1376,7 +1460,7 @@ static int unpack_sparse_callback(int n, unsigned long mask, unsigned long dirma\n \t * \"index\" tree (i.e., names[0]) and adjust 'names', 'n', 'mask', and\n \t * 'dirmask' accordingly.\n \t */\n-\tret = unpack_single_entry(n - 1, mask >> 1, dirmask >> 1, src, names + 1, info);\n+\tret = unpack_single_entry(n - 1, mask >> 1, dirmask >> 1, src, names + 1, info, &is_new_sparse_dir);\n \n \tif (src[0])\n \t\tdiscard_cache_entry(src[0]);\n@@ -1394,6 +1478,7 @@ static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, str\n \tstruct cache_entry *src[MAX_UNPACK_TREES + 1] = { NULL, };\n \tstruct unpack_trees_options *o = info->data;\n \tconst struct name_entry *p = names;\n+\tint is_new_sparse_dir;\n \n \t/* Find first entry with a real name (we could use \"mask\" too) */\n \twhile (!p->mode)\n@@ -1440,7 +1525,7 @@ static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, str\n \t\t}\n \t}\n \n-\tif (unpack_single_entry(n, mask, dirmask, src, names, info) < 0)\n+\tif (unpack_single_entry(n, mask, dirmask, src, names, info, &is_new_sparse_dir))\n \t\treturn -1;\n \n \tif (o->merge && src[0]) {\n@@ -1478,6 +1563,7 @@ static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, str\n \t\t}\n \n \t\tif (!is_sparse_directory_entry(src[0], names, info) &&\n+\t\t    !is_new_sparse_dir &&\n \t\t    traverse_trees_recursive(n, dirmask, mask & ~dirmask,\n \t\t\t\t\t\t    names, info) < 0) {\n \t\t\treturn -1;\n-- \ngitgitgadget\n"},{"id":"460846","messageId":"pull.1312.v3.git.1659985672.gitgitgadget@gmail.com","threadId":"58267","inReplyTo":"pull.1312.v2.git.1659841030.gitgitgadget@gmail.com","subject":"[PATCH v3 0/4] reset/checkout: fix miscellaneous sparse index bugs","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-08T19:07:48Z","receivedAt":"2022-08-08T19:07:59Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"While working on sparse index integration for 'git rm' [1], Shaoxuan found\nthat removed sparse directories, when reset, would no longer be sparse. This\nwas due to how 'unpack_trees()' determined whether a traversed directory was\na sparse directory or not; it would only unpack an entry as a sparse\ndirectory if it existed in the index. However, if the sparse directory was\nremoved, it would be treated like a non-sparse directory and its contents\nwould be individually unpacked.\n\nTo avoid this unnecessary traversal and keep the results of 'reset' as\nsparse as possible, the decision logic for whether a directory is sparse is\nchanged to:\n\n * If the directory is a sparse directory in the index, unpack it.\n * If not, is the directory inside the sparse cone? If so, do not unpack it.\n * If the directory is outside the sparse cone, does it have any child\n   entries in the index? If so, do not unpack it.\n * Otherwise, unpack the entry as a sparse directory.\n\nIn the process of updating 'reset', a separate issue was found in 'checkout'\nwhere collapsed sparse directories did not have modified contents reported\nfile-by-file. A similar bug was found with 'status' in 2c521b0e49 (status:\nfix nested sparse directory diff in sparse index, 2022-03-01), and\n'checkout' was corrected the same way (setting the diff flag 'recursive' to\n1).\n\n\nChanges since V2\n================\n\n * Adjusted 'reset hard with removed sparse dir' test in\n   't1092-sparse-checkout-compatibility.sh' to avoid 'git rm' log message\n   conflicts with [1]\n\n\nChanges since V1\n================\n\n * Reverted the removal of 'index_entry_exists()' to avoid breaking other\n   in-flight series.\n * Renamed 'is_missing_sparse_dir()' to 'is_new_sparse_dir()'; revised\n   comments and commit messages to clarify what that function is doing and\n   why.\n * Handled \"unexpected\" inputs to 'is_new_sparse_dir()' more gently,\n   returning 0 if 'p' is not a directory or the directory already exists in\n   the index (rather than exiting with 'BUG()'). This is intended to make\n   'is_new_sparse_dir()' less reliant on information about the index\n   established by 'unpack_callback()' & 'unpack_single_entry()', resulting\n   in easier-to-read and more reusable code.\n\nThanks!\n\n * Victoria\n\n[1]\nhttps://lore.kernel.org/git/20220803045118.1243087-1-shaoxuan.yuan02@gmail.com/\n\nVictoria Dye (4):\n  checkout: fix nested sparse directory diff in sparse index\n  oneway_diff: handle removed sparse directories\n  cache.h: create 'index_name_pos_sparse()'\n  unpack-trees: unpack new trees as sparse directories\n\n builtin/checkout.c                       |   1 +\n cache.h                                  |   9 ++\n diff-lib.c                               |   5 ++\n read-cache.c                             |   5 ++\n t/t1092-sparse-checkout-compatibility.sh |  25 ++++++\n unpack-trees.c                           | 106 ++++++++++++++++++++---\n 6 files changed, 141 insertions(+), 10 deletions(-)\n\n\nbase-commit: 4af7188bc97f70277d0f10d56d5373022b1fa385\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1312%2Fvdye%2Freset%2Fhandle-missing-dirs-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1312/vdye/reset/handle-missing-dirs-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1312\n\nRange-diff vs v2:\n\n 1:  255318f4dc6 = 1:  255318f4dc6 checkout: fix nested sparse directory diff in sparse index\n 2:  55c77ba4b29 = 2:  55c77ba4b29 oneway_diff: handle removed sparse directories\n 3:  d0bdec63286 = 3:  d0bdec63286 cache.h: create 'index_name_pos_sparse()'\n 4:  97ca668102c ! 4:  010d5e51595 unpack-trees: unpack new trees as sparse directories\n     @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'reset with wildca\n      +test_expect_success 'reset hard with removed sparse dir' '\n      +\tinit_repos &&\n      +\n     -+\ttest_all_match git rm -r --sparse folder1 &&\n     ++\trun_on_all git rm -r --sparse folder1 &&\n      +\ttest_all_match git status --porcelain=v2 &&\n      +\n      +\ttest_all_match git reset --hard &&\n\n-- \ngitgitgadget\n"},{"id":"460847","messageId":"255318f4dc61daef3e892a4dd5c571463985f340.1659985672.git.gitgitgadget@gmail.com","threadId":"58267","inReplyTo":"pull.1312.v3.git.1659985672.gitgitgadget@gmail.com","subject":"[PATCH v3 1/4] checkout: fix nested sparse directory diff in sparse index","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-08T19:07:49Z","receivedAt":"2022-08-08T19:08:00Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye <vdye@github.com>\n\nAdd the 'recursive' diff flag to the local changes reporting done by 'git\ncheckout' in 'show_local_changes()'. Without the flag enabled, unexpanded\nsparse directories will not be recursed into to report the diff of each\nfile's contents, resulting in the reported local changes including\n\"modified\" sparse directories.\n\nThe same issue was found and fixed for 'git status' in 2c521b0e49 (status:\nfix nested sparse directory diff in sparse index, 2022-03-01)\n\nSigned-off-by: Victoria Dye <vdye@github.com>\n---\n builtin/checkout.c                       | 1 +\n t/t1092-sparse-checkout-compatibility.sh | 8 ++++++++\n 2 files changed, 9 insertions(+)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 29c74f898bf..f9d63d80b92 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -626,6 +626,7 @@ static void show_local_changes(struct object *head,\n \trepo_init_revisions(the_repository, &rev, NULL);\n \trev.diffopt.flags = opts->flags;\n \trev.diffopt.output_format |= DIFF_FORMAT_NAME_STATUS;\n+\trev.diffopt.flags.recursive = 1;\n \tdiff_setup_done(&rev.diffopt);\n \tadd_pending_object(&rev, head, NULL);\n \trun_diff_index(&rev, 0);\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 763c6cc6846..99a1425a929 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -372,6 +372,14 @@ test_expect_success 'deep changes during checkout' '\n \ttest_all_match git checkout base\n '\n \n+test_expect_success 'checkout with modified sparse directory' '\n+\tinit_repos &&\n+\n+\ttest_all_match git checkout rename-in-to-out -- . &&\n+\ttest_sparse_match git sparse-checkout reapply &&\n+\ttest_all_match git checkout base\n+'\n+\n test_expect_success 'add outside sparse cone' '\n \tinit_repos &&\n \n-- \ngitgitgadget\n\n"},{"id":"460848","messageId":"55c77ba4b291fffe3031fbef67dcee9aa91a105c.1659985672.git.gitgitgadget@gmail.com","threadId":"58267","inReplyTo":"pull.1312.v3.git.1659985672.gitgitgadget@gmail.com","subject":"[PATCH v3 2/4] oneway_diff: handle removed sparse directories","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-08T19:07:50Z","receivedAt":"2022-08-08T19:08:06Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye <vdye@github.com>\n\nUpdate 'do_oneway_diff()' to perform a 'diff_tree_oid()' on removed sparse\ndirectories, as it does for added or modified sparse directories (see\n9eb00af562 (diff-lib: handle index diffs with sparse dirs, 2021-07-14)).\n\nAt the moment, this update is unreachable code because 'unpack_trees()'\n(currently the only way 'oneway_diff()' can be called, via 'diff_cache()')\nwill always traverse trees down to the individual removed files of a deleted\nsparse directory. A subsequent patch will change this to better preserve a\nsparse index in other uses of 'unpack_tree()', e.g. 'git reset --hard'.\nHowever, making that change without this patch would result in (among other\nissues) 'git status' printing only the name of a deleted sparse directory,\nnot its contents. To avoid introducing that bug, 'do_oneway_diff()' is\nupdated before modifying 'unpack_trees()'.\n\nSigned-off-by: Victoria Dye <vdye@github.com>\n---\n diff-lib.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 7eb66a417aa..2edea41a234 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -466,6 +466,11 @@ static void do_oneway_diff(struct unpack_trees_options *o,\n \t * Something removed from the tree?\n \t */\n \tif (!idx) {\n+\t\tif (S_ISSPARSEDIR(tree->ce_mode)) {\n+\t\t\tdiff_tree_oid(&tree->oid, NULL, tree->name, &revs->diffopt);\n+\t\t\treturn;\n+\t\t}\n+\n \t\tdiff_index_show_file(revs, \"-\", tree, &tree->oid, 1,\n \t\t\t\t     tree->ce_mode, 0);\n \t\treturn;\n-- \ngitgitgadget\n\n"},{"id":"460849","messageId":"d0bdec63286e320a12cca3f0bb8a780ae451caf2.1659985672.git.gitgitgadget@gmail.com","threadId":"58267","inReplyTo":"pull.1312.v3.git.1659985672.gitgitgadget@gmail.com","subject":"[PATCH v3 3/4] cache.h: create 'index_name_pos_sparse()'","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-08T19:07:51Z","receivedAt":"2022-08-08T19:08:14Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye <vdye@github.com>\n\nAdd 'index_name_pos_sparse()', which behaves the same as 'index_name_pos()',\nexcept that it does not expand a sparse index to search for an entry inside\na sparse directory.\n\n'index_entry_exists()' was originally implemented in 20ec2d034c (reset: make\nsparse-aware (except --mixed), 2021-11-29) as an alternative to\n'index_name_pos()' to allow callers to search for an index entry without\nexpanding a sparse index. However, that particular use case only required\nknowing whether the requested entry existed, so 'index_entry_exists()' does\nnot return the index positioning information provided by 'index_name_pos()'.\n\nThis patch implements 'index_name_pos_sparse()' to accommodate callers that\nneed the positioning information of 'index_name_pos()', but do not want to\nexpand the index.\n\nSigned-off-by: Victoria Dye <vdye@github.com>\n---\n cache.h      | 9 +++++++++\n read-cache.c | 5 +++++\n 2 files changed, 14 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex 4aa1bd079d5..4fa3be26393 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -830,6 +830,15 @@ struct cache_entry *index_file_exists(struct index_state *istate, const char *na\n  */\n int index_name_pos(struct index_state *, const char *name, int namelen);\n \n+/*\n+ * Like index_name_pos, returns the position of an entry of the given name in\n+ * the index if one exists, otherwise returns a negative value where the negated\n+ * value minus 1 is the position where the index entry would be inserted. Unlike\n+ * index_name_pos, however, a sparse index is not expanded to find an entry\n+ * inside a sparse directory.\n+ */\n+int index_name_pos_sparse(struct index_state *, const char *name, int namelen);\n+\n /*\n  * Determines whether an entry with the given name exists within the\n  * given index. The return value is 1 if an exact match is found, otherwise\ndiff --git a/read-cache.c b/read-cache.c\nindex 4de207752dc..b09128b1884 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -620,6 +620,11 @@ int index_name_pos(struct index_state *istate, const char *name, int namelen)\n \treturn index_name_stage_pos(istate, name, namelen, 0, EXPAND_SPARSE);\n }\n \n+int index_name_pos_sparse(struct index_state *istate, const char *name, int namelen)\n+{\n+\treturn index_name_stage_pos(istate, name, namelen, 0, NO_EXPAND_SPARSE);\n+}\n+\n int index_entry_exists(struct index_state *istate, const char *name, int namelen)\n {\n \treturn index_name_stage_pos(istate, name, namelen, 0, NO_EXPAND_SPARSE) >= 0;\n-- \ngitgitgadget\n\n"},{"id":"460850","messageId":"010d5e51595bd3f77b6d3606298a2b436c66ecc6.1659985672.git.gitgitgadget@gmail.com","threadId":"58267","inReplyTo":"pull.1312.v3.git.1659985672.gitgitgadget@gmail.com","subject":"[PATCH v3 4/4] unpack-trees: unpack new trees as sparse directories","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-08T19:07:52Z","receivedAt":"2022-08-08T19:08:15Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye <vdye@github.com>\n\nIf 'unpack_single_entry()' is unpacking a new directory tree (that is, one\nnot already present in the index) into a sparse index, unpack the tree as a\nsparse directory rather than traversing its contents and unpacking each file\nindividually. This helps keep the sparse index as collapsed as possible in\ncases such as 'git reset --hard' restoring a outside-of-cone directory\nremoved with 'git rm -r --sparse'.\n\nWithout this patch, 'unpack_single_entry()' will only unpack a directory\ninto the index as a sparse directory (rather than traversing into it and\nunpacking its files one-by-one) if an entry with the same name already\nexists in the index. This patch allows sparse directory unpacking without a\nmatching index entry when the following conditions are met:\n\n1. the directory's path is outside the sparse cone, and\n2. there are no children of the directory in the index\n\nIf a directory meets these requirements (as determined by\n'is_new_sparse_dir()'), 'unpack_single_entry()' unpacks the sparse directory\nindex entry and propagates the decision back up to 'unpack_callback()' to\nprevent unnecessary tree traversal into the unpacked directory.\n\nReported-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>\nSigned-off-by: Victoria Dye <vdye@github.com>\n---\n t/t1092-sparse-checkout-compatibility.sh |  17 ++++\n unpack-trees.c                           | 106 ++++++++++++++++++++---\n 2 files changed, 113 insertions(+), 10 deletions(-)\n\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 99a1425a929..3588dd7b102 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -695,6 +695,23 @@ test_expect_success 'reset with wildcard pathspec' '\n \ttest_all_match git ls-files -s -- folder1\n '\n \n+test_expect_success 'reset hard with removed sparse dir' '\n+\tinit_repos &&\n+\n+\trun_on_all git rm -r --sparse folder1 &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\ttest_all_match git reset --hard &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\tcat >expect <<-\\EOF &&\n+\tfolder1/\n+\tEOF\n+\n+\tgit -C sparse-index ls-files --sparse folder1 >out &&\n+\ttest_cmp expect out\n+'\n+\n test_expect_success 'update-index modify outside sparse definition' '\n \tinit_repos &&\n \ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 8a454e03bff..90b92114be8 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1069,6 +1069,67 @@ static struct cache_entry *create_ce_entry(const struct traverse_info *info,\n \treturn ce;\n }\n \n+/*\n+ * Determine whether the path specified by 'p' should be unpacked as a new\n+ * sparse directory in a sparse index. A new sparse directory 'A/':\n+ * - must be outside the sparse cone.\n+ * - must not already be in the index (i.e., no index entry with name 'A/'\n+ *   exists).\n+ * - must not have any child entries in the index (i.e., no index entry\n+ *   'A/<something>' exists).\n+ * If 'p' meets the above requirements, return 1; otherwise, return 0.\n+ */\n+static int entry_is_new_sparse_dir(const struct traverse_info *info,\n+\t\t\t\t   const struct name_entry *p)\n+{\n+\tint res, pos;\n+\tstruct strbuf dirpath = STRBUF_INIT;\n+\tstruct unpack_trees_options *o = info->data;\n+\n+\tif (!S_ISDIR(p->mode))\n+\t\treturn 0;\n+\n+\t/*\n+\t * If the path is inside the sparse cone, it can't be a sparse directory.\n+\t */\n+\tstrbuf_add(&dirpath, info->traverse_path, info->pathlen);\n+\tstrbuf_add(&dirpath, p->path, p->pathlen);\n+\tstrbuf_addch(&dirpath, '/');\n+\tif (path_in_cone_mode_sparse_checkout(dirpath.buf, o->src_index)) {\n+\t\tres = 0;\n+\t\tgoto cleanup;\n+\t}\n+\n+\tpos = index_name_pos_sparse(o->src_index, dirpath.buf, dirpath.len);\n+\tif (pos >= 0) {\n+\t\t/* Path is already in the index, not a new sparse dir */\n+\t\tres = 0;\n+\t\tgoto cleanup;\n+\t}\n+\n+\t/* Where would this sparse dir be inserted into the index? */\n+\tpos = -pos - 1;\n+\tif (pos >= o->src_index->cache_nr) {\n+\t\t/*\n+\t\t * Sparse dir would be inserted at the end of the index, so we\n+\t\t * know it has no child entries.\n+\t\t */\n+\t\tres = 1;\n+\t\tgoto cleanup;\n+\t}\n+\n+\t/*\n+\t * If the dir has child entries in the index, the first would be at the\n+\t * position the sparse directory would be inserted. If the entry at this\n+\t * position is inside the dir, not a new sparse dir.\n+\t */\n+\tres = strncmp(o->src_index->cache[pos]->name, dirpath.buf, dirpath.len);\n+\n+cleanup:\n+\tstrbuf_release(&dirpath);\n+\treturn res;\n+}\n+\n /*\n  * Note that traverse_by_cache_tree() duplicates some logic in this function\n  * without actually calling it. If you change the logic here you may need to\n@@ -1078,21 +1139,44 @@ static int unpack_single_entry(int n, unsigned long mask,\n \t\t\t       unsigned long dirmask,\n \t\t\t       struct cache_entry **src,\n \t\t\t       const struct name_entry *names,\n-\t\t\t       const struct traverse_info *info)\n+\t\t\t       const struct traverse_info *info,\n+\t\t\t       int *is_new_sparse_dir)\n {\n \tint i;\n \tstruct unpack_trees_options *o = info->data;\n \tunsigned long conflicts = info->df_conflicts | dirmask;\n+\tconst struct name_entry *p = names;\n \n-\tif (mask == dirmask && !src[0])\n-\t\treturn 0;\n+\t*is_new_sparse_dir = 0;\n+\tif (mask == dirmask && !src[0]) {\n+\t\t/*\n+\t\t * If we're not in a sparse index, we can't unpack a directory\n+\t\t * without recursing into it, so we return.\n+\t\t */\n+\t\tif (!o->src_index->sparse_index)\n+\t\t\treturn 0;\n+\n+\t\t/* Find first entry with a real name (we could use \"mask\" too) */\n+\t\twhile (!p->mode)\n+\t\t\tp++;\n+\n+\t\t/*\n+\t\t * If the directory is completely missing from the index but\n+\t\t * would otherwise be a sparse directory, we should unpack it.\n+\t\t * If not, we'll return and continue recursively traversing the\n+\t\t * tree.\n+\t\t */\n+\t\t*is_new_sparse_dir = entry_is_new_sparse_dir(info, p);\n+\t\tif (!*is_new_sparse_dir)\n+\t\t\treturn 0;\n+\t}\n \n \t/*\n-\t * When we have a sparse directory entry for src[0],\n-\t * then this isn't necessarily a directory-file conflict.\n+\t * When we are unpacking a sparse directory, then this isn't necessarily\n+\t * a directory-file conflict.\n \t */\n-\tif (mask == dirmask && src[0] &&\n-\t    S_ISSPARSEDIR(src[0]->ce_mode))\n+\tif (mask == dirmask &&\n+\t    (*is_new_sparse_dir || (src[0] && S_ISSPARSEDIR(src[0]->ce_mode))))\n \t\tconflicts = 0;\n \n \t/*\n@@ -1352,7 +1436,7 @@ static int unpack_sparse_callback(int n, unsigned long mask, unsigned long dirma\n {\n \tstruct cache_entry *src[MAX_UNPACK_TREES + 1] = { NULL, };\n \tstruct unpack_trees_options *o = info->data;\n-\tint ret;\n+\tint ret, is_new_sparse_dir;\n \n \tassert(o->merge);\n \n@@ -1376,7 +1460,7 @@ static int unpack_sparse_callback(int n, unsigned long mask, unsigned long dirma\n \t * \"index\" tree (i.e., names[0]) and adjust 'names', 'n', 'mask', and\n \t * 'dirmask' accordingly.\n \t */\n-\tret = unpack_single_entry(n - 1, mask >> 1, dirmask >> 1, src, names + 1, info);\n+\tret = unpack_single_entry(n - 1, mask >> 1, dirmask >> 1, src, names + 1, info, &is_new_sparse_dir);\n \n \tif (src[0])\n \t\tdiscard_cache_entry(src[0]);\n@@ -1394,6 +1478,7 @@ static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, str\n \tstruct cache_entry *src[MAX_UNPACK_TREES + 1] = { NULL, };\n \tstruct unpack_trees_options *o = info->data;\n \tconst struct name_entry *p = names;\n+\tint is_new_sparse_dir;\n \n \t/* Find first entry with a real name (we could use \"mask\" too) */\n \twhile (!p->mode)\n@@ -1440,7 +1525,7 @@ static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, str\n \t\t}\n \t}\n \n-\tif (unpack_single_entry(n, mask, dirmask, src, names, info) < 0)\n+\tif (unpack_single_entry(n, mask, dirmask, src, names, info, &is_new_sparse_dir))\n \t\treturn -1;\n \n \tif (o->merge && src[0]) {\n@@ -1478,6 +1563,7 @@ static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, str\n \t\t}\n \n \t\tif (!is_sparse_directory_entry(src[0], names, info) &&\n+\t\t    !is_new_sparse_dir &&\n \t\t    traverse_trees_recursive(n, dirmask, mask & ~dirmask,\n \t\t\t\t\t\t    names, info) < 0) {\n \t\t\treturn -1;\n-- \ngitgitgadget\n"},{"id":"460852","messageId":"xmqq1qtqsadz.fsf@gitster.g","threadId":"58267","inReplyTo":"pull.1312.v3.git.1659985672.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 0/4] reset/checkout: fix miscellaneous sparse index bugs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-08T21:17:28Z","receivedAt":"2022-08-08T21:17:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Victoria Dye via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> Changes since V2\n> ================\n>\n>  * Adjusted 'reset hard with removed sparse dir' test in\n>    't1092-sparse-checkout-compatibility.sh' to avoid 'git rm' log message\n>    conflicts with [1]\n\nThe topic looks quite ready for 'next'.\n\nThanks.\n\n"},{"id":"460916","messageId":"ca575fc0-f9ee-9b32-f12b-75c83dd3b031@github.com","threadId":"58267","inReplyTo":"xmqq1qtqsadz.fsf@gitster.g","subject":"Re: [PATCH v3 0/4] reset/checkout: fix miscellaneous sparse index bugs","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-08-09T13:20:19Z","receivedAt":"2022-08-09T13:20:25Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/8/2022 5:17 PM, Junio C Hamano wrote:\n> \"Victoria Dye via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n>> Changes since V2\n>> ================\n>>\n>>  * Adjusted 'reset hard with removed sparse dir' test in\n>>    't1092-sparse-checkout-compatibility.sh' to avoid 'git rm' log message\n>>    conflicts with [1]\n> \n> The topic looks quite ready for 'next'.\n\nI agree. Thank you!\n\n-Stolee\n"}]}