{"thread":{"id":"55795","subject":"bug report: git checkout deletes worktree file even though it is excluded by sparse-checkout","startedAt":"2021-05-28T20:43:57Z","lastAt":"2021-06-05T15:33:18Z","messageCount":10,"participants":["Tim Renouf (open source)","Elijah Newren","Tim Renouf","Derrick Stolee","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"425834","messageId":"F58C0A40-A1B4-4B47-A8EA-20FEAB011B0D@botech.co.uk","threadId":"55795","inReplyTo":null,"subject":"bug report: git checkout deletes worktree file even though it is excluded by sparse-checkout","fromName":"Tim Renouf (open source)","fromEmail":"tpr.ll@botech.co.uk","sentAt":"2021-05-28T20:14:20Z","receivedAt":"2021-05-28T20:43:57Z","isPatch":false,"sender":{"key":"tpr.ll@botech.co.uk","avatar":null},"body":"Hi all\n\nI have a bug report: git checkout deletes a worktree file even though it is excluded by sparse-checkout, even if it is dirty.\n\nWatch this (starting in an empty directory):\n\n$ git init\nInitialised empty Git repository in /mnt/amd/home/trenouf/amd/tmp/git/.git/\n$ echo file1 >file1; echo file2 >file2\n$ git add file1 file2\n$ git commit -m\"commit 1\"\n[master (root-commit) 4f7035d] commit 1\n 2 files changed, 2 insertions(+)\n create mode 100644 file1\n create mode 100644 file2\n$ git rm file2\nrm 'file2'\n$ git commit -m\"rm file2\"\n[master d025c99] rm file2\n 1 file changed, 1 deletion(-)\n delete mode 100644 file2\n$ git checkout master~1\nHEAD is now at 4f7035d commit 1\n$ git sparse-checkout set /file1\n$ ls\nfile1\n$ echo dirty >file2\n$ ls\nfile1  file2\n$ git checkout master\nPrevious HEAD position was 4f7035d commit 1\nSwitched to branch 'master'\n$ ls\nfile1\n\nI set up sparse-checkout to include only file1, not file2. file2 is now not in the worktree, even though it is in the commit I am checked out at. Then I create file2 with arbitrary content. Then a git checkout switching to the commit where file2 is removed also deletes it from the worktree.\n\nI assert that file2 should be left untouched by that checkout, because it is excluded by sparse-checkout. I guess file2 had its skip-worktree bit set before the checkout that removed it from the index; that should stop it being deleted in the worktree.\n\nTo be clear, I expect that last “ls” to still show “file1  file2”.\n\nThank you for your attention if you have got this far.\n\n-tpr\n\n[System Info]\ngit version:\ngit version 2.31.1\ncpu: x86_64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nuname: Linux 5.8.0-53-generic #60~20.04.1-Ubuntu SMP Thu May 6 09:52:46 UTC 2021 x86_64\ncompiler info: gnuc: 9.3\nlibc info: glibc: 2.31\n$SHELL (typically, interactive shell): /bin/bash\n\n"},{"id":"425849","messageId":"CABPp-BGUxHiYLjVcqBc0qpaHpd5MZCN_6S0YpH8tKcP3GSV2Pw@mail.gmail.com","threadId":"55795","inReplyTo":"F58C0A40-A1B4-4B47-A8EA-20FEAB011B0D@botech.co.uk","subject":"Re: bug report: git checkout deletes worktree file even though it is excluded by sparse-checkout","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-05-28T22:44:45Z","receivedAt":"2021-05-28T22:45:28Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi,\n\nOn Fri, May 28, 2021 at 1:46 PM Tim Renouf (open source)\n<tpr.ll@botech.co.uk> wrote:\n>\n> Hi all\n>\n> I have a bug report: git checkout deletes a worktree file even though it is excluded by sparse-checkout, even if it is dirty.\n>\n> Watch this (starting in an empty directory):\n>\n> $ git init\n> Initialised empty Git repository in /mnt/amd/home/trenouf/amd/tmp/git/.git/\n> $ echo file1 >file1; echo file2 >file2\n> $ git add file1 file2\n> $ git commit -m\"commit 1\"\n> [master (root-commit) 4f7035d] commit 1\n>  2 files changed, 2 insertions(+)\n>  create mode 100644 file1\n>  create mode 100644 file2\n> $ git rm file2\n> rm 'file2'\n> $ git commit -m\"rm file2\"\n> [master d025c99] rm file2\n>  1 file changed, 1 deletion(-)\n>  delete mode 100644 file2\n> $ git checkout master~1\n> HEAD is now at 4f7035d commit 1\n> $ git sparse-checkout set /file1\n> $ ls\n> file1\n> $ echo dirty >file2\n> $ ls\n> file1  file2\n> $ git checkout master\n> Previous HEAD position was 4f7035d commit 1\n> Switched to branch 'master'\n> $ ls\n> file1\n>\n> I set up sparse-checkout to include only file1, not file2. file2 is now not in the worktree, even though it is in the commit I am checked out at. Then I create file2 with arbitrary content. Then a git checkout switching to the commit where file2 is removed also deletes it from the worktree.\n>\n> I assert that file2 should be left untouched by that checkout, because it is excluded by sparse-checkout. I guess file2 had its skip-worktree bit set before the checkout that removed it from the index; that should stop it being deleted in the worktree.\n>\n> To be clear, I expect that last “ls” to still show “file1  file2”.\n>\n> Thank you for your attention if you have got this far.\n\nThanks for the report.  It's another example of how\n\"skip-worktree-means-treat-the-file-as-matching-head\" causes\nconfusion.  You can find more issues with\npresent-despite-skip-worktree files at\nhttps://lore.kernel.org/git/CABPp-BF6GpoDtMfpzf=3VWL_puuRH-cNV=9KajdF1003Fe05jA@mail.gmail.com/,\nincluding reasons they come up more often than you'd think.\n\nWe do need to get these fixed up, though I don't want to step on\nStolee's toes with his sparse-index work, and I'm hesitant to open new\nbig projects until merge-ort is complete.\n\n> -tpr\n>\n> [System Info]\n> git version:\n> git version 2.31.1\n> cpu: x86_64\n> no commit associated with this build\n> sizeof-long: 8\n> sizeof-size_t: 8\n> shell-path: /bin/sh\n> uname: Linux 5.8.0-53-generic #60~20.04.1-Ubuntu SMP Thu May 6 09:52:46 UTC 2021 x86_64\n> compiler info: gnuc: 9.3\n> libc info: glibc: 2.31\n> $SHELL (typically, interactive shell): /bin/bash\n"},{"id":"426154","messageId":"20210601183106.3084008-1-tpr.ll@botech.co.uk","threadId":"55795","inReplyTo":"CABPp-BGUxHiYLjVcqBc0qpaHpd5MZCN_6S0YpH8tKcP3GSV2Pw@mail.gmail.com","subject":"[PATCH] unpack-trees: add core.sparseCheckoutRmFiles config","fromName":"Tim Renouf","fromEmail":"tpr.ll@botech.co.uk","sentAt":"2021-06-01T18:31:06Z","receivedAt":"2021-06-01T18:31:16Z","isPatch":true,"sender":{"key":"tpr.ll@botech.co.uk","avatar":null},"body":"When doing a checkout (or other index merge from a tree) causes the\nremoval of a path that is outside sparse-checkout, the file is removed\nfrom the working tree, even if it is dirty.\n\nThat is probably what you want if the file got there by being\nmaterialized in a merge conflict. But it is not what you want if you\ndeliberately put the file there.\n\nThis commit adds the above config item, defaulting to \"true\" to get the\nold behavior. Set it to \"false\" to avoid removing such a file from the\nworktree.\n\nSigned-off-by: Tim Renouf <tpr.ll@botech.co.uk>\n---\nHere is a fix for my problem, hidden under a config option as it might\nnot be what everyone wants (there are a few tests that rely on the\nexisting behavior). I realize this is a bit of a piecemeal approach.\nHopefully it will be superseded by the sparse-index work when that\narrives.\n\n Documentation/git-sparse-checkout.txt | 11 ++++++++\n cache.h                               |  1 +\n config.c                              |  5 ++++\n environment.c                         |  1 +\n t/t1011-read-tree-sparse-checkout.sh  | 36 +++++++++++++++++++++++++++\n unpack-trees.c                        | 14 ++++++++---\n 6 files changed, 64 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-sparse-checkout.txt b/Documentation/git-sparse-checkout.txt\nindex a0eeaeb02e..31adb38b1d 100644\n--- a/Documentation/git-sparse-checkout.txt\n+++ b/Documentation/git-sparse-checkout.txt\n@@ -111,6 +111,17 @@ the sparse-checkout file.\n To repopulate the working directory with all files, use the\n `git sparse-checkout disable` command.\n \n+The `core.sparseCheckoutRmFiles` config setting determines what to do when a\n+checkout (or other index merge from a tree) causes the removal of a path that\n+is outside the sparse-checkout patterns but the file exists in the worktree\n+anyway. The default is `true`, which causes such a file to be removed from the\n+worktree, which is probably what you want to remove outside-sparse-checkout\n+files that were materialized by a merge conflict. Setting it to `false` means\n+that such a file is not removed, which is probably what you want if you\n+deliberately have files in the outside-sparse-checkout part of a worktree.\n+\n+The behavior with regard to worktree files that are outside sparse-checkout\n+patterns is likely to change in the future.\n \n FULL PATTERN SET\n ----------------\ndiff --git a/cache.h b/cache.h\nindex 6fda8091f1..19ee1cfc02 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -964,6 +964,7 @@ extern const char *core_fsmonitor;\n \n extern int core_apply_sparse_checkout;\n extern int core_sparse_checkout_cone;\n+extern int core_sparse_checkout_rm_files;\n \n /*\n  * Include broken refs in all ref iterations, which will\ndiff --git a/config.c b/config.c\nindex 6428393a41..dd24e753a8 100644\n--- a/config.c\n+++ b/config.c\n@@ -1552,6 +1552,11 @@ static int git_default_core_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(var, \"core.sparsecheckoutrmfiles\")) {\n+\t\tcore_sparse_checkout_rm_files = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n \tif (!strcmp(var, \"core.precomposeunicode\")) {\n \t\tprecomposed_unicode = git_config_bool(var, value);\n \t\treturn 0;\ndiff --git a/environment.c b/environment.c\nindex 2f27008424..cff6bbbe62 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -70,6 +70,7 @@ char *notes_ref_name;\n int grafts_replace_parents = 1;\n int core_apply_sparse_checkout;\n int core_sparse_checkout_cone;\n+int core_sparse_checkout_rm_files = 1;\n int merge_log_config = -1;\n int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */\n unsigned long pack_size_limit_cfg;\ndiff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh\nindex 24092c09a9..67690b31c3 100755\n--- a/t/t1011-read-tree-sparse-checkout.sh\n+++ b/t/t1011-read-tree-sparse-checkout.sh\n@@ -280,4 +280,40 @@ test_expect_success 'checkout with --ignore-skip-worktree-bits' '\n \tgit diff --exit-code HEAD\n '\n \n+test_expect_success 'core.sparseCheckoutRmFiles checkout that would remove file outside sparse-checkout removes file from disk' '\n+\tgit config core.sparseCheckout false &&\n+\tgit checkout -f top &&\n+\techo added3 >added3 &&\n+\tgit add added3 &&\n+\tgit commit -madded3 &&\n+\tgit checkout top &&\n+\ttest_path_is_missing added3 &&\n+\tgit config core.sparseCheckout true &&\n+\tgit config core.sparseCheckoutRmFiles true &&\n+\techo init.t >.git/info/sparse-checkout &&\n+\tgit checkout HEAD@{1} &&\n+\ttest_path_is_missing added3 &&\n+\techo dirty >added3 &&\n+\tgit checkout top &&\n+\ttest_path_is_missing added3\n+'\n+\n+test_expect_success '!core.sparseCheckoutRmFiles checkout that would remove file outside sparse-checkout does not remove file from disk' '\n+\tgit config core.sparseCheckout false &&\n+\tgit checkout -f top &&\n+\techo added4 >added4 &&\n+\tgit add added4 &&\n+\tgit commit -madded4 &&\n+\tgit checkout top &&\n+\ttest_path_is_missing added4 &&\n+\tgit config core.sparseCheckout true &&\n+\tgit config core.sparseCheckoutRmFiles false &&\n+\techo init.t >.git/info/sparse-checkout &&\n+\tgit checkout HEAD@{1} &&\n+\ttest_path_is_missing added4 &&\n+\techo dirty >added4 &&\n+\tgit checkout top &&\n+\ttest_path_is_file added4\n+'\n+\n test_done\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 9298fe1d9b..cdc3915974 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1528,7 +1528,9 @@ static void mark_new_skip_worktree(struct pattern_list *pl,\n \n \t/*\n \t * 1. Pretend the narrowest worktree: only unmerged entries\n-\t * are checked out\n+\t * are checked out. If core.sparseCheckoutRmFiles is off, then\n+\t * treat a file being removed as merged, so it does not get\n+\t * removed from the worktree.\n \t */\n \tfor (i = 0; i < istate->cache_nr; i++) {\n \t\tstruct cache_entry *ce = istate->cache[i];\n@@ -1536,7 +1538,8 @@ static void mark_new_skip_worktree(struct pattern_list *pl,\n \t\tif (select_flag && !(ce->ce_flags & select_flag))\n \t\t\tcontinue;\n \n-\t\tif (!ce_stage(ce) && !(ce->ce_flags & CE_CONFLICTED))\n+\t\tif ((!ce_stage(ce) && !(ce->ce_flags & CE_CONFLICTED)) ||\n+\t\t    ((ce->ce_flags & CE_REMOVE) && !core_sparse_checkout_rm_files))\n \t\t\tce->ce_flags |= skip_wt_flag;\n \t\telse\n \t\t\tce->ce_flags &= ~skip_wt_flag;\n@@ -1681,12 +1684,15 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \n \tif (!o->skip_sparse_checkout) {\n \t\t/*\n-\t\t * Sparse checkout loop #2: set NEW_SKIP_WORKTREE on entries not in loop #1\n+\t\t * Sparse checkout loop #2: set NEW_SKIP_WORKTREE on entries not in loop #1.\n+\t\t * If !core.sparseCheckoutRmFiles, include files being removed so ones\n+\t\t * outside sparse-checkout patterns do not get removed from the worktree.\n \t\t * If they will have NEW_SKIP_WORKTREE, also set CE_SKIP_WORKTREE\n \t\t * so apply_sparse_checkout() won't attempt to remove it from worktree\n \t\t */\n+\t\tint mask = core_sparse_checkout_rm_files ? CE_ADDED : CE_ADDED | CE_REMOVE;\n \t\tmark_new_skip_worktree(o->pl, &o->result,\n-\t\t\t\t       CE_ADDED, CE_SKIP_WORKTREE | CE_NEW_SKIP_WORKTREE,\n+\t\t\t\t       mask, CE_SKIP_WORKTREE | CE_NEW_SKIP_WORKTREE,\n \t\t\t\t       o->verbose_update);\n \n \t\tret = 0;\n-- \n2.32.0.rc2.3.g151f456769\n\n"},{"id":"426188","messageId":"28a211cc-f210-8ce8-e7c2-90e02ff449fd@gmail.com","threadId":"55795","inReplyTo":"F58C0A40-A1B4-4B47-A8EA-20FEAB011B0D@botech.co.uk","subject":"Re: bug report: git checkout deletes worktree file even though it is excluded by sparse-checkout","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-06-02T01:37:42Z","receivedAt":"2021-06-02T01:37:51Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/28/2021 4:14 PM, Tim Renouf (open source) wrote:\n> Hi all\n> \n> I have a bug report: git checkout deletes a worktree file even though it is excluded by sparse-checkout, even if it is dirty.\n\n...\n\n> I set up sparse-checkout to include only file1, not file2. file2 is now not in the worktree, even though it is in the commit I am checked out at. Then I create file2 with arbitrary content. Then a git checkout switching to the commit where file2 is removed also deletes it from the worktree.\n>\n> I assert that file2 should be left untouched by that checkout, because it is excluded by sparse-checkout. I guess file2 had its skip-worktree bit set before the checkout that removed it from the index; that should stop it being deleted in the worktree.\n\nIn this case, 'file2' exists in the index at 'master' and\n'git checkout master' made the working directory appear as\nif it was at that position with the sparse-checkout\npatterns applied.\n\nThe confusion happens that Git behaves differently when a\nfile exists outside the sparse-checkout patterns based on\nwhether the file actually exists in the index or not. Only\nan exact path match with a sparse index entry will cause\nthis deletion.\n\nThis is another reason why I prefer the \"cone mode\" patterns:\nit is harder to get this kind of problem. You could easily\nmake it happen with \"folder2/file\" where \"folder2\" is not in\nthe cone mode sparse-checkout, but it's less likely in some\nway.\n \n> To be clear, I expect that last “ls” to still show “file1  file2”.\n\nI can understand this expectation. I don't think that if\nwe changed this behavior that it would cause any confusion.\nThat is, as long as the file disappears when no longer dirty,\n_and_ a command such as \"git reset --hard\" overwrites the\nfile. It is important to provide ways for users to remove\nthe file when they want it gone.\n\nThanks,\n-Stolee\n"},{"id":"426189","messageId":"f6d39636-308c-c846-55b5-3f16a155e69d@gmail.com","threadId":"55795","inReplyTo":"20210601183106.3084008-1-tpr.ll@botech.co.uk","subject":"Re: [PATCH] unpack-trees: add core.sparseCheckoutRmFiles config","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-06-02T02:00:50Z","receivedAt":"2021-06-02T02:00:55Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/1/2021 2:31 PM, Tim Renouf wrote:\n> When doing a checkout (or other index merge from a tree) causes the\n> removal of a path that is outside sparse-checkout, the file is removed\n> from the working tree, even if it is dirty.\n> \n> That is probably what you want if the file got there by being\n> materialized in a merge conflict. But it is not what you want if you\n> deliberately put the file there.\n> \n> This commit adds the above config item, defaulting to \"true\" to get the\n> old behavior. Set it to \"false\" to avoid removing such a file from the\n> worktree.\n\nI don't think config is necessary here. This behavior is niche\nenough to create a behavior-breaking change. However, we do want\nto ensure that the full flow of eventually clearing the file when\nclean is handled.\n\n> diff --git a/Documentation/git-sparse-checkout.txt b/Documentation/git-sparse-checkout.txt\n> index a0eeaeb02e..31adb38b1d 100644\n> --- a/Documentation/git-sparse-checkout.txt\n> +++ b/Documentation/git-sparse-checkout.txt\n> @@ -111,6 +111,17 @@ the sparse-checkout file.\n>  To repopulate the working directory with all files, use the\n>  `git sparse-checkout disable` command.\n>  \n> +The `core.sparseCheckoutRmFiles` config setting\n\nIf we _are_ going to go with a config option, then I'm not a big\nfan of this name. I've also been thinking that the sparse-checkout\nconfig has been squatting in the \"core.*\" space too long. Perhaps\nit is time to create its own section?\n\nWhat about something like sparseCheckout.keepDirtyFiles, which\ndefaults to false?\n\nAlternatively, we could add things to the index.* space. We\nalready have \"index.sparse\" for the sparse index feature. For this\ntopic, a config option \"index.keepDirtySparseFiles\" would have a\nsimilar meaning to my other suggestion.\n\nAt the least, you would need to update the appropriate file in\nDocumentation/config/*.txt.\n\n>  extern int core_apply_sparse_checkout;\n>  extern int core_sparse_checkout_cone;\n> +extern int core_sparse_checkout_rm_files;\n\nThese previous variables being global is unfortunate and it\nmight be time to fix that. At minimum, I think this new\noption might be able to inject somewhere else without\nrunning a lot of git_config_get_value() calls in a loop.\n\nSince your changes are within unpack-trees.c, maybe adding\na flag to 'struct unpack_trees_options' would suffice. It\ncould be initialized in unpack_trees() so only happen once\nper index update.\n\n> +test_expect_success 'core.sparseCheckoutRmFiles checkout that would remove file outside sparse-checkout removes file from disk' '\n\nThis test name is too long. Perhaps\n\n\t'sparse-checkout removes dirty non-matching file'\n\n> +\tgit config core.sparseCheckout false &&\n> +\tgit checkout -f top &&\n> +\techo added3 >added3 &&\n> +\tgit add added3 &&\n> +\tgit commit -madded3 &&\n> +\tgit checkout top &&\n> +\ttest_path_is_missing added3 &&\n> +\tgit config core.sparseCheckout true &&\n> +\tgit config core.sparseCheckoutRmFiles true &&\n> +\techo init.t >.git/info/sparse-checkout &&\n\nPerhaps we could use a more modern approach, such as\n\n\tgit sparse-checkout init &&\n\tgit sparse-checkout set init.t &&\n\n(and use 'git sparse-checkout disable' at the start of the\ntest.)\n\n> +\tgit checkout HEAD@{1} &&\n\nI'd typically expect 'git checkout HEAD~1' instead of\nusing the reflog, since it is more robust to changing\nthe test in the future. Better even to create a new\nbranch earlier and then switch between named branches.\n\n> +\ttest_path_is_missing added3 &&\n> +\techo dirty >added3 &&\n\nI appreciate that you confirm the file is missing before\nyou create it.\n\n> +\tgit checkout top &&\n> +\ttest_path_is_missing added3\n\nThought: does this happen also with 'git sparse-checkout\nreapply'?\n\n> +'\n> +\n> +test_expect_success '!core.sparseCheckoutRmFiles checkout that would remove file outside sparse-checkout does not remove file from disk' '\n\nand here:\n\n\t'sparse-checkout keeps dirty non-matching file'\n\nThese tests are very similar. Perhaps they could be\ngrouped and just have a check at the end that makes\n'added3' dirty and checks the behavior of 'git checkout'\nwith the two config settings?\n\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -1528,7 +1528,9 @@ static void mark_new_skip_worktree(struct pattern_list *pl,\n>  \n>  \t/*\n>  \t * 1. Pretend the narrowest worktree: only unmerged entries\n> -\t * are checked out\n> +\t * are checked out. If core.sparseCheckoutRmFiles is off, then\n> +\t * treat a file being removed as merged, so it does not get\n> +\t * removed from the worktree.\n>  \t */\n>  \tfor (i = 0; i < istate->cache_nr; i++) {\n>  \t\tstruct cache_entry *ce = istate->cache[i];\n> @@ -1536,7 +1538,8 @@ static void mark_new_skip_worktree(struct pattern_list *pl,\n>  \t\tif (select_flag && !(ce->ce_flags & select_flag))\n>  \t\t\tcontinue;\n>  \n> -\t\tif (!ce_stage(ce) && !(ce->ce_flags & CE_CONFLICTED))\n> +\t\tif ((!ce_stage(ce) && !(ce->ce_flags & CE_CONFLICTED)) ||\n> +\t\t    ((ce->ce_flags & CE_REMOVE) && !core_sparse_checkout_rm_files))\n>  \t\t\tce->ce_flags |= skip_wt_flag;\n>  \t\telse\n>  \t\t\tce->ce_flags &= ~skip_wt_flag;\n> @@ -1681,12 +1684,15 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n>  \n>  \tif (!o->skip_sparse_checkout) {\n>  \t\t/*\n> -\t\t * Sparse checkout loop #2: set NEW_SKIP_WORKTREE on entries not in loop #1\n> +\t\t * Sparse checkout loop #2: set NEW_SKIP_WORKTREE on entries not in loop #1.\n> +\t\t * If !core.sparseCheckoutRmFiles, include files being removed so ones\n> +\t\t * outside sparse-checkout patterns do not get removed from the worktree.\n>  \t\t * If they will have NEW_SKIP_WORKTREE, also set CE_SKIP_WORKTREE\n>  \t\t * so apply_sparse_checkout() won't attempt to remove it from worktree\n>  \t\t */\n> +\t\tint mask = core_sparse_checkout_rm_files ? CE_ADDED : CE_ADDED | CE_REMOVE;\n>  \t\tmark_new_skip_worktree(o->pl, &o->result,\n> -\t\t\t\t       CE_ADDED, CE_SKIP_WORKTREE | CE_NEW_SKIP_WORKTREE,\n> +\t\t\t\t       mask, CE_SKIP_WORKTREE | CE_NEW_SKIP_WORKTREE,\n>  \t\t\t\t       o->verbose_update);\n\nI'm a bit worried about this use of CE_REMOVE. I see its use listed in\ncache-tree.c with this comment:\n\n\t\t/*\n\t\t * CE_REMOVE entries are removed before the index is\n\t\t * written to disk. Skip them to remain consistent\n\t\t * with the future on-disk index.\n\t\t */\n\nThis makes me think that the entries are actually not present in the\nwritten index, which is incorrect. It will make it look like we have\nstaged a deletion of that file. Can you check that the output of\n'git status' shows the file with no staged changes, but an unstaged\n_modification_? Alternatively: the modification might be ignored since\nit is a sparse entry, but we would probably want to fix that before\ntaking this change. If my understanding is correct*, then 'git status'\nwill show it as a staged deletion and an unstaged addition.\n\n(*) This is a BIG IF.\n\nThank you for your interest here. Elijah is right that the area is\nfraught with complexity, and I'm learning something new about it\nevery day as I work on my sparse-index feature. I'm still trying\nto grasp the subtleties like this and their ramifications before\nchanging the existing behavior because I want to be _sure_ we are\nmoving in a more stable direction and not just another unstable\npoint that frustrates users.\n\nThis change seems like a focused attempt, but I think we need to\ntrack down those other details before committing to such a change:\n\n1. How does a user with a dirty, tracked, sparse file get back\n   into a state where that file is deleted? What commands do\n   they need to run? Can that be tested and added to the sparse-\n   checkout documentation?\n\n2. What does 'git status' look like when a user is in this state?\n   Is that helpful?\n\nThanks,\n-Stolee\n"},{"id":"426190","messageId":"xmqqmts9q9m2.fsf@gitster.g","threadId":"55795","inReplyTo":"f6d39636-308c-c846-55b5-3f16a155e69d@gmail.com","subject":"Re: [PATCH] unpack-trees: add core.sparseCheckoutRmFiles config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-02T02:32:53Z","receivedAt":"2021-06-02T02:32:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <stolee@gmail.com> writes:\n\n>> This commit adds the above config item, defaulting to \"true\" to get the\n>> old behavior. Set it to \"false\" to avoid removing such a file from the\n>> worktree.\n>\n> I don't think config is necessary here. This behavior is niche\n> enough to create a behavior-breaking change. However, we do want\n> to ensure that the full flow of eventually clearing the file when\n> clean is handled.\n\nI didn't have a chance to get around to commenting on the patch\n(instead I was preparing for -rc3), but you covered pretty much\neverything I wanted to say.  It is unusual for those who are using\nsparse checkout to have a modified (=tracked and dirty) file that\nshouldn't be there, and making sure the user notices these unusual\nfiles, instead of silently losing the changes to them, is probably a\n\"bugfix\".\n\nAn explicit request to destructively overwrite the path (\"git\nrestore -- path\") or remove the working tree file along with\nmodification (\"git reset --hard\") is a good thing to have, but\nthe branch switching \"git switch\" is supposed to preserve local\nmodification (or fail to switch), whether the dirty path is inside\nor outside the sparse checkout area.\n\n> If we _are_ going to go with a config option, then I'm not a big\n> fan of this name. I've also been thinking that the sparse-checkout\n> config has been squatting in the \"core.*\" space too long. Perhaps\n> it is time to create its own section?\n>\n> What about something like sparseCheckout.keepDirtyFiles, which\n> defaults to false?\n\nWhat about not having a configuration?\n\n> 1. How does a user with a dirty, tracked, sparse file get back\n>    into a state where that file is deleted? What commands do\n>    they need to run? Can that be tested and added to the sparse-\n>    checkout documentation?\n>\n> 2. What does 'git status' look like when a user is in this state?\n>    Is that helpful?\n\nGood points.\n\nThanks.\n"},{"id":"426200","messageId":"CABPp-BHs0ACvkCJMB-tO8xkiidB12NrN1hOhLRvm3U_Q=r2YcQ@mail.gmail.com","threadId":"55795","inReplyTo":"xmqqmts9q9m2.fsf@gitster.g","subject":"Re: [PATCH] unpack-trees: add core.sparseCheckoutRmFiles config","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-06-02T05:53:58Z","receivedAt":"2021-06-02T05:54:15Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Derrick and Junio already made some really good points, I'll just try\nto add a few comments.\n\nOn Tue, Jun 1, 2021 at 7:32 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Derrick Stolee <stolee@gmail.com> writes:\n>\n> >> This commit adds the above config item, defaulting to \"true\" to get the\n> >> old behavior. Set it to \"false\" to avoid removing such a file from the\n> >> worktree.\n> >\n> > I don't think config is necessary here. This behavior is niche\n> > enough to create a behavior-breaking change. However, we do want\n> > to ensure that the full flow of eventually clearing the file when\n> > clean is handled.\n>\n> I didn't have a chance to get around to commenting on the patch\n> (instead I was preparing for -rc3), but you covered pretty much\n> everything I wanted to say.  It is unusual for those who are using\n> sparse checkout to have a modified (=tracked and dirty) file that\n> shouldn't be there, and making sure the user notices these unusual\n> files, instead of silently losing the changes to them, is probably a\n> \"bugfix\".\n>\n> An explicit request to destructively overwrite the path (\"git\n> restore -- path\") or remove the working tree file along with\n> modification (\"git reset --hard\") is a good thing to have, but\n> the branch switching \"git switch\" is supposed to preserve local\n> modification (or fail to switch), whether the dirty path is inside\n> or outside the sparse checkout area.\n\nYes, I think this is the best phrasing of the bug.  Since\nSKIP_WORKTREE was originally implemented as \"assume file contents\nmatch HEAD\" (which is sometimes a reasonable first approximation to\nthe intended behavior, but which keeps leading us astray into bad\nbehavior), it's pretty easy to see what's likely happening here:\nunpack-trees.c is almost certainly just assuming that the file\ncontents match HEAD and not even bothering to verify that assumption\nbefore deleting the file.  We should make it verify when a file is\nSKIP_WORKTREE.\n\n> > If we _are_ going to go with a config option, then I'm not a big\n> > fan of this name. I've also been thinking that the sparse-checkout\n> > config has been squatting in the \"core.*\" space too long. Perhaps\n> > it is time to create its own section?\n> >\n> > What about something like sparseCheckout.keepDirtyFiles, which\n> > defaults to false?\n>\n> What about not having a configuration?\n\nI agree; we all concur that the reported behavior is a bug; adding a\nconfig option to turn a bug off for some people just doesn't make any\nsense.  Let's fix the bug instead.\n\nI'm also worried that making a config option may have masked subtle\nbugs in the patch that the rest of the testsuite would have turned up.\n\n> > 1. How does a user with a dirty, tracked, sparse file get back\n> >    into a state where that file is deleted? What commands do\n> >    they need to run? Can that be tested and added to the sparse-\n> >    checkout documentation?\n\nYeah, I've wondered if 'git sparse-checkout reapply' should do\nsomething special here...or whether additional subcommands might be\nneeded.  I'm still not quite sure.\n\n> > 2. What does 'git status' look like when a user is in this state?\n> >    Is that helpful?\n\nI'm worried that trying to make 'git status' report these files would\nbe expensive and defeat some of the advantages of a sparse-checkout.\nEven in cone mode, and even if we could just stat the leading\ndirectories to see they aren't there, would we still end up iterating\nover all the index entries just to verify that their leading directory\nis missing so we don't have to stat it?\n\nWhereas checkout, if it might end up deleting something, it makes\nperfect sense to pay the cost of a stat at that point and throw an\nerror message and abort if things aren't clean.\n"},{"id":"426201","messageId":"9BCB8981-09D5-4BF4-981B-2BF0AA0D6E5A@botech.co.uk","threadId":"55795","inReplyTo":"CABPp-BHs0ACvkCJMB-tO8xkiidB12NrN1hOhLRvm3U_Q=r2YcQ@mail.gmail.com","subject":"Re: [PATCH] unpack-trees: add core.sparseCheckoutRmFiles config","fromName":"Tim Renouf (open source)","fromEmail":"tpr.ll@botech.co.uk","sentAt":"2021-06-02T06:13:57Z","receivedAt":"2021-06-02T06:14:05Z","isPatch":true,"sender":{"key":"tpr.ll@botech.co.uk","avatar":null},"body":"Hi all\n\nThanks for the reviews and comments.\n\nMy use case is that I never want it to remove or otherwise touch those files outside of sparse-checkout that happen to be the same path as index paths. I realize that currently gives me complications (e.g. I must never try and merge/cherry-pick/rebase a commit that might cause a merge conflict out there), and I realize that’s not what everyone else wants. For example, I don’t want git reset --hard or whatever to remove those files. Hence the config option.\n\nAm I right in saying that the sparse-index work makes it easier to achieve my use case? In that those outside-sparse-checkout paths would not ever get merged into the index, even if I merge/cherry-pick/rebase a tree with paths there?\n\nI can go into more details on how I arrived ay my use case if it helps.\n\nSo maybe there are two separate things here:\n\n1. The bug that checkout removes my file when it is dirty, instead of refusing (unless -f) or just ignoring it.\n2. My use case, which is to do its best to never remove or otherwise touch worktree files outside sparse-checkout.\n\n> I'm also worried that making a config option may have masked subtle\n> bugs in the patch that the rest of the testsuite would have turned up.\n\nIt is true that not hiding it in a config option makes a few tests fail, including ones that specifically test that a git reset after a materialization from a merge conflict causes the file to be removed.\n\nThanks.\n\n-tpr\n\n"},{"id":"426273","messageId":"CABPp-BHT3ZrGrDVBcSTuzhSmy0EdtGUeHF_RAVohw=nemGhoOA@mail.gmail.com","threadId":"55795","inReplyTo":"9BCB8981-09D5-4BF4-981B-2BF0AA0D6E5A@botech.co.uk","subject":"Re: [PATCH] unpack-trees: add core.sparseCheckoutRmFiles config","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-06-02T23:41:58Z","receivedAt":"2021-06-02T23:45:30Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi,\n\nOn Tue, Jun 1, 2021 at 11:14 PM Tim Renouf (open source)\n<tpr.ll@botech.co.uk> wrote:\n>\n> Hi all\n>\n> Thanks for the reviews and comments.\n>\n> My use case is that I never want it to remove or otherwise touch those files outside of sparse-checkout that happen to be the same path as index paths. I realize that currently gives me complications (e.g. I must never try and merge/cherry-pick/rebase a commit that might cause a merge conflict out there), and I realize that’s not what everyone else wants. For example, I don’t want git reset --hard or whatever to remove those files. Hence the config option.\n\nEvery case I've ever seen of present-despite-skip-worktree files is an\naccidental and erroneous condition.  If you're trying to use it for\nsomething else, we'd really need to understand the higher level\nuse-cases so that we can make ranges of commands work together nicely.\nThe sparse checkout capability itself was started by a low-level\nimplementational detail (\"treat paths as though they matched HEAD and\ndon't write them to the working tree\") and led to a maze of surprises,\nbugs, edge cases, etc.  I'd rather not compound that problem by adding\nanother configuration option defined via a similar low-level\nimplementational detail.\n\nI'm also leaning towards having `git reset --hard` not clear\npresent-despite-skip-worktree files, and not having `git status`\nreport them; both seem like an unnecessary performance issue for this\nrare accidental/erroneous condition.  I totally agree that `git\nswitch/checkout` should not delete or overwrite such files if they\ndiffer from HEAD; but similar to how having a dirty file prevents a\nbranch switch to some branch that deletes the file, a\npresent-despite-skip-worktree file that differs from HEAD should\nresult in an error message and an aborted switch/checkout.  At least\nfor the standard sparse-checkout behavior; don't know what your\nusecase is.\n\n> Am I right in saying that the sparse-index work makes it easier to achieve my use case? In that those outside-sparse-checkout paths would not ever get merged into the index, even if I merge/cherry-pick/rebase a tree with paths there?\n\nIf you merge/cherry-pick/rebase a commit with changes to a path\noutside the sparse-checkout, and it merges cleanly with your current\nbranch, then with sparse-index it wouldn't even show up in the index\n(though one of its parent trees would be modified because of the\nchanges to that file).  But that's not very different than without the\nsparse-index, at least with the ort merge backend (available and ready\nfor using starting with git-2.32).  The recursive merge strategy (the\ndefault) is known to write files to the working tree despite the\nsparse-checkout requests, even when the merge is clean, but that's\njust a bug in the recursive backend.  (One that isn't easily fixable\nwithin its design, which is one of many reasons it was being written\nin the form of the ort backend.)\n\nIf you merge/cherry-pick/rebase a commit with changes to a path\noutside the sparse-checkout, and it conflicts with your branch, then\nwith or without sparse-index, that file is going to show up in the\nindex with higher order stages and be written to the working tree.\nThat is, so long as you don't have a file in the way.  Since the ort\nbackend makes use of the checkout machinery to switch from the current\nstate to the new state, fixing checkout to throw an error and abort\nwhen a present-despite-skip-worktree file is present would cause\nmerges/rebases/cherry-picks to do the same.\n\n> I can go into more details on how I arrived ay my use case if it helps.\n>\n> So maybe there are two separate things here:\n>\n> 1. The bug that checkout removes my file when it is dirty, instead of refusing (unless -f) or just ignoring it.\n\nYep, we need to fix this.\n\n> 2. My use case, which is to do its best to never remove or otherwise touch worktree files outside sparse-checkout.\n\nCan you expand on this use case a bit?  Should git diff or git status\nreport on your changes for your usecase?  Should \"git restore\" ignore\nthe given paths and not overwrite it?  What if the user explicitly\nlisted the path in question?  Should stash save these changes or\nignore them?  Should add include these changes or ignore them?  Since\nyour indexed file is different than the worktree file, should \"git mv\"\nmove just the file recorded in the index, just the one in the\nworktree, or both?  If someone tries to run \"git archive\", should your\nmodified files be included?  If you don't like anything touching these\npaths, does that mean they are also uninteresting?  So for example,\nshould \"git log -p\" or \"git grep $REVISION\" only return results inside\nthe sparse-checkout list of paths?\n\nFrom a higher level, what are you trying to achieve?  Is it similar to\n`git update-index --assume-unchanged`?  The point of sparse-checkout\nwas to reduce the number of files in the working tree, but it appears\nyou aren't trying to do that; you are putting files back into the\nworking tree anyway.  So, what's the point of sparse-checkout then?\n\nIt's possible sparse-checkout doesn't meet your needs, and just isn't\ndesigned to meet them, and we need another special concept for your\ncase.  Or perhaps there's a config option that's meaningful.  Or maybe\nyou're just struggling with the bugs of sparse-checkout, of which\nthere are *many*.  See e.g.\nhttps://lore.kernel.org/git/CABPp-BGJ_Nvi5TmgriD9Bh6eNXE2EDq2f8e8QKXAeYG3BxZafA@mail.gmail.com/\n\n> > I'm also worried that making a config option may have masked subtle\n> > bugs in the patch that the rest of the testsuite would have turned up.\n>\n> It is true that not hiding it in a config option makes a few tests fail, including ones that specifically test that a git reset after a materialization from a merge conflict causes the file to be removed.\n\nYeah, not so surprising that it has weird interactions with merging\n(and perhaps different weird interactions with different merge\nbackends).  Anything that touches unpack-trees.c either needs to be\npart of standard operation, or if it's behind a special config option,\nthen it needs to be accompanied with a huge number of tests from many\ndifferent commands since it affects so many things.  We're trying to\nadd a battery of tests for sparse-checkout and sparse-index, and we\nstill obviously need a lot more.\n\nHope that helps,\nElijah\n"},{"id":"426495","messageId":"66290CC2-CC83-491F-84BD-3D9107B11EFB@botech.co.uk","threadId":"55795","inReplyTo":"CABPp-BHT3ZrGrDVBcSTuzhSmy0EdtGUeHF_RAVohw=nemGhoOA@mail.gmail.com","subject":"Re: [PATCH] unpack-trees: add core.sparseCheckoutRmFiles config","fromName":"Tim Renouf (open source)","fromEmail":"tpr.ll@botech.co.uk","sentAt":"2021-06-05T15:33:08Z","receivedAt":"2021-06-05T15:33:18Z","isPatch":true,"sender":{"key":"tpr.ll@botech.co.uk","avatar":null},"body":"Hi all\n\n> Can you expand on this use case a bit? \n\nFundamentally, I wanted sparse-checkout to cause all git commands to completely ignore all files outside sparse-checkout patterns, even if they happen to correspond to paths in the index. I realize that that is difficult to achieve, but I was hoping that my patch would make it close enough to be workable. In particular, I would need to avoid doing a merge (cherry-pick, rebase) with conflicts outside the sparse-checkout patterns.\n\nHere is the (maybe slightly specialized) use case:\n\nDue to decisions out of my hands, I have a monorepo containing some shared code and some separate components in subdirectories. There is a “main” branch where the code is built and delivered from. Each component is developed separately and has its own dev branch, e.g. “dev-component1”, “dev-component2”. Many developers develop on just one component, so they check out its dev branch. The “dev-component1” branch still contains the “main” version of the shared code and the other components, so you can merge it up to the main branch.\n\nBut some people want to be able to develop on two components at the same time. So they want to check out component1 on its dev-component1 branch, and component2 on its dev-component2 branch. They would ideally want to do this at the same time as maintaining the overall monorepo directory layout. The monorepo does not give a good way to do that.\n\nI was experimenting with an approach combining sparse-checkout, worktrees, and the core.worktree config item, as follows:\n\nThe monorepo has its main worktree at the root of the overall workspace, with sparse-checkout set to check out just the shared code not considered part of any separate component. Each component has its own worktree in its subdirectory, with sparse-checkout set to only check out that component. To get the monorepo-like directory structure, a component’s worktree has its core.worktree config item set to point back up to the root of the overall workspace.\n\nSo, with that somewhat hairy structure, anything that touches a file outside of the worktree’s sparse-checkout actually touches a file in a different worktree, giving some surprising results.\n\nEven with my patch, there is still a risk of that happening. And having a component’s worktree rooted somewhere other than where you think it is rooted gave some other surprising results.\n\nI think I am going to abandon that approach and go a different way.\n\nThanks for the attention.\n\n-tpr\n"}]}