{"thread":{"id":"46729","subject":"[PATCH 1/1] reset: fix reset when using the sparse-checkout feature.","startedAt":"2017-09-08T18:01:45Z","lastAt":"2017-09-15T21:33:37Z","messageCount":23,"participants":["Kevin Willford","Torsten Bögershausen","Junio C Hamano","Johannes Schindelin","Jacob Keller"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"327787","messageId":"20170908180050.25188-2-kewillf@microsoft.com","threadId":"46729","inReplyTo":"20170908180050.25188-1-kewillf@microsoft.com","subject":"[PATCH 1/1] reset: fix reset when using the sparse-checkout feature.","fromName":"Kevin Willford","fromEmail":"kewillf@microsoft.com","sentAt":"2017-09-08T18:00:50Z","receivedAt":"2017-09-08T18:01:45Z","isPatch":true,"sender":{"key":"kewillf@microsoft.com","avatar":null},"body":"When using the sparse checkout feature the git reset command will add\nentries to the index that will have the skip-worktree bit off but will\nleave the working directory empty. File data is lost because the index\nversion of the files have been changed but there is nothing that is in the\nworking directory. This will cause the next status call to show either\ndeleted for files modified or deleting or nothing for files added. The\nadded files should be shown as untracked and modified files should be\nshown as modified.\n\nTo fix this when the reset is running if there is not a file in the working\ndirectory and if it will be missing with the new index entry or was not\nmissing in the previous version, we create the previous index version of\nthe file in the working directory so that status will report correctly\nand the files will be availble for the user to deal with.\n\nSigned-off-by: Kevin Willford \n---\n builtin/reset.c | 25 +++++++++++++++++\n t/t7114-reset-sparse-checkout.sh | 60 ++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 85 insertions(+)\n create mode 100755 t/t7114-reset-sparse-checkout.sh\n\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex d72c7d1c96..1b8bb45989 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -24,6 +24,7 @@\n #include \"cache-tree.h\"\n #include \"submodule.h\"\n #include \"submodule-config.h\"\n+#include \"dir.h\"\n \n static const char * const git_reset_usage[] = {\n \tN_(\"git reset [--mixed | --soft | --hard | --merge | --keep] [-q] []\"),\n@@ -124,8 +125,32 @@ static void update_index_from_diff(struct diff_queue_struct *q,\n \n \tfor (i = 0; i < q->nr; i++) {\n \t\tstruct diff_filespec *one = q->queue[i]->one;\n+\t\tstruct diff_filespec *two = q->queue[i]->two;\n+\t\tint pos;\n \t\tint is_missing = !(one->mode && !is_null_oid(&one->oid));\n+\t\tint was_missing = !two->mode && is_null_oid(&two->oid);\n \t\tstruct cache_entry *ce;\n+\t\tstruct cache_entry *ceBefore;\n+\t\tstruct checkout state = CHECKOUT_INIT;\n+\n+\t\tif (core_apply_sparse_checkout && !file_exists(two->path)) {\n+\t\t\tpos = cache_name_pos(two->path, strlen(two->path));\n+\t\t\tif ((pos >= 0 && ce_skip_worktree(active_cache[pos])) &&\n+\t\t\t (is_missing || !was_missing))\n+\t\t\t{\n+\t\t\t\tstate.force = 1;\n+\t\t\t\tstate.refresh_cache = 1;\n+\t\t\t\tstate.istate = &the_index;\n+\t\t\t\tceBefore = make_cache_entry(two->mode,\n+\t\t\t\t\t\t\t two->oid.hash,\n+\t\t\t\t\t\t\t two->path, 0, 0);\n+\t\t\t\tif (!ceBefore)\n+\t\t\t\t\tdie(_(\"make_cache_entry failed for path '%s'\"),\n+\t\t\t\t\t\ttwo->path);\n+\n+\t\t\t\tcheckout_entry(ceBefore, &state, NULL);\n+\t\t\t}\n+\t\t}\n \n \t\tif (is_missing && !intent_to_add) {\n \t\t\tremove_file_from_cache(one->path);\ndiff --git a/t/t7114-reset-sparse-checkout.sh b/t/t7114-reset-sparse-checkout.sh\nnew file mode 100755\nindex 0000000000..f2a5426847\n--- /dev/null\n+++ b/t/t7114-reset-sparse-checkout.sh\n@@ -0,0 +1,60 @@\n+#!/bin/sh\n+\n+test_description='reset when using a sparse-checkout'\n+\n+. ./test-lib.sh\n+\n+# reset using a sparse-checkout file\n+\n+test_expect_success 'setup' '\n+\ttest_tick &&\n+\techo \"checkout file\" >c &&\n+\techo \"modify file\" >m &&\n+\techo \"delete file\" >d &&\n+\tgit add . &&\n+\tgit commit -m \"initial commit\" &&\n+\techo \"added file\" >a &&\n+\techo \"modification of a file\" >m &&\n+\tgit rm d &&\n+\tgit add . &&\n+\tgit commit -m \"second commit\" &&\n+\tgit checkout -b endCommit\n+'\n+\n+test_expect_success 'reset when there is a sparse-checkout' '\n+\techo \"/c\" >.git/info/sparse-checkout &&\n+\ttest_config core.sparsecheckout true &&\n+\tgit checkout -b resetBranch &&\n+\ttest_path_is_missing m &&\n+\ttest_path_is_missing a &&\n+\ttest_path_is_missing d &&\n+\tgit reset HEAD~1 &&\n+\ttest \"checkout file\" = \"$(cat c)\" &&\n+\ttest \"modification of a file\" = \"$(cat m)\" &&\n+\ttest \"added file\" = \"$(cat a)\" &&\n+\ttest_path_is_missing d\n+'\n+\n+test_expect_success 'reset after deleting file without skip-worktree bit' '\n+\tgit checkout -f endCommit &&\n+\tgit clean -xdf &&\n+\tcat >.git/info/sparse-checkout <<-\\EOF &&\n+\t/c\n+\t/m\n+\tEOF\n+\ttest_config core.sparsecheckout true &&\n+\tgit checkout -b resetAfterDelete &&\n+\ttest_path_is_file m &&\n+\ttest_path_is_missing a &&\n+\ttest_path_is_missing d &&\n+\trm -f m &&\n+\tgit reset HEAD~1 &&\n+\ttest \"checkout file\" = \"$(cat c)\" &&\n+\ttest \"added file\" = \"$(cat a)\" &&\n+\ttest_path_is_missing m &&\n+\ttest_path_is_missing d\n+'\n+\n+\n+\n+test_done\n-- \n2.14.1.474.g0558484247.dirty\n\n"},{"id":"327788","messageId":"20170908180050.25188-1-kewillf@microsoft.com","threadId":"46729","inReplyTo":null,"subject":"[PATCH 0/1] reset: fix mixed reset when using sparse-checkout","fromName":"Kevin Willford","fromEmail":"kewillf@microsoft.com","sentAt":"2017-09-08T18:00:49Z","receivedAt":"2017-09-08T18:01:50Z","isPatch":true,"sender":{"key":"kewillf@microsoft.com","avatar":null},"body":"Original discussion is here\nhttps://public-inbox.org/git/20170407192357.948-4-kewillf@microsoft.com/\n\nWhen running a reset mixed and using the sparse-checkout the working\ndirectory needs to be updated so that there is not data loss when the\nindex is updated. This is because the index is getting updated\npotentially removing entries without changing the working directory.\nWhen using the sparse-checkout feature the entries removed might not\nbe on disk and are lost.\n\nThis patch writes the before version of the file to disk if the mixed\nreset is going to change the index of a file that had the skip-wortree\nbit so that the file contents before the reset is preserved on disk and\nstatus will reports the correct results.\n\nKevin Willford (1):\n reset: fix reset when using the sparse-checkout feature.\n\n builtin/reset.c | 25 +++++++++++++++++\n t/t7114-reset-sparse-checkout.sh | 60 ++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 85 insertions(+)\n create mode 100755 t/t7114-reset-sparse-checkout.sh\n\n-- \n2.14.1.474.g0558484247.dirty\n\n"},{"id":"327789","messageId":"20170908185443.GA15081@tor.lan","threadId":"46729","inReplyTo":"20170908180050.25188-2-kewillf@microsoft.com","subject":"Re: [PATCH 1/1] reset: fix reset when using the sparse-checkout feature.","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2017-09-08T18:54:43Z","receivedAt":"2017-09-08T18:55:06Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Fri, Sep 08, 2017 at 12:00:50PM -0600, Kevin Willford wrote:\n> When using the sparse checkout feature the git reset command will add\n> entries to the index that will have the skip-worktree bit off but will\n> leave the working directory empty. File data is lost because the index\n> version of the files have been changed but there is nothing that is in the\n> working directory. This will cause the next status call to show either\n> deleted for files modified or deleting or nothing for files added. The\n> added files should be shown as untracked and modified files should be\n> shown as modified.\n> \n> To fix this when the reset is running if there is not a file in the working\n> directory and if it will be missing with the new index entry or was not\n> missing in the previous version, we create the previous index version of\n> the file in the working directory so that status will report correctly\n> and the files will be availble for the user to deal with.\n> \n> Signed-off-by: Kevin Willford \n[]\n> diff --git a/t/t7114-reset-sparse-checkout.sh b/t/t7114-reset-sparse-checkout.sh\n> new file mode 100755\n> index 0000000000..f2a5426847\n> --- /dev/null\n> +++ b/t/t7114-reset-sparse-checkout.sh\n> @@ -0,0 +1,60 @@\n> +#!/bin/sh\n> +\n> +test_description='reset when using a sparse-checkout'\n> +\n> +. ./test-lib.sh\n> +\n> +# reset using a sparse-checkout file\n> +\n> +test_expect_success 'setup' '\n> +\ttest_tick &&\n\nDo we need a test_tick here ?\n\n> +\techo \"checkout file\" >c &&\n> +\techo \"modify file\" >m &&\n> +\techo \"delete file\" >d &&\n> +\tgit add . &&\n> +\tgit commit -m \"initial commit\" &&\n> +\techo \"added file\" >a &&\n> +\techo \"modification of a file\" >m &&\n> +\tgit rm d &&\n> +\tgit add . &&\n> +\tgit commit -m \"second commit\" &&\n> +\tgit checkout -b endCommit\n> +'\n> +\n> +test_expect_success 'reset when there is a sparse-checkout' '\n> +\techo \"/c\" >.git/info/sparse-checkout &&\n> +\ttest_config core.sparsecheckout true &&\n> +\tgit checkout -b resetBranch &&\n> +\ttest_path_is_missing m &&\n> +\ttest_path_is_missing a &&\n> +\ttest_path_is_missing d &&\n> +\tgit reset HEAD~1 &&\n> +\ttest \"checkout file\" = \"$(cat c)\" &&\n> +\ttest \"modification of a file\" = \"$(cat m)\" &&\n> +\ttest \"added file\" = \"$(cat a)\" &&\n> +\ttest_path_is_missing d\n> +'\n> +\n"},{"id":"327790","messageId":"xmqq4lsdypym.fsf@gitster.mtv.corp.google.com","threadId":"46729","inReplyTo":"20170908180050.25188-2-kewillf@microsoft.com","subject":"Re: [PATCH 1/1] reset: fix reset when using the sparse-checkout feature.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-08T19:01:37Z","receivedAt":"2017-09-08T19:01:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Willford writes:\n\n> diff --git a/builtin/reset.c b/builtin/reset.c\n> index d72c7d1c96..1b8bb45989 100644\n> --- a/builtin/reset.c\n> +++ b/builtin/reset.c\n> @@ -24,6 +24,7 @@\n> #include \"cache-tree.h\"\n> #include \"submodule.h\"\n> #include \"submodule-config.h\"\n> +#include \"dir.h\"\n> \n> static const char * const git_reset_usage[] = {\n> \tN_(\"git reset [--mixed | --soft | --hard | --merge | --keep] [-q] []\"),\n> @@ -124,8 +125,32 @@ static void update_index_from_diff(struct diff_queue_struct *q,\n> \n> \tfor (i = 0; i < q->nr; i++) {\n> \t\tstruct diff_filespec *one = q->queue[i]->one;\n> +\t\tstruct diff_filespec *two = q->queue[i]->two;\n> +\t\tint pos;\n> \t\tint is_missing = !(one->mode && !is_null_oid(&one->oid));\n> +\t\tint was_missing = !two->mode && is_null_oid(&two->oid);\n> \t\tstruct cache_entry *ce;\n> +\t\tstruct cache_entry *ceBefore;\n> +\t\tstruct checkout state = CHECKOUT_INIT;\n\nThe five new variables are only used in the new block, so it\nprobably is better to limit their scope to the \"we do something\nunusual when sparse checkout is in effect\" block as well. The scope\nfor the latter two can further be narrowed down to \"we do need to\nforce a checkout of this entry\" block.\n\nWe prefer to name our variables with underscore (e.g. ce_before)\nover camelCase (e.g. ceBefore) unless there is a compelling reason\n(e.g. a platform specific code in compat/ layer to match platform\nconvention).\n\n> +\n> +\t\tif (core_apply_sparse_checkout && !file_exists(two->path)) {\n> +\t\t\tpos = cache_name_pos(two->path, strlen(two->path));\n> +\t\t\tif ((pos >= 0 && ce_skip_worktree(active_cache[pos])) &&\n> +\t\t\t (is_missing || !was_missing))\n> +\t\t\t{\n> +\t\t\t\tstate.force = 1;\n> +\t\t\t\tstate.refresh_cache = 1;\n> +\t\t\t\tstate.istate = &the_index;\n> +\t\t\t\tceBefore = make_cache_entry(two->mode,\n> +\t\t\t\t\t\t\t two->oid.hash,\n> +\t\t\t\t\t\t\t two->path, 0, 0);\n> +\t\t\t\tif (!ceBefore)\n> +\t\t\t\t\tdie(_(\"make_cache_entry failed for path '%s'\"),\n> +\t\t\t\t\t\ttwo->path);\n> +\n> +\t\t\t\tcheckout_entry(ceBefore, &state, NULL);\n> +\t\t\t}\n> +\t\t}\n\nCan we tell between the case where the reason why the path was not\nthere in the working tree was due to the path being excluded by the\nsparse checkout and the path being removed manually by the end user?\n\nI guess ce_skip_worktree() check is sufficient; we force checkout only\nwhen the path is marked to be skipped due to \"sparse\" thing.\n\nDo we have to worry about the reverse case, in which file_exists(two->path)\nis true (i.e. the user created a file there manually) even though\nthe path is marked to be skipped due to \"sparse\" setting?\n\nOther than that, the patch looks quite cleanly done and well explained.\n\nThanks.\n\n> diff --git a/t/t7114-reset-sparse-checkout.sh b/t/t7114-reset-sparse-checkout.sh\n> new file mode 100755\n> index 0000000000..f2a5426847\n> --- /dev/null\n> +++ b/t/t7114-reset-sparse-checkout.sh\n> @@ -0,0 +1,60 @@\n> +#!/bin/sh\n> +\n> +test_description='reset when using a sparse-checkout'\n> +\n> +. ./test-lib.sh\n> +\n> +# reset using a sparse-checkout file\n> +\n> +test_expect_success 'setup' '\n> +\ttest_tick &&\n> +\techo \"checkout file\" >c &&\n> +\techo \"modify file\" >m &&\n> +\techo \"delete file\" >d &&\n> +\tgit add . &&\n> +\tgit commit -m \"initial commit\" &&\n> +\techo \"added file\" >a &&\n> +\techo \"modification of a file\" >m &&\n> +\tgit rm d &&\n> +\tgit add . &&\n> +\tgit commit -m \"second commit\" &&\n> +\tgit checkout -b endCommit\n> +'\n> +\n> +test_expect_success 'reset when there is a sparse-checkout' '\n> +\techo \"/c\" >.git/info/sparse-checkout &&\n> +\ttest_config core.sparsecheckout true &&\n> +\tgit checkout -b resetBranch &&\n> +\ttest_path_is_missing m &&\n> +\ttest_path_is_missing a &&\n> +\ttest_path_is_missing d &&\n> +\tgit reset HEAD~1 &&\n> +\ttest \"checkout file\" = \"$(cat c)\" &&\n> +\ttest \"modification of a file\" = \"$(cat m)\" &&\n> +\ttest \"added file\" = \"$(cat a)\" &&\n> +\ttest_path_is_missing d\n> +'\n> +\n> +test_expect_success 'reset after deleting file without skip-worktree bit' '\n> +\tgit checkout -f endCommit &&\n> +\tgit clean -xdf &&\n> +\tcat >.git/info/sparse-checkout <<-\\EOF &&\n> +\t/c\n> +\t/m\n> +\tEOF\n> +\ttest_config core.sparsecheckout true &&\n> +\tgit checkout -b resetAfterDelete &&\n> +\ttest_path_is_file m &&\n> +\ttest_path_is_missing a &&\n> +\ttest_path_is_missing d &&\n> +\trm -f m &&\n> +\tgit reset HEAD~1 &&\n> +\ttest \"checkout file\" = \"$(cat c)\" &&\n> +\ttest \"added file\" = \"$(cat a)\" &&\n> +\ttest_path_is_missing m &&\n> +\ttest_path_is_missing d\n> +'\n> +\n> +\n> +\n> +test_done\n"},{"id":"327791","messageId":"xmqqzia5xb8y.fsf@gitster.mtv.corp.google.com","threadId":"46729","inReplyTo":"20170908185443.GA15081@tor.lan","subject":"Re: [PATCH 1/1] reset: fix reset when using the sparse-checkout feature.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-08T19:04:45Z","receivedAt":"2017-09-08T19:04:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n>> +test_expect_success 'setup' '\n>> +\ttest_tick &&\n>\n> Do we need a test_tick here ?\n\nAs the test does not check against exact object names, and it does\nnot create commits, the order among which needs to be tiebroken by\nusing the committer timestamp, it is not strictly necessary, but I\ndo not think it would hurt, either.\n\n>\n>> +\techo \"checkout file\" >c &&\n>> +\techo \"modify file\" >m &&\n>> +\techo \"delete file\" >d &&\n>> +\tgit add . &&\n>> +\tgit commit -m \"initial commit\" &&\n>> +\techo \"added file\" >a &&\n>> +\techo \"modification of a file\" >m &&\n>> +\tgit rm d &&\n>> +\tgit add . &&\n>> +\tgit commit -m \"second commit\" &&\n>> +\tgit checkout -b endCommit\n>> +'\n>> +\n>> +test_expect_success 'reset when there is a sparse-checkout' '\n>> +\techo \"/c\" >.git/info/sparse-checkout &&\n>> +\ttest_config core.sparsecheckout true &&\n>> +\tgit checkout -b resetBranch &&\n>> +\ttest_path_is_missing m &&\n>> +\ttest_path_is_missing a &&\n>> +\ttest_path_is_missing d &&\n>> +\tgit reset HEAD~1 &&\n>> +\ttest \"checkout file\" = \"$(cat c)\" &&\n>> +\ttest \"modification of a file\" = \"$(cat m)\" &&\n>> +\ttest \"added file\" = \"$(cat a)\" &&\n>> +\ttest_path_is_missing d\n>> +'\n>> +\n"},{"id":"327792","messageId":"xmqqvaktxawk.fsf@gitster.mtv.corp.google.com","threadId":"46729","inReplyTo":"20170908180050.25188-2-kewillf@microsoft.com","subject":"Re: [PATCH 1/1] reset: fix reset when using the sparse-checkout feature.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-08T19:12:11Z","receivedAt":"2017-09-08T19:12:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Willford writes:\n\n> When using the sparse checkout feature the git reset command will add\n> entries to the index that will have the skip-worktree bit off but will\n> leave the working directory empty. File data is lost because the index\n> version of the files have been changed but there is nothing that is in the\n> working directory. This will cause the next status call to show either\n> deleted for files modified or deleting or nothing for files added. \n\nGetting rid of sparseness may of course be one way to correct the\ndiscrepancy, but it is unclear to me if that is the best way to do\nso. An alternative may be to add entries to the index that does\nhave the bit on for paths that are meant to be skipped due to\n\"sparse\" thing, no? Am I being naive, missing some reason why that\nwould give us a worse result?\n"},{"id":"327793","messageId":"SN1PR21MB0014638E5D9CBFD0D9D85F10B7950@SN1PR21MB0014.namprd21.prod.outlook.com","threadId":"46729","inReplyTo":"xmqqvaktxawk.fsf@gitster.mtv.corp.google.com","subject":"RE: [PATCH 1/1] reset: fix reset when using the sparse-checkout feature.","fromName":"Kevin Willford","fromEmail":"kewillf@microsoft.com","sentAt":"2017-09-08T20:02:33Z","receivedAt":"2017-09-08T20:02:44Z","isPatch":true,"sender":{"key":"kewillf@microsoft.com","avatar":null},"body":"\nFrom: Junio C Hamano [mailto:gitster@pobox.com]\nSent: Friday, September 8, 2017 1:12 PM\n> Kevin Willford writes:\n> \n> > When using the sparse checkout feature the git reset command will add\n> > entries to the index that will have the skip-worktree bit off but will\n> > leave the working directory empty. File data is lost because the index\n> > version of the files have been changed but there is nothing that is in the\n> > working directory. This will cause the next status call to show either\n> > deleted for files modified or deleting or nothing for files added.\n> \n> Getting rid of sparseness may of course be one way to correct the\n> discrepancy, but it is unclear to me if that is the best way to do\n> so. An alternative may be to add entries to the index that does\n> have the bit on for paths that are meant to be skipped due to\n> \"sparse\" thing, no? Am I being naive, missing some reason why that\n> would give us a worse result?\n\nThere are two cases that this is trying to solve.\n\n1. reset mixed when there were files that were added\n\nIn this case the index will no longer have the entry at all because\nthe reset is making the index look like before the file was added\nwhich didn't have it. When not using the sparse-checkout this is fine\nbecause the file is in the working directory and the reset didn't touch\nit. But using the sparse-checkout on that file and if the file was not\nin the working directory, the index gets reset and the entry for that\nfile is gone and if we don't put the index version of the file before the\nreset into the working directory, then we have lost the content for\nthat file\n\n2. reset mixed when there were files modified\n\nThis case is similar but with modified files there is an entry in the index\nbut it is getting changed to a previous version of the file. If we don't get\nthe file on disk then the version of the file that, in the non sparse-checkout\ncase, would be on disk is lost and cannot be recovered. So even if we turn\non the skip-worktree bit for this entry and it doesn't show up in status\ncalls, we lost the previous version of the file.\n\n\n"},{"id":"327795","messageId":"SN1PR21MB0014A3A5B8DFE3D353135184B7950@SN1PR21MB0014.namprd21.prod.outlook.com","threadId":"46729","inReplyTo":"xmqq4lsdypym.fsf@gitster.mtv.corp.google.com","subject":"RE: [PATCH 1/1] reset: fix reset when using the sparse-checkout feature.","fromName":"Kevin Willford","fromEmail":"kewillf@microsoft.com","sentAt":"2017-09-08T20:08:03Z","receivedAt":"2017-09-08T20:08:11Z","isPatch":true,"sender":{"key":"kewillf@microsoft.com","avatar":null},"body":"From: Junio C Hamano [mailto:gitster@pobox.com]\nSent: Friday, September 8, 2017 1:02 PM\n> Kevin Willford writes:\n> \n> > diff --git a/builtin/reset.c b/builtin/reset.c\n> > index d72c7d1c96..1b8bb45989 100644\n> > --- a/builtin/reset.c\n> > +++ b/builtin/reset.c\n> > @@ -24,6 +24,7 @@\n> > #include \"cache-tree.h\"\n> > #include \"submodule.h\"\n> > #include \"submodule-config.h\"\n> > +#include \"dir.h\"\n> >\n> > static const char * const git_reset_usage[] = {\n> > \tN_(\"git reset [--mixed | --soft | --hard | --merge | --keep] [-q]\n> []\"),\n> > @@ -124,8 +125,32 @@ static void update_index_from_diff(struct\n> diff_queue_struct *q,\n> >\n> > \tfor (i = 0; i < q->nr; i++) {\n> > \t\tstruct diff_filespec *one = q->queue[i]->one;\n> > +\t\tstruct diff_filespec *two = q->queue[i]->two;\n> > +\t\tint pos;\n> > \t\tint is_missing = !(one->mode && !is_null_oid(&one->oid));\n> > +\t\tint was_missing = !two->mode && is_null_oid(&two->oid);\n> > \t\tstruct cache_entry *ce;\n> > +\t\tstruct cache_entry *ceBefore;\n> > +\t\tstruct checkout state = CHECKOUT_INIT;\n> \n> The five new variables are only used in the new block, so it\n> probably is better to limit their scope to the \"we do something\n> unusual when sparse checkout is in effect\" block as well. The scope\n> for the latter two can further be narrowed down to \"we do need to\n> force a checkout of this entry\" block.\n> \n> We prefer to name our variables with underscore (e.g. ce_before)\n> over camelCase (e.g. ceBefore) unless there is a compelling reason\n> (e.g. a platform specific code in compat/ layer to match platform\n> convention).\n> \n\nWill update.\n\n> > +\n> > +\t\tif (core_apply_sparse_checkout && !file_exists(two->path)) {\n> > +\t\t\tpos = cache_name_pos(two->path, strlen(two->path));\n> > +\t\t\tif ((pos >= 0 && ce_skip_worktree(active_cache[pos]))\n> &&\n> > +\t\t\t (is_missing || !was_missing))\n> > +\t\t\t{\n> > +\t\t\t\tstate.force = 1;\n> > +\t\t\t\tstate.refresh_cache = 1;\n> > +\t\t\t\tstate.istate = &the_index;\n> > +\t\t\t\tceBefore = make_cache_entry(two->mode,\n> > +\t\t\t\t\t\t\t two->oid.hash,\n> > +\t\t\t\t\t\t\t two->path, 0, 0);\n> > +\t\t\t\tif (!ceBefore)\n> > +\t\t\t\t\tdie(_(\"make_cache_entry failed for path\n> '%s'\"),\n> > +\t\t\t\t\t\ttwo->path);\n> > +\n> > +\t\t\t\tcheckout_entry(ceBefore, &state, NULL);\n> > +\t\t\t}\n> > +\t\t}\n> \n> Can we tell between the case where the reason why the path was not\n> there in the working tree was due to the path being excluded by the\n> sparse checkout and the path being removed manually by the end user?\n> \n> I guess ce_skip_worktree() check is sufficient; we force checkout only\n> when the path is marked to be skipped due to \"sparse\" thing.\n> \n> Do we have to worry about the reverse case, in which file_exists(two->path)\n> is true (i.e. the user created a file there manually) even though\n> the path is marked to be skipped due to \"sparse\" setting?\n> \n\nI don't believe so because if the user has a file there whether they modified\nit or not, it is what the user did and we just leave it there and a diff with\nwhat the index gets reset to will show how the file is different from what the\nindex got reset to.\n\n> Other than that, the patch looks quite cleanly done and well explained.\n> \n> Thanks.\n> \n"},{"id":"327797","messageId":"xmqqr2vgy2yt.fsf@gitster.mtv.corp.google.com","threadId":"46729","inReplyTo":"SN1PR21MB0014638E5D9CBFD0D9D85F10B7950@SN1PR21MB0014.namprd21.prod.outlook.com","subject":"Re: [PATCH 1/1] reset: fix reset when using the sparse-checkout feature.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-09T03:18:18Z","receivedAt":"2017-09-09T03:18:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Willford writes:\n\n> 1. reset mixed when there were files that were added\n>\n> In this case the index will no longer have the entry at all because\n> the reset is making the index look like before the file was added\n> which didn't have it. When not using the sparse-checkout this is fine\n> because the file is in the working directory and the reset didn't touch\n> it. But using the sparse-checkout on that file and if the file was not\n> in the working directory, the index gets reset and the entry for that\n> file is gone and if we don't put the index version of the file before the\n> reset into the working directory, then we have lost the content for\n> that file\n\nI do not quite understand this argument. If you do\n\n\tedit $path\n\tgit add $path\n\trm $path\n\tgit reset\n\nfor a $path that is not involved in the sparse thing, the version\nthat was previously indexed will be lost, but that is fine---the\nuser said that version is expendable by saying \"reset\".\n\nHow would that be different when the $path were not to be\nmaterialized in the working tree due to sparseness? Where did that\n\"blob\" object in the index immediately before you called \"reset\"\ncame from, and why do you say that the user does *not* consider that\none expendable, unlike the case for non-sparse path example above?\n\nI suspect that a similar reasoning would apply to your 2., but I\ndidn't think it through. \n\nThe possible misconception, which I perceive in both of these, is\nthat you are somehow disagreeing with this basic assumption: by\nsaying \"git reset []\", the user is telling us that the\nversion in the index, even if that is different from HEAD,\n, or the file in the working tree, is *unwanted* and be\nreplaced with the one in HEAD (or when given). Touching\nthe working tree files upon \"git reset\" is the last thing the user\nexpects to happen.\n\n\n"},{"id":"327799","messageId":"SN1PR21MB00140C84DC02F3491F4E8469B76A0@SN1PR21MB0014.namprd21.prod.outlook.com","threadId":"46729","inReplyTo":"xmqqr2vgy2yt.fsf@gitster.mtv.corp.google.com","subject":"RE: [PATCH 1/1] reset: fix reset when using the sparse-checkout feature.","fromName":"Kevin Willford","fromEmail":"kewillf@microsoft.com","sentAt":"2017-09-09T04:54:31Z","receivedAt":"2017-09-09T04:54:40Z","isPatch":true,"sender":{"key":"kewillf@microsoft.com","avatar":null},"body":"> From: Junio C Hamano [mailto:gitster@pobox.com]\n> Sent: Friday, September 8, 2017 9:18 PM\n> \n> Kevin Willford writes:\n> \n> > 1. reset mixed when there were files that were added\n> >\n> > In this case the index will no longer have the entry at all because\n> > the reset is making the index look like before the file was added\n> > which didn't have it. When not using the sparse-checkout this is fine\n> > because the file is in the working directory and the reset didn't touch\n> > it. But using the sparse-checkout on that file and if the file was not\n> > in the working directory, the index gets reset and the entry for that\n> > file is gone and if we don't put the index version of the file before the\n> > reset into the working directory, then we have lost the content for\n> > that file\n> \n> I do not quite understand this argument. If you do\n> \n> \tedit $path\n> \tgit add $path\n> \trm $path\n> \tgit reset\n> \n> for a $path that is not involved in the sparse thing, the version\n> that was previously indexed will be lost, but that is fine---the\n> user said that version is expendable by saying \"reset\".\n> \n> How would that be different when the $path were not to be\n> materialized in the working tree due to sparseness? Where did that\n> \"blob\" object in the index immediately before you called \"reset\"\n> came from, and why do you say that the user does *not* consider that\n> one expendable, unlike the case for non-sparse path example above?\n> \n\nI guess that I should have said files that were newly added, meaning they\nare new files that were created and added in the previous commit.\n \nI think that the difference is that the user explicitly removed the file. When\nusing sparse it is git that is causing the removal of the file. For example\nif I have /file in my spare-checkout file so that I am only working on the one\nfile. The previous commit had new file2 added and I run a git reset HEAD~1.\nI as the user do not expect that file2 just disappear, but yet that is what\nhappens. So from you example above if I do.\n\n\tcreate $path\n\tgit add $path\n\tgit commit\n\tgit checkout // where $path is not in the sparse-checkout\n\tgit reset HEAD~1\n\n$path will be gone yet I the user did not remove it. I guess you could\nargue that the user did when they specified their sparse-checkout and\nran checkout but I wouldn't know what would go missing unless I ran\na diff before the reset to see. There could have been X number of\nfiles created and added in the previous commit(s) and status after the\nreset would not report them and they are gone. So I could clone,\nsetup sparse-checkout, checkout, reset HEAD~X and possibly lose\ndata I didn't expect to.\n\nIn the modified case where the previous commits have modifications\nto files outside the sparse-checkout at least the status after the reset reports\nthe file as deleted so the user sees that something has happened to it.\n\nI suppose the entry could stay in the index with the skip-worktree bit\non and not removed like it is now so that a git reset will only apply\nto the entries in the sparse-checkout? That seems like it would be\nchanging the meaning of reset.\n\n> I suspect that a similar reasoning would apply to your 2., but I\n> didn't think it through.\n> \n> The possible misconception, which I perceive in both of these, is\n> that you are somehow disagreeing with this basic assumption: by\n> saying \"git reset []\", the user is telling us that the\n> version in the index, even if that is different from HEAD,\n> , or the file in the working tree, is *unwanted* and be\n> replaced with the one in HEAD (or when given). Touching\n> the working tree files upon \"git reset\" is the last thing the user\n> expects to happen.\n> \n\nI agree with this when you are not dealing with a sparse-checkout.\nWhen using a sparse-checkout I expect git not to touch things\noutside of what I have specified in my sparse-checkout file. If it\ndoes, it should let me know or put my working directory in a\nstate that is expected. Especially when it is changing the\nskip-worktree bits causing files outside the sparse-checkout to be\nreported incorrectly by status.\n\n \n\n"},{"id":"327842","messageId":"xmqqh8w951ek.fsf@gitster.mtv.corp.google.com","threadId":"46729","inReplyTo":"SN1PR21MB00140C84DC02F3491F4E8469B76A0@SN1PR21MB0014.namprd21.prod.outlook.com","subject":"Re: [PATCH 1/1] reset: fix reset when using the sparse-checkout feature.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-11T04:01:39Z","receivedAt":"2017-09-11T04:01:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Willford writes:\n\n> I agree with this when you are not dealing with a sparse-checkout.\n> When using a sparse-checkout I expect git not to touch things\n> outside of what I have specified in my sparse-checkout file. If it\n> does, it should let me know or put my working directory in a\n> state that is expected. Especially when it is changing the\n> skip-worktree bits causing files outside the sparse-checkout to be\n> reported incorrectly by status.\n\nWell, whether using a sparse-checkout or not, I would expect Git not\nto touch *any* filesystem entity when \"git reset\" (not \"--hard\",\njust \"git reset []\") is given, whether the path is inside\nor outside the sparse-checkout area.\n\nStepping back a bit, I am not sure if it is sane or even valid for\nthe end-user to modify paths outside sparse-checkout area, but that\nis probably a separate tangent.\n\n\n"},{"id":"327848","messageId":"alpine.DEB.2.21.1.1709111259430.4132@virtualbox","threadId":"46729","inReplyTo":"xmqqh8w951ek.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/1] reset: fix reset when using the sparse-checkout feature.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-09-11T11:15:36Z","receivedAt":"2017-09-11T11:16:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 11 Sep 2017, Junio C Hamano wrote:\n\n> Stepping back a bit, I am not sure if it is sane or even valid for the\n> end-user to modify paths outside sparse-checkout area, but that is\n> probably a separate tangent.\n\nThat is not at all the scenario that Kevin fixed. Just have a quick look\nat the regression test: in a sparse checkout, the user checked out a\nbranch, then called `reset` to switch to a different commit. No file was\ntouched by the user outside the sparse checkout.\n\nYet without Kevin's fix, `git status` would report that the user *deleted\nfiles outside the sparse checkout*.\n\nThat is such an obvious bug, and Kevin's fix is such an obvious\nimprovement over the current upstream Git version, that I would think the\nonly thing worth discussing is whether the patch goes about it in a way of\nwhich you approve.\n\nFor example, you mentioned that you would want to move the declaration of\n`two` and `was_missing` into the conditional code block. That is a valid\nsuggestion for `was_missing` (but of course not for `two`, which is used\nin the condition of the code block). That suggestion is more about code\nstyle (and of course easily fixed by Kevin using Edit>Refactor>Move\nDefinition Location in VS), though, than about the correctness of the post\nimage.\n\nMuch more interesting would be a review of the conditional code block. And\nI am not talking about the camelCasing of `ceBefore` (which will be fixed\nas easily by Edit>Refactor>Rename). I am talking about the stuff where\ntools cannot help, but where your experience is necessary: is it correct\nto use make_cache_entry()/checkout_entry() in this case? Are the\nparameters passed to those functions correct? Is the call to\ncache_name_pos() followed by ce_skip_worktree() the best way to find out\nwhether the file that is absent was not actually deleted by the user, or\nis there a less CPU-intensive way, seeing as we are already guaranteed to\niterate over the queue diff in alphabetical order?\n\nI understand that those latter questions are a lot harder to answer, sorry\nabout that.\n\nCiao,\nDscho\n"},{"id":"327872","messageId":"xmqq4ls836z3.fsf@gitster.mtv.corp.google.com","threadId":"46729","inReplyTo":"alpine.DEB.2.21.1.1709111259430.4132@virtualbox","subject":"Re: [PATCH 1/1] reset: fix reset when using the sparse-checkout feature.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-12T03:56:32Z","receivedAt":"2017-09-12T03:56:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> Stepping back a bit, I am not sure if it is sane or even valid for the\n>> end-user to modify paths outside sparse-checkout area, but that is\n>> probably a separate tangent.\n>\n> That is not at all the scenario that Kevin fixed.\n\nI know, but in this tangent I was reacting to this part of Kevin's\nmessage\n\n>>\tcreate $path\n>>\tgit add $path\n>>\tgit commit\n>>\tgit checkout // where $path is not in the sparse-checkout\n>>\tgit reset HEAD~1\n\nto which I was responding to.\n\n> ...\n> Yet without Kevin's fix, `git status` would report that the user *deleted\n> files outside the sparse checkout*.\n> ...\n> That is such an obvious bug, and Kevin's fix is such an obvious\n> improvement over the current upstream Git version, that I would think the\n> only thing worth discussing is whether the patch goes about it in a way of\n> which you approve.\n\nWhen reviewing any topic, I'd ask three (or four) questions to\nmyself:\n\n * Are we solving a right problem? Is the problem addressed valid?\n * Are we solving it with a right approach?\n * Does the patch implement the approach correctly?\n\n(the fourth is s/correct/efficient/ of the third, which is optional).\n\nand any \"no\" to an earlier question will make it a moot point to ask\nfurther questions.\n\nLet's imagine a path P that is outside the sparse checkout area.\nAnd we check out a commit that has P. P would still be recorded in\nthe index but it would not materialize in the working tree. \"git\nstatus\" and friends are asked not to treat this as \"locally removed\",\nto prevent \"commit -a\" from recording a removal, of course.\n\nNow, let's further imagine that you have a checkout of the same\nproject but at a commit that does not have P. Then you reset to\nanother commit that does have P. My understanding of what Kevin's\nfirst test wants to demonstrate is that the index is populated with\nP (because you did reset to a commit with that path) but it does not\nmaterialize in the working tree (perhaps because that is outside the\nsparse checkout area?), yet there is something missing compared to\nthe earlier case where \"git status\" and friends are asked not to\ntreat P as \"locally removed\". They instead show P as locally removed,\nand \"commit -a\" would record a removal---that is indeed a problem.\n\nAm I reading the problem description correctly so far? If so, then\nmy answer to my first question (are we solving a right problem?) is\nyes.\n\nMy response to an earlier version of the patch was written while\nassuming (read: without thinking deeply but trusting that the\ncontributor thought things through) that the first two answers were\n\"yes\". If the right approach were to check out what is in the index\nto silence \"git status\" and friends by matching the index and the\nworking tree, then the implementation in the patch (i.e. setting up\nsome cache entry and calling checkout_entry() to make it materialize\nin the working tree) looked correct, and that is what I meant by\n\"Other than that, the patch looks quite cleanly done and well\nexplained.\"\n\nBut this time, I am trying to see if the approach is good. I am not\nso sure if the approach taken by this patch is so obviously good as\nyou seem to think. A logical consequence of the approach \"git\nstatus thinks that P appears in the index and missing in the working\ntree is a local removal, so let's squelch it by creating the file in\nthe working tree\" is that we will end up fully populating the\nworking tree with paths that are clearly outside the area the user\ndesignated as the sparse checkout area---surely that may squelch\n\"status\", but to me, it looks like it is breaking what the user\nwanted to achieve with \"sparse checkout\" in the first place.\n\nWhen we make a sparse checkout that places P outside the checked-out\narea, with P in the index and not in the working tree, what asks\n\"git status\" and friends not to treat P as \"locally removed\"?\nShouldn't \"reset HEAD~1\" that resurrected P that was missing in the\ncommit in the state before you did the \"reset\" be doing the same\n(e.g. flipping the bit in the index for path P that says \"not having\nthis in the working tree is not a removal---it is deliberately not\nchecked out\")? If that is the right approach to solve the issue\n(which we established is a right problem to solve already), and the\napproach that the patch wants to take is quite the opposite of it,\nthen my answer to the second question (are we solving the problem\nwith the right approach?) is no. And at that point, it is moot to\nask if the code correctly and/or efficiently implements that wrong\nsolution, isn't it?\n"},{"id":"327904","messageId":"SN1PR21MB0014435A97BCDA324FD55B46B7690@SN1PR21MB0014.namprd21.prod.outlook.com","threadId":"46729","inReplyTo":"xmqq4ls836z3.fsf@gitster.mtv.corp.google.com","subject":"RE: [PATCH 1/1] reset: fix reset when using the sparse-checkout feature.","fromName":"Kevin Willford","fromEmail":"kewillf@microsoft.com","sentAt":"2017-09-12T20:20:06Z","receivedAt":"2017-09-12T20:20:16Z","isPatch":true,"sender":{"key":"kewillf@microsoft.com","avatar":null},"body":"> From: Junio C Hamano [mailto:gitster@pobox.com]\n> Sent: Monday, September 11, 2017 9:57 PM\n> \n> Let's imagine a path P that is outside the sparse checkout area.\n> And we check out a commit that has P. P would still be recorded in\n> the index but it would not materialize in the working tree. \"git\n> status\" and friends are asked not to treat this as \"locally removed\",\n> to prevent \"commit -a\" from recording a removal, of course.\n> \n> Now, let's further imagine that you have a checkout of the same\n> project but at a commit that does not have P. Then you reset to\n> another commit that does have P. My understanding of what Kevin's\n> first test wants to demonstrate is that the index is populated with\n> P (because you did reset to a commit with that path) but it does not\n> materialize in the working tree (perhaps because that is outside the\n> sparse checkout area?), yet there is something missing compared to\n> the earlier case where \"git status\" and friends are asked not to\n> treat P as \"locally removed\". They instead show P as locally removed,\n> and \"commit -a\" would record a removal---that is indeed a problem.\n> \n> Am I reading the problem description correctly so far? If so, then\n> my answer to my first question (are we solving a right problem?) is\n> yes.\n> \n\nI think this is where I need to do a better job of explaining so here is a\nsimple example.\n\nI have a file \"a\" that was added in the latest commit. \n$ git log --oneline\nc1fa646 (HEAD -> reset, master) add file a\n40b342c Initial commit with file i\n\nRunning the reset without using a sparse-checkout file\n\n$ git reset HEAD~1\n$ git status\nOn branch reset\nUntracked files:\n (use \"git add ...\" to include in what will be committed)\n\n a\n\nnothing added to commit but untracked files present (use \"git add\" to track)\n\nTurning on sparse-checkout and running checkout to make my working\ndirectory sparse\n\n$ git config core.sparsecheckout true\n$ echo /i > .git/info/sparse-checkout\n$ git checkout -f\n\nRunning reset gives me \n$ git reset HEAD~1\n$ git status\nOn branch reset\nnothing to commit, working tree clean\n$ git ls-files\ni\n\nfile a is gone. Not in the index and not in the working directory.\nNothing to let the user know that anything changed.\n\nWith a modified file no sparse-checkout\n$ git log --oneline\n6fbd34a (HEAD -> reset, modified) modified file m\nc734d72 Initial commit with file i and m\n$ git reset HEAD~1\nUnstaged changes after reset:\nM m\n$ git status\nOn branch reset\nChanges not staged for commit:\n (use \"git add ...\" to update what will be committed)\n (use \"git checkout -- ...\" to discard changes in working directory)\n\n modified: m\n\nno changes added to commit (use \"git add\" and/or \"git commit -a\")\n\nWith sparse-checkout\n$ git reset HEAD~1\nUnstaged changes after reset:\nD m\n$ git status\nOn branch reset\nChanges not staged for commit:\n (use \"git add/rm ...\" to update what will be committed)\n (use \"git checkout -- ...\" to discard changes in working directory)\n\n deleted: m\n\nno changes added to commit (use \"git add\" and/or \"git commit -a\")\n\nI think we can agree that this is not the correct behavior.\n\n> But this time, I am trying to see if the approach is good. I am not\n> so sure if the approach taken by this patch is so obviously good as\n> you seem to think. A logical consequence of the approach \"git\n> status thinks that P appears in the index and missing in the working\n> tree is a local removal, so let's squelch it by creating the file in\n> the working tree\" is that we will end up fully populating the\n> working tree with paths that are clearly outside the area the user\n> designated as the sparse checkout area---surely that may squelch\n> \"status\", but to me, it looks like it is breaking what the user\n> wanted to achieve with \"sparse checkout\" in the first place.\n> \n\nI don't think that we are trying to \"squelch\" status so much as make\nit consistent with what the user would expect to happen. If that means\nnot resetting entries with the skip-worktree bit or resetting the entries\nbut keeping the skip-worktree bit on, okay, but I would argue that is\nnot what the user wants because if you are now saying that sparse\nmeans git will not change files outside the sparse-checkout entries,\nwhat about merge, rebase, cherry-pick, apply? Should those only\nchange the files that are in the sparse definition? If so we would\nbe changing the commits from the original, i.e. cherry-pick 123 would\ncreate a different commit depending on whether or not you are using\nsparse as well as a different commit depending on what is in your\nsparse-checkout. \n\nI see reset being a similar scenario in that if everything is clean, after I\n\"reset HEAD~1\" I should be able to run \"add .\" + \"commit\" and have\nthe same commit as before the reset. If this is changed to only reset\nthe sparse entries, there will be staged changes after the reset because\nHEAD has changed but we didn't update the index versions of the files.\nIf we do update the index with the \"HEAD~1\" version of the files and just\nset the skip-worktree bit then the next commit will not have the original\nchanges outside the sparse-checkout for the commit.\n\n> When we make a sparse checkout that places P outside the checked-out\n> area, with P in the index and not in the working tree, what asks\n> \"git status\" and friends not to treat P as \"locally removed\"?\n> Shouldn't \"reset HEAD~1\" that resurrected P that was missing in the\n> commit in the state before you did the \"reset\" be doing the same\n> (e.g. flipping the bit in the index for path P that says \"not having\n> this in the working tree is not a removal---it is deliberately not\n> checked out\")? If that is the right approach to solve the issue\n> (which we established is a right problem to solve already), and the\n> approach that the patch wants to take is quite the opposite of it,\n> then my answer to the second question (are we solving the problem\n> with the right approach?) is no. And at that point, it is moot to\n> ask if the code correctly and/or efficiently implements that wrong\n> solution, isn't it?\n\nI agree that we must determine if there is a problem, which I hope\nwe can agree that there is a valid problem to be addressed. Determining\nif I used the right approach depends on how you see reset working\ncorrectly using the sparse feature. Should it match a reset when not\nusing sparse or should it reset only sparse files? I saw issues\nif only the sparse files are the ones reset, which is the reason that\nI went with this approach.\n\nThanks,\nKevin \n"},{"id":"327905","messageId":"CA+P7+xqxmxexWS=MWNd9=EqG81uhKY-OdG+1mpyWhst6DvH5AA@mail.gmail.com","threadId":"46729","inReplyTo":"SN1PR21MB0014435A97BCDA324FD55B46B7690@SN1PR21MB0014.namprd21.prod.outlook.com","subject":"Re: [PATCH 1/1] reset: fix reset when using the sparse-checkout feature.","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-09-12T22:29:27Z","receivedAt":"2017-09-12T22:29:54Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Tue, Sep 12, 2017 at 1:20 PM, Kevin Willford wrote:\n>> From: Junio C Hamano [mailto:gitster@pobox.com]\n>> Sent: Monday, September 11, 2017 9:57 PM\n>>\n>> Let's imagine a path P that is outside the sparse checkout area.\n>> And we check out a commit that has P. P would still be recorded in\n>> the index but it would not materialize in the working tree. \"git\n>> status\" and friends are asked not to treat this as \"locally removed\",\n>> to prevent \"commit -a\" from recording a removal, of course.\n>>\n>> Now, let's further imagine that you have a checkout of the same\n>> project but at a commit that does not have P. Then you reset to\n>> another commit that does have P. My understanding of what Kevin's\n>> first test wants to demonstrate is that the index is populated with\n>> P (because you did reset to a commit with that path) but it does not\n>> materialize in the working tree (perhaps because that is outside the\n>> sparse checkout area?), yet there is something missing compared to\n>> the earlier case where \"git status\" and friends are asked not to\n>> treat P as \"locally removed\". They instead show P as locally removed,\n>> and \"commit -a\" would record a removal---that is indeed a problem.\n>>\n>> Am I reading the problem description correctly so far? If so, then\n>> my answer to my first question (are we solving a right problem?) is\n>> yes.\n>>\n>\n> I think this is where I need to do a better job of explaining so here is a\n> simple example.\n>\n> I have a file \"a\" that was added in the latest commit.\n> $ git log --oneline\n> c1fa646 (HEAD -> reset, master) add file a\n> 40b342c Initial commit with file i\n>\n> Running the reset without using a sparse-checkout file\n>\n> $ git reset HEAD~1\n> $ git status\n> On branch reset\n> Untracked files:\n> (use \"git add ...\" to include in what will be committed)\n>\n> a\n>\n> nothing added to commit but untracked files present (use \"git add\" to track)\n>\n> Turning on sparse-checkout and running checkout to make my working\n> directory sparse\n>\n> $ git config core.sparsecheckout true\n> $ echo /i > .git/info/sparse-checkout\n> $ git checkout -f\n>\n> Running reset gives me\n> $ git reset HEAD~1\n> $ git status\n> On branch reset\n> nothing to commit, working tree clean\n> $ git ls-files\n> i\n>\n> file a is gone. Not in the index and not in the working directory.\n> Nothing to let the user know that anything changed.\n>\n> With a modified file no sparse-checkout\n> $ git log --oneline\n> 6fbd34a (HEAD -> reset, modified) modified file m\n> c734d72 Initial commit with file i and m\n> $ git reset HEAD~1\n> Unstaged changes after reset:\n> M m\n> $ git status\n> On branch reset\n> Changes not staged for commit:\n> (use \"git add ...\" to update what will be committed)\n> (use \"git checkout -- ...\" to discard changes in working directory)\n>\n> modified: m\n>\n> no changes added to commit (use \"git add\" and/or \"git commit -a\")\n>\n> With sparse-checkout\n> $ git reset HEAD~1\n> Unstaged changes after reset:\n> D m\n> $ git status\n> On branch reset\n> Changes not staged for commit:\n> (use \"git add/rm ...\" to update what will be committed)\n> (use \"git checkout -- ...\" to discard changes in working directory)\n>\n> deleted: m\n>\n> no changes added to commit (use \"git add\" and/or \"git commit -a\")\n>\n\nWasn't \"m\" outside the sparse checkout? Or was it a file in the sparse\ncheckout? I mean to say, the file after setting up sparse checkout was\none of the \"interesting\" files that sparse checked out?\n\nOr was it in fact a separate file which wasn't there?\n\nI would think that in sparse-checkout world, you should only *ever*\nhave the files you list in sparse.\n\nSo files outside sparse world should be ignored, not shown and not\nshow up in status, but they should absolutely not show up in the\nworking tree either.\n\nYou're not \"changing\" any commits, because the status of the file at\nHEAD~1 is exactly what HEAD~1 says it is, but you just don't have a\nchecked out copy of it.\n\nI think the key problem is that reset is clearing the sparse flag of a\nfile so that it no longer shows up as sparse, which is why status\nsubsequently shows the file deleted (since you don't have a local copy\nanymore).\n\nAm I understanding the problem correctly? I think your examples above\nare not clear because they don't seem to be each complete individually\n(The sparse checkout examples should start from a clean world and\nexplain how they got there rather than relying on imformation in the\nprior non sparse example. We should be clear so everyone knows what\nthe problem is).\n\nIf you're performing a sparse checkout and you modify files outside of\nthe sparse area, I think that will cause problems, and we may not be\nmaking the best efforts to resolve it. I do agree that if you have\ncreated a file \"m\" when only \"i\" is in your path, that we should\nabsolutely not delete \"m\" but leave it as is.\n\nThanks,\nJake\n\n> I think we can agree that this is not the correct behavior.\n>\n>> But this time, I am trying to see if the approach is good. I am not\n>> so sure if the approach taken by this patch is so obviously good as\n>> you seem to think. A logical consequence of the approach \"git\n>> status thinks that P appears in the index and missing in the working\n>> tree is a local removal, so let's squelch it by creating the file in\n>> the working tree\" is that we will end up fully populating the\n>> working tree with paths that are clearly outside the area the user\n>> designated as the sparse checkout area---surely that may squelch\n>> \"status\", but to me, it looks like it is breaking what the user\n>> wanted to achieve with \"sparse checkout\" in the first place.\n>>\n>\n> I don't think that we are trying to \"squelch\" status so much as make\n> it consistent with what the user would expect to happen. If that means\n> not resetting entries with the skip-worktree bit or resetting the entries\n> but keeping the skip-worktree bit on, okay, but I would argue that is\n> not what the user wants because if you are now saying that sparse\n> means git will not change files outside the sparse-checkout entries,\n> what about merge, rebase, cherry-pick, apply? Should those only\n> change the files that are in the sparse definition? If so we would\n> be changing the commits from the original, i.e. cherry-pick 123 would\n> create a different commit depending on whether or not you are using\n> sparse as well as a different commit depending on what is in your\n> sparse-checkout.\n>\n> I see reset being a similar scenario in that if everything is clean, after I\n> \"reset HEAD~1\" I should be able to run \"add .\" + \"commit\" and have\n> the same commit as before the reset. If this is changed to only reset\n> the sparse entries, there will be staged changes after the reset because\n> HEAD has changed but we didn't update the index versions of the files.\n> If we do update the index with the \"HEAD~1\" version of the files and just\n> set the skip-worktree bit then the next commit will not have the original\n> changes outside the sparse-checkout for the commit.\n>\n>> When we make a sparse checkout that places P outside the checked-out\n>> area, with P in the index and not in the working tree, what asks\n>> \"git status\" and friends not to treat P as \"locally removed\"?\n>> Shouldn't \"reset HEAD~1\" that resurrected P that was missing in the\n>> commit in the state before you did the \"reset\" be doing the same\n>> (e.g. flipping the bit in the index for path P that says \"not having\n>> this in the working tree is not a removal---it is deliberately not\n>> checked out\")? If that is the right approach to solve the issue\n>> (which we established is a right problem to solve already), and the\n>> approach that the patch wants to take is quite the opposite of it,\n>> then my answer to the second question (are we solving the problem\n>> with the right approach?) is no. And at that point, it is moot to\n>> ask if the code correctly and/or efficiently implements that wrong\n>> solution, isn't it?\n>\n> I agree that we must determine if there is a problem, which I hope\n> we can agree that there is a valid problem to be addressed. Determining\n> if I used the right approach depends on how you see reset working\n> correctly using the sparse feature. Should it match a reset when not\n> using sparse or should it reset only sparse files? I saw issues\n> if only the sparse files are the ones reset, which is the reason that\n> I went with this approach.\n>\n> Thanks,\n> Kevin\n"},{"id":"327909","messageId":"SN1PR21MB001473733DAF15BC91C0E58AB7690@SN1PR21MB0014.namprd21.prod.outlook.com","threadId":"46729","inReplyTo":"CA+P7+xqxmxexWS=MWNd9=EqG81uhKY-OdG+1mpyWhst6DvH5AA@mail.gmail.com","subject":"RE: [PATCH 1/1] reset: fix reset when using the sparse-checkout feature.","fromName":"Kevin Willford","fromEmail":"kewillf@microsoft.com","sentAt":"2017-09-12T23:30:21Z","receivedAt":"2017-09-12T23:30:30Z","isPatch":true,"sender":{"key":"kewillf@microsoft.com","avatar":null},"body":"> From: Jacob Keller [mailto:jacob.keller@gmail.com]\n> Sent: Tuesday, September 12, 2017 4:29 PM\n> \n> On Tue, Sep 12, 2017 at 1:20 PM, Kevin Willford wrote:\n> >\n> > I think this is where I need to do a better job of explaining so here is a\n> > simple example.\n> >\n> > I have a file \"a\" that was added in the latest commit.\n> > $ git log --oneline\n> > c1fa646 (HEAD -> reset, master) add file a\n> > 40b342c Initial commit with file i\n> >\n> > Running the reset without using a sparse-checkout file\n> >\n> > $ git reset HEAD~1\n> > $ git status\n> > On branch reset\n> > Untracked files:\n> > (use \"git add ...\" to include in what will be committed)\n> >\n> > a\n> >\n> > nothing added to commit but untracked files present (use \"git add\" to track)\n> >\n> > Turning on sparse-checkout and running checkout to make my working\n> > directory sparse\n> >\n> > $ git config core.sparsecheckout true\n> > $ echo /i > .git/info/sparse-checkout\n> > $ git checkout -f\n> >\n> > Running reset gives me\n> > $ git reset HEAD~1\n> > $ git status\n> > On branch reset\n> > nothing to commit, working tree clean\n> > $ git ls-files\n> > i\n> >\n> > file a is gone. Not in the index and not in the working directory.\n> > Nothing to let the user know that anything changed.\n> >\n> > With a modified file no sparse-checkout\n> > $ git log --oneline\n> > 6fbd34a (HEAD -> reset, modified) modified file m\n> > c734d72 Initial commit with file i and m\n> > $ git reset HEAD~1\n> > Unstaged changes after reset:\n> > M m\n> > $ git status\n> > On branch reset\n> > Changes not staged for commit:\n> > (use \"git add ...\" to update what will be committed)\n> > (use \"git checkout -- ...\" to discard changes in working directory)\n> >\n> > modified: m\n> >\n> > no changes added to commit (use \"git add\" and/or \"git commit -a\")\n> >\n> > With sparse-checkout\n> > $ git reset HEAD~1\n> > Unstaged changes after reset:\n> > D m\n> > $ git status\n> > On branch reset\n> > Changes not staged for commit:\n> > (use \"git add/rm ...\" to update what will be committed)\n> > (use \"git checkout -- ...\" to discard changes in working directory)\n> >\n> > deleted: m\n> >\n> > no changes added to commit (use \"git add\" and/or \"git commit -a\")\n> >\n> \n> Wasn't \"m\" outside the sparse checkout? Or was it a file in the sparse\n> checkout? I mean to say, the file after setting up sparse checkout was\n> one of the \"interesting\" files that sparse checked out?\n> \n> Or was it in fact a separate file which wasn't there?\n\nSorry. It was not in the sparse-checkout file. \n$ git config core.sparsecheckout true\n$ echo /i > .git/info/sparse-checkout\n$ git checkout -f\nwas ran in the modified file case as well and \"i\" was the only file in the\nworking directory before reset. \n\n> \n> I would think that in sparse-checkout world, you should only *ever*\n> have the files you list in sparse.\n> \n> So files outside sparse world should be ignored, not shown and not\n> show up in status, but they should absolutely not show up in the\n> working tree either.\n\nSparse checkout uses the skip-worktree bit which has the following\ndocumentation:\nhttps://git-scm.com/docs/git-update-index \n\"Skip-worktree bit can be defined in one (long) sentence: When reading\nan entry, if it is marked as skip-worktree, then Git pretends its working\ndirectory version is up to date and read the index version instead.\n\nTo elaborate, \"reading\" means checking for file existence, reading file\nattributes or file content. The working directory version may be present\nor absent. If present, its content may match against the index version or\nnot. Writing is not affected by this bit, content safety is still first priority.\nNote that Git can update working directory file, that is marked skip-worktree,\nif it is safe to do so (i.e. working directory version matches index version)\"\n\nI'm not saying that is the right behavior, just pointing out the documentation.\nAnd from my experience git will turn on and off the skip-worktree flag\ndepending on the command as we see happening with reset.\n\n> \n> You're not \"changing\" any commits, because the status of the file at\n> HEAD~1 is exactly what HEAD~1 says it is, but you just don't have a\n> checked out copy of it.\n\nIn the case of reset yes and it matches HEAD and if the sparse flag was on,\nthe user does not know that the file was changed in any way, which if I add\nand commit will create a commit with the file content at HEAD~1\nthat is outside the sparse area without the user even knowing what\nthose changes were unless after the commit they run a git diff HEAD~1 and\nsee. So files will be changed outside of the sparse area without the\nuser's knowledge.\n\n> \n> I think the key problem is that reset is clearing the sparse flag of a\n> file so that it no longer shows up as sparse, which is why status\n> subsequently shows the file deleted (since you don't have a local copy\n> anymore).\n\nAnd in the case of an added file there is not a sparse flag to clear or keep\nBecause the file didn't exist at HEAD~1 and there is no entry in the index\nto keep and the file and any knowledge of it is gone.\n\n> \n> Am I understanding the problem correctly? I think your examples above\n> are not clear because they don't seem to be each complete individually\n> (The sparse checkout examples should start from a clean world and\n> explain how they got there rather than relying on imformation in the\n> prior non sparse example. We should be clear so everyone knows what\n> the problem is).\n> \n\nI thought that the 'git log --oneline' with the commit message make it\nclear what the state of the repo was before the reset in either case.\nThe only difference is that in the sparse case, it is turned on and one\nfile added to the sparse-checkout file and a checkout is performed\nto make the working directory sparse.\n\n> If you're performing a sparse checkout and you modify files outside of\n> the sparse area, I think that will cause problems, and we may not be\n> making the best efforts to resolve it. I do agree that if you have\n> created a file \"m\" when only \"i\" is in your path, that we should\n> absolutely not delete \"m\" but leave it as is.\n> \n\nAgain documentation says and from my experience, git can update and\ndelete \"m\" if it matches the index version. \n\n> Thanks,\n> Jake\n> \n"},{"id":"327911","messageId":"CA+P7+xqcAh4v4gDQOm-feYGc1EmFztAu0zMg2xnp8Gn4=BzkoQ@mail.gmail.com","threadId":"46729","inReplyTo":"SN1PR21MB001473733DAF15BC91C0E58AB7690@SN1PR21MB0014.namprd21.prod.outlook.com","subject":"Re: [PATCH 1/1] reset: fix reset when using the sparse-checkout feature.","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-09-13T01:39:16Z","receivedAt":"2017-09-13T01:39:42Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Tue, Sep 12, 2017 at 4:30 PM, Kevin Willford wrote:\n>\n> Sorry. It was not in the sparse-checkout file.\n> $ git config core.sparsecheckout true\n> $ echo /i > .git/info/sparse-checkout\n> $ git checkout -f\n> was ran in the modified file case as well and \"i\" was the only file in the\n> working directory before reset.\n>\n\n\nI'm assuming then that you mean that some file \"m\" is modified by the\ncommit, and do not mean to say that it has local modifications in the\nworking tree? That is not what I had inferred from earlier, so I was\nvery much confused.\n\nIn this example, the only file actually checked out is \"i\", as\neverything else will have the skip-worktree bit set..?\n\nSo doing git reset HEAD~1 will reset the branch back one commit, and\nnow because of this reset is clearing the skip worktree flag, and\nsince you never had a copy of it checked out it is notified as\ndeleted, because it's no longer marked as skip-worktree?\n\n\nI think the real fix is to stop having reset clear skip-worktree, no?\n\nBy definition if you do a sparse checkout, you're telling git \"I only\ncare about the files in this sparse checkout, and do not concern me\nwith anything else\"... So the proposed fix is \"since git cleared the\nskip-worktree flag, we should actually also copy the file out again.\"\nbut I think the real problem is that we're clearing skip worktree to\nbegin with?\n"},{"id":"327948","messageId":"SN1PR21MB0014D30A30F70B7DEAA17478B76E0@SN1PR21MB0014.namprd21.prod.outlook.com","threadId":"46729","inReplyTo":"CA+P7+xqcAh4v4gDQOm-feYGc1EmFztAu0zMg2xnp8Gn4=BzkoQ@mail.gmail.com","subject":"RE: [PATCH 1/1] reset: fix reset when using the sparse-checkout feature.","fromName":"Kevin Willford","fromEmail":"kewillf@microsoft.com","sentAt":"2017-09-13T17:09:15Z","receivedAt":"2017-09-13T17:09:23Z","isPatch":true,"sender":{"key":"kewillf@microsoft.com","avatar":null},"body":"> From: Jacob Keller [mailto:jacob.keller@gmail.com]\n> Sent: Tuesday, September 12, 2017 7:39 PM\n> \n> On Tue, Sep 12, 2017 at 4:30 PM, Kevin Willford wrote:\n> >\n> > Sorry. It was not in the sparse-checkout file.\n> > $ git config core.sparsecheckout true\n> > $ echo /i > .git/info/sparse-checkout\n> > $ git checkout -f\n> > was ran in the modified file case as well and \"i\" was the only file in the\n> > working directory before reset.\n> >\n> \n> \n> I'm assuming then that you mean that some file \"m\" is modified by the\n> commit, and do not mean to say that it has local modifications in the\n> working tree? That is not what I had inferred from earlier, so I was\n> very much confused.\n> \n\nYes\n\n> In this example, the only file actually checked out is \"i\", as\n> everything else will have the skip-worktree bit set..?\n> \n\nYes\n\n> So doing git reset HEAD~1 will reset the branch back one commit, and\n> now because of this reset is clearing the skip worktree flag, and\n> since you never had a copy of it checked out it is notified as\n> deleted, because it's no longer marked as skip-worktree?\n> \n\nCorrect\n\n> \n> I think the real fix is to stop having reset clear skip-worktree, no?\n> \n> By definition if you do a sparse checkout, you're telling git \"I only\n> care about the files in this sparse checkout, and do not concern me\n> with anything else\"... So the proposed fix is \"since git cleared the\n> skip-worktree flag, we should actually also copy the file out again.\"\n> but I think the real problem is that we're clearing skip worktree to\n> begin with?\n\nThis certainly is an option but I would have some questions with this\napproach. Does this statement, \"I only care about the files in this\nsparse checkout, and do not concern me with anything else\", mean\nthat git should not change files outside the sparse-checkout whether\nthey are in the working directory or the index? Or does that only\napply to the working directory and the index version of files can\nchange to whatever git decides? So in the example above would\n\"m\" be the HEAD~1 version of the file in the index or the HEAD\nversion before the reset? Does this apply to all git commands,\nmerge, rebase, cherry-pick, etc? What about when there is a merge\nconflict with a file that is outside the sparse checkout?\n\nSeems to me it is a lot more complex than only caring about the files\nin the sparse checkout and no concern for anything else. Personally\nI would like to error on the side of letting the user decide what they\nwant to do, even if that means turning off the skip-worktree bit and\nputting the working directory into an expected state.\n\n\n"},{"id":"328013","messageId":"xmqqy3piz1j4.fsf@gitster.mtv.corp.google.com","threadId":"46729","inReplyTo":"CA+P7+xqcAh4v4gDQOm-feYGc1EmFztAu0zMg2xnp8Gn4=BzkoQ@mail.gmail.com","subject":"Re: [PATCH 1/1] reset: fix reset when using the sparse-checkout feature.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-13T22:17:35Z","receivedAt":"2017-09-13T22:17:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n> By definition if you do a sparse checkout, you're telling git \"I only\n> care about the files in this sparse checkout, and do not concern me\n> with anything else\"... So the proposed fix is \"since git cleared the\n> skip-worktree flag, we should actually also copy the file out again.\"\n> but I think the real problem is that we're clearing skip worktree to\n> begin with?\n\nAs this 100% agree with what I've been saying, I do not have\nanything to add to the discussion at the moment, so I'll stop\nspeaking now but will continue to monitor the thread so that I may\nhear a new argument and datapoint that would make me change my mind.\n\nThanks for a healthy discussion.\n"},{"id":"328054","messageId":"SN1PR21MB0014031012AEC69CED27FADFB76F0@SN1PR21MB0014.namprd21.prod.outlook.com","threadId":"46729","inReplyTo":"xmqqy3piz1j4.fsf@gitster.mtv.corp.google.com","subject":"RE: [PATCH 1/1] reset: fix reset when using the sparse-checkout feature.","fromName":"Kevin Willford","fromEmail":"kewillf@microsoft.com","sentAt":"2017-09-14T14:26:21Z","receivedAt":"2017-09-14T14:26:29Z","isPatch":true,"sender":{"key":"kewillf@microsoft.com","avatar":null},"body":"> From: Junio C Hamano [mailto:gitster@pobox.com]\n> Sent: Wednesday, September 13, 2017 4:18 PM\n> \n> Jacob Keller writes:\n> \n> > By definition if you do a sparse checkout, you're telling git \"I only\n> > care about the files in this sparse checkout, and do not concern me\n> > with anything else\"... So the proposed fix is \"since git cleared the\n> > skip-worktree flag, we should actually also copy the file out again.\"\n> > but I think the real problem is that we're clearing skip worktree to\n> > begin with?\n> \n> As this 100% agree with what I've been saying, I do not have\n> anything to add to the discussion at the moment, so I'll stop\n> speaking now but will continue to monitor the thread so that I may\n> hear a new argument and datapoint that would make me change my mind.\n> \n> Thanks for a healthy discussion.\n\nHi Junio,\n\nThanks for the feedback. I will give this implementation a try. I still\nhave the following unanswered questions, if you wouldn't mind\nanswering.\n\n1. Does this statement, \"I only care about the files in this\nsparse checkout, and do not concern me with anything else\", mean\nthat git should not change files outside the sparse-checkout whether\nthat be in the working directory or in the index? Or does that only\napply to the working directory and the index version of files can\nchange to whatever git the git command would do without using\nsparse? For example if I am doing a 'git reset HEAD~1' should the\nversion in the index of files outside the sparse not be changed or\nchange to the HEAD~1 version with the skip-worktree bit on?\n2. How will this work with other git commands like merge, rebase,\ncherry-pick, etc.? \n3. What about when there is a merge conflict with a file that is outside\nthe sparse checkout? Should there not be a conflict because\ngit shouldn't be trying to merge the file since it is outside the sparse\ncheckout area? Should the conflicted file get written outside the\nsparse checkout so the user can resolve it?\n\nThanks,\nKevin\n\n"},{"id":"328101","messageId":"xmqqh8w4y2se.fsf@gitster.mtv.corp.google.com","threadId":"46729","inReplyTo":"SN1PR21MB0014031012AEC69CED27FADFB76F0@SN1PR21MB0014.namprd21.prod.outlook.com","subject":"Re: [PATCH 1/1] reset: fix reset when using the sparse-checkout feature.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-15T05:00:17Z","receivedAt":"2017-09-15T05:00:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Willford writes:\n\n> 1. Does this statement, \"I only care about the files in this\n> sparse checkout, and do not concern me with anything else\", mean\n> that git should not change files outside the sparse-checkout whether\n> that be in the working directory or in the index? Or does that only\n> apply to the working directory and the index version of files can\n> change to whatever git the git command would do without using\n> sparse? For example if I am doing a 'git reset HEAD~1' should the\n> version in the index of files outside the sparse not be changed or\n> change to the HEAD~1 version with the skip-worktree bit on?\n\nMy understanding of the purpose of \"sparse checkout\" thing is that\nthe user still wants to create correct whole-tree commit even the\nuser does not have the whole-tree checkout. The object names for\nblobs recorded in the index that are meant to be included in the\nnext commit MUST be the same as those that would be in the index\nwhen the \"sparse\" feature is not in use. \"reset HEAD~1\" should\nmatch the index entries to the tree entries in HEAD~1. So, the\nlatter, I would think, among your two alternatives.\n\nIOW, after \"git reset HEAD~\", if you drop the skip-worktree bit from\nall index entries, \"git diff --cached HEAD\" must say \"there is no\nchanges\".\n\nThe only difference between the \"sparse\" and normal would be that,\nbecause the \"sparse\" user does not intend to change anything outside\nthe \"sparse\" area, these paths outside her \"sparse\" area would not\nmaterialize on the filesystem. For the next \"write-tree\" out of the\nindex to still write the correct tree out, the entries outside her\n\"sparse\" area in the index MUST match the tree of the commit she\nstarted working from.\n\n> 2. How will this work with other git commands like merge, rebase,\n> cherry-pick, etc.? \n> 3. What about when there is a merge conflict with a file that is outside\n> the sparse checkout?\n\nI would say, rather than forbidding such a merge, it should let her\nsee and deal with the conflict by dropping the \"this is outside the\nsparse area, so do not bother materializing it to the filesystem\"\nbit, but tell her loudly what it did (\"I checked out a half-merged\nfile outside your sparse-checkout area because you'd need it while\nresolving the conflict\"). By doing things that way, the user can\ndecide if she wants to go ahead and complete the merge, even if the\nconflict is outside the area she is currently interested in, or\npostpone the merge and continue working on what she has been working\non inside the narrowed-down area first.\n\nI do not have a strong opinion whether the sparse-checkout\nconfiguration file should be adjusted to match when the command must\ntentatively bust the sparse checkout area; I'd imagine it can be\nargued either way.\n\nNote that \"sparse\" is not my itch, and I would not be surprised if\nthose who designed it may want it to work differently from my\nknee-jerk reaction in the previous two paragraphs, and I may even\nfind such an alternative solution preferable. \n\nBut it is highly unlikely for any sensible solution would violate\nthe basic premise, i.e. \"the indexed contents will stay the same as\nthe case without any 'sparse', so the next write-tree will do the\nright thing\".\n"},{"id":"328137","messageId":"SN1PR21MB00145406332AC0133FDD89F7B76C0@SN1PR21MB0014.namprd21.prod.outlook.com","threadId":"46729","inReplyTo":"xmqqh8w4y2se.fsf@gitster.mtv.corp.google.com","subject":"RE: [PATCH 1/1] reset: fix reset when using the sparse-checkout feature.","fromName":"Kevin Willford","fromEmail":"kewillf@microsoft.com","sentAt":"2017-09-15T17:21:39Z","receivedAt":"2017-09-15T17:21:53Z","isPatch":true,"sender":{"key":"kewillf@microsoft.com","avatar":null},"body":"From: Junio C Hamano [mailto:gitster@pobox.com]\nSent: Thursday, September 14, 2017 11:00 PM\n> \n> Kevin Willford writes:\n> \n> > 1. Does this statement, \"I only care about the files in this\n> > sparse checkout, and do not concern me with anything else\", mean\n> > that git should not change files outside the sparse-checkout whether\n> > that be in the working directory or in the index? Or does that only\n> > apply to the working directory and the index version of files can\n> > change to whatever git the git command would do without using\n> > sparse? For example if I am doing a 'git reset HEAD~1' should the\n> > version in the index of files outside the sparse not be changed or\n> > change to the HEAD~1 version with the skip-worktree bit on?\n> \n> My understanding of the purpose of \"sparse checkout\" thing is that\n> the user still wants to create correct whole-tree commit even the\n> user does not have the whole-tree checkout. The object names for\n> blobs recorded in the index that are meant to be included in the\n> next commit MUST be the same as those that would be in the index\n> when the \"sparse\" feature is not in use. \"reset HEAD~1\" should\n> match the index entries to the tree entries in HEAD~1. So, the\n> latter, I would think, among your two alternatives.\n> \n> IOW, after \"git reset HEAD~\", if you drop the skip-worktree bit from\n> all index entries, \"git diff --cached HEAD\" must say \"there is no\n> changes\".\n> \n> The only difference between the \"sparse\" and normal would be that,\n> because the \"sparse\" user does not intend to change anything outside\n> the \"sparse\" area, these paths outside her \"sparse\" area would not\n> materialize on the filesystem. For the next \"write-tree\" out of the\n> index to still write the correct tree out, the entries outside her\n> \"sparse\" area in the index MUST match the tree of the commit she\n> started working from.\n> \n\nMakes sense. And even though the reset might only change entries\noutside the sparse area and the next status will report \"nothing to commit,\nworking tree clean\", that's okay because the user hasn't changed\nanything in their sparse area and intended to roll back the index to\nwhatever they specified in their reset command.\n\n> > 2. How will this work with other git commands like merge, rebase,\n> > cherry-pick, etc.?\n> > 3. What about when there is a merge conflict with a file that is outside\n> > the sparse checkout?\n> \n> I would say, rather than forbidding such a merge, it should let her\n> see and deal with the conflict by dropping the \"this is outside the\n> sparse area, so do not bother materializing it to the filesystem\"\n> bit, but tell her loudly what it did (\"I checked out a half-merged\n> file outside your sparse-checkout area because you'd need it while\n> resolving the conflict\"). By doing things that way, the user can\n> decide if she wants to go ahead and complete the merge, even if the\n> conflict is outside the area she is currently interested in, or\n> postpone the merge and continue working on what she has been working\n> on inside the narrowed-down area first.\n> \n> I do not have a strong opinion whether the sparse-checkout\n> configuration file should be adjusted to match when the command must\n> tentatively bust the sparse checkout area; I'd imagine it can be\n> argued either way.\n> \n> Note that \"sparse\" is not my itch, and I would not be surprised if\n> those who designed it may want it to work differently from my\n> knee-jerk reaction in the previous two paragraphs, and I may even\n> find such an alternative solution preferable.\n> \n> But it is highly unlikely for any sensible solution would violate\n> the basic premise, i.e. \"the indexed contents will stay the same as\n> the case without any 'sparse', so the next write-tree will do the\n> right thing\".\n\nThere was one other case that I thought about while implementing\nthis approach and it is when the user creates a file that is outside their\nsparse definition. From your explanation above I will attempt to\nexplain how I think it should work and please correct me if you see\nit working differently.\n\nThe user creates a file that is outside the sparse area and it will show\nup as untracked. No problem here since the untracked are outside the\nscope of using sparse. Next the user adds the untracked file to the index.\nThe skip-worktree bit should be off and stay off since the user could make\nadditional changes and want to add them. Once the user commits the\nnewly created file, I could see turning on the skip-worktree bit and\nremoving the file from the working tree after the commit but since\nthis was a user initiated action and it is sparse \"checkout\" it makes\nsense to wait until the next \"checkout\" command which will use the\nsparse definition to clean this file from the working directory and\nturn on the skip-worktree flag. If the user wants to continue to use\nthe new file, they would need to update their sparse-checkout file\nto include the newly created file so that the next checkout will not\nremove it and turn on the flag.\n"},{"id":"328172","messageId":"CA+P7+xoUmV64Bm2v2pT2yb3S4Pmt52M5Vfwgj+bGyiRsTZr3Ew@mail.gmail.com","threadId":"46729","inReplyTo":"SN1PR21MB00145406332AC0133FDD89F7B76C0@SN1PR21MB0014.namprd21.prod.outlook.com","subject":"Re: [PATCH 1/1] reset: fix reset when using the sparse-checkout feature.","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-09-15T21:33:10Z","receivedAt":"2017-09-15T21:33:37Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Fri, Sep 15, 2017 at 10:21 AM, Kevin Willford wrote:\n> From: Junio C Hamano [mailto:gitster@pobox.com]\n> Sent: Thursday, September 14, 2017 11:00 PM\n>>\n>> Kevin Willford writes:\n>>\n>> > 1. Does this statement, \"I only care about the files in this\n>> > sparse checkout, and do not concern me with anything else\", mean\n>> > that git should not change files outside the sparse-checkout whether\n>> > that be in the working directory or in the index? Or does that only\n>> > apply to the working directory and the index version of files can\n>> > change to whatever git the git command would do without using\n>> > sparse? For example if I am doing a 'git reset HEAD~1' should the\n>> > version in the index of files outside the sparse not be changed or\n>> > change to the HEAD~1 version with the skip-worktree bit on?\n>>\n>> My understanding of the purpose of \"sparse checkout\" thing is that\n>> the user still wants to create correct whole-tree commit even the\n>> user does not have the whole-tree checkout. The object names for\n>> blobs recorded in the index that are meant to be included in the\n>> next commit MUST be the same as those that would be in the index\n>> when the \"sparse\" feature is not in use. \"reset HEAD~1\" should\n>> match the index entries to the tree entries in HEAD~1. So, the\n>> latter, I would think, among your two alternatives.\n>>\n>> IOW, after \"git reset HEAD~\", if you drop the skip-worktree bit from\n>> all index entries, \"git diff --cached HEAD\" must say \"there is no\n>> changes\".\n>>\n>> The only difference between the \"sparse\" and normal would be that,\n>> because the \"sparse\" user does not intend to change anything outside\n>> the \"sparse\" area, these paths outside her \"sparse\" area would not\n>> materialize on the filesystem. For the next \"write-tree\" out of the\n>> index to still write the correct tree out, the entries outside her\n>> \"sparse\" area in the index MUST match the tree of the commit she\n>> started working from.\n>>\n>\n> Makes sense. And even though the reset might only change entries\n> outside the sparse area and the next status will report \"nothing to commit,\n> working tree clean\", that's okay because the user hasn't changed\n> anything in their sparse area and intended to roll back the index to\n> whatever they specified in their reset command.\n>\n>> > 2. How will this work with other git commands like merge, rebase,\n>> > cherry-pick, etc.?\n>> > 3. What about when there is a merge conflict with a file that is outside\n>> > the sparse checkout?\n>>\n>> I would say, rather than forbidding such a merge, it should let her\n>> see and deal with the conflict by dropping the \"this is outside the\n>> sparse area, so do not bother materializing it to the filesystem\"\n>> bit, but tell her loudly what it did (\"I checked out a half-merged\n>> file outside your sparse-checkout area because you'd need it while\n>> resolving the conflict\"). By doing things that way, the user can\n>> decide if she wants to go ahead and complete the merge, even if the\n>> conflict is outside the area she is currently interested in, or\n>> postpone the merge and continue working on what she has been working\n>> on inside the narrowed-down area first.\n>>\n>> I do not have a strong opinion whether the sparse-checkout\n>> configuration file should be adjusted to match when the command must\n>> tentatively bust the sparse checkout area; I'd imagine it can be\n>> argued either way.\n>>\n>> Note that \"sparse\" is not my itch, and I would not be surprised if\n>> those who designed it may want it to work differently from my\n>> knee-jerk reaction in the previous two paragraphs, and I may even\n>> find such an alternative solution preferable.\n>>\n>> But it is highly unlikely for any sensible solution would violate\n>> the basic premise, i.e. \"the indexed contents will stay the same as\n>> the case without any 'sparse', so the next write-tree will do the\n>> right thing\".\n>\n> There was one other case that I thought about while implementing\n> this approach and it is when the user creates a file that is outside their\n> sparse definition. From your explanation above I will attempt to\n> explain how I think it should work and please correct me if you see\n> it working differently.\n>\n> The user creates a file that is outside the sparse area and it will show\n> up as untracked. No problem here since the untracked are outside the\n> scope of using sparse. Next the user adds the untracked file to the index.\n> The skip-worktree bit should be off and stay off since the user could make\n> additional changes and want to add them. Once the user commits the\n> newly created file, I could see turning on the skip-worktree bit and\n> removing the file from the working tree after the commit but since\n> this was a user initiated action and it is sparse \"checkout\" it makes\n> sense to wait until the next \"checkout\" command which will use the\n> sparse definition to clean this file from the working directory and\n> turn on the skip-worktree flag. If the user wants to continue to use\n> the new file, they would need to update their sparse-checkout file\n> to include the newly created file so that the next checkout will not\n> remove it and turn on the flag.\n\n\nThis makes sense to me, but possibly we should warn when a user tries\nto do things with files outside the sparse area? Like when adding a\nfile warn that it already exists?\n\nMaybe users do expect the ability to add new files and those would\nwant to get automatically pulled into the \"sparse\" list? Like if a\nuser adds a completely new file that's never been in the work tree\nbefore...\n\nThanks,\nJake\n"}]}