{"thread":{"id":"56137","subject":"[PATCH 0/5] Sparse Index: Integrate with 'git add'","startedAt":"2021-07-21T21:07:03Z","lastAt":"2021-07-30T12:52:49Z","messageCount":38,"participants":["Derrick Stolee via GitGitGadget","Junio C Hamano","Derrick Stolee","Elijah Newren","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"430791","messageId":"pull.999.git.1626901619.gitgitgadget@gmail.com","threadId":"56137","inReplyTo":null,"subject":"[PATCH 0/5] Sparse Index: Integrate with 'git add'","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-21T21:06:54Z","receivedAt":"2021-07-21T21:07:03Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"This patch series re-submits the 'git add' integration with sparse-index.\nThe performance gains are the same as before.\n\nIt is based on ds/commit-and-checkout-with-sparse-index.\n\nThis series was delayed from its initial submission for a couple reasons.\n\nThe first was because it was colliding with some changes in\nmt/add-rm-in-sparse-checkout, so now we are far enough along that that\nbranch is in our history and we can work forwards.\n\nThe other concern was about how 'git add ' should respond when a path\noutside of the sparse-checkout cone exists. One recommendation (that I am\nfailing to find a link to the message, sorry) was to disallow adding files\nthat would become index entries with SKIP_WORKTREE on. However, as I worked\ntowards that goal I found that change would cause problems for a realistic\nscenario: merge conflicts outside of the sparse-checkout cone.\n\nThe first patch of this series adds tests that create merge conflicts\noutside of the sparse cone and then presents different ways a user could\nresolve the situation. We want all of them to be feasible, and this\nincludes:\n\n 1. Reverting the file to a known version in history.\n 2. Adding the file with its contents on disk.\n 3. Moving the file to a new location in the sparse directory.\n\nWithout maintaining the behavior of adding files outside of the\nsparse-checkout cone, we risk confusing users who get into this state. The\nonly workaround they would have is to modify their sparse-checkout\ndefinition which might lead to expanding much more data than they need to\nresolve the conflicts.\n\nFor that reason, I stopped trying to limit 'git add' to be within the cone.\nI'm open to suggestions here, but we need an approach that works for\nout-of-cone conflicts.\n\nThe one place I did continue to update is 'git add --refresh ' to match the\nbehavior added by mt/add-rm-in-sparse-checkout which outputs an error\nmessage. This happens even when the file exists in the working directory,\nbut that seems appropriate enough.\n\nThanks, -Stolee\n\nDerrick Stolee (5):\n t1092: test merge conflicts outside cone\n add: allow operating on a sparse-only index\n pathspec: stop calling ensure_full_index\n t1092: 'git add --refresh' difference with sparse-index\n add: ignore outside the sparse-checkout in refresh()\n\n builtin/add.c | 13 ++++-\n pathspec.c | 2 -\n t/t1092-sparse-checkout-compatibility.sh | 71 ++++++++++++++++++++----\n 3 files changed, 72 insertions(+), 14 deletions(-)\n\n\nbase-commit: 71e301501c88399711a1bf8515d1747e92cfbb9b\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-999%2Fderrickstolee%2Fsparse-index%2Fadd-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-999/derrickstolee/sparse-index/add-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/999\n-- \ngitgitgadget\n"},{"id":"430792","messageId":"a763a7d15b8e92dec61c1fa328ecb647b6f61682.1626901619.git.gitgitgadget@gmail.com","threadId":"56137","inReplyTo":"pull.999.git.1626901619.gitgitgadget@gmail.com","subject":"[PATCH 1/5] t1092: test merge conflicts outside cone","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-21T21:06:55Z","receivedAt":"2021-07-21T21:07:03Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nSigned-off-by: Derrick Stolee \n---\n t/t1092-sparse-checkout-compatibility.sh | 37 ++++++++++++++++++++++++\n 1 file changed, 37 insertions(+)\n\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 91e30d6ec22..a3c01d588d8 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -114,6 +114,16 @@ test_expect_success 'setup' '\n \t\tgit add . &&\n \t\tgit commit -m \"file to dir\" &&\n \n+\t\tfor side in left right\n+\t\tdo\n+\t\t\tgit checkout -b merge-$side base &&\n+\t\t\techo $side >>deep/deeper2/a &&\n+\t\t\techo $side >>folder1/a &&\n+\t\t\techo $side >>folder2/a &&\n+\t\t\tgit add . &&\n+\t\t\tgit commit -m \"$side\" || return 1\n+\t\tdone &&\n+\n \t\tgit checkout -b deepest base &&\n \t\techo \"updated deepest\" >deep/deeper1/deepest/a &&\n \t\tgit commit -a -m \"update deepest\" &&\n@@ -482,6 +492,33 @@ test_expect_success 'merge' '\n \ttest_all_match git rev-parse HEAD^{tree}\n '\n \n+test_expect_success 'merge with conflict outside cone' '\n+\tinit_repos &&\n+\n+\ttest_all_match git checkout -b merge-tip merge-left &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\ttest_all_match test_must_fail git merge -m merge merge-right &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\t# resolve the conflict in different ways:\n+\t# 1. revert to the base\n+\ttest_all_match git checkout base -- deep/deeper2/a &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\t# 2. add the file with conflict markers\n+\ttest_all_match git add folder1/a &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\t# 3. rename the file to another sparse filename\n+\trun_on_all mv folder2/a folder2/z &&\n+\ttest_all_match git add folder2 &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\ttest_all_match git merge --continue &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\ttest_all_match git rev-parse HEAD^{tree}\n+'\n+\n test_expect_success 'merge with outside renames' '\n \tinit_repos &&\n \n-- \ngitgitgadget\n\n"},{"id":"430793","messageId":"791c6c2c9ade5b065fc0f367e00c52a493d086ef.1626901619.git.gitgitgadget@gmail.com","threadId":"56137","inReplyTo":"pull.999.git.1626901619.gitgitgadget@gmail.com","subject":"[PATCH 2/5] add: allow operating on a sparse-only index","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-21T21:06:56Z","receivedAt":"2021-07-21T21:07:06Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nDisable command_requires_full_index for 'git add'. This does not require\nany additional removals of ensure_full_index(). The main reason is that\n'git add' discovers changes based on the pathspec and the worktree\nitself. These are then inserted into the index directly, and calls to\nindex_name_pos() or index_file_exists() already call expand_to_path() at\nthe appropriate time to support a sparse-index.\n\nAdd a test to check that 'git add -A' and 'git add ' does not\nexpand the index at all, as long as is not within a sparse\ndirectory. This does not help the global 'git add .' case.\n\nWe can measure the improvement using p2000-sparse-operations.sh with\nthese results:\n\nTest HEAD~1 HEAD\n------------------------------------------------------------------------------\n2000.6: git add -A (full-index-v3) 0.35(0.30+0.05) 0.37(0.29+0.06) +5.7%\n2000.7: git add -A (full-index-v4) 0.31(0.26+0.06) 0.33(0.27+0.06) +6.5%\n2000.8: git add -A (sparse-index-v3) 0.57(0.53+0.07) 0.05(0.04+0.08) -91.2%\n2000.9: git add -A (sparse-index-v4) 0.58(0.55+0.06) 0.05(0.05+0.06) -91.4%\n\nWhile the 91% improvement seems impressive, it's important to recognize\nthat previously we had significant overhead for expanding the\nsparse-index. Comparing to the full index case, 'git add -A' goes from\n0.37s to 0.05s, which is \"only\" an 86% improvement.\n\nSigned-off-by: Derrick Stolee \n---\n builtin/add.c | 3 +++\n t/t1092-sparse-checkout-compatibility.sh | 14 ++++++--------\n 2 files changed, 9 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex b773b5a4993..c76e6ddd359 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -528,6 +528,9 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \tadd_new_files = !take_worktree_changes && !refresh_only && !add_renormalize;\n \trequire_pathspec = !(take_worktree_changes || (0 < addremove_explicit));\n \n+\tprepare_repo_settings(the_repository);\n+\tthe_repository->settings.command_requires_full_index = 0;\n+\n \thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n \n \t/*\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex a3c01d588d8..a11d9d7f35d 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -340,13 +340,6 @@ test_expect_success 'status/add: outside sparse cone' '\n \n \ttest_sparse_match git status --porcelain=v2 &&\n \n-\t# This \"git add folder1/a\" fails with a warning\n-\t# in the sparse repos, differing from the full\n-\t# repo. This is intentional.\n-\ttest_sparse_match test_must_fail git add folder1/a &&\n-\ttest_sparse_match test_must_fail git add --refresh folder1/a &&\n-\ttest_all_match git status --porcelain=v2 &&\n-\n \ttest_all_match git add . &&\n \ttest_all_match git status --porcelain=v2 &&\n \ttest_all_match git commit -m folder1/new &&\n@@ -635,7 +628,12 @@ test_expect_success 'sparse-index is not expanded' '\n \tgit -C sparse-index reset --hard &&\n \tensure_not_expanded checkout rename-out-to-out -- deep/deeper1 &&\n \tgit -C sparse-index reset --hard &&\n-\tensure_not_expanded restore -s rename-out-to-out -- deep/deeper1\n+\tensure_not_expanded restore -s rename-out-to-out -- deep/deeper1 &&\n+\n+\techo >>sparse-index/README.md &&\n+\tensure_not_expanded add -A &&\n+\techo >>sparse-index/extra.txt &&\n+\tensure_not_expanded add extra.txt\n '\n \n # NEEDSWORK: a sparse-checkout behaves differently from a full checkout\n-- \ngitgitgadget\n\n"},{"id":"430794","messageId":"a577ea4c74d86e57defcc4f45011871b634bcf56.1626901619.git.gitgitgadget@gmail.com","threadId":"56137","inReplyTo":"pull.999.git.1626901619.gitgitgadget@gmail.com","subject":"[PATCH 3/5] pathspec: stop calling ensure_full_index","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-21T21:06:57Z","receivedAt":"2021-07-21T21:07:07Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe add_pathspec_matches_against_index() focuses on matching a pathspec\nto file entries in the index. This already works correctly for its only\nuse: checking if untracked files exist in the index.\n\nThe compatibility checks in t1092 already test that 'git add '\nworks for a directory outside of the sparse cone. That provides coverage\nfor removing this guard.\n\nThis finalizes our ability to run 'git add .' without expanding a sparse\nindex to a full one. This is evidenced by an update to t1092 and by\nthese performance numbers for p2000-sparse-operations.sh:\n\nTest HEAD~1 HEAD\n--------------------------------------------------------------------------------\n2000.10: git add . (full-index-v3) 0.37(0.28+0.07) 0.36(0.27+0.06) -2.7%\n2000.11: git add . (full-index-v4) 0.33(0.26+0.06) 0.32(0.28+0.05) -3.0%\n2000.12: git add . (sparse-index-v3) 0.57(0.53+0.07) 0.06(0.06+0.07) -89.5%\n2000.13: git add . (sparse-index-v4) 0.57(0.53+0.07) 0.05(0.03+0.09) -91.2%\n\nWhile the ~90% improvement is shown by the test results, it is worth\nnoting that expanding the sparse index was adding overhead in previous\ncommits. Comparing to the full index case, we see the performance go\nfrom 0.33s to 0.05s, an 85% improvement.\n\nSigned-off-by: Derrick Stolee \n---\n pathspec.c | 2 --\n t/t1092-sparse-checkout-compatibility.sh | 7 +++----\n 2 files changed, 3 insertions(+), 6 deletions(-)\n\ndiff --git a/pathspec.c b/pathspec.c\nindex 08f8d3eedc3..44306fdaca2 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -37,8 +37,6 @@ void add_pathspec_matches_against_index(const struct pathspec *pathspec,\n \t\t\tnum_unmatched++;\n \tif (!num_unmatched)\n \t\treturn;\n-\t/* TODO: audit for interaction with sparse-index. */\n-\tensure_full_index(istate);\n \tfor (i = 0; i < istate->cache_nr; i++) {\n \t\tconst struct cache_entry *ce = istate->cache[i];\n \t\tif (sw_action == PS_IGNORE_SKIP_WORKTREE && ce_skip_worktree(ce))\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex a11d9d7f35d..f9e2f5f4aa1 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -322,9 +322,6 @@ test_expect_success 'commit including unstaged changes' '\n test_expect_success 'status/add: outside sparse cone' '\n \tinit_repos &&\n \n-\t# adding a \"missing\" file outside the cone should fail\n-\ttest_sparse_match test_must_fail git add folder1/a &&\n-\n \t# folder1 is at HEAD, but outside the sparse cone\n \trun_on_sparse mkdir folder1 &&\n \tcp initial-repo/folder1/a sparse-checkout/folder1/a &&\n@@ -633,7 +630,9 @@ test_expect_success 'sparse-index is not expanded' '\n \techo >>sparse-index/README.md &&\n \tensure_not_expanded add -A &&\n \techo >>sparse-index/extra.txt &&\n-\tensure_not_expanded add extra.txt\n+\tensure_not_expanded add extra.txt &&\n+\techo >>sparse-index/untracked.txt &&\n+\tensure_not_expanded add .\n '\n \n # NEEDSWORK: a sparse-checkout behaves differently from a full checkout\n-- \ngitgitgadget\n\n"},{"id":"430795","messageId":"89ec6a7ce677815cdd9889fb90c8c19f8f6dd28f.1626901619.git.gitgitgadget@gmail.com","threadId":"56137","inReplyTo":"pull.999.git.1626901619.gitgitgadget@gmail.com","subject":"[PATCH 4/5] t1092: 'git add --refresh' difference with sparse-index","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-21T21:06:58Z","receivedAt":"2021-07-21T21:07:08Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nWhen running 'git add --refresh ', Git does not actually stage the\nchange, but instead updates the index based on the stat() information in\nthe working directory.\n\nThis typically does not make sense in a sparse-checkout scenario, where\nGit wants this file to not exist. However, sometimes the file can exist\non-disk for other reasons, such as a user manually adding the file or a\nmerge conflict outside of the sparse cone.\n\nSince b243012 (refresh_index(): add flag to ignore SKIP_WORKTREE\nentries, 2021-04-08), 'git add --refresh ' will output an advice\nmessage to indicate that this is not allowed when is outside the\nsparse cone. The check goes around the sparse index protections at the\nmoment, so it does not find a match when contained in a sparse directory\nentry. We will update this behavior with a later change, but want to\ndemonstrate the failure now.\n\nSigned-off-by: Derrick Stolee \n---\n t/t1092-sparse-checkout-compatibility.sh | 19 +++++++++++++++++++\n 1 file changed, 19 insertions(+)\n\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex f9e2f5f4aa1..73c48a71d89 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -347,6 +347,25 @@ test_expect_success 'status/add: outside sparse cone' '\n \ttest_all_match git commit -m folder1/newer\n '\n \n+test_expect_failure 'add: pathspec within sparse directory' '\n+\tinit_repos &&\n+\n+\trun_on_sparse mkdir folder1 &&\n+\trun_on_sparse ../edit-contents folder1/a &&\n+\trun_on_all ../edit-contents folder1/new &&\n+\n+\t# This \"git add folder1/a\" fails with a warning\n+\t# in the sparse repos, differing from the full\n+\t# repo. This is intentional.\n+\t#\n+\t# However, in the sparse-index, folder1/a does not\n+\t# match any cache entry and fails with a different\n+\t# error message. This needs work.\n+\ttest_sparse_match test_must_fail git add folder1/a &&\n+\ttest_sparse_match test_must_fail git add --refresh folder1/a &&\n+\ttest_all_match git status --porcelain=v2\n+'\n+\n test_expect_success 'checkout and reset --hard' '\n \tinit_repos &&\n \n-- \ngitgitgadget\n\n"},{"id":"430796","messageId":"76066a78ce077add4bc69819b017516e12ac0d45.1626901619.git.gitgitgadget@gmail.com","threadId":"56137","inReplyTo":"pull.999.git.1626901619.gitgitgadget@gmail.com","subject":"[PATCH 5/5] add: ignore outside the sparse-checkout in refresh()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-21T21:06:59Z","receivedAt":"2021-07-21T21:07:09Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nSince b243012 (refresh_index(): add flag to ignore SKIP_WORKTREE\nentries, 2021-04-08), 'git add --refresh ' will output a warning\nmessage when the path is outside the sparse-checkout definition. The\nimplementation of this warning happened in parallel with the\nsparse-index work to add ensure_full_index() calls throughout the\ncodebase.\n\nUpdate this loop to have the proper logic that checks to see if the\npathspec is outside the sparse-checkout definition. This avoids the need\nto expand the sparse directory entry and determine if the path is\ntracked, untracked, or ignored. We simply avoid updating the stat()\ninformation because there isn't even an entry that matches the path!\n\nSigned-off-by: Derrick Stolee \n---\n builtin/add.c | 10 +++++++++-\n t/t1092-sparse-checkout-compatibility.sh | 6 +-----\n 2 files changed, 10 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex c76e6ddd359..d512ece655b 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -192,13 +192,21 @@ static int refresh(int verbose, const struct pathspec *pathspec)\n \tstruct string_list only_match_skip_worktree = STRING_LIST_INIT_NODUP;\n \tint flags = REFRESH_IGNORE_SKIP_WORKTREE |\n \t\t (verbose ? REFRESH_IN_PORCELAIN : REFRESH_QUIET);\n+\tstruct pattern_list pl = { 0 };\n+\tint sparse_checkout_enabled = !get_sparse_checkout_patterns(&pl);\n \n \tseen = xcalloc(pathspec->nr, 1);\n \trefresh_index(&the_index, flags, pathspec, seen,\n \t\t _(\"Unstaged changes after refreshing the index:\"));\n \tfor (i = 0; i < pathspec->nr; i++) {\n \t\tif (!seen[i]) {\n-\t\t\tif (matches_skip_worktree(pathspec, i, &skip_worktree_seen)) {\n+\t\t\tconst char *path = pathspec->items[i].original;\n+\t\t\tint dtype = DT_REG;\n+\n+\t\t\tif (matches_skip_worktree(pathspec, i, &skip_worktree_seen) ||\n+\t\t\t (sparse_checkout_enabled &&\n+\t\t\t !path_matches_pattern_list(path, strlen(path), NULL,\n+\t\t\t\t\t\t\t&dtype, &pl, &the_index))) {\n \t\t\t\tstring_list_append(&only_match_skip_worktree,\n \t\t\t\t\t\t pathspec->items[i].original);\n \t\t\t} else {\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 73c48a71d89..c61424e2074 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -347,7 +347,7 @@ test_expect_success 'status/add: outside sparse cone' '\n \ttest_all_match git commit -m folder1/newer\n '\n \n-test_expect_failure 'add: pathspec within sparse directory' '\n+test_expect_success 'add: pathspec within sparse directory' '\n \tinit_repos &&\n \n \trun_on_sparse mkdir folder1 &&\n@@ -357,10 +357,6 @@ test_expect_failure 'add: pathspec within sparse directory' '\n \t# This \"git add folder1/a\" fails with a warning\n \t# in the sparse repos, differing from the full\n \t# repo. This is intentional.\n-\t#\n-\t# However, in the sparse-index, folder1/a does not\n-\t# match any cache entry and fails with a different\n-\t# error message. This needs work.\n \ttest_sparse_match test_must_fail git add folder1/a &&\n \ttest_sparse_match test_must_fail git add --refresh folder1/a &&\n \ttest_all_match git status --porcelain=v2\n-- \ngitgitgadget\n"},{"id":"430799","messageId":"xmqqfsw7cnsv.fsf@gitster.g","threadId":"56137","inReplyTo":"791c6c2c9ade5b065fc0f367e00c52a493d086ef.1626901619.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/5] add: allow operating on a sparse-only index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-21T22:19:44Z","receivedAt":"2021-07-21T22:19:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> From: Derrick Stolee \n>\n> Disable command_requires_full_index for 'git add'. This does not require\n> any additional removals of ensure_full_index(). The main reason is that\n> 'git add' discovers changes based on the pathspec and the worktree\n> itself. These are then inserted into the index directly, and calls to\n> index_name_pos() or index_file_exists() already call expand_to_path() at\n> the appropriate time to support a sparse-index.\n\nOK. With that explained, it still is quite surprising that we only\nneed this change (eh, rather, doing this change is safe without\ndoing anything else).\n\n> -\t# This \"git add folder1/a\" fails with a warning\n> -\t# in the sparse repos, differing from the full\n> -\t# repo. This is intentional.\n> -\ttest_sparse_match test_must_fail git add folder1/a &&\n> -\ttest_sparse_match test_must_fail git add --refresh folder1/a &&\n> -\ttest_all_match git status --porcelain=v2 &&\n\nAnd nice to see a known limitation lifted.\n\n> \ttest_all_match git add . &&\n> \ttest_all_match git status --porcelain=v2 &&\n> \ttest_all_match git commit -m folder1/new &&\n> @@ -635,7 +628,12 @@ test_expect_success 'sparse-index is not expanded' '\n> \tgit -C sparse-index reset --hard &&\n> \tensure_not_expanded checkout rename-out-to-out -- deep/deeper1 &&\n> \tgit -C sparse-index reset --hard &&\n> -\tensure_not_expanded restore -s rename-out-to-out -- deep/deeper1\n> +\tensure_not_expanded restore -s rename-out-to-out -- deep/deeper1 &&\n> +\n> +\techo >>sparse-index/README.md &&\n> +\tensure_not_expanded add -A &&\n> +\techo >>sparse-index/extra.txt &&\n> +\tensure_not_expanded add extra.txt\n> '\n> \n> # NEEDSWORK: a sparse-checkout behaves differently from a full checkout\n"},{"id":"430808","messageId":"1c7170b3-c7e3-8cb6-0cb9-5dc96c7e7ce1@gmail.com","threadId":"56137","inReplyTo":"xmqqfsw7cnsv.fsf@gitster.g","subject":"Re: [PATCH 2/5] add: allow operating on a sparse-only index","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-07-21T22:50:17Z","receivedAt":"2021-07-21T22:50:24Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/21/21 6:19 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" writes:\n> \n>> From: Derrick Stolee \n>>\n>> Disable command_requires_full_index for 'git add'. This does not require\n>> any additional removals of ensure_full_index(). The main reason is that\n>> 'git add' discovers changes based on the pathspec and the worktree\n>> itself. These are then inserted into the index directly, and calls to\n>> index_name_pos() or index_file_exists() already call expand_to_path() at\n>> the appropriate time to support a sparse-index.\n> \n> OK. With that explained, it still is quite surprising that we only\n> need this change (eh, rather, doing this change is safe without\n> doing anything else).\n\nYes, all of the hard work was done by the earlier work to expand\na sparse index when we search for a specific path that lands\nwithin a sparse directory. See 95e0321 (read-cache: expand on query\ninto sparse-directory entry, 2021-04-01) for the specifics.\n\n>> -\t# This \"git add folder1/a\" fails with a warning\n>> -\t# in the sparse repos, differing from the full\n>> -\t# repo. This is intentional.\n>> -\ttest_sparse_match test_must_fail git add folder1/a &&\n>> -\ttest_sparse_match test_must_fail git add --refresh folder1/a &&\n>> -\ttest_all_match git status --porcelain=v2 &&\n> \n> And nice to see a known limitation lifted.\n\nThank you for pointing this out. This actually starts to _fail_ now\nthat we allow sparse indexes in 'git add', but it's because the error\nmessages don't match, not that the 'test_must_fail' is violated.\n\nPatch 4 adds a similar test that is then set to work in patch 5. That\nallows us a clear way to describe the behavior change and to motivate\nthe fix in patch 5. This could be explained better, perhaps by merging\nPatch 4 into this one. That helps describe how this specific case\nchanges behavior (for the worse) in this patch, but is handled in a\ncareful way later, once the behavior change is documented.\n\nIf there is a better way to reorganize these patches, then I could\ntry another approach.\n\nThanks,\n-Stolee\n\n"},{"id":"431029","messageId":"CABPp-BGBwbV7x8i=OVbPnMgWcfdLrEh3-yUi=Amw+XBwpf7A6Q@mail.gmail.com","threadId":"56137","inReplyTo":"pull.999.git.1626901619.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/5] Sparse Index: Integrate with 'git add'","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-07-23T12:51:52Z","receivedAt":"2021-07-23T12:52:06Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi,\n\nI haven't read the series yet (I'm going to try to read them all\ntoday), but wanted to comment on a couple things in your cover\nletter...\n\nOn Wed, Jul 21, 2021 at 2:07 PM Derrick Stolee via GitGitGadget\n wrote:\n>\n> This patch series re-submits the 'git add' integration with sparse-index.\n> The performance gains are the same as before.\n>\n> It is based on ds/commit-and-checkout-with-sparse-index.\n>\n> This series was delayed from its initial submission for a couple reasons.\n>\n> The first was because it was colliding with some changes in\n> mt/add-rm-in-sparse-checkout, so now we are far enough along that that\n> branch is in our history and we can work forwards.\n>\n> The other concern was about how 'git add ' should respond when a path\n> outside of the sparse-checkout cone exists. One recommendation (that I am\n> failing to find a link to the message, sorry) was to disallow adding files\n> that would become index entries with SKIP_WORKTREE on. However, as I worked\n\nI think the recommendation was:\n * Permitted: running git add on tracked files with the SKIP_WORKTREE\nbit *clear*\n * Disallowed: running git add on tracked files with the\nSKIP_WORKTREE bit *set*\n * Disallowed: running git add on untracked files that would become\nindex entries with the SKIP_WORKTREE bit set\n\nwhere the latter two exit with an error message that suggests changing\nthe sparsity specification first. I think this is what has existed\nfor some time, other than Matheus adding some error messages to help\nthe user when their add command doesn't match anything otherwise.\n\n> towards that goal I found that change would cause problems for a realistic\n> scenario: merge conflicts outside of the sparse-checkout cone.\n\n...which wouldn't be a problem because these are tracked files whose\nSKIP_WORKTREE bit was cleared by the merge machinery (when it marked\nthem as conflicted and wrote them to the working tree).\n\n> The first patch of this series adds tests that create merge conflicts\n> outside of the sparse cone and then presents different ways a user could\n> resolve the situation. We want all of them to be feasible, and this\n> includes:\n>\n> 1. Reverting the file to a known version in history.\n> 2. Adding the file with its contents on disk.\n> 3. Moving the file to a new location in the sparse directory.\n>\n> Without maintaining the behavior of adding files outside of the\n> sparse-checkout cone, we risk confusing users who get into this state. The\n> only workaround they would have is to modify their sparse-checkout\n> definition which might lead to expanding much more data than they need to\n> resolve the conflicts.\n>\n> For that reason, I stopped trying to limit 'git add' to be within the cone.\n> I'm open to suggestions here, but we need an approach that works for\n> out-of-cone conflicts.\n\nI believe my above suggestion works for out-of-cone conflicts. Some\nimportant other details to keep in mind in regards to how we make add\nbehave:\n\n* We don't want \"git add -A [GLOB_OR_DIRECTORY]\" to accidentally be\ntreated as a directive to remove files from the repository (and\nnaively noticing that SKIP_WORKTREE files are missing but attempting\nto operate on them anyway would give this problematic result).\n* We don't want \"git rm [GLOB_OR_DIRECTORY]\" to nuke SKIP_WORKTREE\nfiles; it should only operate on files that are present.\n* We need add and rm to be consistent with each other in terms of how\nSKIP_WORKTREE files and the sparsity cone are treated\n\nand more generally about not-SKIP_WORKTREE-despite-not-matching-sparsity-paths:\n\n* These files that aren't SKIP_WORKTREE but normally would be are\nprone to \"disappear\" at some random later time after they are made to\nmatch the index. The disappearing can happen either with an explicit\n\"git sparse-checkout reapply\" (which is fine since it was explicit) or\nas a side-effect of various other commands that run through\nunpack_trees() since it attempts to heed the sparsity rules. Users\ntend to get confused by the latter case; they'll understand at some\npoint that it was because the file was outside the sparsity paths, but\nthe randomness in when it's pulled out as a side-effect of other\ncommands can be slightly jarring. So, I'd like to avoid that where we\ncan easily do so, which I think the recommendation above does. (As a\nside note to these kinds of cases, maybe we want to consider having\nthe sparsification logic in unpack_trees() first check whether paths\nbeing removed from the working tree and having their SKIP_WORKTREE bit\nset not only match the index but also match HEAD? That'd be a bit\nmore expensive to check in the sparsification paths, and I'm not sure\nthey all have the relevant information, but it's an idea...)\n\n> The one place I did continue to update is 'git add --refresh ' to match the\n> behavior added by mt/add-rm-in-sparse-checkout which outputs an error\n> message. This happens even when the file exists in the working directory,\n> but that seems appropriate enough.\n>\n> Thanks, -Stolee\n>\n> Derrick Stolee (5):\n> t1092: test merge conflicts outside cone\n> add: allow operating on a sparse-only index\n> pathspec: stop calling ensure_full_index\n> t1092: 'git add --refresh' difference with sparse-index\n> add: ignore outside the sparse-checkout in refresh()\n>\n> builtin/add.c | 13 ++++-\n> pathspec.c | 2 -\n> t/t1092-sparse-checkout-compatibility.sh | 71 ++++++++++++++++++++----\n> 3 files changed, 72 insertions(+), 14 deletions(-)\n>\n>\n> base-commit: 71e301501c88399711a1bf8515d1747e92cfbb9b\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-999%2Fderrickstolee%2Fsparse-index%2Fadd-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-999/derrickstolee/sparse-index/add-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/999\n> --\n> gitgitgadget\n"},{"id":"431066","messageId":"CABPp-BF-UVZKWmsohjCzRLpbHZii+1g=SShEYg_cwScsOHi=5g@mail.gmail.com","threadId":"56137","inReplyTo":"a763a7d15b8e92dec61c1fa328ecb647b6f61682.1626901619.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/5] t1092: test merge conflicts outside cone","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-07-23T17:34:00Z","receivedAt":"2021-07-23T17:34:22Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Jul 21, 2021 at 2:07 PM Derrick Stolee via GitGitGadget\n wrote:\n>\n> From: Derrick Stolee \n>\n> Signed-off-by: Derrick Stolee \n> ---\n> t/t1092-sparse-checkout-compatibility.sh | 37 ++++++++++++++++++++++++\n> 1 file changed, 37 insertions(+)\n>\n> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\n> index 91e30d6ec22..a3c01d588d8 100755\n> --- a/t/t1092-sparse-checkout-compatibility.sh\n> +++ b/t/t1092-sparse-checkout-compatibility.sh\n> @@ -114,6 +114,16 @@ test_expect_success 'setup' '\n> git add . &&\n> git commit -m \"file to dir\" &&\n>\n> + for side in left right\n> + do\n> + git checkout -b merge-$side base &&\n> + echo $side >>deep/deeper2/a &&\n> + echo $side >>folder1/a &&\n> + echo $side >>folder2/a &&\n> + git add . &&\n> + git commit -m \"$side\" || return 1\n\nWhy is this \"|| return 1\" here?\n\nIt looks like there are a number of other cases of this in the file\ntoo, which I must have overlooked previously, because I don't\nunderstand any of them.\n\n> + done &&\n> +\n> git checkout -b deepest base &&\n> echo \"updated deepest\" >deep/deeper1/deepest/a &&\n> git commit -a -m \"update deepest\" &&\n> @@ -482,6 +492,33 @@ test_expect_success 'merge' '\n> test_all_match git rev-parse HEAD^{tree}\n> '\n>\n> +test_expect_success 'merge with conflict outside cone' '\n> + init_repos &&\n> +\n> + test_all_match git checkout -b merge-tip merge-left &&\n> + test_all_match git status --porcelain=v2 &&\n> + test_all_match test_must_fail git merge -m merge merge-right &&\n> + test_all_match git status --porcelain=v2 &&\n> +\n> + # resolve the conflict in different ways:\n> + # 1. revert to the base\n> + test_all_match git checkout base -- deep/deeper2/a &&\n> + test_all_match git status --porcelain=v2 &&\n> +\n> + # 2. add the file with conflict markers\n> + test_all_match git add folder1/a &&\n> + test_all_match git status --porcelain=v2 &&\n> +\n> + # 3. rename the file to another sparse filename\n\nBut...that doesn't resolve the conflict. Shouldn't this be titled\n\"accept the conflict & rename the file elsewhere\"?\n\n> + run_on_all mv folder2/a folder2/z &&\n> + test_all_match git add folder2 &&\n\n'mv' rather than 'git mv', then followed by 'git add'? Any reason for\nthis order rather than git add followed by git mv?\n\nAlso, if you really do want to move first, did you use mv instead of\n\"git mv\" due to the latter's shortcoming of only operating on stage 0?\n(https://lore.kernel.org/git/CABPp-BGJdwpwhQUp4Wa4bKBp4hQFB9OM3N1FXH7SzY0mvLDa7Q@mail.gmail.com/)\n\nRegardless of order, though, I still think mv or add should require a\n--force to rename or add a file outside the sparsity paths given the\ndeferred negative surprises for users around such files. (Or come up\nwith a solid way to remove those surprises.)\n\n> + test_all_match git status --porcelain=v2 &&\n> +\n> + test_all_match git merge --continue &&\n> + test_all_match git status --porcelain=v2 &&\n> + test_all_match git rev-parse HEAD^{tree}\n> +'\n> +\n> test_expect_success 'merge with outside renames' '\n> init_repos &&\n"},{"id":"431070","messageId":"CAPig+cQwxXGmN=exrr9YN174LXDuHU2yH847uOCzQQKsTEVWtg@mail.gmail.com","threadId":"56137","inReplyTo":"CABPp-BF-UVZKWmsohjCzRLpbHZii+1g=SShEYg_cwScsOHi=5g@mail.gmail.com","subject":"Re: [PATCH 1/5] t1092: test merge conflicts outside cone","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-07-23T17:44:15Z","receivedAt":"2021-07-23T17:44:29Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Jul 23, 2021 at 1:34 PM Elijah Newren wrote:\n> On Wed, Jul 21, 2021 at 2:07 PM Derrick Stolee via GitGitGadget\n> wrote:\n> > + for side in left right\n> > + do\n> > + git checkout -b merge-$side base &&\n> > + echo $side >>deep/deeper2/a &&\n> > + echo $side >>folder1/a &&\n> > + echo $side >>folder2/a &&\n> > + git add . &&\n> > + git commit -m \"$side\" || return 1\n>\n> Why is this \"|| return 1\" here?\n>\n> It looks like there are a number of other cases of this in the file\n> too, which I must have overlooked previously, because I don't\n> understand any of them.\n\nA shell for-loop won't automatically terminate just because some\ncommand in its body fails. Instead it will run to completion and\nreturn the status of the last command of the last iteration, which may\nnot be the iteration which failed, thus a failure can be hidden.\nTherefore, we need to proactively stop the loop iteration _and_ ensure\nthat the return status of the loop itself reflects the failure, which\nwe do by `|| return 1`. (If this loop was inside a subshell, we'd use\n`|| exit 1` instead.)\n"},{"id":"431071","messageId":"CABPp-BHQ52WpCKc8dxaV+u1QZCeT-YieQynTmK_w84r_Tc=VGQ@mail.gmail.com","threadId":"56137","inReplyTo":"791c6c2c9ade5b065fc0f367e00c52a493d086ef.1626901619.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/5] add: allow operating on a sparse-only index","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-07-23T17:45:50Z","receivedAt":"2021-07-23T17:46:05Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Jul 21, 2021 at 2:07 PM Derrick Stolee via GitGitGadget\n wrote:\n>\n> From: Derrick Stolee \n>\n> Disable command_requires_full_index for 'git add'. This does not require\n> any additional removals of ensure_full_index(). The main reason is that\n> 'git add' discovers changes based on the pathspec and the worktree\n> itself. These are then inserted into the index directly, and calls to\n> index_name_pos() or index_file_exists() already call expand_to_path() at\n> the appropriate time to support a sparse-index.\n\nNice.\n\n> Add a test to check that 'git add -A' and 'git add ' does not\n> expand the index at all, as long as is not within a sparse\n> directory. This does not help the global 'git add .' case.\n\nGood idea.\n\n> We can measure the improvement using p2000-sparse-operations.sh with\n> these results:\n>\n> Test HEAD~1 HEAD\n> ------------------------------------------------------------------------------\n> 2000.6: git add -A (full-index-v3) 0.35(0.30+0.05) 0.37(0.29+0.06) +5.7%\n> 2000.7: git add -A (full-index-v4) 0.31(0.26+0.06) 0.33(0.27+0.06) +6.5%\n> 2000.8: git add -A (sparse-index-v3) 0.57(0.53+0.07) 0.05(0.04+0.08) -91.2%\n> 2000.9: git add -A (sparse-index-v4) 0.58(0.55+0.06) 0.05(0.05+0.06) -91.4%\n>\n> While the 91% improvement seems impressive, it's important to recognize\n> that previously we had significant overhead for expanding the\n> sparse-index. Comparing to the full index case, 'git add -A' goes from\n> 0.37s to 0.05s, which is \"only\" an 86% improvement.\n\nHehe. Yep, it's so \"disappointing\" to \"only\" have the code be 7x faster. :-)\n\nOut of curiosity, IIRC any operation involving the index took ~10s on\nsome of the Microsoft repos. What does the speedup look like over\nthere for these changes to git-add?\n\n>\n> Signed-off-by: Derrick Stolee \n> ---\n> builtin/add.c | 3 +++\n> t/t1092-sparse-checkout-compatibility.sh | 14 ++++++--------\n> 2 files changed, 9 insertions(+), 8 deletions(-)\n>\n> diff --git a/builtin/add.c b/builtin/add.c\n> index b773b5a4993..c76e6ddd359 100644\n> --- a/builtin/add.c\n> +++ b/builtin/add.c\n> @@ -528,6 +528,9 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n> add_new_files = !take_worktree_changes && !refresh_only && !add_renormalize;\n> require_pathspec = !(take_worktree_changes || (0 < addremove_explicit));\n>\n> + prepare_repo_settings(the_repository);\n> + the_repository->settings.command_requires_full_index = 0;\n> +\n> hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n>\n> /*\n> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\n> index a3c01d588d8..a11d9d7f35d 100755\n> --- a/t/t1092-sparse-checkout-compatibility.sh\n> +++ b/t/t1092-sparse-checkout-compatibility.sh\n> @@ -340,13 +340,6 @@ test_expect_success 'status/add: outside sparse cone' '\n>\n> test_sparse_match git status --porcelain=v2 &&\n>\n> - # This \"git add folder1/a\" fails with a warning\n> - # in the sparse repos, differing from the full\n> - # repo. This is intentional.\n> - test_sparse_match test_must_fail git add folder1/a &&\n> - test_sparse_match test_must_fail git add --refresh folder1/a &&\n> - test_all_match git status --porcelain=v2 &&\n> -\n\nWhy was this chunk removed? Nothing in the commit message mentions\nthis, and it's not clear to me the reason for it.\n\nI tried adding it back in at the end of the series and it still works\n(and further I can't change test_sparse_match to test_all_match and\nhave the test work).\n\n> test_all_match git add . &&\n> test_all_match git status --porcelain=v2 &&\n> test_all_match git commit -m folder1/new &&\n> @@ -635,7 +628,12 @@ test_expect_success 'sparse-index is not expanded' '\n> git -C sparse-index reset --hard &&\n> ensure_not_expanded checkout rename-out-to-out -- deep/deeper1 &&\n> git -C sparse-index reset --hard &&\n> - ensure_not_expanded restore -s rename-out-to-out -- deep/deeper1\n> + ensure_not_expanded restore -s rename-out-to-out -- deep/deeper1 &&\n> +\n> + echo >>sparse-index/README.md &&\n> + ensure_not_expanded add -A &&\n> + echo >>sparse-index/extra.txt &&\n> + ensure_not_expanded add extra.txt\n\n...and here's the extra test you mentioned in the commit message. Looks good.\n\n> '\n>\n> # NEEDSWORK: a sparse-checkout behaves differently from a full checkout\n> --\n> gitgitgadget\n"},{"id":"431072","messageId":"CABPp-BEBD_4j+r2nVGrJgEPBYUPoUfZ839w_oZVnUSYofVDhLA@mail.gmail.com","threadId":"56137","inReplyTo":"CAPig+cQwxXGmN=exrr9YN174LXDuHU2yH847uOCzQQKsTEVWtg@mail.gmail.com","subject":"Re: [PATCH 1/5] t1092: test merge conflicts outside cone","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-07-23T17:47:02Z","receivedAt":"2021-07-23T17:47:16Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Jul 23, 2021 at 10:44 AM Eric Sunshine wrote:\n>\n> On Fri, Jul 23, 2021 at 1:34 PM Elijah Newren wrote:\n> > On Wed, Jul 21, 2021 at 2:07 PM Derrick Stolee via GitGitGadget\n> > wrote:\n> > > + for side in left right\n> > > + do\n> > > + git checkout -b merge-$side base &&\n> > > + echo $side >>deep/deeper2/a &&\n> > > + echo $side >>folder1/a &&\n> > > + echo $side >>folder2/a &&\n> > > + git add . &&\n> > > + git commit -m \"$side\" || return 1\n> >\n> > Why is this \"|| return 1\" here?\n> >\n> > It looks like there are a number of other cases of this in the file\n> > too, which I must have overlooked previously, because I don't\n> > understand any of them.\n>\n> A shell for-loop won't automatically terminate just because some\n> command in its body fails. Instead it will run to completion and\n> return the status of the last command of the last iteration, which may\n> not be the iteration which failed, thus a failure can be hidden.\n> Therefore, we need to proactively stop the loop iteration _and_ ensure\n> that the return status of the loop itself reflects the failure, which\n> we do by `|| return 1`. (If this loop was inside a subshell, we'd use\n> `|| exit 1` instead.)\n\nAh, thanks for the explanation.\n"},{"id":"431077","messageId":"CABPp-BFPDoqfJs1YoeEBhJnEtz=5Owr61=nBwHyV8Gho7uTn3Q@mail.gmail.com","threadId":"56137","inReplyTo":"a577ea4c74d86e57defcc4f45011871b634bcf56.1626901619.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/5] pathspec: stop calling ensure_full_index","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-07-23T18:17:07Z","receivedAt":"2021-07-23T18:17:23Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Jul 21, 2021 at 2:07 PM Derrick Stolee via GitGitGadget\n wrote:\n>\n> From: Derrick Stolee \n>\n> The add_pathspec_matches_against_index() focuses on matching a pathspec\n> to file entries in the index. This already works correctly for its only\n> use: checking if untracked files exist in the index.\n>\n> The compatibility checks in t1092 already test that 'git add '\n> works for a directory outside of the sparse cone. That provides coverage\n> for removing this guard.\n>\n> This finalizes our ability to run 'git add .' without expanding a sparse\n> index to a full one. This is evidenced by an update to t1092 and by\n> these performance numbers for p2000-sparse-operations.sh:\n>\n> Test HEAD~1 HEAD\n> --------------------------------------------------------------------------------\n> 2000.10: git add . (full-index-v3) 0.37(0.28+0.07) 0.36(0.27+0.06) -2.7%\n> 2000.11: git add . (full-index-v4) 0.33(0.26+0.06) 0.32(0.28+0.05) -3.0%\n> 2000.12: git add . (sparse-index-v3) 0.57(0.53+0.07) 0.06(0.06+0.07) -89.5%\n> 2000.13: git add . (sparse-index-v4) 0.57(0.53+0.07) 0.05(0.03+0.09) -91.2%\n>\n> While the ~90% improvement is shown by the test results, it is worth\n> noting that expanding the sparse index was adding overhead in previous\n> commits. Comparing to the full index case, we see the performance go\n> from 0.33s to 0.05s, an 85% improvement.\n\nThese perf improvements are pretty sweet.\n\n> Signed-off-by: Derrick Stolee \n> ---\n> pathspec.c | 2 --\n> t/t1092-sparse-checkout-compatibility.sh | 7 +++----\n> 2 files changed, 3 insertions(+), 6 deletions(-)\n>\n> diff --git a/pathspec.c b/pathspec.c\n> index 08f8d3eedc3..44306fdaca2 100644\n> --- a/pathspec.c\n> +++ b/pathspec.c\n> @@ -37,8 +37,6 @@ void add_pathspec_matches_against_index(const struct pathspec *pathspec,\n> num_unmatched++;\n> if (!num_unmatched)\n> return;\n> - /* TODO: audit for interaction with sparse-index. */\n> - ensure_full_index(istate);\n> for (i = 0; i < istate->cache_nr; i++) {\n> const struct cache_entry *ce = istate->cache[i];\n> if (sw_action == PS_IGNORE_SKIP_WORKTREE && ce_skip_worktree(ce))\n> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\n> index a11d9d7f35d..f9e2f5f4aa1 100755\n> --- a/t/t1092-sparse-checkout-compatibility.sh\n> +++ b/t/t1092-sparse-checkout-compatibility.sh\n> @@ -322,9 +322,6 @@ test_expect_success 'commit including unstaged changes' '\n> test_expect_success 'status/add: outside sparse cone' '\n> init_repos &&\n>\n> - # adding a \"missing\" file outside the cone should fail\n> - test_sparse_match test_must_fail git add folder1/a &&\n> -\n\nSo this is removed because of non-matching errors. In particular,\nsparse-checkout shows\n\"\"\"\nThe following pathspecs didn't match any eligible path, but they do match index\nentries outside the current sparse checkout:\nfolder1/a\nhint: Disable or modify the sparsity rules if you intend to update such entries.\nhint: Disable this message with \"git config advice.updateSparsePath false\"\n\"\"\"\nwhile sparse-index now shows:\n\"\"\"\nfatal: pathspec 'folder1/a' did not match any files\n\"\"\"\n\nThe new error seems entirely reasonable to me. No objection here.\n\n\nBut allow me to go on a bit of a diversion...\n\nIf we modify this setup slightly by running:\n\n$ mkdir folder1\n$ echo garbage >folder1/a\n$ git add folder1/a\n\nThen you'll get the first of those errors in both the sparse-index and\nthe sparse-checkout. I also like this behavior.\n\nIf you unset the SKIP_WORKTREE bit manually, and then add:\n\n$ git update-index --no-skip-worktree folder1/a\n$ git add folder1/a\n\nThen the file is added with no error or warning. I like this behavior too.\n\nIf you further change the setup with:\n\n$ echo more garbage >folder1/z\n$ git add folder1/z\n\nThen you get no error, despite folder1/z being an untracked file\noutside of sparsity paths. No bueno. :-(\n\n> # folder1 is at HEAD, but outside the sparse cone\n> run_on_sparse mkdir folder1 &&\n> cp initial-repo/folder1/a sparse-checkout/folder1/a &&\n> @@ -633,7 +630,9 @@ test_expect_success 'sparse-index is not expanded' '\n> echo >>sparse-index/README.md &&\n> ensure_not_expanded add -A &&\n> echo >>sparse-index/extra.txt &&\n> - ensure_not_expanded add extra.txt\n> + ensure_not_expanded add extra.txt &&\n> + echo >>sparse-index/untracked.txt &&\n> + ensure_not_expanded add .\n\n:-)\n\n> '\n>\n> # NEEDSWORK: a sparse-checkout behaves differently from a full checkout\n> --\n\nSo I added a lot of comments here, in part because I thought I'd test\na bit more of what I said in response to your cover letter and see how\nclose to it we were. The patch in question looks fine.\n\nI just added an aside as a convenient place to check whether the\nbehavior at the end of the series matches what you proposed in the\ncover letter, or what I proposed in response. It appears it matches\nneither (though that's not due to this specific patch).\n"},{"id":"431091","messageId":"CABPp-BHNS=6xX2=NxZnoVgtE8MqR3Tj9RE9m9ocV_+oHRjogoA@mail.gmail.com","threadId":"56137","inReplyTo":"76066a78ce077add4bc69819b017516e12ac0d45.1626901619.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 5/5] add: ignore outside the sparse-checkout in refresh()","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-07-23T19:46:52Z","receivedAt":"2021-07-23T19:47:05Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Jul 21, 2021 at 2:07 PM Derrick Stolee via GitGitGadget\n wrote:\n>\n> From: Derrick Stolee \n>\n> Since b243012 (refresh_index(): add flag to ignore SKIP_WORKTREE\n> entries, 2021-04-08), 'git add --refresh ' will output a warning\n> message when the path is outside the sparse-checkout definition. The\n> implementation of this warning happened in parallel with the\n> sparse-index work to add ensure_full_index() calls throughout the\n> codebase.\n>\n> Update this loop to have the proper logic that checks to see if the\n> pathspec is outside the sparse-checkout definition. This avoids the need\n> to expand the sparse directory entry and determine if the path is\n> tracked, untracked, or ignored. We simply avoid updating the stat()\n> information because there isn't even an entry that matches the path!\n>\n> Signed-off-by: Derrick Stolee \n> ---\n> builtin/add.c | 10 +++++++++-\n> t/t1092-sparse-checkout-compatibility.sh | 6 +-----\n> 2 files changed, 10 insertions(+), 6 deletions(-)\n>\n> diff --git a/builtin/add.c b/builtin/add.c\n> index c76e6ddd359..d512ece655b 100644\n> --- a/builtin/add.c\n> +++ b/builtin/add.c\n> @@ -192,13 +192,21 @@ static int refresh(int verbose, const struct pathspec *pathspec)\n> struct string_list only_match_skip_worktree = STRING_LIST_INIT_NODUP;\n> int flags = REFRESH_IGNORE_SKIP_WORKTREE |\n> (verbose ? REFRESH_IN_PORCELAIN : REFRESH_QUIET);\n> + struct pattern_list pl = { 0 };\n> + int sparse_checkout_enabled = !get_sparse_checkout_patterns(&pl);\n>\n> seen = xcalloc(pathspec->nr, 1);\n> refresh_index(&the_index, flags, pathspec, seen,\n> _(\"Unstaged changes after refreshing the index:\"));\n> for (i = 0; i < pathspec->nr; i++) {\n> if (!seen[i]) {\n> - if (matches_skip_worktree(pathspec, i, &skip_worktree_seen)) {\n> + const char *path = pathspec->items[i].original;\n> + int dtype = DT_REG;\n> +\n> + if (matches_skip_worktree(pathspec, i, &skip_worktree_seen) ||\n> + (sparse_checkout_enabled &&\n> + !path_matches_pattern_list(path, strlen(path), NULL,\n> + &dtype, &pl, &the_index))) {\n\nI was slightly worried from the description in the commit message\nabout the case where you have a file without the SKIP_WORKTREE bit set\ndespite not matching sparsity paths. I was worried that you'd skip\nrefreshing it, but I tweaked your testcases and couldn't trigger it.\n\n> string_list_append(&only_match_skip_worktree,\n> pathspec->items[i].original);\n> } else {\n> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\n> index 73c48a71d89..c61424e2074 100755\n> --- a/t/t1092-sparse-checkout-compatibility.sh\n> +++ b/t/t1092-sparse-checkout-compatibility.sh\n> @@ -347,7 +347,7 @@ test_expect_success 'status/add: outside sparse cone' '\n> test_all_match git commit -m folder1/newer\n> '\n>\n> -test_expect_failure 'add: pathspec within sparse directory' '\n> +test_expect_success 'add: pathspec within sparse directory' '\n> init_repos &&\n>\n> run_on_sparse mkdir folder1 &&\n> @@ -357,10 +357,6 @@ test_expect_failure 'add: pathspec within sparse directory' '\n> # This \"git add folder1/a\" fails with a warning\n> # in the sparse repos, differing from the full\n> # repo. This is intentional.\n> - #\n> - # However, in the sparse-index, folder1/a does not\n> - # match any cache entry and fails with a different\n> - # error message. This needs work.\n> test_sparse_match test_must_fail git add folder1/a &&\n> test_sparse_match test_must_fail git add --refresh folder1/a &&\n> test_all_match git status --porcelain=v2\n> --\n> gitgitgadget\n\nThis and Patch 4/5 look good to me.\n"},{"id":"431093","messageId":"CABPp-BFMZwUOJgHDWHuHQ6LXt9HvC8WFJKn=duVFtvB=EXy0fQ@mail.gmail.com","threadId":"56137","inReplyTo":"CABPp-BGBwbV7x8i=OVbPnMgWcfdLrEh3-yUi=Amw+XBwpf7A6Q@mail.gmail.com","subject":"Re: [PATCH 0/5] Sparse Index: Integrate with 'git add'","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-07-23T20:10:43Z","receivedAt":"2021-07-23T20:10:57Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Jul 23, 2021 at 5:51 AM Elijah Newren wrote:\n>\n> Hi,\n>\n> I haven't read the series yet (I'm going to try to read them all\n> today), but wanted to comment on a couple things in your cover\n> letter...\n>\n> On Wed, Jul 21, 2021 at 2:07 PM Derrick Stolee via GitGitGadget\n> wrote:\n> >\n> > This patch series re-submits the 'git add' integration with sparse-index.\n> > The performance gains are the same as before.\n> >\n> > It is based on ds/commit-and-checkout-with-sparse-index.\n> >\n> > This series was delayed from its initial submission for a couple reasons.\n> >\n> > The first was because it was colliding with some changes in\n> > mt/add-rm-in-sparse-checkout, so now we are far enough along that that\n> > branch is in our history and we can work forwards.\n> >\n> > The other concern was about how 'git add ' should respond when a path\n> > outside of the sparse-checkout cone exists. One recommendation (that I am\n> > failing to find a link to the message, sorry) was to disallow adding files\n> > that would become index entries with SKIP_WORKTREE on. However, as I worked\n>\n> I think the recommendation was:\n> * Permitted: running git add on tracked files with the SKIP_WORKTREE\n> bit *clear*\n> * Disallowed: running git add on tracked files with the\n> SKIP_WORKTREE bit *set*\n> * Disallowed: running git add on untracked files that would become\n> index entries with the SKIP_WORKTREE bit set\n>\n> where the latter two exit with an error message that suggests changing\n> the sparsity specification first. I think this is what has existed\n> for some time, other than Matheus adding some error messages to help\n> the user when their add command doesn't match anything otherwise.\n>\n> > towards that goal I found that change would cause problems for a realistic\n> > scenario: merge conflicts outside of the sparse-checkout cone.\n>\n> ...which wouldn't be a problem because these are tracked files whose\n> SKIP_WORKTREE bit was cleared by the merge machinery (when it marked\n> them as conflicted and wrote them to the working tree).\n>\n> > The first patch of this series adds tests that create merge conflicts\n> > outside of the sparse cone and then presents different ways a user could\n> > resolve the situation. We want all of them to be feasible, and this\n> > includes:\n> >\n> > 1. Reverting the file to a known version in history.\n> > 2. Adding the file with its contents on disk.\n> > 3. Moving the file to a new location in the sparse directory.\n> >\n> > Without maintaining the behavior of adding files outside of the\n> > sparse-checkout cone, we risk confusing users who get into this state. The\n> > only workaround they would have is to modify their sparse-checkout\n> > definition which might lead to expanding much more data than they need to\n> > resolve the conflicts.\n> >\n> > For that reason, I stopped trying to limit 'git add' to be within the cone.\n> > I'm open to suggestions here, but we need an approach that works for\n> > out-of-cone conflicts.\n>\n> I believe my above suggestion works for out-of-cone conflicts. Some\n> important other details to keep in mind in regards to how we make add\n> behave:\n>\n> * We don't want \"git add -A [GLOB_OR_DIRECTORY]\" to accidentally be\n> treated as a directive to remove files from the repository (and\n> naively noticing that SKIP_WORKTREE files are missing but attempting\n> to operate on them anyway would give this problematic result).\n> * We don't want \"git rm [GLOB_OR_DIRECTORY]\" to nuke SKIP_WORKTREE\n> files; it should only operate on files that are present.\n> * We need add and rm to be consistent with each other in terms of how\n> SKIP_WORKTREE files and the sparsity cone are treated\n>\n> and more generally about not-SKIP_WORKTREE-despite-not-matching-sparsity-paths:\n>\n> * These files that aren't SKIP_WORKTREE but normally would be are\n> prone to \"disappear\" at some random later time after they are made to\n> match the index. The disappearing can happen either with an explicit\n> \"git sparse-checkout reapply\" (which is fine since it was explicit) or\n> as a side-effect of various other commands that run through\n> unpack_trees() since it attempts to heed the sparsity rules. Users\n> tend to get confused by the latter case; they'll understand at some\n> point that it was because the file was outside the sparsity paths, but\n> the randomness in when it's pulled out as a side-effect of other\n> commands can be slightly jarring. So, I'd like to avoid that where we\n> can easily do so, which I think the recommendation above does. (As a\n> side note to these kinds of cases, maybe we want to consider having\n> the sparsification logic in unpack_trees() first check whether paths\n> being removed from the working tree and having their SKIP_WORKTREE bit\n> set not only match the index but also match HEAD? That'd be a bit\n> more expensive to check in the sparsification paths, and I'm not sure\n> they all have the relevant information, but it's an idea...)\n>\n> > The one place I did continue to update is 'git add --refresh ' to match the\n> > behavior added by mt/add-rm-in-sparse-checkout which outputs an error\n> > message. This happens even when the file exists in the working directory,\n> > but that seems appropriate enough.\n\nI've read through the patches now. They look pretty good, and the\nperformance improvements are definitely appealing.\n\nMy comments on patch 1/5 probably reflect the differences in proposed\npaths between your cover letter and my proposal. My comments on patch\n3/5 point out that the current behavior matches neither your\ndescription in your cover letter (or my understanding thereof), nor\nmine in my earlier response. However, it also suggests that the\ndifferences between my proposal and the code isn't due to your series\nbut a pre-existing issue, and thus wouldn't necessarily need to hold\nyour series up.\n\nMy comments on patch 2/5, asking about why you removed a hunk are thus\nthe biggest outstanding item for the series to move forward.\n\nIt would be nice for us to figure out and agree on the end goal for\nadd/rm/mv behavior, though.\n"},{"id":"431186","messageId":"8a7f504a-99af-ce89-307e-5a662d69f5fa@gmail.com","threadId":"56137","inReplyTo":"CABPp-BHQ52WpCKc8dxaV+u1QZCeT-YieQynTmK_w84r_Tc=VGQ@mail.gmail.com","subject":"Re: [PATCH 2/5] add: allow operating on a sparse-only index","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-07-26T13:11:30Z","receivedAt":"2021-07-26T13:11:35Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/23/2021 1:45 PM, Elijah Newren wrote:\n> On Wed, Jul 21, 2021 at 2:07 PM Derrick Stolee via GitGitGadget\n> wrote:\n...\n>> Test HEAD~1 HEAD\n>> ------------------------------------------------------------------------------\n>> 2000.6: git add -A (full-index-v3) 0.35(0.30+0.05) 0.37(0.29+0.06) +5.7%\n>> 2000.7: git add -A (full-index-v4) 0.31(0.26+0.06) 0.33(0.27+0.06) +6.5%\n>> 2000.8: git add -A (sparse-index-v3) 0.57(0.53+0.07) 0.05(0.04+0.08) -91.2%\n>> 2000.9: git add -A (sparse-index-v4) 0.58(0.55+0.06) 0.05(0.05+0.06) -91.4%\n>>\n>> While the 91% improvement seems impressive, it's important to recognize\n>> that previously we had significant overhead for expanding the\n>> sparse-index. Comparing to the full index case, 'git add -A' goes from\n>> 0.37s to 0.05s, which is \"only\" an 86% improvement.\n> \n> Hehe. Yep, it's so \"disappointing\" to \"only\" have the code be 7x faster. :-)\n> \n> Out of curiosity, IIRC any operation involving the index took ~10s on\n> some of the Microsoft repos. What does the speedup look like over\n> there for these changes to git-add?\n\nThe latest numbers I have for a repo with ~2 million tracked files is that\n index reads take about half a second (because of the threaded reads) and\nwrites take at least one second. There was a lot of work by Ben Peart, Jeff\nHostetler, and Kevin Willford to reduce this cost as much as possible a few\nyears ago. VFS for Git is still limited by this bottleneck, but Scalar's\nuse of sparse-checkout enables the use of the sparse index.\n\nWe have an experimental release [1] out to users right now, and I will\nreport to the mailing list about how that went after we get sufficient\nadoption that the data can be significant. When focusing on individual\nusers I can find things like one user seeing \"git commit\" going from 4.3s\nto 0.35s and \"git add\" going from 6.1s to 0.13s. (The \"git add\" time\nmight also be conflated with a change from the FS Monitor hook to the\nbuiltin FS Monitor.)\n\n[1] https://github.com/microsoft/git/releases/tag/v2.32.0.vfs.0.102.exp\n\nThanks,\n-Stolee\n"},{"id":"431187","messageId":"26eb25b6-2805-6dff-fc59-f6bc52a03142@gmail.com","threadId":"56137","inReplyTo":"CABPp-BHQ52WpCKc8dxaV+u1QZCeT-YieQynTmK_w84r_Tc=VGQ@mail.gmail.com","subject":"Re: [PATCH 2/5] add: allow operating on a sparse-only index","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-07-26T13:33:32Z","receivedAt":"2021-07-26T13:33:38Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/23/2021 1:45 PM, Elijah Newren wrote:\n> On Wed, Jul 21, 2021 at 2:07 PM Derrick Stolee via GitGitGadget\n> wrote:\n>>\n...\n>> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\n>> index a3c01d588d8..a11d9d7f35d 100755\n>> --- a/t/t1092-sparse-checkout-compatibility.sh\n>> +++ b/t/t1092-sparse-checkout-compatibility.sh\n>> @@ -340,13 +340,6 @@ test_expect_success 'status/add: outside sparse cone' '\n>>\n>> test_sparse_match git status --porcelain=v2 &&\n>>\n>> - # This \"git add folder1/a\" fails with a warning\n>> - # in the sparse repos, differing from the full\n>> - # repo. This is intentional.\n>> - test_sparse_match test_must_fail git add folder1/a &&\n>> - test_sparse_match test_must_fail git add --refresh folder1/a &&\n>> - test_all_match git status --porcelain=v2 &&\n>> -\n> \n> Why was this chunk removed? Nothing in the commit message mentions\n> this, and it's not clear to me the reason for it.\n> \n> I tried adding it back in at the end of the series and it still works\n> (and further I can't change test_sparse_match to test_all_match and\n> have the test work).\n\nI mentioned this in a reply to Junio, but this hunk removal is confusing.\n\nAs of this patch, this hunk causes a failure due to an error message not\nmatching, specifically this error:\n\n\n+ diff -u sparse-checkout-err sparse-index-err\n--- sparse-checkout-err 2021-07-26 13:30:50.304291264 +0000\n+++ sparse-index-err 2021-07-26 13:30:50.308291259 +0000\n@@ -1,5 +1 @@\n-The following pathspecs didn't match any eligible path, but they do match index\n-entries outside the current sparse checkout:\n-folder1/a\n-hint: Disable or modify the sparsity rules if you intend to update such entries.\n-hint: Disable this message with \"git config advice.updateSparsePath false\"\n+fatal: pathspec 'folder1/a' did not match any files\n\n\nA similar test is added as a failure case in patch 4, then marked as success\nin patch 5. This organization of test changes could be organized better, so\nI will work on that in v2, along with your other suggestions.\n\nThanks,\n-Stolee\n"},{"id":"431189","messageId":"ced1c678-225b-c915-d019-f9ebfb3898b6@gmail.com","threadId":"56137","inReplyTo":"CABPp-BF-UVZKWmsohjCzRLpbHZii+1g=SShEYg_cwScsOHi=5g@mail.gmail.com","subject":"Re: [PATCH 1/5] t1092: test merge conflicts outside cone","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-07-26T14:10:24Z","receivedAt":"2021-07-26T14:10:29Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/23/2021 1:34 PM, Elijah Newren wrote:\n> On Wed, Jul 21, 2021 at 2:07 PM Derrick Stolee via GitGitGadget\n> wrote:\n>>\n>> From: Derrick Stolee \n\n...\n\n>> + # 3. rename the file to another sparse filename\n> \n> But...that doesn't resolve the conflict. Shouldn't this be titled\n> \"accept the conflict & rename the file elsewhere\"?\n\nSure. I'm less focused on the content of the file and more the steps\nthe user might have taken to resolve the conflict.\n \n>> + run_on_all mv folder2/a folder2/z &&\n>> + test_all_match git add folder2 &&\n> \n> 'mv' rather than 'git mv', then followed by 'git add'? Any reason for\n> this order rather than git add followed by git mv?\n\nI'm trying to mimic that a user might realize that a filename might\nneed to be renamed (say, because a naming convention changed that is\ncausing the conflict) and I don't expect users to use 'git mv' to do\nthis action.\n\n> Also, if you really do want to move first, did you use mv instead of\n> \"git mv\" due to the latter's shortcoming of only operating on stage 0?\n> (https://lore.kernel.org/git/CABPp-BGJdwpwhQUp4Wa4bKBp4hQFB9OM3N1FXH7SzY0mvLDa7Q@mail.gmail.com/)\n\n'git mv' had not occurred to me as a thing to do in this case. I'm\nfocused on ensuring that 'git add' works as expected to update the\nindex in response to filesystem changes.\n\n> Regardless of order, though, I still think mv or add should require a\n> --force to rename or add a file outside the sparsity paths given the\n> deferred negative surprises for users around such files. (Or come up\n> with a solid way to remove those surprises.)\n\n--force is focused on _ignored_ files. I imagine that it could be\nrepurposed to allow entries outside of the sparse-checkout definition,\nbut we would want to be careful for users who are adding the entire\ndirectory, not just the individual files, as they might _also_ get any\nignored files that exist in that directory. That might justify creating\na new option instead.\n\nFurther, the error message reported when adding something outside of\nthe sparse cone should probably mention whatever option exists as a\nway for users to bypass this limitation. I'll collect my thoughts (in\nresponse to your detailed thoughts shared on my cover letter) and\nstart a new thread about hardening this behavior. I've got an internal\nticket tracking this, and I want to wrap my head around all of the\ninteresting commands (add, mv, rm, update-index?) and create a full\nrecommendation to bring as an RFC.\n\nOf course, if someone else wants to create this clear vision in the\nmeantime I will not complain.\n\nThanks,\n-Stolee\n"},{"id":"431194","messageId":"8f2fd9370fe078923256381ddd6ff4534d736ec7.1627312727.git.gitgitgadget@gmail.com","threadId":"56137","inReplyTo":"pull.999.v2.git.1627312727.gitgitgadget@gmail.com","subject":"[PATCH v2 1/5] t1092: test merge conflicts outside cone","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-26T15:18:43Z","receivedAt":"2021-07-26T15:18:55Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nSigned-off-by: Derrick Stolee \n---\n t/t1092-sparse-checkout-compatibility.sh | 38 ++++++++++++++++++++++++\n 1 file changed, 38 insertions(+)\n\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 91e30d6ec22..47f8e5e54e3 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -114,6 +114,16 @@ test_expect_success 'setup' '\n \t\tgit add . &&\n \t\tgit commit -m \"file to dir\" &&\n \n+\t\tfor side in left right\n+\t\tdo\n+\t\t\tgit checkout -b merge-$side base &&\n+\t\t\techo $side >>deep/deeper2/a &&\n+\t\t\techo $side >>folder1/a &&\n+\t\t\techo $side >>folder2/a &&\n+\t\t\tgit add . &&\n+\t\t\tgit commit -m \"$side\" || return 1\n+\t\tdone &&\n+\n \t\tgit checkout -b deepest base &&\n \t\techo \"updated deepest\" >deep/deeper1/deepest/a &&\n \t\tgit commit -a -m \"update deepest\" &&\n@@ -482,6 +492,34 @@ test_expect_success 'merge' '\n \ttest_all_match git rev-parse HEAD^{tree}\n '\n \n+test_expect_success 'merge with conflict outside cone' '\n+\tinit_repos &&\n+\n+\ttest_all_match git checkout -b merge-tip merge-left &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\ttest_all_match test_must_fail git merge -m merge merge-right &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\t# Resolve the conflict in different ways:\n+\t# 1. Revert to the base\n+\ttest_all_match git checkout base -- deep/deeper2/a &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\t# 2. Add the file with conflict markers\n+\ttest_all_match git add folder1/a &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\t# 3. Rename the file to another sparse filename and\n+\t# accept conflict markers as resolved content.\n+\trun_on_all mv folder2/a folder2/z &&\n+\ttest_all_match git add folder2 &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\ttest_all_match git merge --continue &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\ttest_all_match git rev-parse HEAD^{tree}\n+'\n+\n test_expect_success 'merge with outside renames' '\n \tinit_repos &&\n \n-- \ngitgitgadget\n\n"},{"id":"431195","messageId":"pull.999.v2.git.1627312727.gitgitgadget@gmail.com","threadId":"56137","inReplyTo":"pull.999.git.1626901619.gitgitgadget@gmail.com","subject":"[PATCH v2 0/5] Sparse Index: Integrate with 'git add'","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-26T15:18:42Z","receivedAt":"2021-07-26T15:19:09Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"This patch series re-submits the 'git add' integration with sparse-index.\nThe performance gains are the same as before.\n\nIt is based on ds/commit-and-checkout-with-sparse-index.\n\nThis series was delayed from its initial submission for a couple reasons.\n\nThe first was because it was colliding with some changes in\nmt/add-rm-in-sparse-checkout, so now we are far enough along that that\nbranch is in our history and we can work forwards.\n\nThe other concern was about how 'git add ' should respond when a path\noutside of the sparse-checkout cone exists. One recommendation (that I am\nfailing to find a link to the message, sorry) was to disallow adding files\nthat would become index entries with SKIP_WORKTREE on. However, as I worked\ntowards that goal I found that change would cause problems for a realistic\nscenario: merge conflicts outside of the sparse-checkout cone.\n\nUpdate: Elijah points out that the SKIP_WORKTREE bit is removed from\nconflict files, which allows adding the conflicted files without warning.\n(However, we also need to be careful about untracked files, as documented in\nthe test added here.)\n\nThe first patch of this series adds tests that create merge conflicts\noutside of the sparse cone and then presents different ways a user could\nresolve the situation. We want all of them to be feasible, and this\nincludes:\n\n 1. Reverting the file to a known version in history.\n 2. Adding the file with its contents on disk.\n 3. Moving the file to a new location in the sparse directory.\n\nThe one place I did continue to update is 'git add --refresh ' to match the\nbehavior added by mt/add-rm-in-sparse-checkout which outputs an error\nmessage. This happens even when the file exists in the working directory,\nbut that seems appropriate enough.\n\n\nUpdates in V2\n=============\n\n * Test comments in patch 1 are improved.\n\n * The test hunk that was removed in patch 2 and reintroduced in the old\n patch 4 is modified to clarify how the behavior changes with that patch.\n Then, the test is modified by future patches.\n\n * Another instance of ensure_full_index() is removed from the --renormalize\n option. This option already ignored files with the SKIP_WORKTREE bit, so\n this should be an obviously-correct removal.\n\n * a full proposal for what to do with \"git (add|mv|rm)\" and paths outside\n the cone is delayed to another series (with an RFC round) because the\n behavior of the sparse-index matches a full index with sparse-checkout.\n\nThanks, -Stolee\n\nDerrick Stolee (5):\n t1092: test merge conflicts outside cone\n add: allow operating on a sparse-only index\n pathspec: stop calling ensure_full_index\n add: ignore outside the sparse-checkout in refresh()\n add: remove ensure_full_index() with --renormalize\n\n builtin/add.c | 15 ++++--\n pathspec.c | 2 -\n t/t1092-sparse-checkout-compatibility.sh | 62 ++++++++++++++++++++----\n 3 files changed, 65 insertions(+), 14 deletions(-)\n\n\nbase-commit: 71e301501c88399711a1bf8515d1747e92cfbb9b\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-999%2Fderrickstolee%2Fsparse-index%2Fadd-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-999/derrickstolee/sparse-index/add-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/999\n\nRange-diff vs v1:\n\n 1: a763a7d15b8 ! 1: 8f2fd9370fe t1092: test merge conflicts outside cone\n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'merge' '\n +\ttest_all_match test_must_fail git merge -m merge merge-right &&\n +\ttest_all_match git status --porcelain=v2 &&\n +\n -+\t# resolve the conflict in different ways:\n -+\t# 1. revert to the base\n ++\t# Resolve the conflict in different ways:\n ++\t# 1. Revert to the base\n +\ttest_all_match git checkout base -- deep/deeper2/a &&\n +\ttest_all_match git status --porcelain=v2 &&\n +\n -+\t# 2. add the file with conflict markers\n ++\t# 2. Add the file with conflict markers\n +\ttest_all_match git add folder1/a &&\n +\ttest_all_match git status --porcelain=v2 &&\n +\n -+\t# 3. rename the file to another sparse filename\n ++\t# 3. Rename the file to another sparse filename and\n ++\t# accept conflict markers as resolved content.\n +\trun_on_all mv folder2/a folder2/z &&\n +\ttest_all_match git add folder2 &&\n +\ttest_all_match git status --porcelain=v2 &&\n 2: 791c6c2c9ad ! 2: 6e43f118fa0 add: allow operating on a sparse-only index\n @@ Commit message\n sparse-index. Comparing to the full index case, 'git add -A' goes from\n 0.37s to 0.05s, which is \"only\" an 86% improvement.\n \n + This modification to 'git add' creates some behavior change depending on\n + the use of a sparse index. We modify a test in t1092 to demonstrate\n + these changes which will be remedied in future changes.\n +\n Signed-off-by: Derrick Stolee \n \n ## builtin/add.c ##\n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'status/add: outsi\n -\t# This \"git add folder1/a\" fails with a warning\n -\t# in the sparse repos, differing from the full\n -\t# repo. This is intentional.\n --\ttest_sparse_match test_must_fail git add folder1/a &&\n ++\t# Adding the path outside of the sparse-checkout cone should fail.\n + \ttest_sparse_match test_must_fail git add folder1/a &&\n -\ttest_sparse_match test_must_fail git add --refresh folder1/a &&\n -\ttest_all_match git status --porcelain=v2 &&\n --\n ++\n ++\ttest_must_fail git -C sparse-checkout add --refresh folder1/a 2>sparse-checkout-err &&\n ++\ttest_must_fail git -C sparse-index add --refresh folder1/a 2>sparse-index-err &&\n ++\t# NEEDSWORK: A sparse index changes the error message.\n ++\t! test_cmp sparse-checkout-err sparse-index-err &&\n ++\n ++\t# NEEDSWORK: Adding a newly-tracked file outside the cone succeeds\n ++\ttest_sparse_match git add folder1/new &&\n + \n \ttest_all_match git add . &&\n \ttest_all_match git status --porcelain=v2 &&\n \ttest_all_match git commit -m folder1/new &&\n ++\ttest_all_match git rev-parse HEAD^{tree} &&\n + \n + \trun_on_all ../edit-contents folder1/newer &&\n + \ttest_all_match git add folder1/ &&\n + \ttest_all_match git status --porcelain=v2 &&\n +-\ttest_all_match git commit -m folder1/newer\n ++\ttest_all_match git commit -m folder1/newer &&\n ++\ttest_all_match git rev-parse HEAD^{tree}\n + '\n + \n + test_expect_success 'checkout and reset --hard' '\n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse-index is not expanded' '\n \tgit -C sparse-index reset --hard &&\n \tensure_not_expanded checkout rename-out-to-out -- deep/deeper1 &&\n 3: a577ea4c74d = 3: 2ae91e0af29 pathspec: stop calling ensure_full_index\n 4: 89ec6a7ce67 < -: ----------- t1092: 'git add --refresh' difference with sparse-index\n 5: 76066a78ce0 ! 4: a79728d4c64 add: ignore outside the sparse-checkout in refresh()\n @@ builtin/add.c: static int refresh(int verbose, const struct pathspec *pathspec)\n \n ## t/t1092-sparse-checkout-compatibility.sh ##\n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'status/add: outside sparse cone' '\n - \ttest_all_match git commit -m folder1/newer\n - '\n \n --test_expect_failure 'add: pathspec within sparse directory' '\n -+test_expect_success 'add: pathspec within sparse directory' '\n - \tinit_repos &&\n - \n - \trun_on_sparse mkdir folder1 &&\n -@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_failure 'add: pathspec within sparse directory' '\n - \t# This \"git add folder1/a\" fails with a warning\n - \t# in the sparse repos, differing from the full\n - \t# repo. This is intentional.\n --\t#\n --\t# However, in the sparse-index, folder1/a does not\n --\t# match any cache entry and fails with a different\n --\t# error message. This needs work.\n + \t# Adding the path outside of the sparse-checkout cone should fail.\n \ttest_sparse_match test_must_fail git add folder1/a &&\n - \ttest_sparse_match test_must_fail git add --refresh folder1/a &&\n - \ttest_all_match git status --porcelain=v2\n +-\n +-\ttest_must_fail git -C sparse-checkout add --refresh folder1/a 2>sparse-checkout-err &&\n +-\ttest_must_fail git -C sparse-index add --refresh folder1/a 2>sparse-index-err &&\n +-\t# NEEDSWORK: A sparse index changes the error message.\n +-\t! test_cmp sparse-checkout-err sparse-index-err &&\n ++\ttest_sparse_match test_must_fail git add --refresh folder1/a &&\n + \n + \t# NEEDSWORK: Adding a newly-tracked file outside the cone succeeds\n + \ttest_sparse_match git add folder1/new &&\n -: ----------- > 5: 1543550a4e8 add: remove ensure_full_index() with --renormalize\n\n-- \ngitgitgadget\n"},{"id":"431196","messageId":"6e43f118fa0b7a6ba2e841f75a90406d847d9f09.1627312727.git.gitgitgadget@gmail.com","threadId":"56137","inReplyTo":"pull.999.v2.git.1627312727.gitgitgadget@gmail.com","subject":"[PATCH v2 2/5] add: allow operating on a sparse-only index","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-26T15:18:44Z","receivedAt":"2021-07-26T15:19:11Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nDisable command_requires_full_index for 'git add'. This does not require\nany additional removals of ensure_full_index(). The main reason is that\n'git add' discovers changes based on the pathspec and the worktree\nitself. These are then inserted into the index directly, and calls to\nindex_name_pos() or index_file_exists() already call expand_to_path() at\nthe appropriate time to support a sparse-index.\n\nAdd a test to check that 'git add -A' and 'git add ' does not\nexpand the index at all, as long as is not within a sparse\ndirectory. This does not help the global 'git add .' case.\n\nWe can measure the improvement using p2000-sparse-operations.sh with\nthese results:\n\nTest HEAD~1 HEAD\n------------------------------------------------------------------------------\n2000.6: git add -A (full-index-v3) 0.35(0.30+0.05) 0.37(0.29+0.06) +5.7%\n2000.7: git add -A (full-index-v4) 0.31(0.26+0.06) 0.33(0.27+0.06) +6.5%\n2000.8: git add -A (sparse-index-v3) 0.57(0.53+0.07) 0.05(0.04+0.08) -91.2%\n2000.9: git add -A (sparse-index-v4) 0.58(0.55+0.06) 0.05(0.05+0.06) -91.4%\n\nWhile the 91% improvement seems impressive, it's important to recognize\nthat previously we had significant overhead for expanding the\nsparse-index. Comparing to the full index case, 'git add -A' goes from\n0.37s to 0.05s, which is \"only\" an 86% improvement.\n\nThis modification to 'git add' creates some behavior change depending on\nthe use of a sparse index. We modify a test in t1092 to demonstrate\nthese changes which will be remedied in future changes.\n\nSigned-off-by: Derrick Stolee \n---\n builtin/add.c | 3 +++\n t/t1092-sparse-checkout-compatibility.sh | 25 +++++++++++++++++-------\n 2 files changed, 21 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex b773b5a4993..c76e6ddd359 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -528,6 +528,9 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \tadd_new_files = !take_worktree_changes && !refresh_only && !add_renormalize;\n \trequire_pathspec = !(take_worktree_changes || (0 < addremove_explicit));\n \n+\tprepare_repo_settings(the_repository);\n+\tthe_repository->settings.command_requires_full_index = 0;\n+\n \thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n \n \t/*\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 47f8e5e54e3..19d38f18ed6 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -340,21 +340,27 @@ test_expect_success 'status/add: outside sparse cone' '\n \n \ttest_sparse_match git status --porcelain=v2 &&\n \n-\t# This \"git add folder1/a\" fails with a warning\n-\t# in the sparse repos, differing from the full\n-\t# repo. This is intentional.\n+\t# Adding the path outside of the sparse-checkout cone should fail.\n \ttest_sparse_match test_must_fail git add folder1/a &&\n-\ttest_sparse_match test_must_fail git add --refresh folder1/a &&\n-\ttest_all_match git status --porcelain=v2 &&\n+\n+\ttest_must_fail git -C sparse-checkout add --refresh folder1/a 2>sparse-checkout-err &&\n+\ttest_must_fail git -C sparse-index add --refresh folder1/a 2>sparse-index-err &&\n+\t# NEEDSWORK: A sparse index changes the error message.\n+\t! test_cmp sparse-checkout-err sparse-index-err &&\n+\n+\t# NEEDSWORK: Adding a newly-tracked file outside the cone succeeds\n+\ttest_sparse_match git add folder1/new &&\n \n \ttest_all_match git add . &&\n \ttest_all_match git status --porcelain=v2 &&\n \ttest_all_match git commit -m folder1/new &&\n+\ttest_all_match git rev-parse HEAD^{tree} &&\n \n \trun_on_all ../edit-contents folder1/newer &&\n \ttest_all_match git add folder1/ &&\n \ttest_all_match git status --porcelain=v2 &&\n-\ttest_all_match git commit -m folder1/newer\n+\ttest_all_match git commit -m folder1/newer &&\n+\ttest_all_match git rev-parse HEAD^{tree}\n '\n \n test_expect_success 'checkout and reset --hard' '\n@@ -636,7 +642,12 @@ test_expect_success 'sparse-index is not expanded' '\n \tgit -C sparse-index reset --hard &&\n \tensure_not_expanded checkout rename-out-to-out -- deep/deeper1 &&\n \tgit -C sparse-index reset --hard &&\n-\tensure_not_expanded restore -s rename-out-to-out -- deep/deeper1\n+\tensure_not_expanded restore -s rename-out-to-out -- deep/deeper1 &&\n+\n+\techo >>sparse-index/README.md &&\n+\tensure_not_expanded add -A &&\n+\techo >>sparse-index/extra.txt &&\n+\tensure_not_expanded add extra.txt\n '\n \n # NEEDSWORK: a sparse-checkout behaves differently from a full checkout\n-- \ngitgitgadget\n\n"},{"id":"431197","messageId":"a79728d4c648533468716ea1dcc16b3d4e0673b9.1627312727.git.gitgitgadget@gmail.com","threadId":"56137","inReplyTo":"pull.999.v2.git.1627312727.gitgitgadget@gmail.com","subject":"[PATCH v2 4/5] add: ignore outside the sparse-checkout in refresh()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-26T15:18:46Z","receivedAt":"2021-07-26T15:19:12Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nSince b243012 (refresh_index(): add flag to ignore SKIP_WORKTREE\nentries, 2021-04-08), 'git add --refresh ' will output a warning\nmessage when the path is outside the sparse-checkout definition. The\nimplementation of this warning happened in parallel with the\nsparse-index work to add ensure_full_index() calls throughout the\ncodebase.\n\nUpdate this loop to have the proper logic that checks to see if the\npathspec is outside the sparse-checkout definition. This avoids the need\nto expand the sparse directory entry and determine if the path is\ntracked, untracked, or ignored. We simply avoid updating the stat()\ninformation because there isn't even an entry that matches the path!\n\nSigned-off-by: Derrick Stolee \n---\n builtin/add.c | 10 +++++++++-\n t/t1092-sparse-checkout-compatibility.sh | 6 +-----\n 2 files changed, 10 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex c76e6ddd359..d512ece655b 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -192,13 +192,21 @@ static int refresh(int verbose, const struct pathspec *pathspec)\n \tstruct string_list only_match_skip_worktree = STRING_LIST_INIT_NODUP;\n \tint flags = REFRESH_IGNORE_SKIP_WORKTREE |\n \t\t (verbose ? REFRESH_IN_PORCELAIN : REFRESH_QUIET);\n+\tstruct pattern_list pl = { 0 };\n+\tint sparse_checkout_enabled = !get_sparse_checkout_patterns(&pl);\n \n \tseen = xcalloc(pathspec->nr, 1);\n \trefresh_index(&the_index, flags, pathspec, seen,\n \t\t _(\"Unstaged changes after refreshing the index:\"));\n \tfor (i = 0; i < pathspec->nr; i++) {\n \t\tif (!seen[i]) {\n-\t\t\tif (matches_skip_worktree(pathspec, i, &skip_worktree_seen)) {\n+\t\t\tconst char *path = pathspec->items[i].original;\n+\t\t\tint dtype = DT_REG;\n+\n+\t\t\tif (matches_skip_worktree(pathspec, i, &skip_worktree_seen) ||\n+\t\t\t (sparse_checkout_enabled &&\n+\t\t\t !path_matches_pattern_list(path, strlen(path), NULL,\n+\t\t\t\t\t\t\t&dtype, &pl, &the_index))) {\n \t\t\t\tstring_list_append(&only_match_skip_worktree,\n \t\t\t\t\t\t pathspec->items[i].original);\n \t\t\t} else {\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 50dce0e0f99..3e201546577 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -339,11 +339,7 @@ test_expect_success 'status/add: outside sparse cone' '\n \n \t# Adding the path outside of the sparse-checkout cone should fail.\n \ttest_sparse_match test_must_fail git add folder1/a &&\n-\n-\ttest_must_fail git -C sparse-checkout add --refresh folder1/a 2>sparse-checkout-err &&\n-\ttest_must_fail git -C sparse-index add --refresh folder1/a 2>sparse-index-err &&\n-\t# NEEDSWORK: A sparse index changes the error message.\n-\t! test_cmp sparse-checkout-err sparse-index-err &&\n+\ttest_sparse_match test_must_fail git add --refresh folder1/a &&\n \n \t# NEEDSWORK: Adding a newly-tracked file outside the cone succeeds\n \ttest_sparse_match git add folder1/new &&\n-- \ngitgitgadget\n\n"},{"id":"431198","messageId":"2ae91e0af29bcc5ba118cba4ba051deefd3b9764.1627312727.git.gitgitgadget@gmail.com","threadId":"56137","inReplyTo":"pull.999.v2.git.1627312727.gitgitgadget@gmail.com","subject":"[PATCH v2 3/5] pathspec: stop calling ensure_full_index","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-26T15:18:45Z","receivedAt":"2021-07-26T15:19:13Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe add_pathspec_matches_against_index() focuses on matching a pathspec\nto file entries in the index. This already works correctly for its only\nuse: checking if untracked files exist in the index.\n\nThe compatibility checks in t1092 already test that 'git add '\nworks for a directory outside of the sparse cone. That provides coverage\nfor removing this guard.\n\nThis finalizes our ability to run 'git add .' without expanding a sparse\nindex to a full one. This is evidenced by an update to t1092 and by\nthese performance numbers for p2000-sparse-operations.sh:\n\nTest HEAD~1 HEAD\n--------------------------------------------------------------------------------\n2000.10: git add . (full-index-v3) 0.37(0.28+0.07) 0.36(0.27+0.06) -2.7%\n2000.11: git add . (full-index-v4) 0.33(0.26+0.06) 0.32(0.28+0.05) -3.0%\n2000.12: git add . (sparse-index-v3) 0.57(0.53+0.07) 0.06(0.06+0.07) -89.5%\n2000.13: git add . (sparse-index-v4) 0.57(0.53+0.07) 0.05(0.03+0.09) -91.2%\n\nWhile the ~90% improvement is shown by the test results, it is worth\nnoting that expanding the sparse index was adding overhead in previous\ncommits. Comparing to the full index case, we see the performance go\nfrom 0.33s to 0.05s, an 85% improvement.\n\nSigned-off-by: Derrick Stolee \n---\n pathspec.c | 2 --\n t/t1092-sparse-checkout-compatibility.sh | 7 +++----\n 2 files changed, 3 insertions(+), 6 deletions(-)\n\ndiff --git a/pathspec.c b/pathspec.c\nindex 08f8d3eedc3..44306fdaca2 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -37,8 +37,6 @@ void add_pathspec_matches_against_index(const struct pathspec *pathspec,\n \t\t\tnum_unmatched++;\n \tif (!num_unmatched)\n \t\treturn;\n-\t/* TODO: audit for interaction with sparse-index. */\n-\tensure_full_index(istate);\n \tfor (i = 0; i < istate->cache_nr; i++) {\n \t\tconst struct cache_entry *ce = istate->cache[i];\n \t\tif (sw_action == PS_IGNORE_SKIP_WORKTREE && ce_skip_worktree(ce))\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 19d38f18ed6..50dce0e0f99 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -322,9 +322,6 @@ test_expect_success 'commit including unstaged changes' '\n test_expect_success 'status/add: outside sparse cone' '\n \tinit_repos &&\n \n-\t# adding a \"missing\" file outside the cone should fail\n-\ttest_sparse_match test_must_fail git add folder1/a &&\n-\n \t# folder1 is at HEAD, but outside the sparse cone\n \trun_on_sparse mkdir folder1 &&\n \tcp initial-repo/folder1/a sparse-checkout/folder1/a &&\n@@ -647,7 +644,9 @@ test_expect_success 'sparse-index is not expanded' '\n \techo >>sparse-index/README.md &&\n \tensure_not_expanded add -A &&\n \techo >>sparse-index/extra.txt &&\n-\tensure_not_expanded add extra.txt\n+\tensure_not_expanded add extra.txt &&\n+\techo >>sparse-index/untracked.txt &&\n+\tensure_not_expanded add .\n '\n \n # NEEDSWORK: a sparse-checkout behaves differently from a full checkout\n-- \ngitgitgadget\n\n"},{"id":"431199","messageId":"1543550a4e8cc4dda495c08c924034291001386a.1627312727.git.gitgitgadget@gmail.com","threadId":"56137","inReplyTo":"pull.999.v2.git.1627312727.gitgitgadget@gmail.com","subject":"[PATCH v2 5/5] add: remove ensure_full_index() with --renormalize","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-26T15:18:47Z","receivedAt":"2021-07-26T15:19:17Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe --renormalize option updates the EOL conversions for the tracked\nfiles. However, the loop already ignores files marked with the\nSKIP_WORKTREE bit, so it will continue to do so with a sparse index\nbecause the sparse directory entries also have this bit set.\n\nSigned-off-by: Derrick Stolee \n---\n builtin/add.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex d512ece655b..c49e179abc3 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -144,8 +144,6 @@ static int renormalize_tracked_files(const struct pathspec *pathspec, int flags)\n {\n \tint i, retval = 0;\n \n-\t/* TODO: audit for interaction with sparse-index. */\n-\tensure_full_index(&the_index);\n \tfor (i = 0; i < active_nr; i++) {\n \t\tstruct cache_entry *ce = active_cache[i];\n \n-- \ngitgitgadget\n"},{"id":"431446","messageId":"CABPp-BHnvEPuYahFAoVSF58k99t__N2-M4OKKHDAAK2qrhY3WQ@mail.gmail.com","threadId":"56137","inReplyTo":"pull.999.v2.git.1627312727.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/5] Sparse Index: Integrate with 'git add'","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-07-28T23:13:33Z","receivedAt":"2021-07-28T23:13:47Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Jul 26, 2021 at 9:18 AM Derrick Stolee via GitGitGadget\n wrote:\n>\n> This patch series re-submits the 'git add' integration with sparse-index.\n> The performance gains are the same as before.\n>\n> It is based on ds/commit-and-checkout-with-sparse-index.\n>\n> This series was delayed from its initial submission for a couple reasons.\n>\n> The first was because it was colliding with some changes in\n> mt/add-rm-in-sparse-checkout, so now we are far enough along that that\n> branch is in our history and we can work forwards.\n>\n> The other concern was about how 'git add ' should respond when a path\n> outside of the sparse-checkout cone exists. One recommendation (that I am\n> failing to find a link to the message, sorry) was to disallow adding files\n> that would become index entries with SKIP_WORKTREE on. However, as I worked\n> towards that goal I found that change would cause problems for a realistic\n> scenario: merge conflicts outside of the sparse-checkout cone.\n>\n> Update: Elijah points out that the SKIP_WORKTREE bit is removed from\n> conflict files, which allows adding the conflicted files without warning.\n> (However, we also need to be careful about untracked files, as documented in\n> the test added here.)\n>\n> The first patch of this series adds tests that create merge conflicts\n> outside of the sparse cone and then presents different ways a user could\n> resolve the situation. We want all of them to be feasible, and this\n> includes:\n>\n> 1. Reverting the file to a known version in history.\n> 2. Adding the file with its contents on disk.\n> 3. Moving the file to a new location in the sparse directory.\n>\n> The one place I did continue to update is 'git add --refresh ' to match the\n> behavior added by mt/add-rm-in-sparse-checkout which outputs an error\n> message. This happens even when the file exists in the working directory,\n> but that seems appropriate enough.\n>\n>\n> Updates in V2\n> =============\n>\n> * Test comments in patch 1 are improved.\n>\n> * The test hunk that was removed in patch 2 and reintroduced in the old\n> patch 4 is modified to clarify how the behavior changes with that patch.\n> Then, the test is modified by future patches.\n>\n> * Another instance of ensure_full_index() is removed from the --renormalize\n> option. This option already ignored files with the SKIP_WORKTREE bit, so\n> this should be an obviously-correct removal.\n>\n> * a full proposal for what to do with \"git (add|mv|rm)\" and paths outside\n> the cone is delayed to another series (with an RFC round) because the\n> behavior of the sparse-index matches a full index with sparse-checkout.\n\nI think this makes sense.\n\nI've read through the patches, and I like this version...with one\nexception. Can we mark the test added in patch 1 under\n\n # 3. Rename the file to another sparse filename and\n # accept conflict markers as resolved content.\n\nas NEEDSWORK or even MAYNEEDWORK? I'm still quite unconvinced that it\nis testing for correct behavior, and don't want to paint ourselves\ninto a corner. In particular, we don't allow folks to \"git add\n$IGNORED_FILE\" without a --force override because it's likely to be a\nmistake. I think the same logic holds for adding untracked files\noutside the sparsity cone. But it's actually even worse than that\ncase because there's a secondary level of surprise too: adding files\noutside the sparsity cone will result in delayed user surprises when\nthe next git command that happens to call unpack_trees() (which are\nfound all over the codebase) removes the file from the working tree.\nI've had some such reports already.\n\nIf that test is marked as NEEDSWORK or even as the correct behavior\nstill being under dispute, then you can happily apply my:\n\nReviewed-by: Elijah Newren \n\n> Thanks, -Stolee\n>\n> Derrick Stolee (5):\n> t1092: test merge conflicts outside cone\n> add: allow operating on a sparse-only index\n> pathspec: stop calling ensure_full_index\n> add: ignore outside the sparse-checkout in refresh()\n> add: remove ensure_full_index() with --renormalize\n>\n> builtin/add.c | 15 ++++--\n> pathspec.c | 2 -\n> t/t1092-sparse-checkout-compatibility.sh | 62 ++++++++++++++++++++----\n> 3 files changed, 65 insertions(+), 14 deletions(-)\n>\n>\n> base-commit: 71e301501c88399711a1bf8515d1747e92cfbb9b\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-999%2Fderrickstolee%2Fsparse-index%2Fadd-v2\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-999/derrickstolee/sparse-index/add-v2\n> Pull-Request: https://github.com/gitgitgadget/git/pull/999\n>\n> Range-diff vs v1:\n>\n> 1: a763a7d15b8 ! 1: 8f2fd9370fe t1092: test merge conflicts outside cone\n> @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'merge' '\n> + test_all_match test_must_fail git merge -m merge merge-right &&\n> + test_all_match git status --porcelain=v2 &&\n> +\n> -+ # resolve the conflict in different ways:\n> -+ # 1. revert to the base\n> ++ # Resolve the conflict in different ways:\n> ++ # 1. Revert to the base\n> + test_all_match git checkout base -- deep/deeper2/a &&\n> + test_all_match git status --porcelain=v2 &&\n> +\n> -+ # 2. add the file with conflict markers\n> ++ # 2. Add the file with conflict markers\n> + test_all_match git add folder1/a &&\n> + test_all_match git status --porcelain=v2 &&\n> +\n> -+ # 3. rename the file to another sparse filename\n> ++ # 3. Rename the file to another sparse filename and\n> ++ # accept conflict markers as resolved content.\n> + run_on_all mv folder2/a folder2/z &&\n> + test_all_match git add folder2 &&\n> + test_all_match git status --porcelain=v2 &&\n> 2: 791c6c2c9ad ! 2: 6e43f118fa0 add: allow operating on a sparse-only index\n> @@ Commit message\n> sparse-index. Comparing to the full index case, 'git add -A' goes from\n> 0.37s to 0.05s, which is \"only\" an 86% improvement.\n>\n> + This modification to 'git add' creates some behavior change depending on\n> + the use of a sparse index. We modify a test in t1092 to demonstrate\n> + these changes which will be remedied in future changes.\n> +\n> Signed-off-by: Derrick Stolee \n>\n> ## builtin/add.c ##\n> @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'status/add: outsi\n> - # This \"git add folder1/a\" fails with a warning\n> - # in the sparse repos, differing from the full\n> - # repo. This is intentional.\n> -- test_sparse_match test_must_fail git add folder1/a &&\n> ++ # Adding the path outside of the sparse-checkout cone should fail.\n> + test_sparse_match test_must_fail git add folder1/a &&\n> - test_sparse_match test_must_fail git add --refresh folder1/a &&\n> - test_all_match git status --porcelain=v2 &&\n> --\n> ++\n> ++ test_must_fail git -C sparse-checkout add --refresh folder1/a 2>sparse-checkout-err &&\n> ++ test_must_fail git -C sparse-index add --refresh folder1/a 2>sparse-index-err &&\n> ++ # NEEDSWORK: A sparse index changes the error message.\n> ++ ! test_cmp sparse-checkout-err sparse-index-err &&\n> ++\n> ++ # NEEDSWORK: Adding a newly-tracked file outside the cone succeeds\n> ++ test_sparse_match git add folder1/new &&\n> +\n> test_all_match git add . &&\n> test_all_match git status --porcelain=v2 &&\n> test_all_match git commit -m folder1/new &&\n> ++ test_all_match git rev-parse HEAD^{tree} &&\n> +\n> + run_on_all ../edit-contents folder1/newer &&\n> + test_all_match git add folder1/ &&\n> + test_all_match git status --porcelain=v2 &&\n> +- test_all_match git commit -m folder1/newer\n> ++ test_all_match git commit -m folder1/newer &&\n> ++ test_all_match git rev-parse HEAD^{tree}\n> + '\n> +\n> + test_expect_success 'checkout and reset --hard' '\n> @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse-index is not expanded' '\n> git -C sparse-index reset --hard &&\n> ensure_not_expanded checkout rename-out-to-out -- deep/deeper1 &&\n> 3: a577ea4c74d = 3: 2ae91e0af29 pathspec: stop calling ensure_full_index\n> 4: 89ec6a7ce67 < -: ----------- t1092: 'git add --refresh' difference with sparse-index\n> 5: 76066a78ce0 ! 4: a79728d4c64 add: ignore outside the sparse-checkout in refresh()\n> @@ builtin/add.c: static int refresh(int verbose, const struct pathspec *pathspec)\n>\n> ## t/t1092-sparse-checkout-compatibility.sh ##\n> @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'status/add: outside sparse cone' '\n> - test_all_match git commit -m folder1/newer\n> - '\n>\n> --test_expect_failure 'add: pathspec within sparse directory' '\n> -+test_expect_success 'add: pathspec within sparse directory' '\n> - init_repos &&\n> -\n> - run_on_sparse mkdir folder1 &&\n> -@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_failure 'add: pathspec within sparse directory' '\n> - # This \"git add folder1/a\" fails with a warning\n> - # in the sparse repos, differing from the full\n> - # repo. This is intentional.\n> -- #\n> -- # However, in the sparse-index, folder1/a does not\n> -- # match any cache entry and fails with a different\n> -- # error message. This needs work.\n> + # Adding the path outside of the sparse-checkout cone should fail.\n> test_sparse_match test_must_fail git add folder1/a &&\n> - test_sparse_match test_must_fail git add --refresh folder1/a &&\n> - test_all_match git status --porcelain=v2\n> +-\n> +- test_must_fail git -C sparse-checkout add --refresh folder1/a 2>sparse-checkout-err &&\n> +- test_must_fail git -C sparse-index add --refresh folder1/a 2>sparse-index-err &&\n> +- # NEEDSWORK: A sparse index changes the error message.\n> +- ! test_cmp sparse-checkout-err sparse-index-err &&\n> ++ test_sparse_match test_must_fail git add --refresh folder1/a &&\n> +\n> + # NEEDSWORK: Adding a newly-tracked file outside the cone succeeds\n> + test_sparse_match git add folder1/new &&\n> -: ----------- > 5: 1543550a4e8 add: remove ensure_full_index() with --renormalize\n>\n> --\n> gitgitgadget\n"},{"id":"431454","messageId":"6a63736a-feb8-b74b-ef68-73cc71009e1d@gmail.com","threadId":"56137","inReplyTo":"CABPp-BHnvEPuYahFAoVSF58k99t__N2-M4OKKHDAAK2qrhY3WQ@mail.gmail.com","subject":"Re: [PATCH v2 0/5] Sparse Index: Integrate with 'git add'","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-07-29T02:03:36Z","receivedAt":"2021-07-29T02:03:43Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/28/2021 7:13 PM, Elijah Newren wrote:\n> On Mon, Jul 26, 2021 at 9:18 AM Derrick Stolee via GitGitGadget\n> wrote:\n...\n>> * a full proposal for what to do with \"git (add|mv|rm)\" and paths outside\n>> the cone is delayed to another series (with an RFC round) because the\n>> behavior of the sparse-index matches a full index with sparse-checkout.\n> \n> I think this makes sense.\n> \n> I've read through the patches, and I like this version...with one\n> exception. Can we mark the test added in patch 1 under\n> \n> # 3. Rename the file to another sparse filename and\n> # accept conflict markers as resolved content.\n> \n> as NEEDSWORK or even MAYNEEDWORK?\n\nI have no objection to adding a blurb such as:\n\n\t# NEEDSWORK: allowing adds outside the sparse cone can be\n\t# confusingto users, as the file can disappear from the\n\t# worktree without warning in later Git commands.\n\nAnd perhaps I'm misunderstanding the situation a bit, but that\nseems to apply not just to this third case, but all of them. I\ndon't see why the untracked case is special compared to the\ntracked case. More investigation may be required on my part.\n\n> I'm still quite unconvinced that it\n> is testing for correct behavior, and don't want to paint ourselves\n> into a corner. In particular, we don't allow folks to \"git add\n> $IGNORED_FILE\" without a --force override because it's likely to be a\n> mistake. \n\nI agree about ignored files, and that is true whether or not they\nare in the sparse cone.\n\n> I think the same logic holds for adding untracked files\n> outside the sparsity cone. But it's actually even worse than that\n> case because there's a secondary level of surprise too: adding files\n> outside the sparsity cone will result in delayed user surprises when\n> the next git command that happens to call unpack_trees() (which are\n> found all over the codebase) removes the file from the working tree.\n> I've had some such reports already.\n\nI believe this is testing a realistic scenario that users are\nhitting in the wild today. I would believe that users succeed with\nthese commands more often than they are confused by the file\ndisappearing from the worktree in a later Git command, so having\nthis sequence of events be documented as a potential use case has\nsome value.\n\nI simultaneously don't think it is behavior we want to commit to\nas a contract for all future Git versions, but there is value in\nshowing how this situation changes with any future meddling. In\nparticular: will users be able to self-discover the \"new\" way of\ndoing things?\n\nThe proposal part of changing how add/mv/rm behave in these cases\nwould need to adjust this test with something that would also help\ndirect users to a helpful resolution. For example, the first run\nof\n\n\tgit add sparse/dir/file\n\ncould error out with an error message saying \"The pathspec is\noutside of your sparse cone, so staging the file might lead to\na staged change that is removed from your working directory.\"\nBut we should _also_ include two strategies for getting out of\nthis state:\n\n1. Adjust your sparse-checkout definition so this file is in scope.\n\n-or- (and this is the part that would be new)\n\n2. If you understand the risks of staging a file outside the sparse\n cone, then run 'git add --sparse sparse/dir/file'.\n\n(Insert whatever option would be appropriate for --sparse here.)\n\nSuch a warning message would allow users who follow the steps listed\nin the test to know how to adjust their usage to then get into a\ngood state.\n\n> If that test is marked as NEEDSWORK or even as the correct behavior\n> still being under dispute, then you can happily apply my:\n\nI would classify this as \"The test documents current behavior, but\nisn't a contract for future behavior.\" With a concept such as my\nsuggestion above, the test could be modified to check for the\nwarning and then run the second command with the extra option and\ncomplete the test's expectations. Having the existing behavior\ndocumented in a test helps demonstrate how behavior is changing.\n\nWe we've discussed, we want to give such a behavior change the\nright venue for feedback and suggestions for alternate approaches,\nand this series is not the right place for that. Hopefully you\ncan tell that it is on my mind and that I want to recommend a\nchange in the near future.\n\nThanks,\n-Stolee\n"},{"id":"431456","messageId":"CABPp-BGUTg=GarkhP0MwjWKWmDyRJiEL2J75wFz52y2xi_50mw@mail.gmail.com","threadId":"56137","inReplyTo":"6a63736a-feb8-b74b-ef68-73cc71009e1d@gmail.com","subject":"Re: [PATCH v2 0/5] Sparse Index: Integrate with 'git add'","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-07-29T02:57:14Z","receivedAt":"2021-07-29T02:57:28Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Jul 28, 2021 at 8:03 PM Derrick Stolee wrote:\n>\n> On 7/28/2021 7:13 PM, Elijah Newren wrote:\n> > On Mon, Jul 26, 2021 at 9:18 AM Derrick Stolee via GitGitGadget\n> > wrote:\n> ...\n> >> * a full proposal for what to do with \"git (add|mv|rm)\" and paths outside\n> >> the cone is delayed to another series (with an RFC round) because the\n> >> behavior of the sparse-index matches a full index with sparse-checkout.\n> >\n> > I think this makes sense.\n> >\n> > I've read through the patches, and I like this version...with one\n> > exception. Can we mark the test added in patch 1 under\n> >\n> > # 3. Rename the file to another sparse filename and\n> > # accept conflict markers as resolved content.\n> >\n> > as NEEDSWORK or even MAYNEEDWORK?\n>\n> I have no objection to adding a blurb such as:\n>\n> # NEEDSWORK: allowing adds outside the sparse cone can be\n> # confusingto users, as the file can disappear from the\n> # worktree without warning in later Git commands.\n>\n\nSounds great to me other than the simple typo (s/confusingto/confusing to/)\n\n> And perhaps I'm misunderstanding the situation a bit, but that\n> seems to apply not just to this third case, but all of them. I\n> don't see why the untracked case is special compared to the\n> tracked case. More investigation may be required on my part.\n\nThe possible cases for files outside the sparsity patterns are:\n a) untracked\n b) tracked and SKIP_WORKTREE\n c) tracked and !SKIP_WORKTREE (e.g. because merge conflicts)\n\nFrom the above set, we've been talking about untracked and I think\nwe're on the same page about those. Case (b) was already corrected by\nMatheus a number of releases back; git-add will throw an error\nexplaining the situation and prevent the adding. The error tells the\nuser to expand their sparsity set to work on those files. For case\n(c), you are right that those are problematic in the same way (they\ncan disappear later after a git-add)...but we're also in the situation\nwhere the only way to get rid of the conflicting stages is to run git\nadd. So, in my mind, case (c) puts us between a rock and a hard\nplace, and we probably need to allow the git-add.\n\n> > I'm still quite unconvinced that it\n> > is testing for correct behavior, and don't want to paint ourselves\n> > into a corner. In particular, we don't allow folks to \"git add\n> > $IGNORED_FILE\" without a --force override because it's likely to be a\n> > mistake.\n>\n> I agree about ignored files, and that is true whether or not they\n> are in the sparse cone.\n\nYes, and...\n\n> > I think the same logic holds for adding untracked files\n> > outside the sparsity cone.\n\nIn my opinion, \"outside the sparsity cone\" is another form of \"being\nignored\", and in my mind should be treated similarly -- it should\ngenerally require an override to add such files. (Case (c) possibly\nbeing an exception, though maybe even it shouldn't be.)\n\n> > But it's actually even worse than that\n> > case because there's a secondary level of surprise too: adding files\n> > outside the sparsity cone will result in delayed user surprises when\n> > the next git command that happens to call unpack_trees() (which are\n> > found all over the codebase) removes the file from the working tree.\n> > I've had some such reports already.\n>\n> I believe this is testing a realistic scenario that users are\n> hitting in the wild today. I would believe that users succeed with\n> these commands more often than they are confused by the file\n> disappearing from the worktree in a later Git command, so having\n> this sequence of events be documented as a potential use case has\n> some value.\n>\n> I simultaneously don't think it is behavior we want to commit to\n> as a contract for all future Git versions, but there is value in\n> showing how this situation changes with any future meddling. In\n> particular: will users be able to self-discover the \"new\" way of\n> doing things?\n\nOh, I totally agree that documenting how things work definitely has\nvalue. I've added several test_expect_failure cases and whatnot to\nthe testsuite. But there's a big difference between documenting how\nthings work and documenting how we expect them to work. If the two\ndiffer, then any good provided by documenting how things work with a\ntest marked as test_expect_success may be counterbalanced or even\noverwhelmed by the harm it also causes, particularly in areas where\nworking around backward compatibility constraints are more difficult.\n\nFor example, not that long ago, it seemed people agreed (even Junio)\nthat commit hooks were never intended to be part of rebase (they\naren't part of the apply backend, and were only part of the\nmerge/interactive backend due to historical accident) and could be\nremoved (being replaced by just a rebase hook called at the end of the\nrebase instead of with every commit). There were user complaints\nabout the commit hooks being triggered when the default backend\nswitched, backing up the expectation. But no one jumped in to fix it\nat the time. Then when it was brought up again recently, Junio said\nwe couldn't just remove those because of backward compatibility.\nThat's forcing me to consider suggesting a bunch of new arguments to\nrebase to let users get unbroken when they discover they need it, or\nmaybe even a new toplevel command because we painted ourselves into a\ncorner (there are more backward compatibility corners in rebase\ntoo...).\n\nTrying to get out of a corner we paint ourselves into with\nsparse-checkout would be massively harder, which is why I keep harping\non this kind of thing. I'm very concerned it's happening even despite\nmy numerous comments and worries about it.\n\n> The proposal part of changing how add/mv/rm behave in these cases\n> would need to adjust this test with something that would also help\n> direct users to a helpful resolution. For example, the first run\n> of\n>\n> git add sparse/dir/file\n>\n> could error out with an error message saying \"The pathspec is\n> outside of your sparse cone, so staging the file might lead to\n> a staged change that is removed from your working directory.\"\n\nYes, much like we currently do with tracked files which are SKIP_WORKTREE.\n\n> But we should _also_ include two strategies for getting out of\n> this state:\n>\n> 1. Adjust your sparse-checkout definition so this file is in scope.\n>\n> -or- (and this is the part that would be new)\n>\n> 2. If you understand the risks of staging a file outside the sparse\n> cone, then run 'git add --sparse sparse/dir/file'.\n>\n> (Insert whatever option would be appropriate for --sparse here.)\n>\n> Such a warning message would allow users who follow the steps listed\n> in the test to know how to adjust their usage to then get into a\n> good state.\n\nChoice 2 doesn't exist yet, but yeah your suggestion makes sense.\n\n> > If that test is marked as NEEDSWORK or even as the correct behavior\n> > still being under dispute, then you can happily apply my:\n>\n> I would classify this as \"The test documents current behavior, but\n> isn't a contract for future behavior.\" With a concept such as my\n> suggestion above, the test could be modified to check for the\n> warning and then run the second command with the extra option and\n> complete the test's expectations. Having the existing behavior\n> documented in a test helps demonstrate how behavior is changing.\n>\n> We we've discussed, we want to give such a behavior change the\n> right venue for feedback and suggestions for alternate approaches,\n> and this series is not the right place for that. Hopefully you\n> can tell that it is on my mind and that I want to recommend a\n> change in the near future.\n\nI'm totally fine with such changes not being part of this series. I\njust don't want a test_expect_success that checks for behavior that I\nconsider buggy unless it comes with a disclaimer that it's checking\nfor existing rather than expected behavior.\n"},{"id":"431484","messageId":"2a24488f-d0c8-d1f4-7115-36ac0745658d@gmail.com","threadId":"56137","inReplyTo":"CABPp-BGUTg=GarkhP0MwjWKWmDyRJiEL2J75wFz52y2xi_50mw@mail.gmail.com","subject":"Re: [PATCH v2 0/5] Sparse Index: Integrate with 'git add'","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-07-29T14:49:32Z","receivedAt":"2021-07-29T14:49:40Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/28/2021 10:57 PM, Elijah Newren wrote:\n> On Wed, Jul 28, 2021 at 8:03 PM Derrick Stolee wrote:\n>>\n>> On 7/28/2021 7:13 PM, Elijah Newren wrote:\n>>> On Mon, Jul 26, 2021 at 9:18 AM Derrick Stolee via GitGitGadget\n>>> wrote:\n>> ...\n>>>> * a full proposal for what to do with \"git (add|mv|rm)\" and paths outside\n>>>> the cone is delayed to another series (with an RFC round) because the\n>>>> behavior of the sparse-index matches a full index with sparse-checkout.\n>>>\n>>> I think this makes sense.\n>>>\n>>> I've read through the patches, and I like this version...with one\n>>> exception. Can we mark the test added in patch 1 under\n>>>\n>>> # 3. Rename the file to another sparse filename and\n>>> # accept conflict markers as resolved content.\n>>>\n>>> as NEEDSWORK or even MAYNEEDWORK?\n>>\n>> I have no objection to adding a blurb such as:\n>>\n>> # NEEDSWORK: allowing adds outside the sparse cone can be\n>> # confusingto users, as the file can disappear from the\n>> # worktree without warning in later Git commands.\n>>\n> \n> Sounds great to me other than the simple typo (s/confusingto/confusing to/)\n> \n>> And perhaps I'm misunderstanding the situation a bit, but that\n>> seems to apply not just to this third case, but all of them. I\n>> don't see why the untracked case is special compared to the\n>> tracked case. More investigation may be required on my part.\n> \n> The possible cases for files outside the sparsity patterns are:\n> a) untracked\n> b) tracked and SKIP_WORKTREE\n> c) tracked and !SKIP_WORKTREE (e.g. because merge conflicts)\n> \n> From the above set, we've been talking about untracked and I think\n> we're on the same page about those. Case (b) was already corrected by\n> Matheus a number of releases back; git-add will throw an error\n> explaining the situation and prevent the adding. The error tells the\n> user to expand their sparsity set to work on those files. For case\n> (c), you are right that those are problematic in the same way (they\n> can disappear later after a git-add)...but we're also in the situation\n> where the only way to get rid of the conflicting stages is to run git\n> add. So, in my mind, case (c) puts us between a rock and a hard\n> place, and we probably need to allow the git-add.\n\nI appreciate this additional context. Thanks.\n \n>>> I'm still quite unconvinced that it\n>>> is testing for correct behavior, and don't want to paint ourselves\n>>> into a corner. In particular, we don't allow folks to \"git add\n>>> $IGNORED_FILE\" without a --force override because it's likely to be a\n>>> mistake.\n>>\n>> I agree about ignored files, and that is true whether or not they\n>> are in the sparse cone.\n> \n> Yes, and...\n> \n>>> I think the same logic holds for adding untracked files\n>>> outside the sparsity cone.\n> \n> In my opinion, \"outside the sparsity cone\" is another form of \"being\n> ignored\", and in my mind should be treated similarly -- it should\n> generally require an override to add such files. (Case (c) possibly\n> being an exception, though maybe even it shouldn't be.)\n\nI don't hold that same interpretation. I think of it instead as\n\"hidden\" files, but they still matter. I also think that advising\none to adjust their sparsity patterns might be dangerous because\nnot all users know the ramifications of doing that. They might\naccidentally download an enormous amount of data to correct a\nsingle file.\n\nHaving an override seems like the best option, and we can hopefully\nmake it consistent across all the cases and commands.\n\n...\n\n> Trying to get out of a corner we paint ourselves into with\n> sparse-checkout would be massively harder, which is why I keep harping\n> on this kind of thing. I'm very concerned it's happening even despite\n> my numerous comments and worries about it.\n...\n> I'm totally fine with such changes not being part of this series. I\n> just don't want a test_expect_success that checks for behavior that I\n> consider buggy unless it comes with a disclaimer that it's checking\n> for existing rather than expected behavior.\n\nI understand your perspective. I'll send a v3 soon that adds a\ncomment on top of the entire test signalling the things we talked\nabout here: this is a documentation of behavior, not an endorsement,\nand we should probably change it because users can get confused.\n\nThanks,\n-Stolee\n"},{"id":"431485","messageId":"pull.999.v3.git.1627570327.gitgitgadget@gmail.com","threadId":"56137","inReplyTo":"pull.999.v2.git.1627312727.gitgitgadget@gmail.com","subject":"[PATCH v3 0/5] Sparse Index: Integrate with 'git add'","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-29T14:52:02Z","receivedAt":"2021-07-29T14:52:14Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"This patch series re-submits the 'git add' integration with sparse-index.\nThe performance gains are the same as before.\n\nIt is based on ds/commit-and-checkout-with-sparse-index.\n\nThis series was delayed from its initial submission for a couple reasons.\n\nThe first was because it was colliding with some changes in\nmt/add-rm-in-sparse-checkout, so now we are far enough along that that\nbranch is in our history and we can work forwards.\n\nThe other concern was about how 'git add ' should respond when a path\noutside of the sparse-checkout cone exists. One recommendation (that I am\nfailing to find a link to the message, sorry) was to disallow adding files\nthat would become index entries with SKIP_WORKTREE on. However, as I worked\ntowards that goal I found that change would cause problems for a realistic\nscenario: merge conflicts outside of the sparse-checkout cone.\n\nUpdate: Elijah points out that the SKIP_WORKTREE bit is removed from\nconflict files, which allows adding the conflicted files without warning.\n(However, we also need to be careful about untracked files, as documented in\nthe test added here.)\n\nThe first patch of this series adds tests that create merge conflicts\noutside of the sparse cone and then presents different ways a user could\nresolve the situation. We want all of them to be feasible, and this\nincludes:\n\n 1. Reverting the file to a known version in history.\n 2. Adding the file with its contents on disk.\n 3. Moving the file to a new location in the sparse directory.\n\nThe one place I did continue to update is 'git add --refresh ' to match the\nbehavior added by mt/add-rm-in-sparse-checkout which outputs an error\nmessage. This happens even when the file exists in the working directory,\nbut that seems appropriate enough.\n\n\nUpdates in V3\n=============\n\n * Added disclaimer to the merge-conflict test that this is documenting\n current behavior, not endorsing it.\n\n * Added Elijah's reviewed-by. Thanks for the review!\n\nThanks, -Stolee\n\nDerrick Stolee (5):\n t1092: test merge conflicts outside cone\n add: allow operating on a sparse-only index\n pathspec: stop calling ensure_full_index\n add: ignore outside the sparse-checkout in refresh()\n add: remove ensure_full_index() with --renormalize\n\n builtin/add.c | 15 ++++--\n pathspec.c | 2 -\n t/t1092-sparse-checkout-compatibility.sh | 67 ++++++++++++++++++++----\n 3 files changed, 70 insertions(+), 14 deletions(-)\n\n\nbase-commit: 71e301501c88399711a1bf8515d1747e92cfbb9b\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-999%2Fderrickstolee%2Fsparse-index%2Fadd-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-999/derrickstolee/sparse-index/add-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/999\n\nRange-diff vs v2:\n\n 1: 8f2fd9370fe ! 1: 5e96df4df58 t1092: test merge conflicts outside cone\n @@ Metadata\n ## Commit message ##\n t1092: test merge conflicts outside cone\n \n + Conflicts can occur outside of the sparse-checkout definition, and in\n + that case users might try to resolve the conflicts in several ways.\n + Document a few of these ways in a test. Make it clear that this behavior\n + is not necessarily the optimal flow, since users can become confused\n + when Git deletes these files from the worktree in later commands.\n +\n + Reviewed-by: Elijah Newren \n Signed-off-by: Derrick Stolee \n \n ## t/t1092-sparse-checkout-compatibility.sh ##\n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'merge' '\n \ttest_all_match git rev-parse HEAD^{tree}\n '\n \n ++# NEEDSWORK: This test is documenting current behavior, but that\n ++# behavior can be confusing to users so there is desire to change it.\n ++# Right now, users might be using this flow to work through conflicts,\n ++# so any solution should present advice to users who try this sequence\n ++# of commands to follow whatever new method we create.\n +test_expect_success 'merge with conflict outside cone' '\n +\tinit_repos &&\n +\n 2: 6e43f118fa0 ! 2: defab1b86d3 add: allow operating on a sparse-only index\n @@ Commit message\n the use of a sparse index. We modify a test in t1092 to demonstrate\n these changes which will be remedied in future changes.\n \n + Reviewed-by: Elijah Newren \n Signed-off-by: Derrick Stolee \n \n ## builtin/add.c ##\n 3: 2ae91e0af29 ! 3: 9fc4313c889 pathspec: stop calling ensure_full_index\n @@ Commit message\n commits. Comparing to the full index case, we see the performance go\n from 0.33s to 0.05s, an 85% improvement.\n \n + Reviewed-by: Elijah Newren \n Signed-off-by: Derrick Stolee \n \n ## pathspec.c ##\n 4: a79728d4c64 ! 4: 0ec03ab021d add: ignore outside the sparse-checkout in refresh()\n @@ Commit message\n tracked, untracked, or ignored. We simply avoid updating the stat()\n information because there isn't even an entry that matches the path!\n \n + Reviewed-by: Elijah Newren \n Signed-off-by: Derrick Stolee \n \n ## builtin/add.c ##\n 5: 1543550a4e8 ! 5: adf5b15ac3d add: remove ensure_full_index() with --renormalize\n @@ Commit message\n SKIP_WORKTREE bit, so it will continue to do so with a sparse index\n because the sparse directory entries also have this bit set.\n \n + Reviewed-by: Elijah Newren \n Signed-off-by: Derrick Stolee \n \n ## builtin/add.c ##\n\n-- \ngitgitgadget\n"},{"id":"431486","messageId":"defab1b86d3427c9e369e81cc481083a7dacace7.1627570327.git.gitgitgadget@gmail.com","threadId":"56137","inReplyTo":"pull.999.v3.git.1627570327.gitgitgadget@gmail.com","subject":"[PATCH v3 2/5] add: allow operating on a sparse-only index","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-29T14:52:04Z","receivedAt":"2021-07-29T14:52:15Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nDisable command_requires_full_index for 'git add'. This does not require\nany additional removals of ensure_full_index(). The main reason is that\n'git add' discovers changes based on the pathspec and the worktree\nitself. These are then inserted into the index directly, and calls to\nindex_name_pos() or index_file_exists() already call expand_to_path() at\nthe appropriate time to support a sparse-index.\n\nAdd a test to check that 'git add -A' and 'git add ' does not\nexpand the index at all, as long as is not within a sparse\ndirectory. This does not help the global 'git add .' case.\n\nWe can measure the improvement using p2000-sparse-operations.sh with\nthese results:\n\nTest HEAD~1 HEAD\n------------------------------------------------------------------------------\n2000.6: git add -A (full-index-v3) 0.35(0.30+0.05) 0.37(0.29+0.06) +5.7%\n2000.7: git add -A (full-index-v4) 0.31(0.26+0.06) 0.33(0.27+0.06) +6.5%\n2000.8: git add -A (sparse-index-v3) 0.57(0.53+0.07) 0.05(0.04+0.08) -91.2%\n2000.9: git add -A (sparse-index-v4) 0.58(0.55+0.06) 0.05(0.05+0.06) -91.4%\n\nWhile the 91% improvement seems impressive, it's important to recognize\nthat previously we had significant overhead for expanding the\nsparse-index. Comparing to the full index case, 'git add -A' goes from\n0.37s to 0.05s, which is \"only\" an 86% improvement.\n\nThis modification to 'git add' creates some behavior change depending on\nthe use of a sparse index. We modify a test in t1092 to demonstrate\nthese changes which will be remedied in future changes.\n\nReviewed-by: Elijah Newren \nSigned-off-by: Derrick Stolee \n---\n builtin/add.c | 3 +++\n t/t1092-sparse-checkout-compatibility.sh | 25 +++++++++++++++++-------\n 2 files changed, 21 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex b773b5a4993..c76e6ddd359 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -528,6 +528,9 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \tadd_new_files = !take_worktree_changes && !refresh_only && !add_renormalize;\n \trequire_pathspec = !(take_worktree_changes || (0 < addremove_explicit));\n \n+\tprepare_repo_settings(the_repository);\n+\tthe_repository->settings.command_requires_full_index = 0;\n+\n \thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n \n \t/*\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 4c3bcb34999..77343cb6d95 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -340,21 +340,27 @@ test_expect_success 'status/add: outside sparse cone' '\n \n \ttest_sparse_match git status --porcelain=v2 &&\n \n-\t# This \"git add folder1/a\" fails with a warning\n-\t# in the sparse repos, differing from the full\n-\t# repo. This is intentional.\n+\t# Adding the path outside of the sparse-checkout cone should fail.\n \ttest_sparse_match test_must_fail git add folder1/a &&\n-\ttest_sparse_match test_must_fail git add --refresh folder1/a &&\n-\ttest_all_match git status --porcelain=v2 &&\n+\n+\ttest_must_fail git -C sparse-checkout add --refresh folder1/a 2>sparse-checkout-err &&\n+\ttest_must_fail git -C sparse-index add --refresh folder1/a 2>sparse-index-err &&\n+\t# NEEDSWORK: A sparse index changes the error message.\n+\t! test_cmp sparse-checkout-err sparse-index-err &&\n+\n+\t# NEEDSWORK: Adding a newly-tracked file outside the cone succeeds\n+\ttest_sparse_match git add folder1/new &&\n \n \ttest_all_match git add . &&\n \ttest_all_match git status --porcelain=v2 &&\n \ttest_all_match git commit -m folder1/new &&\n+\ttest_all_match git rev-parse HEAD^{tree} &&\n \n \trun_on_all ../edit-contents folder1/newer &&\n \ttest_all_match git add folder1/ &&\n \ttest_all_match git status --porcelain=v2 &&\n-\ttest_all_match git commit -m folder1/newer\n+\ttest_all_match git commit -m folder1/newer &&\n+\ttest_all_match git rev-parse HEAD^{tree}\n '\n \n test_expect_success 'checkout and reset --hard' '\n@@ -641,7 +647,12 @@ test_expect_success 'sparse-index is not expanded' '\n \tgit -C sparse-index reset --hard &&\n \tensure_not_expanded checkout rename-out-to-out -- deep/deeper1 &&\n \tgit -C sparse-index reset --hard &&\n-\tensure_not_expanded restore -s rename-out-to-out -- deep/deeper1\n+\tensure_not_expanded restore -s rename-out-to-out -- deep/deeper1 &&\n+\n+\techo >>sparse-index/README.md &&\n+\tensure_not_expanded add -A &&\n+\techo >>sparse-index/extra.txt &&\n+\tensure_not_expanded add extra.txt\n '\n \n # NEEDSWORK: a sparse-checkout behaves differently from a full checkout\n-- \ngitgitgadget\n\n"},{"id":"431487","messageId":"5e96df4df582744a89b10ce55887a2c2aacc7e70.1627570327.git.gitgitgadget@gmail.com","threadId":"56137","inReplyTo":"pull.999.v3.git.1627570327.gitgitgadget@gmail.com","subject":"[PATCH v3 1/5] t1092: test merge conflicts outside cone","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-29T14:52:03Z","receivedAt":"2021-07-29T14:52:16Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nConflicts can occur outside of the sparse-checkout definition, and in\nthat case users might try to resolve the conflicts in several ways.\nDocument a few of these ways in a test. Make it clear that this behavior\nis not necessarily the optimal flow, since users can become confused\nwhen Git deletes these files from the worktree in later commands.\n\nReviewed-by: Elijah Newren \nSigned-off-by: Derrick Stolee \n---\n t/t1092-sparse-checkout-compatibility.sh | 43 ++++++++++++++++++++++++\n 1 file changed, 43 insertions(+)\n\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 91e30d6ec22..4c3bcb34999 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -114,6 +114,16 @@ test_expect_success 'setup' '\n \t\tgit add . &&\n \t\tgit commit -m \"file to dir\" &&\n \n+\t\tfor side in left right\n+\t\tdo\n+\t\t\tgit checkout -b merge-$side base &&\n+\t\t\techo $side >>deep/deeper2/a &&\n+\t\t\techo $side >>folder1/a &&\n+\t\t\techo $side >>folder2/a &&\n+\t\t\tgit add . &&\n+\t\t\tgit commit -m \"$side\" || return 1\n+\t\tdone &&\n+\n \t\tgit checkout -b deepest base &&\n \t\techo \"updated deepest\" >deep/deeper1/deepest/a &&\n \t\tgit commit -a -m \"update deepest\" &&\n@@ -482,6 +492,39 @@ test_expect_success 'merge' '\n \ttest_all_match git rev-parse HEAD^{tree}\n '\n \n+# NEEDSWORK: This test is documenting current behavior, but that\n+# behavior can be confusing to users so there is desire to change it.\n+# Right now, users might be using this flow to work through conflicts,\n+# so any solution should present advice to users who try this sequence\n+# of commands to follow whatever new method we create.\n+test_expect_success 'merge with conflict outside cone' '\n+\tinit_repos &&\n+\n+\ttest_all_match git checkout -b merge-tip merge-left &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\ttest_all_match test_must_fail git merge -m merge merge-right &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\t# Resolve the conflict in different ways:\n+\t# 1. Revert to the base\n+\ttest_all_match git checkout base -- deep/deeper2/a &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\t# 2. Add the file with conflict markers\n+\ttest_all_match git add folder1/a &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\t# 3. Rename the file to another sparse filename and\n+\t# accept conflict markers as resolved content.\n+\trun_on_all mv folder2/a folder2/z &&\n+\ttest_all_match git add folder2 &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\ttest_all_match git merge --continue &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\ttest_all_match git rev-parse HEAD^{tree}\n+'\n+\n test_expect_success 'merge with outside renames' '\n \tinit_repos &&\n \n-- \ngitgitgadget\n\n"},{"id":"431488","messageId":"0ec03ab021da8b8a2278af3e14960c5cf4689646.1627570327.git.gitgitgadget@gmail.com","threadId":"56137","inReplyTo":"pull.999.v3.git.1627570327.gitgitgadget@gmail.com","subject":"[PATCH v3 4/5] add: ignore outside the sparse-checkout in refresh()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-29T14:52:06Z","receivedAt":"2021-07-29T14:52:17Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nSince b243012 (refresh_index(): add flag to ignore SKIP_WORKTREE\nentries, 2021-04-08), 'git add --refresh ' will output a warning\nmessage when the path is outside the sparse-checkout definition. The\nimplementation of this warning happened in parallel with the\nsparse-index work to add ensure_full_index() calls throughout the\ncodebase.\n\nUpdate this loop to have the proper logic that checks to see if the\npathspec is outside the sparse-checkout definition. This avoids the need\nto expand the sparse directory entry and determine if the path is\ntracked, untracked, or ignored. We simply avoid updating the stat()\ninformation because there isn't even an entry that matches the path!\n\nReviewed-by: Elijah Newren \nSigned-off-by: Derrick Stolee \n---\n builtin/add.c | 10 +++++++++-\n t/t1092-sparse-checkout-compatibility.sh | 6 +-----\n 2 files changed, 10 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex c76e6ddd359..d512ece655b 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -192,13 +192,21 @@ static int refresh(int verbose, const struct pathspec *pathspec)\n \tstruct string_list only_match_skip_worktree = STRING_LIST_INIT_NODUP;\n \tint flags = REFRESH_IGNORE_SKIP_WORKTREE |\n \t\t (verbose ? REFRESH_IN_PORCELAIN : REFRESH_QUIET);\n+\tstruct pattern_list pl = { 0 };\n+\tint sparse_checkout_enabled = !get_sparse_checkout_patterns(&pl);\n \n \tseen = xcalloc(pathspec->nr, 1);\n \trefresh_index(&the_index, flags, pathspec, seen,\n \t\t _(\"Unstaged changes after refreshing the index:\"));\n \tfor (i = 0; i < pathspec->nr; i++) {\n \t\tif (!seen[i]) {\n-\t\t\tif (matches_skip_worktree(pathspec, i, &skip_worktree_seen)) {\n+\t\t\tconst char *path = pathspec->items[i].original;\n+\t\t\tint dtype = DT_REG;\n+\n+\t\t\tif (matches_skip_worktree(pathspec, i, &skip_worktree_seen) ||\n+\t\t\t (sparse_checkout_enabled &&\n+\t\t\t !path_matches_pattern_list(path, strlen(path), NULL,\n+\t\t\t\t\t\t\t&dtype, &pl, &the_index))) {\n \t\t\t\tstring_list_append(&only_match_skip_worktree,\n \t\t\t\t\t\t pathspec->items[i].original);\n \t\t\t} else {\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex dee20db5bb1..ddc86bb4152 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -339,11 +339,7 @@ test_expect_success 'status/add: outside sparse cone' '\n \n \t# Adding the path outside of the sparse-checkout cone should fail.\n \ttest_sparse_match test_must_fail git add folder1/a &&\n-\n-\ttest_must_fail git -C sparse-checkout add --refresh folder1/a 2>sparse-checkout-err &&\n-\ttest_must_fail git -C sparse-index add --refresh folder1/a 2>sparse-index-err &&\n-\t# NEEDSWORK: A sparse index changes the error message.\n-\t! test_cmp sparse-checkout-err sparse-index-err &&\n+\ttest_sparse_match test_must_fail git add --refresh folder1/a &&\n \n \t# NEEDSWORK: Adding a newly-tracked file outside the cone succeeds\n \ttest_sparse_match git add folder1/new &&\n-- \ngitgitgadget\n\n"},{"id":"431489","messageId":"9fc4313c88959cb6eab43b2e34750b4ed889771a.1627570327.git.gitgitgadget@gmail.com","threadId":"56137","inReplyTo":"pull.999.v3.git.1627570327.gitgitgadget@gmail.com","subject":"[PATCH v3 3/5] pathspec: stop calling ensure_full_index","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-29T14:52:05Z","receivedAt":"2021-07-29T14:52:18Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe add_pathspec_matches_against_index() focuses on matching a pathspec\nto file entries in the index. This already works correctly for its only\nuse: checking if untracked files exist in the index.\n\nThe compatibility checks in t1092 already test that 'git add '\nworks for a directory outside of the sparse cone. That provides coverage\nfor removing this guard.\n\nThis finalizes our ability to run 'git add .' without expanding a sparse\nindex to a full one. This is evidenced by an update to t1092 and by\nthese performance numbers for p2000-sparse-operations.sh:\n\nTest HEAD~1 HEAD\n--------------------------------------------------------------------------------\n2000.10: git add . (full-index-v3) 0.37(0.28+0.07) 0.36(0.27+0.06) -2.7%\n2000.11: git add . (full-index-v4) 0.33(0.26+0.06) 0.32(0.28+0.05) -3.0%\n2000.12: git add . (sparse-index-v3) 0.57(0.53+0.07) 0.06(0.06+0.07) -89.5%\n2000.13: git add . (sparse-index-v4) 0.57(0.53+0.07) 0.05(0.03+0.09) -91.2%\n\nWhile the ~90% improvement is shown by the test results, it is worth\nnoting that expanding the sparse index was adding overhead in previous\ncommits. Comparing to the full index case, we see the performance go\nfrom 0.33s to 0.05s, an 85% improvement.\n\nReviewed-by: Elijah Newren \nSigned-off-by: Derrick Stolee \n---\n pathspec.c | 2 --\n t/t1092-sparse-checkout-compatibility.sh | 7 +++----\n 2 files changed, 3 insertions(+), 6 deletions(-)\n\ndiff --git a/pathspec.c b/pathspec.c\nindex 08f8d3eedc3..44306fdaca2 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -37,8 +37,6 @@ void add_pathspec_matches_against_index(const struct pathspec *pathspec,\n \t\t\tnum_unmatched++;\n \tif (!num_unmatched)\n \t\treturn;\n-\t/* TODO: audit for interaction with sparse-index. */\n-\tensure_full_index(istate);\n \tfor (i = 0; i < istate->cache_nr; i++) {\n \t\tconst struct cache_entry *ce = istate->cache[i];\n \t\tif (sw_action == PS_IGNORE_SKIP_WORKTREE && ce_skip_worktree(ce))\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 77343cb6d95..dee20db5bb1 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -322,9 +322,6 @@ test_expect_success 'commit including unstaged changes' '\n test_expect_success 'status/add: outside sparse cone' '\n \tinit_repos &&\n \n-\t# adding a \"missing\" file outside the cone should fail\n-\ttest_sparse_match test_must_fail git add folder1/a &&\n-\n \t# folder1 is at HEAD, but outside the sparse cone\n \trun_on_sparse mkdir folder1 &&\n \tcp initial-repo/folder1/a sparse-checkout/folder1/a &&\n@@ -652,7 +649,9 @@ test_expect_success 'sparse-index is not expanded' '\n \techo >>sparse-index/README.md &&\n \tensure_not_expanded add -A &&\n \techo >>sparse-index/extra.txt &&\n-\tensure_not_expanded add extra.txt\n+\tensure_not_expanded add extra.txt &&\n+\techo >>sparse-index/untracked.txt &&\n+\tensure_not_expanded add .\n '\n \n # NEEDSWORK: a sparse-checkout behaves differently from a full checkout\n-- \ngitgitgadget\n\n"},{"id":"431490","messageId":"adf5b15ac3d44d92e0438451ef36631ed3ee2a63.1627570327.git.gitgitgadget@gmail.com","threadId":"56137","inReplyTo":"pull.999.v3.git.1627570327.gitgitgadget@gmail.com","subject":"[PATCH v3 5/5] add: remove ensure_full_index() with --renormalize","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-29T14:52:07Z","receivedAt":"2021-07-29T14:52:19Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe --renormalize option updates the EOL conversions for the tracked\nfiles. However, the loop already ignores files marked with the\nSKIP_WORKTREE bit, so it will continue to do so with a sparse index\nbecause the sparse directory entries also have this bit set.\n\nReviewed-by: Elijah Newren \nSigned-off-by: Derrick Stolee \n---\n builtin/add.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex d512ece655b..c49e179abc3 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -144,8 +144,6 @@ static int renormalize_tracked_files(const struct pathspec *pathspec, int flags)\n {\n \tint i, retval = 0;\n \n-\t/* TODO: audit for interaction with sparse-index. */\n-\tensure_full_index(&the_index);\n \tfor (i = 0; i < active_nr; i++) {\n \t\tstruct cache_entry *ce = active_cache[i];\n \n-- \ngitgitgadget\n"},{"id":"431491","messageId":"CABPp-BFOW+oUiFEQG8ZYsocuVx35b5ayYyXP6M462QzJ8U7d0g@mail.gmail.com","threadId":"56137","inReplyTo":"pull.999.v3.git.1627570327.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 0/5] Sparse Index: Integrate with 'git add'","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-07-29T14:58:01Z","receivedAt":"2021-07-29T14:58:14Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Jul 29, 2021 at 8:52 AM Derrick Stolee via GitGitGadget\n wrote:\n>\n> This patch series re-submits the 'git add' integration with sparse-index.\n> The performance gains are the same as before.\n>\n> It is based on ds/commit-and-checkout-with-sparse-index.\n>\n> This series was delayed from its initial submission for a couple reasons.\n>\n> The first was because it was colliding with some changes in\n> mt/add-rm-in-sparse-checkout, so now we are far enough along that that\n> branch is in our history and we can work forwards.\n>\n> The other concern was about how 'git add ' should respond when a path\n> outside of the sparse-checkout cone exists. One recommendation (that I am\n> failing to find a link to the message, sorry) was to disallow adding files\n> that would become index entries with SKIP_WORKTREE on. However, as I worked\n> towards that goal I found that change would cause problems for a realistic\n> scenario: merge conflicts outside of the sparse-checkout cone.\n>\n> Update: Elijah points out that the SKIP_WORKTREE bit is removed from\n> conflict files, which allows adding the conflicted files without warning.\n> (However, we also need to be careful about untracked files, as documented in\n> the test added here.)\n>\n> The first patch of this series adds tests that create merge conflicts\n> outside of the sparse cone and then presents different ways a user could\n> resolve the situation. We want all of them to be feasible, and this\n> includes:\n>\n> 1. Reverting the file to a known version in history.\n> 2. Adding the file with its contents on disk.\n> 3. Moving the file to a new location in the sparse directory.\n>\n> The one place I did continue to update is 'git add --refresh ' to match the\n> behavior added by mt/add-rm-in-sparse-checkout which outputs an error\n> message. This happens even when the file exists in the working directory,\n> but that seems appropriate enough.\n>\n>\n> Updates in V3\n> =============\n>\n> * Added disclaimer to the merge-conflict test that this is documenting\n> current behavior, not endorsing it.\n>\n> * Added Elijah's reviewed-by. Thanks for the review!\n\nYep, this one looks ready to merge down to me. Thanks!\n\n> Thanks, -Stolee\n>\n> Derrick Stolee (5):\n> t1092: test merge conflicts outside cone\n> add: allow operating on a sparse-only index\n> pathspec: stop calling ensure_full_index\n> add: ignore outside the sparse-checkout in refresh()\n> add: remove ensure_full_index() with --renormalize\n>\n> builtin/add.c | 15 ++++--\n> pathspec.c | 2 -\n> t/t1092-sparse-checkout-compatibility.sh | 67 ++++++++++++++++++++----\n> 3 files changed, 70 insertions(+), 14 deletions(-)\n>\n>\n> base-commit: 71e301501c88399711a1bf8515d1747e92cfbb9b\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-999%2Fderrickstolee%2Fsparse-index%2Fadd-v3\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-999/derrickstolee/sparse-index/add-v3\n> Pull-Request: https://github.com/gitgitgadget/git/pull/999\n>\n> Range-diff vs v2:\n>\n> 1: 8f2fd9370fe ! 1: 5e96df4df58 t1092: test merge conflicts outside cone\n> @@ Metadata\n> ## Commit message ##\n> t1092: test merge conflicts outside cone\n>\n> + Conflicts can occur outside of the sparse-checkout definition, and in\n> + that case users might try to resolve the conflicts in several ways.\n> + Document a few of these ways in a test. Make it clear that this behavior\n> + is not necessarily the optimal flow, since users can become confused\n> + when Git deletes these files from the worktree in later commands.\n> +\n> + Reviewed-by: Elijah Newren \n> Signed-off-by: Derrick Stolee \n>\n> ## t/t1092-sparse-checkout-compatibility.sh ##\n> @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'merge' '\n> test_all_match git rev-parse HEAD^{tree}\n> '\n>\n> ++# NEEDSWORK: This test is documenting current behavior, but that\n> ++# behavior can be confusing to users so there is desire to change it.\n> ++# Right now, users might be using this flow to work through conflicts,\n> ++# so any solution should present advice to users who try this sequence\n> ++# of commands to follow whatever new method we create.\n> +test_expect_success 'merge with conflict outside cone' '\n> + init_repos &&\n> +\n> 2: 6e43f118fa0 ! 2: defab1b86d3 add: allow operating on a sparse-only index\n> @@ Commit message\n> the use of a sparse index. We modify a test in t1092 to demonstrate\n> these changes which will be remedied in future changes.\n>\n> + Reviewed-by: Elijah Newren \n> Signed-off-by: Derrick Stolee \n>\n> ## builtin/add.c ##\n> 3: 2ae91e0af29 ! 3: 9fc4313c889 pathspec: stop calling ensure_full_index\n> @@ Commit message\n> commits. Comparing to the full index case, we see the performance go\n> from 0.33s to 0.05s, an 85% improvement.\n>\n> + Reviewed-by: Elijah Newren \n> Signed-off-by: Derrick Stolee \n>\n> ## pathspec.c ##\n> 4: a79728d4c64 ! 4: 0ec03ab021d add: ignore outside the sparse-checkout in refresh()\n> @@ Commit message\n> tracked, untracked, or ignored. We simply avoid updating the stat()\n> information because there isn't even an entry that matches the path!\n>\n> + Reviewed-by: Elijah Newren \n> Signed-off-by: Derrick Stolee \n>\n> ## builtin/add.c ##\n> 5: 1543550a4e8 ! 5: adf5b15ac3d add: remove ensure_full_index() with --renormalize\n> @@ Commit message\n> SKIP_WORKTREE bit, so it will continue to do so with a sparse index\n> because the sparse directory entries also have this bit set.\n>\n> + Reviewed-by: Elijah Newren \n> Signed-off-by: Derrick Stolee \n>\n> ## builtin/add.c ##\n>\n> --\n> gitgitgadget\n"},{"id":"431535","messageId":"xmqqr1fg3eug.fsf@gitster.g","threadId":"56137","inReplyTo":"pull.999.v3.git.1627570327.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 0/5] Sparse Index: Integrate with 'git add'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-29T23:00:39Z","receivedAt":"2021-07-29T23:00:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> Update: Elijah points out that the SKIP_WORKTREE bit is removed from\n> conflict files, which allows adding the conflicted files without warning.\n> (However, we also need to be careful about untracked files, as documented in\n> the test added here.)\n>\n> The first patch of this series adds tests that create merge conflicts\n> outside of the sparse cone and then presents different ways a user could\n> resolve the situation. We want all of them to be feasible, and this\n> includes:\n>\n> 1. Reverting the file to a known version in history.\n> 2. Adding the file with its contents on disk.\n> 3. Moving the file to a new location in the sparse directory.\n>\n> The one place I did continue to update is 'git add --refresh ' to match the\n> behavior added by mt/add-rm-in-sparse-checkout which outputs an error\n> message. This happens even when the file exists in the working directory,\n> but that seems appropriate enough.\n>\n>\n> Updates in V3\n> =============\n>\n> * Added disclaimer to the merge-conflict test that this is documenting\n> current behavior, not endorsing it.\n>\n> * Added Elijah's reviewed-by. Thanks for the review!\n>\n> Thanks, -Stolee\n\nThanks, both. Let's queue this and start merging it down.\n"},{"id":"431563","messageId":"CABPp-BE58FqpD3u2ECZUzMJ=3oNBYHshkKzf+sZWkaeHG8SxWA@mail.gmail.com","threadId":"56137","inReplyTo":"2a24488f-d0c8-d1f4-7115-36ac0745658d@gmail.com","subject":"Re: [PATCH v2 0/5] Sparse Index: Integrate with 'git add'","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-07-30T12:52:35Z","receivedAt":"2021-07-30T12:52:49Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Jul 29, 2021 at 8:49 AM Derrick Stolee wrote:\n>\n> On 7/28/2021 10:57 PM, Elijah Newren wrote:\n> > On Wed, Jul 28, 2021 at 8:03 PM Derrick Stolee wrote:\n> >>\n> >> On 7/28/2021 7:13 PM, Elijah Newren wrote:\n> >>> On Mon, Jul 26, 2021 at 9:18 AM Derrick Stolee via GitGitGadget\n> >>> wrote:\n...\n> >> I agree about ignored files, and that is true whether or not they\n> >> are in the sparse cone.\n> >\n> > Yes, and...\n> >\n> >>> I think the same logic holds for adding untracked files\n> >>> outside the sparsity cone.\n> >\n> > In my opinion, \"outside the sparsity cone\" is another form of \"being\n> > ignored\", and in my mind should be treated similarly -- it should\n> > generally require an override to add such files. (Case (c) possibly\n> > being an exception, though maybe even it shouldn't be.)\n>\n> I don't hold that same interpretation. I think of it instead as\n> \"hidden\" files, but they still matter. I also think that advising\n> one to adjust their sparsity patterns might be dangerous because\n> not all users know the ramifications of doing that. They might\n> accidentally download an enormous amount of data to correct a\n> single file.\n>\n> Having an override seems like the best option, and we can hopefully\n> make it consistent across all the cases and commands.\n\nI think we might be arguing two sides of the same coin at this point.\nWe don't have a more general term for special in a way that shouldn't\nbe included by default with git-add, and I couldn't think of a good\nsynonym, so I used the words \"another form of being ignored\" (not\ntrying to imply that it was the same as .gitignored, but just that the\ntwo were special in a very similar way) while you tried to highlight\nthe differences using \"hidden\" but agreed they were similar in that\nthey should have an override.\n\nFair point on adjusting sparsity patterns and the data download it can cause.\n\n> ...\n>\n> > Trying to get out of a corner we paint ourselves into with\n> > sparse-checkout would be massively harder, which is why I keep harping\n> > on this kind of thing. I'm very concerned it's happening even despite\n> > my numerous comments and worries about it.\n> ...\n> > I'm totally fine with such changes not being part of this series. I\n> > just don't want a test_expect_success that checks for behavior that I\n> > consider buggy unless it comes with a disclaimer that it's checking\n> > for existing rather than expected behavior.\n>\n> I understand your perspective. I'll send a v3 soon that adds a\n> comment on top of the entire test signalling the things we talked\n> about here: this is a documentation of behavior, not an endorsement,\n> and we should probably change it because users can get confused.\n\nThanks for doing that. :-)\n"}]}