{"thread":{"id":"58261","subject":"[PATCH v1 0/4] rm: integrate with sparse-index","startedAt":"2022-08-03T04:51:47Z","lastAt":"2022-08-12T18:36:52Z","messageCount":25,"participants":["Shaoxuan Yuan","Derrick Stolee","Junio C Hamano","Victoria Dye"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"460528","messageId":"20220803045118.1243087-1-shaoxuan.yuan02@gmail.com","threadId":"58261","inReplyTo":null,"subject":"[PATCH v1 0/4] rm: integrate with sparse-index","fromName":"Shaoxuan Yuan","fromEmail":"shaoxuan.yuan02@gmail.com","sentAt":"2022-08-03T04:51:14Z","receivedAt":"2022-08-03T04:51:47Z","isPatch":true,"sender":{"key":"shaoxuan.yuan02@gmail.com","avatar":"https://avatars.githubusercontent.com/u/46557895?v=4"},"body":"Turn on sparse-index feature within `git-rm` command.\nAdd necessary modifications and test them.\n\nShaoxuan Yuan (4):\n t1092: add tests for `git-rm`\n pathspec.h: move pathspec_needs_expanded_index() from reset.c to here\n rm: expand the index only when necessary\n rm: integrate with sparse-index\n\n builtin/reset.c | 84 +---------------------\n builtin/rm.c | 7 +-\n pathspec.c | 89 ++++++++++++++++++++++++\n pathspec.h | 12 ++++\n t/perf/p2000-sparse-operations.sh | 1 +\n t/t1092-sparse-checkout-compatibility.sh | 73 ++++++++++++++++++-\n 6 files changed, 180 insertions(+), 86 deletions(-)\n\n\nbase-commit: 350dc9f0e8974b6fcbdeb3808186c5a79c3e7386\n-- \n2.37.0\n\n"},{"id":"460529","messageId":"20220803045118.1243087-3-shaoxuan.yuan02@gmail.com","threadId":"58261","inReplyTo":"20220803045118.1243087-1-shaoxuan.yuan02@gmail.com","subject":"[PATCH v1 2/4] pathspec.h: move pathspec_needs_expanded_index() from reset.c to here","fromName":"Shaoxuan Yuan","fromEmail":"shaoxuan.yuan02@gmail.com","sentAt":"2022-08-03T04:51:16Z","receivedAt":"2022-08-03T04:51:48Z","isPatch":true,"sender":{"key":"shaoxuan.yuan02@gmail.com","avatar":"https://avatars.githubusercontent.com/u/46557895?v=4"},"body":"Method pathspec_needs_expanded_index() in reset.c from 4d1cfc1351\n(reset: make --mixed sparse-aware, 2021-11-29) is reusable when we\nneed to verify if the index needs to be expanded when the command\nis utilizing a pathspec rather than a literal path.\n\nMove it to pathspec.h for reusability.\n\nAdd a few items to the function so it can better serve its purpose as\na standalone public function:\n\n* Add a check in front so if the index is not sparse, return early since\n no expansion is needed.\n\n* Add documentation to the function.\n\nSigned-off-by: Shaoxuan Yuan \n---\n builtin/reset.c | 84 +---------------------------------------------\n pathspec.c | 89 +++++++++++++++++++++++++++++++++++++++++++++++++\n pathspec.h | 12 +++++++\n 3 files changed, 102 insertions(+), 83 deletions(-)\n\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 344fff8f3a..fdce6f8c85 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -174,88 +174,6 @@ static void update_index_from_diff(struct diff_queue_struct *q,\n \t}\n }\n \n-static int pathspec_needs_expanded_index(const struct pathspec *pathspec)\n-{\n-\tunsigned int i, pos;\n-\tint res = 0;\n-\tchar *skip_worktree_seen = NULL;\n-\n-\t/*\n-\t * When using a magic pathspec, assume for the sake of simplicity that\n-\t * the index needs to be expanded to match all matchable files.\n-\t */\n-\tif (pathspec->magic)\n-\t\treturn 1;\n-\n-\tfor (i = 0; i < pathspec->nr; i++) {\n-\t\tstruct pathspec_item item = pathspec->items[i];\n-\n-\t\t/*\n-\t\t * If the pathspec item has a wildcard, the index should be expanded\n-\t\t * if the pathspec has the possibility of matching a subset of entries inside\n-\t\t * of a sparse directory (but not the entire directory).\n-\t\t *\n-\t\t * If the pathspec item is a literal path, the index only needs to be expanded\n-\t\t * if a) the pathspec isn't in the sparse checkout cone (to make sure we don't\n-\t\t * expand for in-cone files) and b) it doesn't match any sparse directories\n-\t\t * (since we can reset whole sparse directories without expanding them).\n-\t\t */\n-\t\tif (item.nowildcard_len < item.len) {\n-\t\t\t/*\n-\t\t\t * Special case: if the pattern is a path inside the cone\n-\t\t\t * followed by only wildcards, the pattern cannot match\n-\t\t\t * partial sparse directories, so we know we don't need to\n-\t\t\t * expand the index.\n-\t\t\t *\n-\t\t\t * Examples:\n-\t\t\t * - in-cone/foo***: doesn't need expanded index\n-\t\t\t * - not-in-cone/bar*: may need expanded index\n-\t\t\t * - **.c: may need expanded index\n-\t\t\t */\n-\t\t\tif (strspn(item.original + item.nowildcard_len, \"*\") == item.len - item.nowildcard_len &&\n-\t\t\t path_in_cone_mode_sparse_checkout(item.original, &the_index))\n-\t\t\t\tcontinue;\n-\n-\t\t\tfor (pos = 0; pos < active_nr; pos++) {\n-\t\t\t\tstruct cache_entry *ce = active_cache[pos];\n-\n-\t\t\t\tif (!S_ISSPARSEDIR(ce->ce_mode))\n-\t\t\t\t\tcontinue;\n-\n-\t\t\t\t/*\n-\t\t\t\t * If the pre-wildcard length is longer than the sparse\n-\t\t\t\t * directory name and the sparse directory is the first\n-\t\t\t\t * component of the pathspec, need to expand the index.\n-\t\t\t\t */\n-\t\t\t\tif (item.nowildcard_len > ce_namelen(ce) &&\n-\t\t\t\t !strncmp(item.original, ce->name, ce_namelen(ce))) {\n-\t\t\t\t\tres = 1;\n-\t\t\t\t\tbreak;\n-\t\t\t\t}\n-\n-\t\t\t\t/*\n-\t\t\t\t * If the pre-wildcard length is shorter than the sparse\n-\t\t\t\t * directory and the pathspec does not match the whole\n-\t\t\t\t * directory, need to expand the index.\n-\t\t\t\t */\n-\t\t\t\tif (!strncmp(item.original, ce->name, item.nowildcard_len) &&\n-\t\t\t\t wildmatch(item.original, ce->name, 0)) {\n-\t\t\t\t\tres = 1;\n-\t\t\t\t\tbreak;\n-\t\t\t\t}\n-\t\t\t}\n-\t\t} else if (!path_in_cone_mode_sparse_checkout(item.original, &the_index) &&\n-\t\t\t !matches_skip_worktree(pathspec, i, &skip_worktree_seen))\n-\t\t\tres = 1;\n-\n-\t\tif (res > 0)\n-\t\t\tbreak;\n-\t}\n-\n-\tfree(skip_worktree_seen);\n-\treturn res;\n-}\n-\n static int read_from_tree(const struct pathspec *pathspec,\n \t\t\t struct object_id *tree_oid,\n \t\t\t int intent_to_add)\n@@ -273,7 +191,7 @@ static int read_from_tree(const struct pathspec *pathspec,\n \topt.change = diff_change;\n \topt.add_remove = diff_addremove;\n \n-\tif (pathspec->nr && the_index.sparse_index && pathspec_needs_expanded_index(pathspec))\n+\tif (pathspec->nr && pathspec_needs_expanded_index(&the_index, pathspec))\n \t\tensure_full_index(&the_index);\n \n \tif (do_diff_cache(tree_oid, &opt))\ndiff --git a/pathspec.c b/pathspec.c\nindex 84ad9c73cf..46e77a85fe 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -759,3 +759,92 @@ int match_pathspec_attrs(struct index_state *istate,\n \n \treturn 1;\n }\n+\n+int pathspec_needs_expanded_index(struct index_state *istate,\n+\t\t\t\t const struct pathspec *pathspec)\n+{\n+\tunsigned int i, pos;\n+\tint res = 0;\n+\tchar *skip_worktree_seen = NULL;\n+\n+\t/*\n+\t * If index is not sparse, no index expansion is needed.\n+\t */\n+\tif (!istate->sparse_index)\n+\t\treturn 0;\n+\n+\t/*\n+\t * When using a magic pathspec, assume for the sake of simplicity that\n+\t * the index needs to be expanded to match all matchable files.\n+\t */\n+\tif (pathspec->magic)\n+\t\treturn 1;\n+\n+\tfor (i = 0; i < pathspec->nr; i++) {\n+\t\tstruct pathspec_item item = pathspec->items[i];\n+\n+\t\t/*\n+\t\t * If the pathspec item has a wildcard, the index should be expanded\n+\t\t * if the pathspec has the possibility of matching a subset of entries inside\n+\t\t * of a sparse directory (but not the entire directory).\n+\t\t *\n+\t\t * If the pathspec item is a literal path, the index only needs to be expanded\n+\t\t * if a) the pathspec isn't in the sparse checkout cone (to make sure we don't\n+\t\t * expand for in-cone files) and b) it doesn't match any sparse directories\n+\t\t * (since we can reset whole sparse directories without expanding them).\n+\t\t */\n+\t\tif (item.nowildcard_len < item.len) {\n+\t\t\t/*\n+\t\t\t * Special case: if the pattern is a path inside the cone\n+\t\t\t * followed by only wildcards, the pattern cannot match\n+\t\t\t * partial sparse directories, so we know we don't need to\n+\t\t\t * expand the index.\n+\t\t\t *\n+\t\t\t * Examples:\n+\t\t\t * - in-cone/foo***: doesn't need expanded index\n+\t\t\t * - not-in-cone/bar*: may need expanded index\n+\t\t\t * - **.c: may need expanded index\n+\t\t\t */\n+\t\t\tif (strspn(item.original + item.nowildcard_len, \"*\") == item.len - item.nowildcard_len &&\n+\t\t\t path_in_cone_mode_sparse_checkout(item.original, istate))\n+\t\t\t\tcontinue;\n+\n+\t\t\tfor (pos = 0; pos < istate->cache_nr; pos++) {\n+\t\t\t\tstruct cache_entry *ce = istate->cache[pos];\n+\n+\t\t\t\tif (!S_ISSPARSEDIR(ce->ce_mode))\n+\t\t\t\t\tcontinue;\n+\n+\t\t\t\t/*\n+\t\t\t\t * If the pre-wildcard length is longer than the sparse\n+\t\t\t\t * directory name and the sparse directory is the first\n+\t\t\t\t * component of the pathspec, need to expand the index.\n+\t\t\t\t */\n+\t\t\t\tif (item.nowildcard_len > ce_namelen(ce) &&\n+\t\t\t\t !strncmp(item.original, ce->name, ce_namelen(ce))) {\n+\t\t\t\t\tres = 1;\n+\t\t\t\t\tbreak;\n+\t\t\t\t}\n+\n+\t\t\t\t/*\n+\t\t\t\t * If the pre-wildcard length is shorter than the sparse\n+\t\t\t\t * directory and the pathspec does not match the whole\n+\t\t\t\t * directory, need to expand the index.\n+\t\t\t\t */\n+\t\t\t\tif (!strncmp(item.original, ce->name, item.nowildcard_len) &&\n+\t\t\t\t wildmatch(item.original, ce->name, 0)) {\n+\t\t\t\t\tres = 1;\n+\t\t\t\t\tbreak;\n+\t\t\t\t}\n+\t\t\t}\n+\t\t} else if (!path_in_cone_mode_sparse_checkout(item.original, istate) &&\n+\t\t\t !matches_skip_worktree(pathspec, i, &skip_worktree_seen))\n+\t\t\tres = 1;\n+\n+\t\tif (res > 0)\n+\t\t\tbreak;\n+\t}\n+\n+\tfree(skip_worktree_seen);\n+\treturn res;\n+}\ndiff --git a/pathspec.h b/pathspec.h\nindex 402ebb8080..41f6adfbb4 100644\n--- a/pathspec.h\n+++ b/pathspec.h\n@@ -171,4 +171,16 @@ int match_pathspec_attrs(struct index_state *istate,\n \t\t\t const char *name, int namelen,\n \t\t\t const struct pathspec_item *item);\n \n+/*\n+ * Determine whether a pathspec will match only entire index entries (non-sparse\n+ * files and/or entire sparse directories). If the pathspec has the potential to\n+ * match partial contents of a sparse directory, return 1 to indicate the index\n+ * should be expanded to match the appropriate index entries.\n+ *\n+ * For the sake of simplicity, always return 1 if using a more complex \"magic\"\n+ * pathspec.\n+ */\n+int pathspec_needs_expanded_index(struct index_state *istate,\n+\t\t\t\t const struct pathspec *pathspec);\n+\n #endif /* PATHSPEC_H */\n-- \n2.37.0\n\n"},{"id":"460530","messageId":"20220803045118.1243087-4-shaoxuan.yuan02@gmail.com","threadId":"58261","inReplyTo":"20220803045118.1243087-1-shaoxuan.yuan02@gmail.com","subject":"[PATCH v1 3/4] rm: expand the index only when necessary","fromName":"Shaoxuan Yuan","fromEmail":"shaoxuan.yuan02@gmail.com","sentAt":"2022-08-03T04:51:17Z","receivedAt":"2022-08-03T04:51:50Z","isPatch":true,"sender":{"key":"shaoxuan.yuan02@gmail.com","avatar":"https://avatars.githubusercontent.com/u/46557895?v=4"},"body":"Originally, rm a pathspec that is out-of-cone in a sparse-index\nenvironment, Git dies with \"pathspec '' did not match any files\",\nmainly because it does not expand the index so nothing is matched.\n\nRemove the `ensure_full_index()` method so `git-rm` does not always\nexpand the index when the expansion is unnecessary, i.e. when\n does not have any possibilities to match anything outside\nof sparse-checkout definition.\n\nExpand the index when the needs an expanded index, i.e. the\n contains wildcard that may need a full-index or the\n is simply outside of sparse-checkout definition.\n\nSigned-off-by: Shaoxuan Yuan \n---\n builtin/rm.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex 84a935a16e..58ed924f0d 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -296,8 +296,9 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \n \tseen = xcalloc(pathspec.nr, 1);\n \n-\t/* TODO: audit for interaction with sparse-index. */\n-\tensure_full_index(&the_index);\n+\tif (pathspec_needs_expanded_index(&the_index, &pathspec))\n+\t\tensure_full_index(&the_index);\n+\n \tfor (i = 0; i < active_nr; i++) {\n \t\tconst struct cache_entry *ce = active_cache[i];\n \n-- \n2.37.0\n\n"},{"id":"460531","messageId":"20220803045118.1243087-2-shaoxuan.yuan02@gmail.com","threadId":"58261","inReplyTo":"20220803045118.1243087-1-shaoxuan.yuan02@gmail.com","subject":"[PATCH v1 1/4] t1092: add tests for `git-rm`","fromName":"Shaoxuan Yuan","fromEmail":"shaoxuan.yuan02@gmail.com","sentAt":"2022-08-03T04:51:15Z","receivedAt":"2022-08-03T04:51:51Z","isPatch":true,"sender":{"key":"shaoxuan.yuan02@gmail.com","avatar":"https://avatars.githubusercontent.com/u/46557895?v=4"},"body":"Add tests for `git-rm`, make sure it behaves as expected when\n is both inside or outside of sparse-checkout definition.\n\nAlso add ensure_not_expanded test to make sure `git-rm` does not\naccidentally expand the index when is within the\nsparse-checkout definition.\n\nSigned-off-by: Shaoxuan Yuan \n---\n t/t1092-sparse-checkout-compatibility.sh | 71 ++++++++++++++++++++++++\n 1 file changed, 71 insertions(+)\n\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 763c6cc684..75649e3265 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -1853,4 +1853,75 @@ test_expect_success 'mv directory from out-of-cone to in-cone' '\n \tgrep -e \"H deep/0/1\" actual\n '\n \n+test_expect_success 'rm pathspec inside sparse definition' '\n+\tinit_repos &&\n+\n+\ttest_all_match git rm deep/a &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\t# test wildcard\n+\trun_on_all git reset --hard &&\n+\ttest_all_match git rm deep/* &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\t# test recursive rm\n+\trun_on_all git reset --hard &&\n+\ttest_all_match git rm -r deep &&\n+\ttest_all_match git status --porcelain=v2\n+'\n+\n+test_expect_failure 'rm pathspec outside sparse definition' '\n+\tinit_repos &&\n+\n+\tfor file in folder1/a folder1/0/1\n+\tdo\n+\t\ttest_sparse_match test_must_fail git rm $file &&\n+\t\ttest_sparse_match test_must_fail git rm --cached $file &&\n+\t\ttest_sparse_match git rm --sparse $file &&\n+\t\ttest_sparse_match git status --porcelain=v2\n+\tdone &&\n+\n+\tcat >folder1-full <<-EOF &&\n+\trm ${SQ}folder1/0/0/0${SQ}\n+\trm ${SQ}folder1/0/1${SQ}\n+\trm ${SQ}folder1/a${SQ}\n+\tEOF\n+\n+\tcat >folder1-sparse <<-EOF &&\n+\trm ${SQ}folder1/${SQ}\n+\tEOF\n+\n+\t# test wildcard\n+\trun_on_sparse git reset --hard &&\n+\trun_on_sparse git sparse-checkout reapply &&\n+\ttest_sparse_match test_must_fail git rm folder1/* &&\n+\trun_on_sparse git rm --sparse folder1/* &&\n+\ttest_cmp folder1-full sparse-checkout-out &&\n+\ttest_cmp folder1-sparse sparse-index-out &&\n+\ttest_sparse_match git status --porcelain=v2 &&\n+\n+\t# test recursive rm\n+\trun_on_sparse git reset --hard &&\n+\trun_on_sparse git sparse-checkout reapply &&\n+\ttest_sparse_match test_must_fail git rm --sparse folder1 &&\n+\trun_on_sparse git rm --sparse -r folder1 &&\n+\ttest_cmp folder1-full sparse-checkout-out &&\n+\ttest_cmp folder1-sparse sparse-index-out &&\n+\ttest_sparse_match git status --porcelain=v2\n+'\n+\n+test_expect_failure 'sparse index is not expanded: rm' '\n+\tinit_repos &&\n+\n+\tensure_not_expanded rm deep/a &&\n+\n+\t# test in-cone wildcard\n+\tgit -C sparse-index reset --hard &&\n+\tensure_not_expanded rm deep/* &&\n+\n+\t# test recursive rm\n+\tgit -C sparse-index reset --hard &&\n+\tensure_not_expanded rm -r deep\n+'\n+\n test_done\n-- \n2.37.0\n\n"},{"id":"460532","messageId":"20220803045118.1243087-5-shaoxuan.yuan02@gmail.com","threadId":"58261","inReplyTo":"20220803045118.1243087-1-shaoxuan.yuan02@gmail.com","subject":"[PATCH v1 4/4] rm: integrate with sparse-index","fromName":"Shaoxuan Yuan","fromEmail":"shaoxuan.yuan02@gmail.com","sentAt":"2022-08-03T04:51:18Z","receivedAt":"2022-08-03T04:51:57Z","isPatch":true,"sender":{"key":"shaoxuan.yuan02@gmail.com","avatar":"https://avatars.githubusercontent.com/u/46557895?v=4"},"body":"Enable the sparse index within the `git-rm` command.\n\nThe `p2000` tests demonstrate a ~96% execution time reduction for\n'git rm' using a sparse index.\n\nTest before after\n-------------------------------------------------------------\n2000.74: git rm -f f2/f4/a (full-v3) 0.66 0.88 +33.0%\n2000.75: git rm -f f2/f4/a (full-v4) 0.67 0.75 +12.0%\n2000.76: git rm -f f2/f4/a (sparse-v3) 1.99 0.08 -96.0%\n2000.77: git rm -f f2/f4/a (sparse-v4) 2.06 0.07 -96.6%\n\nAlso, normalize a behavioral difference of `git-rm` under sparse-index.\nSee related discussion [1].\n\n`git-rm` a sparse-directory entry within a sparse-index enabled repo\nbehaves differently from a sparse directory within a sparse-checkout\nenabled repo.\n\nFor example, in a sparse-index repo, where 'folder1' is a\nsparse-directory entry, `git rm -r --sparse folder1` provides this:\n\n rm 'folder1/'\n\nWhereas in a sparse-checkout repo *without* sparse-index, doing so\nprovides this:\n\n rm 'folder1/0/0/0'\n rm 'folder1/0/1'\n rm 'folder1/a'\n\nBecause `git rm` a sparse-directory entry does not need to expand the\nindex, therefore we should accept the current behavior, which is faster\nthan \"expand the sparse-directory entry to match the sparse-checkout\nsituation\".\n\nModify a previous test so such difference is not considered as an error.\n\n[1] https://github.com/ffyuanda/git/pull/6#discussion_r934861398\n\nSigned-off-by: Shaoxuan Yuan \n---\n builtin/rm.c | 2 ++\n t/perf/p2000-sparse-operations.sh | 1 +\n t/t1092-sparse-checkout-compatibility.sh | 6 +++---\n 3 files changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex 58ed924f0d..b6ba859fe4 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -287,6 +287,8 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \tif (!index_only)\n \t\tsetup_work_tree();\n \n+\tprepare_repo_settings(the_repository);\n+\tthe_repository->settings.command_requires_full_index = 0;\n \thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n \n \tif (read_cache() < 0)\ndiff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh\nindex c181110a43..853513eb9b 100755\n--- a/t/perf/p2000-sparse-operations.sh\n+++ b/t/perf/p2000-sparse-operations.sh\n@@ -123,5 +123,6 @@ test_perf_on_all git blame $SPARSE_CONE/f3/a\n test_perf_on_all git read-tree -mu HEAD\n test_perf_on_all git checkout-index -f --all\n test_perf_on_all git update-index --add --remove $SPARSE_CONE/a\n+test_perf_on_all git rm -f $SPARSE_CONE/a\n \n test_done\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 75649e3265..58632fe483 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -912,7 +912,7 @@ test_expect_success 'read-tree --prefix' '\n \ttest_all_match git read-tree --prefix=deep/deeper1/deepest -u deepest &&\n \ttest_all_match git status --porcelain=v2 &&\n \n-\ttest_all_match git rm -rf --sparse folder1/ &&\n+\trun_on_all git rm -rf --sparse folder1/ &&\n \ttest_all_match git read-tree --prefix=folder1/ -u update-folder1 &&\n \ttest_all_match git status --porcelain=v2 &&\n \n@@ -1870,7 +1870,7 @@ test_expect_success 'rm pathspec inside sparse definition' '\n \ttest_all_match git status --porcelain=v2\n '\n \n-test_expect_failure 'rm pathspec outside sparse definition' '\n+test_expect_success 'rm pathspec outside sparse definition' '\n \tinit_repos &&\n \n \tfor file in folder1/a folder1/0/1\n@@ -1910,7 +1910,7 @@ test_expect_failure 'rm pathspec outside sparse definition' '\n \ttest_sparse_match git status --porcelain=v2\n '\n \n-test_expect_failure 'sparse index is not expanded: rm' '\n+test_expect_success 'sparse index is not expanded: rm' '\n \tinit_repos &&\n \n \tensure_not_expanded rm deep/a &&\n-- \n2.37.0\n\n"},{"id":"460548","messageId":"16b76622-0242-84be-5842-2dea39138643@github.com","threadId":"58261","inReplyTo":"20220803045118.1243087-2-shaoxuan.yuan02@gmail.com","subject":"Re: [PATCH v1 1/4] t1092: add tests for `git-rm`","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-08-03T14:32:03Z","receivedAt":"2022-08-03T14:32:09Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/3/2022 12:51 AM, Shaoxuan Yuan wrote:\n> Add tests for `git-rm`, make sure it behaves as expected when\n> is both inside or outside of sparse-checkout definition.\n\nThis is good to demonstrate that we already have feature parity,\neven if it is because we expand the sparse index immediately.\n \n> Also add ensure_not_expanded test to make sure `git-rm` does not\n> accidentally expand the index when is within the\n> sparse-checkout definition.\n\n> +test_expect_failure 'sparse index is not expanded: rm' '\n> +\tinit_repos &&\n> +\n> +\tensure_not_expanded rm deep/a &&\n> +\n> +\t# test in-cone wildcard\n> +\tgit -C sparse-index reset --hard &&\n> +\tensure_not_expanded rm deep/* &&\n> +\n> +\t# test recursive rm\n> +\tgit -C sparse-index reset --hard &&\n> +\tensure_not_expanded rm -r deep\n> +'\n> +\n\nInstead of adding a test_expect_failure here, I would wait to add\nthis as a test_expect_success in patch 4.\n\nThanks,\n-Stolee\n"},{"id":"460549","messageId":"90f817f1-340d-48e0-22b1-c6644d62f19f@github.com","threadId":"58261","inReplyTo":"20220803045118.1243087-3-shaoxuan.yuan02@gmail.com","subject":"Re: [PATCH v1 2/4] pathspec.h: move pathspec_needs_expanded_index() from reset.c to here","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-08-03T14:35:29Z","receivedAt":"2022-08-03T14:35:39Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/3/2022 12:51 AM, Shaoxuan Yuan wrote:\n> Method pathspec_needs_expanded_index() in reset.c from 4d1cfc1351\n> (reset: make --mixed sparse-aware, 2021-11-29) is reusable when we\n> need to verify if the index needs to be expanded when the command\n> is utilizing a pathspec rather than a literal path.\n> \n> Move it to pathspec.h for reusability.\n> \n> Add a few items to the function so it can better serve its purpose as\n> a standalone public function:\n> \n> * Add a check in front so if the index is not sparse, return early since\n> no expansion is needed.\n> \n> * Add documentation to the function.\n\nI took a look at this diff on my machine with --color-moved, which\nhighlighted the other valuable thing about this move: it takes an\narbitrary 'struct index_state' pointer instead of using the_index and\nactive_cache. These are good things that might be worth mentioning in\nthe commit message.\n\nThanks,\n-Stolee\n\n"},{"id":"460550","messageId":"475e8617-2adf-c75a-b697-d239dc4830b8@github.com","threadId":"58261","inReplyTo":"20220803045118.1243087-4-shaoxuan.yuan02@gmail.com","subject":"Re: [PATCH v1 3/4] rm: expand the index only when necessary","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-08-03T14:40:42Z","receivedAt":"2022-08-03T14:40:49Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/3/2022 12:51 AM, Shaoxuan Yuan wrote:\n> Originally, rm a pathspec that is out-of-cone in a sparse-index\n> environment, Git dies with \"pathspec '' did not match any files\",\n> mainly because it does not expand the index so nothing is matched.\n\nThis paragraph appears to be assuming that we've stopped expanding the\nsparse index already. It might be worthwhile to rewrite this to say\n\"Before integrating 'git rm' with the sparse index, we need to...\" or\nsomething like that. \n\n> Remove the `ensure_full_index()` method so `git-rm` does not always\n> expand the index when the expansion is unnecessary, i.e. when\n> does not have any possibilities to match anything outside\n> of sparse-checkout definition.\n> \n> Expand the index when the needs an expanded index, i.e. the\n> contains wildcard that may need a full-index or the\n> is simply outside of sparse-checkout definition.\n> \n> Signed-off-by: Shaoxuan Yuan \n> ---\n> builtin/rm.c | 5 +++--\n> 1 file changed, 3 insertions(+), 2 deletions(-)\n> \n> diff --git a/builtin/rm.c b/builtin/rm.c\n> index 84a935a16e..58ed924f0d 100644\n> --- a/builtin/rm.c\n> +++ b/builtin/rm.c\n> @@ -296,8 +296,9 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n> \n> \tseen = xcalloc(pathspec.nr, 1);\n> \n> -\t/* TODO: audit for interaction with sparse-index. */\n> -\tensure_full_index(&the_index);\n> +\tif (pathspec_needs_expanded_index(&the_index, &pathspec))\n> +\t\tensure_full_index(&the_index);\n> +\n> \tfor (i = 0; i < active_nr; i++) {\n> \t\tconst struct cache_entry *ce = active_cache[i];\n\nLooking back on the tests in patch 1, I don't see any tests that really\nemphasize the kinds of pathspecs that could not ever integrate with the\nsparse index. They are all of the form \"folder1/*\" or similar, making it\nbe something that could be seen as a prefix match. Such a pattern _could_\nbe integrated carefully with the sparse index.\n\nInstead, something like `git rm \"*/a\"` would be much harder to integrate\nwith the sparse index. Could we add a test (in this patch) that checks\nthat kind of case. That would also help justify this as its own patch and\nnot squashed with patch 4.\n\nThanks,\n-Stolee\n"},{"id":"460621","messageId":"999169c6-a727-af2a-3361-51ac7b1f1d80@github.com","threadId":"58261","inReplyTo":"20220803045118.1243087-5-shaoxuan.yuan02@gmail.com","subject":"Re: [PATCH v1 4/4] rm: integrate with sparse-index","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-08-04T14:48:12Z","receivedAt":"2022-08-04T14:48:17Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/3/2022 12:51 AM, Shaoxuan Yuan wrote:\n> Enable the sparse index within the `git-rm` command.\n> \n> The `p2000` tests demonstrate a ~96% execution time reduction for\n> 'git rm' using a sparse index.\n\nSorry that I got sidetracked yesterday when I was reviewing this\nseries, but I noticed something looking at these results:\n \n> Test before after\n> -------------------------------------------------------------\n> 2000.74: git rm -f f2/f4/a (full-v3) 0.66 0.88 +33.0%\n> 2000.75: git rm -f f2/f4/a (full-v4) 0.67 0.75 +12.0%\n\nThe range of _growth_ here seemed odd, so I wanted to check if this was\ndue to a small sample size or not.\n\n> 2000.76: git rm -f f2/f4/a (sparse-v3) 1.99 0.08 -96.0%\n> 2000.77: git rm -f f2/f4/a (sparse-v4) 2.06 0.07 -96.6%\n\nThese numbers are as expected.\n\n> test_perf_on_all git read-tree -mu HEAD\n> test_perf_on_all git checkout-index -f --all\n> test_perf_on_all git update-index --add --remove $SPARSE_CONE/a\n> +test_perf_on_all git rm -f $SPARSE_CONE/a\n\nAt first, I was confused why we needed '-f' and thought that maybe\nthis was turning into a no-op after the first deletion. However, the\ntest_perf_on_all helper does an \"echo >>$SPARSE_CONE/a\" before hand,\nso the file exists _in the worktree_ every time. That requires '-f'\nsince otherwise Git complains that we have modifications.\n\nHowever, after the first instance the file no longer exists in the\nindex, so we are losing some testing of the index modification.\n\nWe can fix this by resetting the index in each test loop:\n\n test_perf_on_all \"git rm -f $SPARSE_CONE/a && git checkout HEAD -- $SPARSE_CONE/a\"\n\nRunning this version of the test with GIT_PERF_REPEAT_COUNT=10 and\nusing the Git repository itself, I get these numbers:\n\nTest HEAD~1 HEAD\n--------------------------------------------------------------------------\n2000.74: git rm ... (full-v3) 0.41(0.37+0.05) 0.43(0.36+0.07) +4.9% \n2000.75: git rm ... (full-v4) 0.38(0.34+0.05) 0.39(0.35+0.05) +2.6% \n2000.76: git rm ... (sparse-v3) 0.57(0.56+0.01) 0.05(0.05+0.00) -91.2%\n2000.77: git rm ... (sparse-v4) 0.57(0.55+0.02) 0.03(0.03+0.00) -94.7%\n\nYes, the 'git checkout' command is contributing to the overall\nnumbers, but it also already has the performance improvements of\nthe sparse-index, so it contributes only a little to the performance\non the left.\n\n(Also note that the full index cases change only by amounts within\nreasonable noise. The repeat count helps there.)\n\nThanks,\n-Stolee\n"},{"id":"460697","messageId":"892e718a-f9ab-51d9-619f-7aa661ddcda6@gmail.com","threadId":"58261","inReplyTo":"90f817f1-340d-48e0-22b1-c6644d62f19f@github.com","subject":"Re: [PATCH v1 2/4] pathspec.h: move pathspec_needs_expanded_index() from reset.c to here","fromName":"Shaoxuan Yuan","fromEmail":"shaoxuan.yuan02@gmail.com","sentAt":"2022-08-05T07:53:05Z","receivedAt":"2022-08-05T07:53:14Z","isPatch":true,"sender":{"key":"shaoxuan.yuan02@gmail.com","avatar":"https://avatars.githubusercontent.com/u/46557895?v=4"},"body":"On 8/3/2022 10:35 PM, Derrick Stolee wrote:\n > On 8/3/2022 12:51 AM, Shaoxuan Yuan wrote:\n >> Method pathspec_needs_expanded_index() in reset.c from 4d1cfc1351\n >> (reset: make --mixed sparse-aware, 2021-11-29) is reusable when we\n >> need to verify if the index needs to be expanded when the command\n >> is utilizing a pathspec rather than a literal path.\n >>\n >> Move it to pathspec.h for reusability.\n >>\n >> Add a few items to the function so it can better serve its purpose as\n >> a standalone public function:\n >>\n >> * Add a check in front so if the index is not sparse, return early since\n >>   no expansion is needed.\n >>\n >> * Add documentation to the function.\n >\n > I took a look at this diff on my machine with --color-moved, which\n > highlighted the other valuable thing about this move: it takes an\n > arbitrary 'struct index_state' pointer instead of using the_index and\n > active_cache. These are good things that might be worth mentioning in\n > the commit message.\n\nThanks for pointing it out! Will add.\n\n--\nThanks,\nShaoxuan\n\n"},{"id":"460700","messageId":"d31d7ea2-4b7e-feb3-9d67-066520e0d053@gmail.com","threadId":"58261","inReplyTo":"475e8617-2adf-c75a-b697-d239dc4830b8@github.com","subject":"Re: [PATCH v1 3/4] rm: expand the index only when necessary","fromName":"Shaoxuan Yuan","fromEmail":"shaoxuan.yuan02@gmail.com","sentAt":"2022-08-05T08:07:51Z","receivedAt":"2022-08-05T08:08:00Z","isPatch":true,"sender":{"key":"shaoxuan.yuan02@gmail.com","avatar":"https://avatars.githubusercontent.com/u/46557895?v=4"},"body":"\n\nOn 8/3/2022 10:40 PM, Derrick Stolee wrote:\n > On 8/3/2022 12:51 AM, Shaoxuan Yuan wrote:\n >> Originally, rm a pathspec that is out-of-cone in a sparse-index\n >> environment, Git dies with \"pathspec '' did not match any files\",\n >> mainly because it does not expand the index so nothing is matched.\n >\n > This paragraph appears to be assuming that we've stopped expanding the\n > sparse index already. It might be worthwhile to rewrite this to say\n > \"Before integrating 'git rm' with the sparse index, we need to...\" or\n > something like that.\n\nI have absolutely no idea why I wrote this paragraph this way, maybe\nI was zoning out composing it. Will fix.\n\n >> Remove the `ensure_full_index()` method so `git-rm` does not always\n >> expand the index when the expansion is unnecessary, i.e. when\n >> does not have any possibilities to match anything outside\n >> of sparse-checkout definition.\n >>\n >> Expand the index when the needs an expanded index, i.e. the\n >> contains wildcard that may need a full-index or the\n >> is simply outside of sparse-checkout definition.\n >>\n >> Signed-off-by: Shaoxuan Yuan \n >> ---\n >>  builtin/rm.c | 5 +++--\n >>  1 file changed, 3 insertions(+), 2 deletions(-)\n >>\n >> diff --git a/builtin/rm.c b/builtin/rm.c\n >> index 84a935a16e..58ed924f0d 100644\n >> --- a/builtin/rm.c\n >> +++ b/builtin/rm.c\n >> @@ -296,8 +296,9 @@ int cmd_rm(int argc, const char **argv, const \nchar *prefix)\n >>\n >>      seen = xcalloc(pathspec.nr, 1);\n >>\n >> -    /* TODO: audit for interaction with sparse-index. */\n >> -    ensure_full_index(&the_index);\n >> +    if (pathspec_needs_expanded_index(&the_index, &pathspec))\n >> +        ensure_full_index(&the_index);\n >> +\n >>      for (i = 0; i < active_nr; i++) {\n >>          const struct cache_entry *ce = active_cache[i];\n >\n > Looking back on the tests in patch 1, I don't see any tests that really\n > emphasize the kinds of pathspecs that could not ever integrate with the\n > sparse index. They are all of the form \"folder1/*\" or similar, making it\n > be something that could be seen as a prefix match. Such a pattern _could_\n > be integrated carefully with the sparse index.\n >\n > Instead, something like `git rm \"*/a\"` would be much harder to integrate\n > with the sparse index. Could we add a test (in this patch) that checks\n > that kind of case. That would also help justify this as its own patch and\n > not squashed with patch 4.\n\nMakes sense. Will fix.\n\n--\nThanks,\nShaoxuan\n\n"},{"id":"460745","messageId":"12afcbe9-218b-528d-6d81-f39628388ba9@gmail.com","threadId":"58261","inReplyTo":"999169c6-a727-af2a-3361-51ac7b1f1d80@github.com","subject":"Re: [PATCH v1 4/4] rm: integrate with sparse-index","fromName":"Shaoxuan Yuan","fromEmail":"shaoxuan.yuan02@gmail.com","sentAt":"2022-08-06T03:18:14Z","receivedAt":"2022-08-06T03:18:25Z","isPatch":true,"sender":{"key":"shaoxuan.yuan02@gmail.com","avatar":"https://avatars.githubusercontent.com/u/46557895?v=4"},"body":"On 8/4/2022 10:48 PM, Derrick Stolee wrote:\n > On 8/3/2022 12:51 AM, Shaoxuan Yuan wrote:\n >> Enable the sparse index within the `git-rm` command.\n >>\n >> The `p2000` tests demonstrate a ~96% execution time reduction for\n >> 'git rm' using a sparse index.\n >\n > Sorry that I got sidetracked yesterday when I was reviewing this\n > series, but I noticed something looking at these results:\n >\n >> Test                                     before  after\n >> -------------------------------------------------------------\n >> 2000.74: git rm -f f2/f4/a (full-v3)     0.66    0.88 +33.0%\n >> 2000.75: git rm -f f2/f4/a (full-v4)     0.67    0.75 +12.0%\n >\n > The range of _growth_ here seemed odd, so I wanted to check if this was\n > due to a small sample size or not.\n\nYes, I do feel they are odd, as I've been checking pervious\nintegrations and p2000 results, they usuallly fall below 10% range.\nBut I was not discerning enough to name a problem :-(\n\n >> 2000.76: git rm -f f2/f4/a (sparse-v3)   1.99    0.08 -96.0%\n >> 2000.77: git rm -f f2/f4/a (sparse-v4)   2.06    0.07 -96.6%\n >\n > These numbers are as expected.\n >\n >>  test_perf_on_all git read-tree -mu HEAD\n >>  test_perf_on_all git checkout-index -f --all\n >>  test_perf_on_all git update-index --add --remove $SPARSE_CONE/a\n >> +test_perf_on_all git rm -f $SPARSE_CONE/a\n >\n > At first, I was confused why we needed '-f' and thought that maybe\n > this was turning into a no-op after the first deletion. However, the\n > test_perf_on_all helper does an \"echo >>$SPARSE_CONE/a\" before hand,\n > so the file exists _in the worktree_ every time. That requires '-f'\n > since otherwise Git complains that we have modifications.\n\nYeah, it took me some time to find out.\n\n > However, after the first instance the file no longer exists in the\n > index, so we are losing some testing of the index modification.\n\nSo true, I didn't realize at all.\n\n > We can fix this by resetting the index in each test loop:\n >\n >   test_perf_on_all \"git rm -f $SPARSE_CONE/a && git checkout HEAD -- \n$SPARSE_CONE/a\"\n >\n > Running this version of the test with GIT_PERF_REPEAT_COUNT=10 and\n > using the Git repository itself, I get these numbers:\n >\n > Test                              HEAD~1            HEAD\n > \n--------------------------------------------------------------------------\n > 2000.74: git rm ... (full-v3)     0.41(0.37+0.05) 0.43(0.36+0.07) +4.9%\n > 2000.75: git rm ... (full-v4)     0.38(0.34+0.05) 0.39(0.35+0.05) +2.6%\n > 2000.76: git rm ... (sparse-v3)   0.57(0.56+0.01) 0.05(0.05+0.00) -91.2%\n > 2000.77: git rm ... (sparse-v4)   0.57(0.55+0.02) 0.03(0.03+0.00) -94.7%\n >\n > Yes, the 'git checkout' command is contributing to the overall\n > numbers, but it also already has the performance improvements of\n > the sparse-index, so it contributes only a little to the performance\n > on the left.\n >\n > (Also note that the full index cases change only by amounts within\n > reasonable noise. The repeat count helps there.)\n\nNew thing learned, repeat to average out noise.\n\n--\nThanks,\nShaoxuan\n\n"},{"id":"460778","messageId":"20220807041335.1790658-1-shaoxuan.yuan02@gmail.com","threadId":"58261","inReplyTo":"20220803045118.1243087-1-shaoxuan.yuan02@gmail.com","subject":"[PATCH v2 0/4] rm: integrate with sparse-index","fromName":"Shaoxuan Yuan","fromEmail":"shaoxuan.yuan02@gmail.com","sentAt":"2022-08-07T04:13:31Z","receivedAt":"2022-08-07T04:13:58Z","isPatch":true,"sender":{"key":"shaoxuan.yuan02@gmail.com","avatar":"https://avatars.githubusercontent.com/u/46557895?v=4"},"body":"## Changes since PATCH v1 ##\n\n1. Move `ensure_not_expanded` test from the first patch to the last one.\n\n2. Mention the parameter of `pathspec_needs_expanded_index()` is\n changed to use `struct index_state`.\n\n3. Modify `ensure_not_expanded` method to record Git commands' stderr\n and stdout.\n\n4. Add a test 'rm pathspec expands index when necessary' to test\n the expected index expansion when different pathspec is supplied.\n\n5. Modify p2000 test by resetting the index in each test loop, so the\n index modification is properly tested. Update the perf stats using\n the results from the modified test.\n\n## PATCH v1 info ##\n\nTurn on sparse-index feature within `git-rm` command.\nAdd necessary modifications and test them.\n\nShaoxuan Yuan (4):\n t1092: add tests for `git-rm`\n pathspec.h: move pathspec_needs_expanded_index() from reset.c to here\n rm: expand the index only when necessary\n rm: integrate with sparse-index\n\n builtin/reset.c | 84 +------------------\n builtin/rm.c | 7 +-\n pathspec.c | 89 ++++++++++++++++++++\n pathspec.h | 12 +++\n t/perf/p2000-sparse-operations.sh | 1 +\n t/t1092-sparse-checkout-compatibility.sh | 100 ++++++++++++++++++++++-\n 6 files changed, 205 insertions(+), 88 deletions(-)\n\nRange-diff against v1:\n1: 6b424a1eb1 ! 1: ea4162c6ab t1092: add tests for `git-rm`\n @@ Commit message\n Add tests for `git-rm`, make sure it behaves as expected when\n is both inside or outside of sparse-checkout definition.\n \n - Also add ensure_not_expanded test to make sure `git-rm` does not\n - accidentally expand the index when is within the\n - sparse-checkout definition.\n -\n + Helped-by: Victoria Dye \n + Helped-by: Derrick Stolee \n Signed-off-by: Shaoxuan Yuan \n \n ## t/t1092-sparse-checkout-compatibility.sh ##\n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'mv directory from\n +\ttest_cmp folder1-sparse sparse-index-out &&\n +\ttest_sparse_match git status --porcelain=v2\n +'\n -+\n -+test_expect_failure 'sparse index is not expanded: rm' '\n -+\tinit_repos &&\n -+\n -+\tensure_not_expanded rm deep/a &&\n -+\n -+\t# test in-cone wildcard\n -+\tgit -C sparse-index reset --hard &&\n -+\tensure_not_expanded rm deep/* &&\n -+\n -+\t# test recursive rm\n -+\tgit -C sparse-index reset --hard &&\n -+\tensure_not_expanded rm -r deep\n -+'\n +\n test_done\n2: c2cf8b3c86 ! 2: 061c675c46 pathspec.h: move pathspec_needs_expanded_index() from reset.c to here\n @@ Commit message\n * Add a check in front so if the index is not sparse, return early since\n no expansion is needed.\n \n + * It now takes an arbitrary 'struct index_state' pointer instead of\n + using `the_index` and `active_cache`.\n +\n * Add documentation to the function.\n \n + Helped-by: Victoria Dye \n + Helped-by: Derrick Stolee \n Signed-off-by: Shaoxuan Yuan \n \n ## builtin/reset.c ##\n3: 443ca7a682 ! 3: 1c4a85fad3 rm: expand the index only when necessary\n @@ Metadata\n ## Commit message ##\n rm: expand the index only when necessary\n \n - Originally, rm a pathspec that is out-of-cone in a sparse-index\n - environment, Git dies with \"pathspec '' did not match any files\",\n - mainly because it does not expand the index so nothing is matched.\n -\n Remove the `ensure_full_index()` method so `git-rm` does not always\n expand the index when the expansion is unnecessary, i.e. when\n does not have any possibilities to match anything outside\n @@ Commit message\n contains wildcard that may need a full-index or the\n is simply outside of sparse-checkout definition.\n \n + Notice that the test 'rm pathspec expands index when necessary' in\n + t1092 *is* testing this code change behavior, though it will be marked\n + as 'test_expect_success' only in the next patch, where we officially\n + mark `command_requires_full_index = 0`, so the index does not expand\n + unless we tell it to do so.\n +\n + Notice that because we also want `ensure_full_index` to record the\n + stdout and stderr from Git command, a corresponding modification\n + is also included in this patch. The reason we want the \"sparse-index-out\"\n + and \"sparse-index-err\", is that we need to make sure there is no error\n + from Git command itself, so we can rely on the `test_region` result\n + and determine if the index is expanded or not.\n +\n + Helped-by: Victoria Dye \n + Helped-by: Derrick Stolee \n Signed-off-by: Shaoxuan Yuan \n \n ## builtin/rm.c ##\n @@ builtin/rm.c: int cmd_rm(int argc, const char **argv, const char *prefix)\n \tfor (i = 0; i < active_nr; i++) {\n \t\tconst struct cache_entry *ce = active_cache[i];\n \n +\n + ## t/t1092-sparse-checkout-compatibility.sh ##\n +@@ t/t1092-sparse-checkout-compatibility.sh: ensure_not_expanded () {\n + \t\tshift &&\n + \t\ttest_must_fail env \\\n + \t\t\tGIT_TRACE2_EVENT=\"$(pwd)/trace2.txt\" \\\n +-\t\t\tgit -C sparse-index \"$@\" || return 1\n ++\t\t\tgit -C sparse-index \"$@\" \\\n ++\t\t\t>sparse-index-out \\\n ++\t\t\t2>sparse-index-error || return 1\n + \telse\n + \t\tGIT_TRACE2_EVENT=\"$(pwd)/trace2.txt\" \\\n +-\t\t\tgit -C sparse-index \"$@\" || return 1\n ++\t\t\tgit -C sparse-index \"$@\" \\\n ++\t\t\t>sparse-index-out \\\n ++\t\t\t2>sparse-index-error || return 1\n + \tfi &&\n + \ttest_region ! index ensure_full_index trace2.txt\n + }\n +@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_failure 'rm pathspec outside sparse definition' '\n + \ttest_sparse_match git status --porcelain=v2\n + '\n + \n ++test_expect_failure 'rm pathspec expands index when necessary' '\n ++\tinit_repos &&\n ++\n ++\t# in-cone pathspec (do not expand)\n ++\tensure_not_expanded rm \"deep/deep*\" &&\n ++\ttest_must_be_empty sparse-index-err &&\n ++\n ++\t# out-of-cone pathspec (expand)\n ++\t! ensure_not_expanded rm --sparse \"folder1/a*\" &&\n ++\ttest_must_be_empty sparse-index-err &&\n ++\n ++\t# pathspec that should expand index\n ++\t! ensure_not_expanded rm \"*/a\" &&\n ++\ttest_must_be_empty sparse-index-err &&\n ++\n ++\t! ensure_not_expanded rm \"**a\" &&\n ++\ttest_must_be_empty sparse-index-err\n ++'\n ++\n + test_done\n4: adb62ca9bf ! 4: 861be8a91e rm: integrate with sparse-index\n @@ Commit message\n \n Enable the sparse index within the `git-rm` command.\n \n - The `p2000` tests demonstrate a ~96% execution time reduction for\n + The `p2000` tests demonstrate a ~92% execution time reduction for\n 'git rm' using a sparse index.\n \n - Test before after\n - -------------------------------------------------------------\n - 2000.74: git rm -f f2/f4/a (full-v3) 0.66 0.88 +33.0%\n - 2000.75: git rm -f f2/f4/a (full-v4) 0.67 0.75 +12.0%\n - 2000.76: git rm -f f2/f4/a (sparse-v3) 1.99 0.08 -96.0%\n - 2000.77: git rm -f f2/f4/a (sparse-v4) 2.06 0.07 -96.6%\n + Test HEAD~1 HEAD\n + --------------------------------------------------------------------------\n + 2000.74: git rm ... (full-v3) 0.41(0.37+0.05) 0.43(0.36+0.07) +4.9%\n + 2000.75: git rm ... (full-v4) 0.38(0.34+0.05) 0.39(0.35+0.05) +2.6%\n + 2000.76: git rm ... (sparse-v3) 0.57(0.56+0.01) 0.05(0.05+0.00) -91.2%\n + 2000.77: git rm ... (sparse-v4) 0.57(0.55+0.02) 0.03(0.03+0.00) -94.7%\n \n ----\n Also, normalize a behavioral difference of `git-rm` under sparse-index.\n @@ Commit message\n \n [1] https://github.com/ffyuanda/git/pull/6#discussion_r934861398\n \n + Helped-by: Victoria Dye \n + Helped-by: Derrick Stolee \n Signed-off-by: Shaoxuan Yuan \n \n ## builtin/rm.c ##\n @@ t/perf/p2000-sparse-operations.sh: test_perf_on_all git blame $SPARSE_CONE/f3/a\n test_perf_on_all git read-tree -mu HEAD\n test_perf_on_all git checkout-index -f --all\n test_perf_on_all git update-index --add --remove $SPARSE_CONE/a\n -+test_perf_on_all git rm -f $SPARSE_CONE/a\n ++test_perf_on_all \"git rm -f $SPARSE_CONE/a && git checkout HEAD -- $SPARSE_CONE/a\"\n \n test_done\n \n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_failure 'rm pathspec outsi\n \ttest_sparse_match git status --porcelain=v2\n '\n \n --test_expect_failure 'sparse index is not expanded: rm' '\n -+test_expect_success 'sparse index is not expanded: rm' '\n +-test_expect_failure 'rm pathspec expands index when necessary' '\n ++test_expect_success 'rm pathspec expands index when necessary' '\n \tinit_repos &&\n \n - \tensure_not_expanded rm deep/a &&\n + \t# in-cone pathspec (do not expand)\n +@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_failure 'rm pathspec expands index when necessary' '\n + \ttest_must_be_empty sparse-index-err\n + '\n + \n ++test_expect_success 'sparse index is not expanded: rm' '\n ++\tinit_repos &&\n ++\n ++\tensure_not_expanded rm deep/a &&\n ++\n ++\t# test in-cone wildcard\n ++\tgit -C sparse-index reset --hard &&\n ++\tensure_not_expanded rm deep/* &&\n ++\n ++\t# test recursive rm\n ++\tgit -C sparse-index reset --hard &&\n ++\tensure_not_expanded rm -r deep\n ++'\n ++\n + test_done\n\nbase-commit: 679aad9e82d0dfd8ef3d1f98fa4629665496cec9\n-- \n2.37.0\n\n"},{"id":"460779","messageId":"20220807041335.1790658-2-shaoxuan.yuan02@gmail.com","threadId":"58261","inReplyTo":"20220807041335.1790658-1-shaoxuan.yuan02@gmail.com","subject":"[PATCH v2 1/4] t1092: add tests for `git-rm`","fromName":"Shaoxuan Yuan","fromEmail":"shaoxuan.yuan02@gmail.com","sentAt":"2022-08-07T04:13:32Z","receivedAt":"2022-08-07T04:14:07Z","isPatch":true,"sender":{"key":"shaoxuan.yuan02@gmail.com","avatar":"https://avatars.githubusercontent.com/u/46557895?v=4"},"body":"Add tests for `git-rm`, make sure it behaves as expected when\n is both inside or outside of sparse-checkout definition.\n\nHelped-by: Victoria Dye \nHelped-by: Derrick Stolee \nSigned-off-by: Shaoxuan Yuan \n---\n t/t1092-sparse-checkout-compatibility.sh | 57 ++++++++++++++++++++++++\n 1 file changed, 57 insertions(+)\n\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 763c6cc684..c9300b77dd 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -1853,4 +1853,61 @@ test_expect_success 'mv directory from out-of-cone to in-cone' '\n \tgrep -e \"H deep/0/1\" actual\n '\n \n+test_expect_success 'rm pathspec inside sparse definition' '\n+\tinit_repos &&\n+\n+\ttest_all_match git rm deep/a &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\t# test wildcard\n+\trun_on_all git reset --hard &&\n+\ttest_all_match git rm deep/* &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\t# test recursive rm\n+\trun_on_all git reset --hard &&\n+\ttest_all_match git rm -r deep &&\n+\ttest_all_match git status --porcelain=v2\n+'\n+\n+test_expect_failure 'rm pathspec outside sparse definition' '\n+\tinit_repos &&\n+\n+\tfor file in folder1/a folder1/0/1\n+\tdo\n+\t\ttest_sparse_match test_must_fail git rm $file &&\n+\t\ttest_sparse_match test_must_fail git rm --cached $file &&\n+\t\ttest_sparse_match git rm --sparse $file &&\n+\t\ttest_sparse_match git status --porcelain=v2\n+\tdone &&\n+\n+\tcat >folder1-full <<-EOF &&\n+\trm ${SQ}folder1/0/0/0${SQ}\n+\trm ${SQ}folder1/0/1${SQ}\n+\trm ${SQ}folder1/a${SQ}\n+\tEOF\n+\n+\tcat >folder1-sparse <<-EOF &&\n+\trm ${SQ}folder1/${SQ}\n+\tEOF\n+\n+\t# test wildcard\n+\trun_on_sparse git reset --hard &&\n+\trun_on_sparse git sparse-checkout reapply &&\n+\ttest_sparse_match test_must_fail git rm folder1/* &&\n+\trun_on_sparse git rm --sparse folder1/* &&\n+\ttest_cmp folder1-full sparse-checkout-out &&\n+\ttest_cmp folder1-sparse sparse-index-out &&\n+\ttest_sparse_match git status --porcelain=v2 &&\n+\n+\t# test recursive rm\n+\trun_on_sparse git reset --hard &&\n+\trun_on_sparse git sparse-checkout reapply &&\n+\ttest_sparse_match test_must_fail git rm --sparse folder1 &&\n+\trun_on_sparse git rm --sparse -r folder1 &&\n+\ttest_cmp folder1-full sparse-checkout-out &&\n+\ttest_cmp folder1-sparse sparse-index-out &&\n+\ttest_sparse_match git status --porcelain=v2\n+'\n+\n test_done\n-- \n2.37.0\n\n"},{"id":"460780","messageId":"20220807041335.1790658-3-shaoxuan.yuan02@gmail.com","threadId":"58261","inReplyTo":"20220807041335.1790658-1-shaoxuan.yuan02@gmail.com","subject":"[PATCH v2 2/4] pathspec.h: move pathspec_needs_expanded_index() from reset.c to here","fromName":"Shaoxuan Yuan","fromEmail":"shaoxuan.yuan02@gmail.com","sentAt":"2022-08-07T04:13:33Z","receivedAt":"2022-08-07T04:14:13Z","isPatch":true,"sender":{"key":"shaoxuan.yuan02@gmail.com","avatar":"https://avatars.githubusercontent.com/u/46557895?v=4"},"body":"Method pathspec_needs_expanded_index() in reset.c from 4d1cfc1351\n(reset: make --mixed sparse-aware, 2021-11-29) is reusable when we\nneed to verify if the index needs to be expanded when the command\nis utilizing a pathspec rather than a literal path.\n\nMove it to pathspec.h for reusability.\n\nAdd a few items to the function so it can better serve its purpose as\na standalone public function:\n\n* Add a check in front so if the index is not sparse, return early since\n no expansion is needed.\n\n* It now takes an arbitrary 'struct index_state' pointer instead of\n using `the_index` and `active_cache`.\n\n* Add documentation to the function.\n\nHelped-by: Victoria Dye \nHelped-by: Derrick Stolee \nSigned-off-by: Shaoxuan Yuan \n---\n builtin/reset.c | 84 +---------------------------------------------\n pathspec.c | 89 +++++++++++++++++++++++++++++++++++++++++++++++++\n pathspec.h | 12 +++++++\n 3 files changed, 102 insertions(+), 83 deletions(-)\n\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 344fff8f3a..fdce6f8c85 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -174,88 +174,6 @@ static void update_index_from_diff(struct diff_queue_struct *q,\n \t}\n }\n \n-static int pathspec_needs_expanded_index(const struct pathspec *pathspec)\n-{\n-\tunsigned int i, pos;\n-\tint res = 0;\n-\tchar *skip_worktree_seen = NULL;\n-\n-\t/*\n-\t * When using a magic pathspec, assume for the sake of simplicity that\n-\t * the index needs to be expanded to match all matchable files.\n-\t */\n-\tif (pathspec->magic)\n-\t\treturn 1;\n-\n-\tfor (i = 0; i < pathspec->nr; i++) {\n-\t\tstruct pathspec_item item = pathspec->items[i];\n-\n-\t\t/*\n-\t\t * If the pathspec item has a wildcard, the index should be expanded\n-\t\t * if the pathspec has the possibility of matching a subset of entries inside\n-\t\t * of a sparse directory (but not the entire directory).\n-\t\t *\n-\t\t * If the pathspec item is a literal path, the index only needs to be expanded\n-\t\t * if a) the pathspec isn't in the sparse checkout cone (to make sure we don't\n-\t\t * expand for in-cone files) and b) it doesn't match any sparse directories\n-\t\t * (since we can reset whole sparse directories without expanding them).\n-\t\t */\n-\t\tif (item.nowildcard_len < item.len) {\n-\t\t\t/*\n-\t\t\t * Special case: if the pattern is a path inside the cone\n-\t\t\t * followed by only wildcards, the pattern cannot match\n-\t\t\t * partial sparse directories, so we know we don't need to\n-\t\t\t * expand the index.\n-\t\t\t *\n-\t\t\t * Examples:\n-\t\t\t * - in-cone/foo***: doesn't need expanded index\n-\t\t\t * - not-in-cone/bar*: may need expanded index\n-\t\t\t * - **.c: may need expanded index\n-\t\t\t */\n-\t\t\tif (strspn(item.original + item.nowildcard_len, \"*\") == item.len - item.nowildcard_len &&\n-\t\t\t path_in_cone_mode_sparse_checkout(item.original, &the_index))\n-\t\t\t\tcontinue;\n-\n-\t\t\tfor (pos = 0; pos < active_nr; pos++) {\n-\t\t\t\tstruct cache_entry *ce = active_cache[pos];\n-\n-\t\t\t\tif (!S_ISSPARSEDIR(ce->ce_mode))\n-\t\t\t\t\tcontinue;\n-\n-\t\t\t\t/*\n-\t\t\t\t * If the pre-wildcard length is longer than the sparse\n-\t\t\t\t * directory name and the sparse directory is the first\n-\t\t\t\t * component of the pathspec, need to expand the index.\n-\t\t\t\t */\n-\t\t\t\tif (item.nowildcard_len > ce_namelen(ce) &&\n-\t\t\t\t !strncmp(item.original, ce->name, ce_namelen(ce))) {\n-\t\t\t\t\tres = 1;\n-\t\t\t\t\tbreak;\n-\t\t\t\t}\n-\n-\t\t\t\t/*\n-\t\t\t\t * If the pre-wildcard length is shorter than the sparse\n-\t\t\t\t * directory and the pathspec does not match the whole\n-\t\t\t\t * directory, need to expand the index.\n-\t\t\t\t */\n-\t\t\t\tif (!strncmp(item.original, ce->name, item.nowildcard_len) &&\n-\t\t\t\t wildmatch(item.original, ce->name, 0)) {\n-\t\t\t\t\tres = 1;\n-\t\t\t\t\tbreak;\n-\t\t\t\t}\n-\t\t\t}\n-\t\t} else if (!path_in_cone_mode_sparse_checkout(item.original, &the_index) &&\n-\t\t\t !matches_skip_worktree(pathspec, i, &skip_worktree_seen))\n-\t\t\tres = 1;\n-\n-\t\tif (res > 0)\n-\t\t\tbreak;\n-\t}\n-\n-\tfree(skip_worktree_seen);\n-\treturn res;\n-}\n-\n static int read_from_tree(const struct pathspec *pathspec,\n \t\t\t struct object_id *tree_oid,\n \t\t\t int intent_to_add)\n@@ -273,7 +191,7 @@ static int read_from_tree(const struct pathspec *pathspec,\n \topt.change = diff_change;\n \topt.add_remove = diff_addremove;\n \n-\tif (pathspec->nr && the_index.sparse_index && pathspec_needs_expanded_index(pathspec))\n+\tif (pathspec->nr && pathspec_needs_expanded_index(&the_index, pathspec))\n \t\tensure_full_index(&the_index);\n \n \tif (do_diff_cache(tree_oid, &opt))\ndiff --git a/pathspec.c b/pathspec.c\nindex 84ad9c73cf..46e77a85fe 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -759,3 +759,92 @@ int match_pathspec_attrs(struct index_state *istate,\n \n \treturn 1;\n }\n+\n+int pathspec_needs_expanded_index(struct index_state *istate,\n+\t\t\t\t const struct pathspec *pathspec)\n+{\n+\tunsigned int i, pos;\n+\tint res = 0;\n+\tchar *skip_worktree_seen = NULL;\n+\n+\t/*\n+\t * If index is not sparse, no index expansion is needed.\n+\t */\n+\tif (!istate->sparse_index)\n+\t\treturn 0;\n+\n+\t/*\n+\t * When using a magic pathspec, assume for the sake of simplicity that\n+\t * the index needs to be expanded to match all matchable files.\n+\t */\n+\tif (pathspec->magic)\n+\t\treturn 1;\n+\n+\tfor (i = 0; i < pathspec->nr; i++) {\n+\t\tstruct pathspec_item item = pathspec->items[i];\n+\n+\t\t/*\n+\t\t * If the pathspec item has a wildcard, the index should be expanded\n+\t\t * if the pathspec has the possibility of matching a subset of entries inside\n+\t\t * of a sparse directory (but not the entire directory).\n+\t\t *\n+\t\t * If the pathspec item is a literal path, the index only needs to be expanded\n+\t\t * if a) the pathspec isn't in the sparse checkout cone (to make sure we don't\n+\t\t * expand for in-cone files) and b) it doesn't match any sparse directories\n+\t\t * (since we can reset whole sparse directories without expanding them).\n+\t\t */\n+\t\tif (item.nowildcard_len < item.len) {\n+\t\t\t/*\n+\t\t\t * Special case: if the pattern is a path inside the cone\n+\t\t\t * followed by only wildcards, the pattern cannot match\n+\t\t\t * partial sparse directories, so we know we don't need to\n+\t\t\t * expand the index.\n+\t\t\t *\n+\t\t\t * Examples:\n+\t\t\t * - in-cone/foo***: doesn't need expanded index\n+\t\t\t * - not-in-cone/bar*: may need expanded index\n+\t\t\t * - **.c: may need expanded index\n+\t\t\t */\n+\t\t\tif (strspn(item.original + item.nowildcard_len, \"*\") == item.len - item.nowildcard_len &&\n+\t\t\t path_in_cone_mode_sparse_checkout(item.original, istate))\n+\t\t\t\tcontinue;\n+\n+\t\t\tfor (pos = 0; pos < istate->cache_nr; pos++) {\n+\t\t\t\tstruct cache_entry *ce = istate->cache[pos];\n+\n+\t\t\t\tif (!S_ISSPARSEDIR(ce->ce_mode))\n+\t\t\t\t\tcontinue;\n+\n+\t\t\t\t/*\n+\t\t\t\t * If the pre-wildcard length is longer than the sparse\n+\t\t\t\t * directory name and the sparse directory is the first\n+\t\t\t\t * component of the pathspec, need to expand the index.\n+\t\t\t\t */\n+\t\t\t\tif (item.nowildcard_len > ce_namelen(ce) &&\n+\t\t\t\t !strncmp(item.original, ce->name, ce_namelen(ce))) {\n+\t\t\t\t\tres = 1;\n+\t\t\t\t\tbreak;\n+\t\t\t\t}\n+\n+\t\t\t\t/*\n+\t\t\t\t * If the pre-wildcard length is shorter than the sparse\n+\t\t\t\t * directory and the pathspec does not match the whole\n+\t\t\t\t * directory, need to expand the index.\n+\t\t\t\t */\n+\t\t\t\tif (!strncmp(item.original, ce->name, item.nowildcard_len) &&\n+\t\t\t\t wildmatch(item.original, ce->name, 0)) {\n+\t\t\t\t\tres = 1;\n+\t\t\t\t\tbreak;\n+\t\t\t\t}\n+\t\t\t}\n+\t\t} else if (!path_in_cone_mode_sparse_checkout(item.original, istate) &&\n+\t\t\t !matches_skip_worktree(pathspec, i, &skip_worktree_seen))\n+\t\t\tres = 1;\n+\n+\t\tif (res > 0)\n+\t\t\tbreak;\n+\t}\n+\n+\tfree(skip_worktree_seen);\n+\treturn res;\n+}\ndiff --git a/pathspec.h b/pathspec.h\nindex 402ebb8080..41f6adfbb4 100644\n--- a/pathspec.h\n+++ b/pathspec.h\n@@ -171,4 +171,16 @@ int match_pathspec_attrs(struct index_state *istate,\n \t\t\t const char *name, int namelen,\n \t\t\t const struct pathspec_item *item);\n \n+/*\n+ * Determine whether a pathspec will match only entire index entries (non-sparse\n+ * files and/or entire sparse directories). If the pathspec has the potential to\n+ * match partial contents of a sparse directory, return 1 to indicate the index\n+ * should be expanded to match the appropriate index entries.\n+ *\n+ * For the sake of simplicity, always return 1 if using a more complex \"magic\"\n+ * pathspec.\n+ */\n+int pathspec_needs_expanded_index(struct index_state *istate,\n+\t\t\t\t const struct pathspec *pathspec);\n+\n #endif /* PATHSPEC_H */\n-- \n2.37.0\n\n"},{"id":"460781","messageId":"20220807041335.1790658-4-shaoxuan.yuan02@gmail.com","threadId":"58261","inReplyTo":"20220807041335.1790658-1-shaoxuan.yuan02@gmail.com","subject":"[PATCH v2 3/4] rm: expand the index only when necessary","fromName":"Shaoxuan Yuan","fromEmail":"shaoxuan.yuan02@gmail.com","sentAt":"2022-08-07T04:13:34Z","receivedAt":"2022-08-07T04:14:14Z","isPatch":true,"sender":{"key":"shaoxuan.yuan02@gmail.com","avatar":"https://avatars.githubusercontent.com/u/46557895?v=4"},"body":"Remove the `ensure_full_index()` method so `git-rm` does not always\nexpand the index when the expansion is unnecessary, i.e. when\n does not have any possibilities to match anything outside\nof sparse-checkout definition.\n\nExpand the index when the needs an expanded index, i.e. the\n contains wildcard that may need a full-index or the\n is simply outside of sparse-checkout definition.\n\nNotice that the test 'rm pathspec expands index when necessary' in\nt1092 *is* testing this code change behavior, though it will be marked\nas 'test_expect_success' only in the next patch, where we officially\nmark `command_requires_full_index = 0`, so the index does not expand\nunless we tell it to do so.\n\nNotice that because we also want `ensure_full_index` to record the\nstdout and stderr from Git command, a corresponding modification\nis also included in this patch. The reason we want the \"sparse-index-out\"\nand \"sparse-index-err\", is that we need to make sure there is no error\nfrom Git command itself, so we can rely on the `test_region` result\nand determine if the index is expanded or not.\n\nHelped-by: Victoria Dye \nHelped-by: Derrick Stolee \nSigned-off-by: Shaoxuan Yuan \n---\n builtin/rm.c | 5 +++--\n t/t1092-sparse-checkout-compatibility.sh | 27 ++++++++++++++++++++++--\n 2 files changed, 28 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex 84a935a16e..58ed924f0d 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -296,8 +296,9 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \n \tseen = xcalloc(pathspec.nr, 1);\n \n-\t/* TODO: audit for interaction with sparse-index. */\n-\tensure_full_index(&the_index);\n+\tif (pathspec_needs_expanded_index(&the_index, &pathspec))\n+\t\tensure_full_index(&the_index);\n+\n \tfor (i = 0; i < active_nr; i++) {\n \t\tconst struct cache_entry *ce = active_cache[i];\n \ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex c9300b77dd..94464cf911 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -1340,10 +1340,14 @@ ensure_not_expanded () {\n \t\tshift &&\n \t\ttest_must_fail env \\\n \t\t\tGIT_TRACE2_EVENT=\"$(pwd)/trace2.txt\" \\\n-\t\t\tgit -C sparse-index \"$@\" || return 1\n+\t\t\tgit -C sparse-index \"$@\" \\\n+\t\t\t>sparse-index-out \\\n+\t\t\t2>sparse-index-error || return 1\n \telse\n \t\tGIT_TRACE2_EVENT=\"$(pwd)/trace2.txt\" \\\n-\t\t\tgit -C sparse-index \"$@\" || return 1\n+\t\t\tgit -C sparse-index \"$@\" \\\n+\t\t\t>sparse-index-out \\\n+\t\t\t2>sparse-index-error || return 1\n \tfi &&\n \ttest_region ! index ensure_full_index trace2.txt\n }\n@@ -1910,4 +1914,23 @@ test_expect_failure 'rm pathspec outside sparse definition' '\n \ttest_sparse_match git status --porcelain=v2\n '\n \n+test_expect_failure 'rm pathspec expands index when necessary' '\n+\tinit_repos &&\n+\n+\t# in-cone pathspec (do not expand)\n+\tensure_not_expanded rm \"deep/deep*\" &&\n+\ttest_must_be_empty sparse-index-err &&\n+\n+\t# out-of-cone pathspec (expand)\n+\t! ensure_not_expanded rm --sparse \"folder1/a*\" &&\n+\ttest_must_be_empty sparse-index-err &&\n+\n+\t# pathspec that should expand index\n+\t! ensure_not_expanded rm \"*/a\" &&\n+\ttest_must_be_empty sparse-index-err &&\n+\n+\t! ensure_not_expanded rm \"**a\" &&\n+\ttest_must_be_empty sparse-index-err\n+'\n+\n test_done\n-- \n2.37.0\n\n"},{"id":"460782","messageId":"20220807041335.1790658-5-shaoxuan.yuan02@gmail.com","threadId":"58261","inReplyTo":"20220807041335.1790658-1-shaoxuan.yuan02@gmail.com","subject":"[PATCH v2 4/4] rm: integrate with sparse-index","fromName":"Shaoxuan Yuan","fromEmail":"shaoxuan.yuan02@gmail.com","sentAt":"2022-08-07T04:13:35Z","receivedAt":"2022-08-07T04:14:16Z","isPatch":true,"sender":{"key":"shaoxuan.yuan02@gmail.com","avatar":"https://avatars.githubusercontent.com/u/46557895?v=4"},"body":"Enable the sparse index within the `git-rm` command.\n\nThe `p2000` tests demonstrate a ~92% execution time reduction for\n'git rm' using a sparse index.\n\nTest HEAD~1 HEAD\n--------------------------------------------------------------------------\n2000.74: git rm ... (full-v3) 0.41(0.37+0.05) 0.43(0.36+0.07) +4.9%\n2000.75: git rm ... (full-v4) 0.38(0.34+0.05) 0.39(0.35+0.05) +2.6%\n2000.76: git rm ... (sparse-v3) 0.57(0.56+0.01) 0.05(0.05+0.00) -91.2%\n2000.77: git rm ... (sparse-v4) 0.57(0.55+0.02) 0.03(0.03+0.00) -94.7%\n\n----\nAlso, normalize a behavioral difference of `git-rm` under sparse-index.\nSee related discussion [1].\n\n`git-rm` a sparse-directory entry within a sparse-index enabled repo\nbehaves differently from a sparse directory within a sparse-checkout\nenabled repo.\n\nFor example, in a sparse-index repo, where 'folder1' is a\nsparse-directory entry, `git rm -r --sparse folder1` provides this:\n\n rm 'folder1/'\n\nWhereas in a sparse-checkout repo *without* sparse-index, doing so\nprovides this:\n\n rm 'folder1/0/0/0'\n rm 'folder1/0/1'\n rm 'folder1/a'\n\nBecause `git rm` a sparse-directory entry does not need to expand the\nindex, therefore we should accept the current behavior, which is faster\nthan \"expand the sparse-directory entry to match the sparse-checkout\nsituation\".\n\nModify a previous test so such difference is not considered as an error.\n\n[1] https://github.com/ffyuanda/git/pull/6#discussion_r934861398\n\nHelped-by: Victoria Dye \nHelped-by: Derrick Stolee \nSigned-off-by: Shaoxuan Yuan \n---\n builtin/rm.c | 2 ++\n t/perf/p2000-sparse-operations.sh | 1 +\n t/t1092-sparse-checkout-compatibility.sh | 20 +++++++++++++++++---\n 3 files changed, 20 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex 58ed924f0d..b6ba859fe4 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -287,6 +287,8 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \tif (!index_only)\n \t\tsetup_work_tree();\n \n+\tprepare_repo_settings(the_repository);\n+\tthe_repository->settings.command_requires_full_index = 0;\n \thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n \n \tif (read_cache() < 0)\ndiff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh\nindex c181110a43..fce8151d41 100755\n--- a/t/perf/p2000-sparse-operations.sh\n+++ b/t/perf/p2000-sparse-operations.sh\n@@ -123,5 +123,6 @@ test_perf_on_all git blame $SPARSE_CONE/f3/a\n test_perf_on_all git read-tree -mu HEAD\n test_perf_on_all git checkout-index -f --all\n test_perf_on_all git update-index --add --remove $SPARSE_CONE/a\n+test_perf_on_all \"git rm -f $SPARSE_CONE/a && git checkout HEAD -- $SPARSE_CONE/a\"\n \n test_done\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 94464cf911..68ded9063b 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -912,7 +912,7 @@ test_expect_success 'read-tree --prefix' '\n \ttest_all_match git read-tree --prefix=deep/deeper1/deepest -u deepest &&\n \ttest_all_match git status --porcelain=v2 &&\n \n-\ttest_all_match git rm -rf --sparse folder1/ &&\n+\trun_on_all git rm -rf --sparse folder1/ &&\n \ttest_all_match git read-tree --prefix=folder1/ -u update-folder1 &&\n \ttest_all_match git status --porcelain=v2 &&\n \n@@ -1874,7 +1874,7 @@ test_expect_success 'rm pathspec inside sparse definition' '\n \ttest_all_match git status --porcelain=v2\n '\n \n-test_expect_failure 'rm pathspec outside sparse definition' '\n+test_expect_success 'rm pathspec outside sparse definition' '\n \tinit_repos &&\n \n \tfor file in folder1/a folder1/0/1\n@@ -1914,7 +1914,7 @@ test_expect_failure 'rm pathspec outside sparse definition' '\n \ttest_sparse_match git status --porcelain=v2\n '\n \n-test_expect_failure 'rm pathspec expands index when necessary' '\n+test_expect_success 'rm pathspec expands index when necessary' '\n \tinit_repos &&\n \n \t# in-cone pathspec (do not expand)\n@@ -1933,4 +1933,18 @@ test_expect_failure 'rm pathspec expands index when necessary' '\n \ttest_must_be_empty sparse-index-err\n '\n \n+test_expect_success 'sparse index is not expanded: rm' '\n+\tinit_repos &&\n+\n+\tensure_not_expanded rm deep/a &&\n+\n+\t# test in-cone wildcard\n+\tgit -C sparse-index reset --hard &&\n+\tensure_not_expanded rm deep/* &&\n+\n+\t# test recursive rm\n+\tgit -C sparse-index reset --hard &&\n+\tensure_not_expanded rm -r deep\n+'\n+\n test_done\n-- \n2.37.0\n\n"},{"id":"460836","messageId":"xmqqmtcesl6e.fsf@gitster.g","threadId":"58261","inReplyTo":"20220807041335.1790658-1-shaoxuan.yuan02@gmail.com","subject":"Re: [PATCH v2 0/4] rm: integrate with sparse-index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-08T17:24:25Z","receivedAt":"2022-08-08T17:24:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shaoxuan Yuan writes:\n\n> Turn on sparse-index feature within `git-rm` command.\n\nThat is a clearly written single-line summary.\n\n> Add necessary modifications and test them.\n\nThis states an obvious without adding any useful information. What\nmodifications were necessary and why they were necessary, what old\nbehaviour was undesirable and added tests prevent them to appear\nagain? These details are better left to the proposed log message of\nindividual patches.\n\nThis series, when queued on top of 'master' without anything else,\nseems to pass its own tests, but when combined with the \"reset and\ncheckout fixes\" \nby Victoria, the last one t1092 fails.\n\n---- ---- ---- ---- ---- ---- ---- ---- ---- ---- \nexpecting success of 1092.27 'reset hard with removed sparse dir':\n init_repos &&\n\n test_all_match git rm -r --sparse folder1 &&\n test_all_match git status --porcelain=v2 &&\n\n test_all_match git reset --hard &&\n test_all_match git status --porcelain=v2 &&\n\n cat >expect <<-\\EOF &&\n folder1/\n EOF\n\n git -C sparse-index ls-files --sparse folder1 >out &&\n test_cmp expect out\n\nHEAD is now at 703fd3e initial commit\nHEAD is now at 703fd3e initial commit\nHEAD is now at 703fd3e initial commit\n--- full-checkout-out 2022-08-08 17:19:19.820840016 +0000\n+++ sparse-index-out 2022-08-08 17:19:19.836841239 +0000\n@@ -1,3 +1 @@\n-rm 'folder1/0/0/0'\n-rm 'folder1/0/1'\n-rm 'folder1/a'\n+rm 'folder1/'\nnot ok 27 - reset hard with removed sparse dir\n#\n# init_repos &&\n#\n# test_all_match git rm -r --sparse folder1 &&\n# test_all_match git status --porcelain=v2 &&\n#\n# test_all_match git reset --hard &&\n# test_all_match git status --porcelain=v2 &&\n#\n# cat >expect <<-\\EOF &&\n# folder1/\n# EOF\n#\n# git -C sparse-index ls-files --sparse folder1 >out &&\n# test_cmp expect out\n#\n---- ---- ---- ---- ---- ---- ---- ---- ---- ---- \n\nWhen we have the index (incorrectly) fully expanded, and may have\n(incorrectly) working tree files outside of our sparse-cone of\ninterest, we may have paths under the 'folder1/' that we may need to\nremove (and report as removed), but after the bug that causes us to\n\"incorrectly check out\" gets fixed, perhaps the 'folder1/' is the\nonly thing that needs removed if it is outside our sparse-cone of\ninterest? IOW, is the test hardcoding the behaviour of a bug that\nwas fixed? I dunno.\n\n\n\n"},{"id":"460840","messageId":"9ae61888-f7eb-0b36-8ed6-cf72104efb9d@github.com","threadId":"58261","inReplyTo":"xmqqmtcesl6e.fsf@gitster.g","subject":"Re: [PATCH v2 0/4] rm: integrate with sparse-index","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-08-08T17:51:20Z","receivedAt":"2022-08-08T17:51:26Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Junio C Hamano wrote:\n> Shaoxuan Yuan writes:\n> \n>> Turn on sparse-index feature within `git-rm` command.\n> \n> That is a clearly written single-line summary.\n> \n>> Add necessary modifications and test them.\n> \n> This states an obvious without adding any useful information. What\n> modifications were necessary and why they were necessary, what old\n> behaviour was undesirable and added tests prevent them to appear\n> again? These details are better left to the proposed log message of\n> individual patches.\n> \n> This series, when queued on top of 'master' without anything else,\n> seems to pass its own tests, but when combined with the \"reset and\n> checkout fixes\" \n> by Victoria, the last one t1092 fails.\n> \n> ---- ---- ---- ---- ---- ---- ---- ---- ---- ---- \n> expecting success of 1092.27 'reset hard with removed sparse dir':\n> init_repos &&\n> \n> test_all_match git rm -r --sparse folder1 &&\n> test_all_match git status --porcelain=v2 &&\n> \n> test_all_match git reset --hard &&\n> test_all_match git status --porcelain=v2 &&\n> \n> cat >expect <<-\\EOF &&\n> folder1/\n> EOF\n> \n> git -C sparse-index ls-files --sparse folder1 >out &&\n> test_cmp expect out\n> \n> HEAD is now at 703fd3e initial commit\n> HEAD is now at 703fd3e initial commit\n> HEAD is now at 703fd3e initial commit\n> --- full-checkout-out 2022-08-08 17:19:19.820840016 +0000\n> +++ sparse-index-out 2022-08-08 17:19:19.836841239 +0000\n> @@ -1,3 +1 @@\n> -rm 'folder1/0/0/0'\n> -rm 'folder1/0/1'\n> -rm 'folder1/a'\n> +rm 'folder1/'\n> not ok 27 - reset hard with removed sparse dir\n> #\n> # init_repos &&\n> #\n> # test_all_match git rm -r --sparse folder1 &&\n> # test_all_match git status --porcelain=v2 &&\n> #\n> # test_all_match git reset --hard &&\n> # test_all_match git status --porcelain=v2 &&\n> #\n> # cat >expect <<-\\EOF &&\n> # folder1/\n> # EOF\n> #\n> # git -C sparse-index ls-files --sparse folder1 >out &&\n> # test_cmp expect out\n> #\n> ---- ---- ---- ---- ---- ---- ---- ---- ---- ---- \n> \n> When we have the index (incorrectly) fully expanded, and may have\n> (incorrectly) working tree files outside of our sparse-cone of\n> interest, we may have paths under the 'folder1/' that we may need to\n> remove (and report as removed), but after the bug that causes us to\n> \"incorrectly check out\" gets fixed, perhaps the 'folder1/' is the\n> only thing that needs removed if it is outside our sparse-cone of\n> interest? IOW, is the test hardcoding the behaviour of a bug that\n> was fixed? I dunno.\n> \n\nThis test failure is a result of a behavior change in the logging of 'git\nrm' in this series when removing a sparse directory. Patch 4 talks about it\nin more detail [1]; I failed to account for it in my series.\n\nI'll re-roll my series and replace the 'test_all_match' on that line to\n'run_on_all' to avoid the failure. This isn't the first conflict my series\nhas caused with this one, so I'll make sure everything builds and tests pass\nwith the changes from both series before resubmitting.\n\nThanks for catching this, and sorry for the inconvenience.\n\n[1] https://lore.kernel.org/git/20220807041335.1790658-5-shaoxuan.yuan02@gmail.com/\n"},{"id":"460845","messageId":"xmqqczdasgp4.fsf@gitster.g","threadId":"58261","inReplyTo":"9ae61888-f7eb-0b36-8ed6-cf72104efb9d@github.com","subject":"Re: [PATCH v2 0/4] rm: integrate with sparse-index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-08T19:01:11Z","receivedAt":"2022-08-08T19:01:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Victoria Dye writes:\n\n> This test failure is a result of a behavior change in the logging of 'git\n> rm' in this series when removing a sparse directory. Patch 4 talks about it\n> in more detail [1]; I failed to account for it in my series.\n\nAh, OK. When a directory being \"git rm\"'ed is outside the cone(s)\nof our interest, it indeed does feel like a waste to expand the\nindex only to report which paths in that directory are being\nremoved. It would be OK for the behaviour to be different inside\nand outside the cone(s).\n\n\n"},{"id":"460955","messageId":"2c0cb658-cd5a-420a-d313-6839149b9b40@github.com","threadId":"58261","inReplyTo":"20220807041335.1790658-4-shaoxuan.yuan02@gmail.com","subject":"Re: [PATCH v2 3/4] rm: expand the index only when necessary","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-08-10T00:24:30Z","receivedAt":"2022-08-10T00:24:37Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Shaoxuan Yuan wrote:\n> Remove the `ensure_full_index()` method so `git-rm` does not always\n> expand the index when the expansion is unnecessary, i.e. when\n> does not have any possibilities to match anything outside\n> of sparse-checkout definition.\n> \n> Expand the index when the needs an expanded index, i.e. the\n> contains wildcard that may need a full-index or the\n> is simply outside of sparse-checkout definition.\n> \n> Notice that the test 'rm pathspec expands index when necessary' in\n> t1092 *is* testing this code change behavior, though it will be marked\n> as 'test_expect_success' only in the next patch, where we officially\n> mark `command_requires_full_index = 0`, so the index does not expand\n> unless we tell it to do so.\n> \n> Notice that because we also want `ensure_full_index` to record the\n> stdout and stderr from Git command, a corresponding modification\n> is also included in this patch. The reason we want the \"sparse-index-out\"\n> and \"sparse-index-err\", is that we need to make sure there is no error\n> from Git command itself, so we can rely on the `test_region` result\n> and determine if the index is expanded or not.\n\nI think this patch might make more sense _after_ patch 4. Without the\nchanges in patch 4, modifying how a sparse index is handled here doesn't\nimmediately change any functionality. Then, patch 4 effectively makes its\nown changes (enable the sparse index) + \"turns on\" the changes from this\nseries, all at once.\n\nI usually recommend trying to make the effects of a patch testable in that\npatch (as long as it doesn't make a series more complicated/confusing). In\nthis case, it looks like you could swap the order of the commits and only\nneed to adjust the 'test_expect_success'/'test_expect_failure' settings on\nthe tests, making it a good candidate for this kind of reordering.\n\nAll that said, I don't think changing this is worth a re-roll on its own -\nit's moreso intended as \"things to consider for future series\". :) \n\n> \n> Helped-by: Victoria Dye \n> Helped-by: Derrick Stolee \n> Signed-off-by: Shaoxuan Yuan \n> ---\n> builtin/rm.c | 5 +++--\n> t/t1092-sparse-checkout-compatibility.sh | 27 ++++++++++++++++++++++--\n> 2 files changed, 28 insertions(+), 4 deletions(-)\n> \n> diff --git a/builtin/rm.c b/builtin/rm.c\n> index 84a935a16e..58ed924f0d 100644\n> --- a/builtin/rm.c\n> +++ b/builtin/rm.c\n> @@ -296,8 +296,9 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n> \n> \tseen = xcalloc(pathspec.nr, 1);\n> \n> -\t/* TODO: audit for interaction with sparse-index. */\n> -\tensure_full_index(&the_index);\n> +\tif (pathspec_needs_expanded_index(&the_index, &pathspec))\n> +\t\tensure_full_index(&the_index);\n> +\n> \tfor (i = 0; i < active_nr; i++) {\n> \t\tconst struct cache_entry *ce = active_cache[i];\n> \n> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\n> index c9300b77dd..94464cf911 100755\n> --- a/t/t1092-sparse-checkout-compatibility.sh\n> +++ b/t/t1092-sparse-checkout-compatibility.sh\n> @@ -1340,10 +1340,14 @@ ensure_not_expanded () {\n> \t\tshift &&\n> \t\ttest_must_fail env \\\n> \t\t\tGIT_TRACE2_EVENT=\"$(pwd)/trace2.txt\" \\\n> -\t\t\tgit -C sparse-index \"$@\" || return 1\n> +\t\t\tgit -C sparse-index \"$@\" \\\n> +\t\t\t>sparse-index-out \\\n> +\t\t\t2>sparse-index-error || return 1\n> \telse\n> \t\tGIT_TRACE2_EVENT=\"$(pwd)/trace2.txt\" \\\n> -\t\t\tgit -C sparse-index \"$@\" || return 1\n> +\t\t\tgit -C sparse-index \"$@\" \\\n> +\t\t\t>sparse-index-out \\\n> +\t\t\t2>sparse-index-error || return 1\n> \tfi &&\n> \ttest_region ! index ensure_full_index trace2.txt\n> }\n> @@ -1910,4 +1914,23 @@ test_expect_failure 'rm pathspec outside sparse definition' '\n> \ttest_sparse_match git status --porcelain=v2\n> '\n> \n> +test_expect_failure 'rm pathspec expands index when necessary' '\n> +\tinit_repos &&\n> +\n> +\t# in-cone pathspec (do not expand)\n> +\tensure_not_expanded rm \"deep/deep*\" &&\n> +\ttest_must_be_empty sparse-index-err &&\n> +\n> +\t# out-of-cone pathspec (expand)\n> +\t! ensure_not_expanded rm --sparse \"folder1/a*\" &&\n> +\ttest_must_be_empty sparse-index-err &&\n> +\n> +\t# pathspec that should expand index\n> +\t! ensure_not_expanded rm \"*/a\" &&\n> +\ttest_must_be_empty sparse-index-err &&\n> +\n> +\t! ensure_not_expanded rm \"**a\" &&\n> +\ttest_must_be_empty sparse-index-err\n> +'\n> +\n> test_done\n\n"},{"id":"460956","messageId":"8a76428d-e236-88bc-ec67-356b4c6f67fa@github.com","threadId":"58261","inReplyTo":"20220807041335.1790658-1-shaoxuan.yuan02@gmail.com","subject":"Re: [PATCH v2 0/4] rm: integrate with sparse-index","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-08-10T00:27:35Z","receivedAt":"2022-08-10T00:27:42Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Shaoxuan Yuan wrote:\n> ## Changes since PATCH v1 ##\n> \n> 1. Move `ensure_not_expanded` test from the first patch to the last one.\n> \n> 2. Mention the parameter of `pathspec_needs_expanded_index()` is\n> changed to use `struct index_state`.\n> \n> 3. Modify `ensure_not_expanded` method to record Git commands' stderr\n> and stdout.\n> \n> 4. Add a test 'rm pathspec expands index when necessary' to test\n> the expected index expansion when different pathspec is supplied.\n> \n> 5. Modify p2000 test by resetting the index in each test loop, so the\n> index modification is properly tested. Update the perf stats using\n> the results from the modified test.\n> \n> ## PATCH v1 info ##\n> \n> Turn on sparse-index feature within `git-rm` command.\n> Add necessary modifications and test them.\n\nOther than a completely optional recommendation on commit ordering [1], I didn't have any comments on any individual patches. This series looks good to me!\n\n[1] https://lore.kernel.org/git/2c0cb658-cd5a-420a-d313-6839149b9b40@github.com/\n\n> \n> Shaoxuan Yuan (4):\n> t1092: add tests for `git-rm`\n> pathspec.h: move pathspec_needs_expanded_index() from reset.c to here\n> rm: expand the index only when necessary\n> rm: integrate with sparse-index\n> \n> builtin/reset.c | 84 +------------------\n> builtin/rm.c | 7 +-\n> pathspec.c | 89 ++++++++++++++++++++\n> pathspec.h | 12 +++\n> t/perf/p2000-sparse-operations.sh | 1 +\n> t/t1092-sparse-checkout-compatibility.sh | 100 ++++++++++++++++++++++-\n> 6 files changed, 205 insertions(+), 88 deletions(-)\n> \n> Range-diff against v1:\n> 1: 6b424a1eb1 ! 1: ea4162c6ab t1092: add tests for `git-rm`\n> @@ Commit message\n> Add tests for `git-rm`, make sure it behaves as expected when\n> is both inside or outside of sparse-checkout definition.\n> \n> - Also add ensure_not_expanded test to make sure `git-rm` does not\n> - accidentally expand the index when is within the\n> - sparse-checkout definition.\n> -\n> + Helped-by: Victoria Dye \n> + Helped-by: Derrick Stolee \n> Signed-off-by: Shaoxuan Yuan \n> \n> ## t/t1092-sparse-checkout-compatibility.sh ##\n> @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'mv directory from\n> +\ttest_cmp folder1-sparse sparse-index-out &&\n> +\ttest_sparse_match git status --porcelain=v2\n> +'\n> -+\n> -+test_expect_failure 'sparse index is not expanded: rm' '\n> -+\tinit_repos &&\n> -+\n> -+\tensure_not_expanded rm deep/a &&\n> -+\n> -+\t# test in-cone wildcard\n> -+\tgit -C sparse-index reset --hard &&\n> -+\tensure_not_expanded rm deep/* &&\n> -+\n> -+\t# test recursive rm\n> -+\tgit -C sparse-index reset --hard &&\n> -+\tensure_not_expanded rm -r deep\n> -+'\n> +\n> test_done\n> 2: c2cf8b3c86 ! 2: 061c675c46 pathspec.h: move pathspec_needs_expanded_index() from reset.c to here\n> @@ Commit message\n> * Add a check in front so if the index is not sparse, return early since\n> no expansion is needed.\n> \n> + * It now takes an arbitrary 'struct index_state' pointer instead of\n> + using `the_index` and `active_cache`.\n> +\n> * Add documentation to the function.\n> \n> + Helped-by: Victoria Dye \n> + Helped-by: Derrick Stolee \n> Signed-off-by: Shaoxuan Yuan \n> \n> ## builtin/reset.c ##\n> 3: 443ca7a682 ! 3: 1c4a85fad3 rm: expand the index only when necessary\n> @@ Metadata\n> ## Commit message ##\n> rm: expand the index only when necessary\n> \n> - Originally, rm a pathspec that is out-of-cone in a sparse-index\n> - environment, Git dies with \"pathspec '' did not match any files\",\n> - mainly because it does not expand the index so nothing is matched.\n> -\n> Remove the `ensure_full_index()` method so `git-rm` does not always\n> expand the index when the expansion is unnecessary, i.e. when\n> does not have any possibilities to match anything outside\n> @@ Commit message\n> contains wildcard that may need a full-index or the\n> is simply outside of sparse-checkout definition.\n> \n> + Notice that the test 'rm pathspec expands index when necessary' in\n> + t1092 *is* testing this code change behavior, though it will be marked\n> + as 'test_expect_success' only in the next patch, where we officially\n> + mark `command_requires_full_index = 0`, so the index does not expand\n> + unless we tell it to do so.\n> +\n> + Notice that because we also want `ensure_full_index` to record the\n> + stdout and stderr from Git command, a corresponding modification\n> + is also included in this patch. The reason we want the \"sparse-index-out\"\n> + and \"sparse-index-err\", is that we need to make sure there is no error\n> + from Git command itself, so we can rely on the `test_region` result\n> + and determine if the index is expanded or not.\n> +\n> + Helped-by: Victoria Dye \n> + Helped-by: Derrick Stolee \n> Signed-off-by: Shaoxuan Yuan \n> \n> ## builtin/rm.c ##\n> @@ builtin/rm.c: int cmd_rm(int argc, const char **argv, const char *prefix)\n> \tfor (i = 0; i < active_nr; i++) {\n> \t\tconst struct cache_entry *ce = active_cache[i];\n> \n> +\n> + ## t/t1092-sparse-checkout-compatibility.sh ##\n> +@@ t/t1092-sparse-checkout-compatibility.sh: ensure_not_expanded () {\n> + \t\tshift &&\n> + \t\ttest_must_fail env \\\n> + \t\t\tGIT_TRACE2_EVENT=\"$(pwd)/trace2.txt\" \\\n> +-\t\t\tgit -C sparse-index \"$@\" || return 1\n> ++\t\t\tgit -C sparse-index \"$@\" \\\n> ++\t\t\t>sparse-index-out \\\n> ++\t\t\t2>sparse-index-error || return 1\n> + \telse\n> + \t\tGIT_TRACE2_EVENT=\"$(pwd)/trace2.txt\" \\\n> +-\t\t\tgit -C sparse-index \"$@\" || return 1\n> ++\t\t\tgit -C sparse-index \"$@\" \\\n> ++\t\t\t>sparse-index-out \\\n> ++\t\t\t2>sparse-index-error || return 1\n> + \tfi &&\n> + \ttest_region ! index ensure_full_index trace2.txt\n> + }\n> +@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_failure 'rm pathspec outside sparse definition' '\n> + \ttest_sparse_match git status --porcelain=v2\n> + '\n> + \n> ++test_expect_failure 'rm pathspec expands index when necessary' '\n> ++\tinit_repos &&\n> ++\n> ++\t# in-cone pathspec (do not expand)\n> ++\tensure_not_expanded rm \"deep/deep*\" &&\n> ++\ttest_must_be_empty sparse-index-err &&\n> ++\n> ++\t# out-of-cone pathspec (expand)\n> ++\t! ensure_not_expanded rm --sparse \"folder1/a*\" &&\n> ++\ttest_must_be_empty sparse-index-err &&\n> ++\n> ++\t# pathspec that should expand index\n> ++\t! ensure_not_expanded rm \"*/a\" &&\n> ++\ttest_must_be_empty sparse-index-err &&\n> ++\n> ++\t! ensure_not_expanded rm \"**a\" &&\n> ++\ttest_must_be_empty sparse-index-err\n> ++'\n> ++\n> + test_done\n> 4: adb62ca9bf ! 4: 861be8a91e rm: integrate with sparse-index\n> @@ Commit message\n> \n> Enable the sparse index within the `git-rm` command.\n> \n> - The `p2000` tests demonstrate a ~96% execution time reduction for\n> + The `p2000` tests demonstrate a ~92% execution time reduction for\n> 'git rm' using a sparse index.\n> \n> - Test before after\n> - -------------------------------------------------------------\n> - 2000.74: git rm -f f2/f4/a (full-v3) 0.66 0.88 +33.0%\n> - 2000.75: git rm -f f2/f4/a (full-v4) 0.67 0.75 +12.0%\n> - 2000.76: git rm -f f2/f4/a (sparse-v3) 1.99 0.08 -96.0%\n> - 2000.77: git rm -f f2/f4/a (sparse-v4) 2.06 0.07 -96.6%\n> + Test HEAD~1 HEAD\n> + --------------------------------------------------------------------------\n> + 2000.74: git rm ... (full-v3) 0.41(0.37+0.05) 0.43(0.36+0.07) +4.9%\n> + 2000.75: git rm ... (full-v4) 0.38(0.34+0.05) 0.39(0.35+0.05) +2.6%\n> + 2000.76: git rm ... (sparse-v3) 0.57(0.56+0.01) 0.05(0.05+0.00) -91.2%\n> + 2000.77: git rm ... (sparse-v4) 0.57(0.55+0.02) 0.03(0.03+0.00) -94.7%\n> \n> ----\n> Also, normalize a behavioral difference of `git-rm` under sparse-index.\n> @@ Commit message\n> \n> [1] https://github.com/ffyuanda/git/pull/6#discussion_r934861398\n> \n> + Helped-by: Victoria Dye \n> + Helped-by: Derrick Stolee \n> Signed-off-by: Shaoxuan Yuan \n> \n> ## builtin/rm.c ##\n> @@ t/perf/p2000-sparse-operations.sh: test_perf_on_all git blame $SPARSE_CONE/f3/a\n> test_perf_on_all git read-tree -mu HEAD\n> test_perf_on_all git checkout-index -f --all\n> test_perf_on_all git update-index --add --remove $SPARSE_CONE/a\n> -+test_perf_on_all git rm -f $SPARSE_CONE/a\n> ++test_perf_on_all \"git rm -f $SPARSE_CONE/a && git checkout HEAD -- $SPARSE_CONE/a\"\n> \n> test_done\n> \n> @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_failure 'rm pathspec outsi\n> \ttest_sparse_match git status --porcelain=v2\n> '\n> \n> --test_expect_failure 'sparse index is not expanded: rm' '\n> -+test_expect_success 'sparse index is not expanded: rm' '\n> +-test_expect_failure 'rm pathspec expands index when necessary' '\n> ++test_expect_success 'rm pathspec expands index when necessary' '\n> \tinit_repos &&\n> \n> - \tensure_not_expanded rm deep/a &&\n> + \t# in-cone pathspec (do not expand)\n> +@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_failure 'rm pathspec expands index when necessary' '\n> + \ttest_must_be_empty sparse-index-err\n> + '\n> + \n> ++test_expect_success 'sparse index is not expanded: rm' '\n> ++\tinit_repos &&\n> ++\n> ++\tensure_not_expanded rm deep/a &&\n> ++\n> ++\t# test in-cone wildcard\n> ++\tgit -C sparse-index reset --hard &&\n> ++\tensure_not_expanded rm deep/* &&\n> ++\n> ++\t# test recursive rm\n> ++\tgit -C sparse-index reset --hard &&\n> ++\tensure_not_expanded rm -r deep\n> ++'\n> ++\n> + test_done\n> \n> base-commit: 679aad9e82d0dfd8ef3d1f98fa4629665496cec9\n\n"},{"id":"460957","messageId":"5b093198-8dc0-6dcc-8a3b-6762b6dc11bf@gmail.com","threadId":"58261","inReplyTo":"8a76428d-e236-88bc-ec67-356b4c6f67fa@github.com","subject":"Re: [PATCH v2 0/4] rm: integrate with sparse-index","fromName":"Shaoxuan Yuan","fromEmail":"shaoxuan.yuan02@gmail.com","sentAt":"2022-08-10T00:31:10Z","receivedAt":"2022-08-10T00:31:19Z","isPatch":true,"sender":{"key":"shaoxuan.yuan02@gmail.com","avatar":"https://avatars.githubusercontent.com/u/46557895?v=4"},"body":"On 8/10/2022 8:27 AM, Victoria Dye wrote:\n > Shaoxuan Yuan wrote:\n >> ## Changes since PATCH v1 ##\n >>\n >> 1. Move `ensure_not_expanded` test from the first patch to the last one.\n >>\n >> 2. Mention the parameter of `pathspec_needs_expanded_index()` is\n >>    changed to use `struct index_state`.\n >>\n >> 3. Modify `ensure_not_expanded` method to record Git commands' stderr\n >>    and stdout.\n >>\n >> 4. Add a test 'rm pathspec expands index when necessary' to test\n >>    the expected index expansion when different pathspec is supplied.\n >>\n >> 5. Modify p2000 test by resetting the index in each test loop, so the\n >>    index modification is properly tested. Update the perf stats using\n >>    the results from the modified test.\n >>\n >> ## PATCH v1 info ##\n >>\n >> Turn on sparse-index feature within `git-rm` command.\n >> Add necessary modifications and test them.\n >\n > Other than a completely optional recommendation on commit ordering \n[1], I didn't have any comments on any individual patches. This series \nlooks good to me!\n >\n > [1] \nhttps://lore.kernel.org/git/2c0cb658-cd5a-420a-d313-6839149b9b40@github.com/\n\nThanks for reviewing! :)\nI think I'll just leave the commit ordering as-is.\n\n--\nThanks,\nShaoxuan\n\n\n\n"},{"id":"460978","messageId":"afc04510-3c68-0226-b366-f541ca933a14@github.com","threadId":"58261","inReplyTo":"20220807041335.1790658-2-shaoxuan.yuan02@gmail.com","subject":"Re: [PATCH v2 1/4] t1092: add tests for `git-rm`","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-08-10T12:47:32Z","receivedAt":"2022-08-10T12:47:38Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/7/22 12:13 AM, Shaoxuan Yuan wrote:\n\n> +test_expect_failure 'rm pathspec outside sparse definition' '\n\nMy only concern with this version is a minor one, and I didn't\nnotice it until this version: this test_expect_failure.\n\ntest_expect_failure doesn't help too much except to say \"something\nfails in this test\". It could be the very first command, or it\ncould be the last.\n\n> +\tinit_repos &&\n> +\n> +\tfor file in folder1/a folder1/0/1\n> +\tdo\n> +\t\ttest_sparse_match test_must_fail git rm $file &&\n> +\t\ttest_sparse_match test_must_fail git rm --cached $file &&\n> +\t\ttest_sparse_match git rm --sparse $file &&\n> +\t\ttest_sparse_match git status --porcelain=v2\n> +\tdone &&\n> +\n> +\tcat >folder1-full <<-EOF &&\n> +\trm ${SQ}folder1/0/0/0${SQ}\n> +\trm ${SQ}folder1/0/1${SQ}\n> +\trm ${SQ}folder1/a${SQ}\n> +\tEOF\n> +\n> +\tcat >folder1-sparse <<-EOF &&\n> +\trm ${SQ}folder1/${SQ}\n> +\tEOF\n\nThe difference you are demonstrating is that this output is\ndifferent. I think that at the point of this patch, they are\nthe same. The goal of this patch is to establish a common\npoint of reference for the full index and sparse index cases.\n\nIf everything below was \"test_sparse_match\" in this patch,\nthen I believe the test would pass.\n\nThe behavior changes when we enable the sparse index in the\n'rm' builtin. Demonstrating the changes to the test at that\ntime helps collect all of the different ways behavior changes\nwith a sparse index, making it really easy to audit what\nexactly is different between the modes.\n\nAnother approach would be to integrate the sparse index with\nthe builtin early, but keep the ensure_full_index() calls in\ncertain places (so we still expand to a full index) and slowly\nadd modes that do not expand. This is even trickier to do than\nto delay the test changes to the end.\n\nThat said, finding out how to organize these tests is very\ndifficult because there is a bit of a chicken-or-egg problem:\nHow can we test the custom integration logic without enabling\nthe sparse index across the entire builtin? How can we enable\nthe sparse index across the builtin without having all of the\nintegration logic implemented?\n\nSo please take my ramblings here as food for thought, but not\nany need to make changes to this series. v2 looks good to me.\n\nThanks,\n-Stolee\n"},{"id":"461131","messageId":"xmqqczd5b96r.fsf@gitster.g","threadId":"58261","inReplyTo":"8a76428d-e236-88bc-ec67-356b4c6f67fa@github.com","subject":"Re: [PATCH v2 0/4] rm: integrate with sparse-index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-12T18:36:44Z","receivedAt":"2022-08-12T18:36:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Victoria Dye writes:\n\n> Other than a completely optional recommendation on commit ordering\n> [1], I didn't have any comments on any individual patches. This\n> series looks good to me!\n\nThanks, all, for working on and reviewing these patches.\n\nLet's merge it down to 'next' soonish.\n\n"}]}