{"thread":{"id":"57279","subject":"[PATCH] sparse-checkout: create leading directory","startedAt":"2022-01-20T18:55:55Z","lastAt":"2022-01-25T07:03:48Z","messageCount":14,"participants":["Jonathan Tan","Junio C Hamano","Ævar Arnfjörð Bjarmason","Elijah Newren","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"446592","messageId":"20220120185548.3648549-1-jonathantanmy@google.com","threadId":"57279","inReplyTo":null,"subject":"[PATCH] sparse-checkout: create leading directory","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-01-20T18:55:48Z","receivedAt":"2022-01-20T18:55:55Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When creating the sparse-checkout file, Git does not create the leading\ndirectory, \"$GIT_DIR/info\", if it does not exist. This causes problems\nif the repository does not have that directory. Therefore, ensure that\nthe leading directory is created.\n\nThis is the only \"open\" in builtin/sparse-checkout.c that does not have\na leading directory check. (The other one in write_patterns_and_update()\ndoes.)\n\nNote that the test needs to explicitly specify a template when running\n\"git init\" because the default template used in the tests has the\n\"info/\" directory included.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nThis problem is being discussed in [1], and we also noticed this problem\ninternally at $DAYJOB. Here's a fix.\n\n[1] https://lore.kernel.org/git/db6f47a3-0df3-505b-b391-6ca289fd61b5@gmail.com/\n---\n builtin/sparse-checkout.c          | 3 +++\n t/t1091-sparse-checkout-builtin.sh | 7 +++++++\n 2 files changed, 10 insertions(+)\n\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex 679c107036..2b0e1db2d2 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -471,6 +471,9 @@ static int sparse_checkout_init(int argc, const char **argv)\n \t\tFILE *fp;\n \n \t\t/* assume we are in a fresh repo, but update the sparse-checkout file */\n+\t\tif (safe_create_leading_directories(sparse_filename))\n+\t\t\tdie(_(\"unable to create leading directories of %s\"),\n+\t\t\t    sparse_filename);\n \t\tfp = xfopen(sparse_filename, \"w\");\n \t\tif (!fp)\n \t\t\tdie(_(\"failed to open '%s'\"), sparse_filename);\ndiff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\nindex 42776984fe..dba0737599 100755\n--- a/t/t1091-sparse-checkout-builtin.sh\n+++ b/t/t1091-sparse-checkout-builtin.sh\n@@ -79,6 +79,13 @@ test_expect_success 'git sparse-checkout init' '\n \tcheck_files repo a\n '\n \n+test_expect_success 'git sparse-checkout init in empty repo' '\n+\ttest_when_finished rm -rf empty-repo blank-template &&\n+\tmkdir blank-template &&\n+\tgit init --template=blank-template empty-repo &&\n+\tgit -C empty-repo sparse-checkout init\n+'\n+\n test_expect_success 'git sparse-checkout list after init' '\n \tgit -C repo sparse-checkout list >actual &&\n \tcat >expect <<-\\EOF &&\n-- \n2.34.1.703.g22d0c6ccf7-goog\n\n"},{"id":"446597","messageId":"xmqqv8yegnmq.fsf@gitster.g","threadId":"57279","inReplyTo":"20220120185548.3648549-1-jonathantanmy@google.com","subject":"Re: [PATCH] sparse-checkout: create leading directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-20T20:26:05Z","receivedAt":"2022-01-20T20:26:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> When creating the sparse-checkout file, Git does not create the leading\n> directory, \"$GIT_DIR/info\", if it does not exist. This causes problems\n> if the repository does not have that directory. Therefore, ensure that\n> the leading directory is created.\n>\n> This is the only \"open\" in builtin/sparse-checkout.c that does not have\n> a leading directory check. (The other one in write_patterns_and_update()\n> does.)\n>\n> Note that the test needs to explicitly specify a template when running\n> \"git init\" because the default template used in the tests has the\n> \"info/\" directory included.\n>\n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n> This problem is being discussed in [1], and we also noticed this problem\n> internally at $DAYJOB. Here's a fix.\n\nInteresting, as $GIT_DIR/info is created during \"git init\".\n\nBut it is possible to lose the directory if there is no need for any\nof the files---the user may rmdir or rm -fr it, and it is too harsh\nto call the repository \"corrupted\".\n\nWill queue.\n"},{"id":"446599","messageId":"220120.86mtjqks1b.gmgdl@evledraar.gmail.com","threadId":"57279","inReplyTo":"20220120185548.3648549-1-jonathantanmy@google.com","subject":"Re: [PATCH] sparse-checkout: create leading directory","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-01-20T21:30:03Z","receivedAt":"2022-01-20T21:37:30Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jan 20 2022, Jonathan Tan wrote:\n\n> When creating the sparse-checkout file, Git does not create the leading\n> directory, \"$GIT_DIR/info\", if it does not exist. This causes problems\n> if the repository does not have that directory. Therefore, ensure that\n> the leading directory is created.\n>\n> This is the only \"open\" in builtin/sparse-checkout.c that does not have\n> a leading directory check. (The other one in write_patterns_and_update()\n> does.)\n>\n> Note that the test needs to explicitly specify a template when running\n> \"git init\" because the default template used in the tests has the\n> \"info/\" directory included.\n>\n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n> This problem is being discussed in [1], and we also noticed this problem\n> internally at $DAYJOB. Here's a fix.\n>\n> [1] https://lore.kernel.org/git/db6f47a3-0df3-505b-b391-6ca289fd61b5@gmail.com/\n\nThanks. This fix looks good to me.\n\nLooking at my [1] my preliminary analysis there wasn't correct,\ni.e. there are other cases where we get the \"sparse_filename\" that you\ndon't cover here, but e.g. if you do:\n\n    <your test> &&\n    rm -rf .git/info &&\n    git sparse-checkout add foo\n\nIt looks like it will fail either way, so for those other codepaths we\ndidn't need to ensure the .git/info.\n\nStill, it would be nice to have the fix test for those cases too,\ni.e. how various operations are expected to behave if the user were to\nrm -rf .git/info. That'll obviously yield a broken repo, but we should\nstill try to handle it gracefully.\n\n1. https://lore.kernel.org/git/211220.86zgovt9bi.gmgdl@evledraar.gmail.com/\n\n> ---\n>  builtin/sparse-checkout.c          | 3 +++\n>  t/t1091-sparse-checkout-builtin.sh | 7 +++++++\n>  2 files changed, 10 insertions(+)\n>\n> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\n> index 679c107036..2b0e1db2d2 100644\n> --- a/builtin/sparse-checkout.c\n> +++ b/builtin/sparse-checkout.c\n> @@ -471,6 +471,9 @@ static int sparse_checkout_init(int argc, const char **argv)\n>  \t\tFILE *fp;\n>  \n>  \t\t/* assume we are in a fresh repo, but update the sparse-checkout file */\n> +\t\tif (safe_create_leading_directories(sparse_filename))\n> +\t\t\tdie(_(\"unable to create leading directories of %s\"),\n> +\t\t\t    sparse_filename);\n>  \t\tfp = xfopen(sparse_filename, \"w\");\n>  \t\tif (!fp)\n>  \t\t\tdie(_(\"failed to open '%s'\"), sparse_filename);\n> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\n> index 42776984fe..dba0737599 100755\n> --- a/t/t1091-sparse-checkout-builtin.sh\n> +++ b/t/t1091-sparse-checkout-builtin.sh\n> @@ -79,6 +79,13 @@ test_expect_success 'git sparse-checkout init' '\n>  \tcheck_files repo a\n>  '\n>  \n> +test_expect_success 'git sparse-checkout init in empty repo' '\n> +\ttest_when_finished rm -rf empty-repo blank-template &&\n> +\tmkdir blank-template &&\n> +\tgit init --template=blank-template empty-repo &&\n> +\tgit -C empty-repo sparse-checkout init\n> +'\n> +\n>  test_expect_success 'git sparse-checkout list after init' '\n>  \tgit -C repo sparse-checkout list >actual &&\n>  \tcat >expect <<-\\EOF &&\n\nYou're using an overly verbose way to say \"no templates, please\". You\ncan squash this in, i.e. --template= is an explicitly supported way to\ndo that.\n\ndiff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\nindex dba07375993..1fe741d9cd5 100755\n--- a/t/t1091-sparse-checkout-builtin.sh\n+++ b/t/t1091-sparse-checkout-builtin.sh\n@@ -80,9 +80,8 @@ test_expect_success 'git sparse-checkout init' '\n '\n \n test_expect_success 'git sparse-checkout init in empty repo' '\n-\ttest_when_finished rm -rf empty-repo blank-template &&\n-\tmkdir blank-template &&\n-\tgit init --template=blank-template empty-repo &&\n+\ttest_when_finished rm -rf empty-repo &&\n+\tgit init --template= empty-repo &&\n \tgit -C empty-repo sparse-checkout init\n '\n \n"},{"id":"446635","messageId":"20220121174441.3991963-1-jonathantanmy@google.com","threadId":"57279","inReplyTo":"20220120185548.3648549-1-jonathantanmy@google.com","subject":"[PATCH v2] sparse-checkout: create leading directory","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-01-21T17:44:41Z","receivedAt":"2022-01-21T17:45:33Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When creating the sparse-checkout file, Git does not create the leading\ndirectory, \"$GIT_DIR/info\", if it does not exist. This causes problems\nif the repository does not have that directory. Therefore, ensure that\nthe leading directory is created.\n\nThis is the only \"open\" in builtin/sparse-checkout.c that does not have\na leading directory check. (The other one in write_patterns_and_update()\ndoes.)\n\nNote that the test needs to explicitly specify a template when running\n\"git init\" because the default template used in the tests has the\n\"info/\" directory included.\n\nHelped-by: Jose Lopes <jabolopes@google.com>\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nChanges from v1:\n - made test simpler\n - added attribution to Jose Lopes for finding and making the first\n   draft of this patch (after confirming with them)\n\nÆvar mentioned \"git sparse-checkout add\" but I think that that is a\ndifferent problem - in the \"git sparse-checkout init\" case, we could get\ninto this case with a template that does not have .git/info, but in the\n\"git sparse-checkout add\" case, the user would have had to explicitly\nremove the info directory. So I'll limit this patch to the \"init\" case,\nfor now.\n---\n builtin/sparse-checkout.c          | 3 +++\n t/t1091-sparse-checkout-builtin.sh | 6 ++++++\n 2 files changed, 9 insertions(+)\n\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex 679c107036..2b0e1db2d2 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -471,6 +471,9 @@ static int sparse_checkout_init(int argc, const char **argv)\n \t\tFILE *fp;\n \n \t\t/* assume we are in a fresh repo, but update the sparse-checkout file */\n+\t\tif (safe_create_leading_directories(sparse_filename))\n+\t\t\tdie(_(\"unable to create leading directories of %s\"),\n+\t\t\t    sparse_filename);\n \t\tfp = xfopen(sparse_filename, \"w\");\n \t\tif (!fp)\n \t\t\tdie(_(\"failed to open '%s'\"), sparse_filename);\ndiff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\nindex 42776984fe..3189d3da96 100755\n--- a/t/t1091-sparse-checkout-builtin.sh\n+++ b/t/t1091-sparse-checkout-builtin.sh\n@@ -79,6 +79,12 @@ test_expect_success 'git sparse-checkout init' '\n \tcheck_files repo a\n '\n \n+test_expect_success 'git sparse-checkout init in empty repo' '\n+\ttest_when_finished rm -rf empty-repo blank-template &&\n+\tgit init --template= empty-repo &&\n+\tgit -C empty-repo sparse-checkout init\n+'\n+\n test_expect_success 'git sparse-checkout list after init' '\n \tgit -C repo sparse-checkout list >actual &&\n \tcat >expect <<-\\EOF &&\n-- \n2.35.0.rc0.227.g00780c9af4-goog\n\n"},{"id":"446640","messageId":"220121.8635lgkj17.gmgdl@evledraar.gmail.com","threadId":"57279","inReplyTo":"20220121174441.3991963-1-jonathantanmy@google.com","subject":"Re: [PATCH v2] sparse-checkout: create leading directory","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-01-21T18:48:46Z","receivedAt":"2022-01-21T19:04:08Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Jan 21 2022, Jonathan Tan wrote:\n\n> When creating the sparse-checkout file, Git does not create the leading\n> directory, \"$GIT_DIR/info\", if it does not exist. This causes problems\n> if the repository does not have that directory. Therefore, ensure that\n> the leading directory is created.\n>\n> This is the only \"open\" in builtin/sparse-checkout.c that does not have\n> a leading directory check. (The other one in write_patterns_and_update()\n> does.)\n>\n> Note that the test needs to explicitly specify a template when running\n> \"git init\" because the default template used in the tests has the\n> \"info/\" directory included.\n>\n> Helped-by: Jose Lopes <jabolopes@google.com>\n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n> Changes from v1:\n>  - made test simpler\n\nThis partial application of the fix-up I suggested in\nhttps://lore.kernel.org/git/220120.86mtjqks1b.gmgdl@evledraar.gmail.com/\nleaves the now-unused \"blank-template\"\n\n>  - added attribution to Jose Lopes for finding and making the first\n>    draft of this patch (after confirming with them)\n>\n> Ævar mentioned \"git sparse-checkout add\" but I think that that is a\n> different problem - in the \"git sparse-checkout init\" case, we could get\n> into this case with a template that does not have .git/info, but in the\n> \"git sparse-checkout add\" case, the user would have had to explicitly\n> remove the info directory. So I'll limit this patch to the \"init\" case,\n> for now.\n> ---\n>  builtin/sparse-checkout.c          | 3 +++\n>  t/t1091-sparse-checkout-builtin.sh | 6 ++++++\n>  2 files changed, 9 insertions(+)\n>\n> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\n> index 679c107036..2b0e1db2d2 100644\n> --- a/builtin/sparse-checkout.c\n> +++ b/builtin/sparse-checkout.c\n> @@ -471,6 +471,9 @@ static int sparse_checkout_init(int argc, const char **argv)\n>  \t\tFILE *fp;\n>  \n>  \t\t/* assume we are in a fresh repo, but update the sparse-checkout file */\n> +\t\tif (safe_create_leading_directories(sparse_filename))\n> +\t\t\tdie(_(\"unable to create leading directories of %s\"),\n> +\t\t\t    sparse_filename);\n>  \t\tfp = xfopen(sparse_filename, \"w\");\n>  \t\tif (!fp)\n>  \t\t\tdie(_(\"failed to open '%s'\"), sparse_filename);\n> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\n> index 42776984fe..3189d3da96 100755\n> --- a/t/t1091-sparse-checkout-builtin.sh\n> +++ b/t/t1091-sparse-checkout-builtin.sh\n> @@ -79,6 +79,12 @@ test_expect_success 'git sparse-checkout init' '\n>  \tcheck_files repo a\n>  '\n>  \n> +test_expect_success 'git sparse-checkout init in empty repo' '\n> +\ttest_when_finished rm -rf empty-repo blank-template &&\n> +\tgit init --template= empty-repo &&\n> +\tgit -C empty-repo sparse-checkout init\n> +'\n\nI agree that it's a slightly different problem, but I was just\nadvocating for us testing what happened in these cases.\n\nThe below fix-up does that. I think we should use warning_errno() there\ninstead of some specutalite \"file may not exist\", but with/without this\npatch these tests show that only the \"init\" case was broken.\n\nAs a more general issue I don't understand why \"add\" and \"init\" need to\nbe conceptually different operations. If what defines a sparse checkout\nis just that it has that file and the 2 default patterns, which unless\nI'm missing something is the case. Why isn't \"add\" merely an \"init\"\nthat'll optimistically add whatever pattern you asked for, in addition\nto doing an \"init\" if you didn't already?\n\nThen \"add\" and \"init\" will share the same error recovery behavior, and\nyou won't needlessly have to run \"init/add x\" just to start using\nsparse-checkout with a pattern of \"x\".\n\nBut I've never *actually* used this command, so maybe I'm just missing\nsomething obvious...\n\ndiff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\nindex 3189d3da965..6b56d9d177f 100755\n--- a/t/t1091-sparse-checkout-builtin.sh\n+++ b/t/t1091-sparse-checkout-builtin.sh\n@@ -80,11 +80,37 @@ test_expect_success 'git sparse-checkout init' '\n '\n \n test_expect_success 'git sparse-checkout init in empty repo' '\n-\ttest_when_finished rm -rf empty-repo blank-template &&\n+\ttest_when_finished rm -rf empty-repo &&\n \tgit init --template= empty-repo &&\n \tgit -C empty-repo sparse-checkout init\n '\n \n+test_expect_success 'git sparse-checkout add -- info/sparse-checkout missing' '\n+\ttest_when_finished \"rm -rf empty\" &&\n+\tgit init --template= empty &&\n+\tgit -C empty sparse-checkout init &&\n+\trm -rf empty/.git/info &&\n+\n+\tcat >expect <<-\\EOF &&\n+\tfatal: unable to load existing sparse-checkout patterns\n+\tEOF\n+\ttest_expect_code 128 git -C empty sparse-checkout add bar 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'git sparse-checkout list -- info/sparse-checkout missing' '\n+\ttest_when_finished \"rm -rf empty\" &&\n+\tgit init --template= empty &&\n+\tgit -C empty sparse-checkout init &&\n+\trm -rf empty/.git/info &&\n+\n+\tcat >expect <<-\\EOF &&\n+\twarning: this worktree is not sparse (sparse-checkout file may not exist)\n+\tEOF\n+\tgit -C empty sparse-checkout list 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'git sparse-checkout list after init' '\n \tgit -C repo sparse-checkout list >actual &&\n \tcat >expect <<-\\EOF &&\n"},{"id":"446674","messageId":"CABPp-BG9rxJsA9XwgUxpny5TDW4W7Z85PAkBdzKJXrSJxAim1g@mail.gmail.com","threadId":"57279","inReplyTo":"20220121174441.3991963-1-jonathantanmy@google.com","subject":"Re: [PATCH v2] sparse-checkout: create leading directory","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-01-22T02:33:18Z","receivedAt":"2022-01-22T02:33:33Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Jan 21, 2022 at 6:05 PM Jonathan Tan <jonathantanmy@google.com> wrote:\n>\n> When creating the sparse-checkout file, Git does not create the leading\n> directory, \"$GIT_DIR/info\", if it does not exist. This causes problems\n> if the repository does not have that directory. Therefore, ensure that\n> the leading directory is created.\n>\n> This is the only \"open\" in builtin/sparse-checkout.c that does not have\n> a leading directory check. (The other one in write_patterns_and_update()\n> does.)\n>\n> Note that the test needs to explicitly specify a template when running\n> \"git init\" because the default template used in the tests has the\n> \"info/\" directory included.\n\nIf wanted, you could avoid that by using `git worktree add ...`;\ngit-worktree will create a $GIT_DIR that does not contain an info\nsubdirectory.\n\n(No need to resubmit or anything, mostly I'm just mentioning another\nway folks might have triggered this issue.)\n\n> Helped-by: Jose Lopes <jabolopes@google.com>\n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n> Changes from v1:\n>  - made test simpler\n>  - added attribution to Jose Lopes for finding and making the first\n>    draft of this patch (after confirming with them)\n>\n> Ævar mentioned \"git sparse-checkout add\" but I think that that is a\n> different problem - in the \"git sparse-checkout init\" case, we could get\n> into this case with a template that does not have .git/info, but in the\n> \"git sparse-checkout add\" case, the user would have had to explicitly\n> remove the info directory. So I'll limit this patch to the \"init\" case,\n> for now.\n> ---\n>  builtin/sparse-checkout.c          | 3 +++\n>  t/t1091-sparse-checkout-builtin.sh | 6 ++++++\n>  2 files changed, 9 insertions(+)\n>\n> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\n> index 679c107036..2b0e1db2d2 100644\n> --- a/builtin/sparse-checkout.c\n> +++ b/builtin/sparse-checkout.c\n> @@ -471,6 +471,9 @@ static int sparse_checkout_init(int argc, const char **argv)\n>                 FILE *fp;\n>\n>                 /* assume we are in a fresh repo, but update the sparse-checkout file */\n> +               if (safe_create_leading_directories(sparse_filename))\n> +                       die(_(\"unable to create leading directories of %s\"),\n> +                           sparse_filename);\n>                 fp = xfopen(sparse_filename, \"w\");\n>                 if (!fp)\n>                         die(_(\"failed to open '%s'\"), sparse_filename);\n> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\n> index 42776984fe..3189d3da96 100755\n> --- a/t/t1091-sparse-checkout-builtin.sh\n> +++ b/t/t1091-sparse-checkout-builtin.sh\n> @@ -79,6 +79,12 @@ test_expect_success 'git sparse-checkout init' '\n>         check_files repo a\n>  '\n>\n> +test_expect_success 'git sparse-checkout init in empty repo' '\n> +       test_when_finished rm -rf empty-repo blank-template &&\n> +       git init --template= empty-repo &&\n> +       git -C empty-repo sparse-checkout init\n> +'\n> +\n>  test_expect_success 'git sparse-checkout list after init' '\n>         git -C repo sparse-checkout list >actual &&\n>         cat >expect <<-\\EOF &&\n> --\n> 2.35.0.rc0.227.g00780c9af4-goog\n\nPatch looks good to me:\n\nReviewed-by: Elijah Newren <newren@gmail.com>\n"},{"id":"446677","messageId":"CABPp-BHgRQ+qNvq19wsBsMCR+Cn7FS+0FMYXVyyzEAZTsg-wjA@mail.gmail.com","threadId":"57279","inReplyTo":"220121.8635lgkj17.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2] sparse-checkout: create leading directory","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-01-22T05:21:20Z","receivedAt":"2022-01-22T05:21:35Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Jan 21, 2022 at 6:12 PM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n>\n> On Fri, Jan 21 2022, Jonathan Tan wrote:\n>\n...\n> This partial application of the fix-up I suggested in\n> https://lore.kernel.org/git/220120.86mtjqks1b.gmgdl@evledraar.gmail.com/\n> leaves the now-unused \"blank-template\"\n\nGood point; no need to remove something that isn't being created anymore.\n\n>\n> >  - added attribution to Jose Lopes for finding and making the first\n> >    draft of this patch (after confirming with them)\n> >\n> > Ævar mentioned \"git sparse-checkout add\" but I think that that is a\n> > different problem - in the \"git sparse-checkout init\" case, we could get\n> > into this case with a template that does not have .git/info, but in the\n> > \"git sparse-checkout add\" case, the user would have had to explicitly\n> > remove the info directory. So I'll limit this patch to the \"init\" case,\n> > for now.\n...\n>\n> I agree that it's a slightly different problem, but I was just\n> advocating for us testing what happened in these cases.\n>\n> The below fix-up does that.\n\nDifferent problem...addressed with a \"fix-up\"?  Why would we squash\nextra testing of a different problem into the same commit?  I think\nit'd at least deserve its own commit message.\n\n> I think we should use warning_errno() there\n> instead of some specutalite \"file may not exist\", but with/without this\n> patch these tests show that only the \"init\" case was broken.\n>\n> As a more general issue I don't understand why \"add\" and \"init\" need to\n> be conceptually different operations. If what defines a sparse checkout\n> is just that it has that file and the 2 default patterns, which unless\n> I'm missing something is the case.\n>\n> Why isn't \"add\" merely an \"init\"\n> that'll optimistically add whatever pattern you asked for, in addition\n> to doing an \"init\" if you didn't already?\n>\n> Then \"add\" and \"init\" will share the same error recovery behavior, and\n> you won't needlessly have to run \"init/add x\" just to start using\n> sparse-checkout with a pattern of \"x\".\n\nThe high level idea you propose (\" 'add' can initialize sparsity state\nif not currently in a sparse checkout\") could make sense.  It isn't\njust a straightforward switchover with the various other config\nsettings and such, and would also necessitate adding additional\ncommand line flags to the 'add' subcommand, but it could be done.\nDidn't occur to me before; I'm not sure if such a change in UI would\nbe better or worse.  I'm inclined to leave it as it is, though,\nespecially since...\n\nThe low level idea you propose (sharing code down to error paths) does\nnot make sense in this particular case, for reasons that are rather\nnon-obvious.  In particular, we need to be careful to avoid sharing\nsome code with \"init\".  The \"init\" subcommand is deprecated as of\nba2f3f58ac (\"git-sparse-checkout.txt: update to document\ninit/set/reapply changes\", 2021-12-14).  I initially wanted to remove\nthe separate \"init\" codepath, and just forward \"git sparse-checkout\ninit\" calls over to the \"git sparse-checkout set\" codepath (the only\ndifference being that \"init\" always comes with an empty set of\ndirectories/patterns).  However, \"init\" had some problematic behavior\nthat I didn't want to copy to \"set\".  I also didn't want to kill that\nbehavior in \"init\" for backwards compatibility reasons.  And, this\nxfopen call was tied up in that tangle, which means that it will\ndefinitely remain separate, and thus needs to be fixed in isolation.\n\n> But I've never *actually* used this command, so maybe I'm just missing\n> something obvious...\n>\n> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\n> index 3189d3da965..6b56d9d177f 100755\n> --- a/t/t1091-sparse-checkout-builtin.sh\n> +++ b/t/t1091-sparse-checkout-builtin.sh\n> @@ -80,11 +80,37 @@ test_expect_success 'git sparse-checkout init' '\n>  '\n>\n>  test_expect_success 'git sparse-checkout init in empty repo' '\n> -       test_when_finished rm -rf empty-repo blank-template &&\n> +       test_when_finished rm -rf empty-repo &&\n\nThis hunk looks like a good fixup to Jonathan's patch.\n\n>         git init --template= empty-repo &&\n>         git -C empty-repo sparse-checkout init\n>  '\n>\n> +test_expect_success 'git sparse-checkout add -- info/sparse-checkout missing' '\n> +       test_when_finished \"rm -rf empty\" &&\n> +       git init --template= empty &&\n> +       git -C empty sparse-checkout init &&\n> +       rm -rf empty/.git/info &&\n> +\n> +       cat >expect <<-\\EOF &&\n> +       fatal: unable to load existing sparse-checkout patterns\n> +       EOF\n> +       test_expect_code 128 git -C empty sparse-checkout add bar 2>actual &&\n> +       test_cmp expect actual\n> +'\n> +\n> +test_expect_success 'git sparse-checkout list -- info/sparse-checkout missing' '\n> +       test_when_finished \"rm -rf empty\" &&\n> +       git init --template= empty &&\n> +       git -C empty sparse-checkout init &&\n> +       rm -rf empty/.git/info &&\n> +\n> +       cat >expect <<-\\EOF &&\n> +       warning: this worktree is not sparse (sparse-checkout file may not exist)\n> +       EOF\n> +       git -C empty sparse-checkout list 2>actual &&\n> +       test_cmp expect actual\n> +'\n> +\n\nSo...you're trying to test what happens when a user intentionally\nbricks their repository?\n\n(Note that `sparse-checkout init` sets core.sparseCheckout=true, as\nexplicitly documented.  core.sparseCheckout=true instructs git to pay\nattention to $GIT_DIR/info/sparse-checkout for every unpack_trees()\ncall that updates the working tree, which basically means nearly any\nsignificant Git operation involving a worktree update now needs that\nfile in order to function.  So, your commands told Git that this\ndirectory is mandatory, and then you nuked the directory.)\n\nNow, if you could find a testcase based on `git worktree add ...`\n(which doesn't create an \"info\" directory) and then triggers problems\nsomehow without the intentional bricking, then what you'd have would\nbe more in line with what Jonathan is addressing here, but as it\nstands it's hard to even call your testcases related.  There may be\nsome merit to testing deliberately broken repositories, but I'm just\nnot sure if that's what you really intended and were suggesting.  Was\nit?  Or am I just missing something here?\n"},{"id":"446680","messageId":"20220122075851.GA1718@szeder.dev","threadId":"57279","inReplyTo":"220120.86mtjqks1b.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] sparse-checkout: create leading directory","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-01-22T07:58:51Z","receivedAt":"2022-01-22T07:59:00Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Jan 20, 2022 at 10:30:03PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> > diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\n> > index 42776984fe..dba0737599 100755\n> > --- a/t/t1091-sparse-checkout-builtin.sh\n> > +++ b/t/t1091-sparse-checkout-builtin.sh\n> > @@ -79,6 +79,13 @@ test_expect_success 'git sparse-checkout init' '\n> >  \tcheck_files repo a\n> >  '\n> >  \n> > +test_expect_success 'git sparse-checkout init in empty repo' '\n> > +\ttest_when_finished rm -rf empty-repo blank-template &&\n> > +\tmkdir blank-template &&\n> > +\tgit init --template=blank-template empty-repo &&\n> > +\tgit -C empty-repo sparse-checkout init\n> > +'\n> > +\n> >  test_expect_success 'git sparse-checkout list after init' '\n> >  \tgit -C repo sparse-checkout list >actual &&\n> >  \tcat >expect <<-\\EOF &&\n> \n> You're using an overly verbose way to say \"no templates, please\". You\n> can squash this in, i.e. --template= is an explicitly supported way to\n> do that.\n\nIt would be even more expressive to use 'git init --no-template', but,\nalas, that doesn't work:\n\n  $ git init --no-template /tmp/foo\n  Initialized empty Git repository in /tmp/foo/.git/\n  $ ls /tmp/foo/.git\n  branches  config  description  HEAD  hooks  info  objects  refs\n\n"},{"id":"446691","messageId":"220122.86tudwht17.gmgdl@evledraar.gmail.com","threadId":"57279","inReplyTo":"CABPp-BHgRQ+qNvq19wsBsMCR+Cn7FS+0FMYXVyyzEAZTsg-wjA@mail.gmail.com","subject":"Re: [PATCH v2] sparse-checkout: create leading directory","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-01-22T12:06:02Z","receivedAt":"2022-01-22T12:08:41Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Jan 21 2022, Elijah Newren wrote:\n\n> On Fri, Jan 21, 2022 at 6:12 PM Ævar Arnfjörð Bjarmason\n> <avarab@gmail.com> wrote:\n>>\n>> On Fri, Jan 21 2022, Jonathan Tan wrote:\n>>\n> ...\n>> This partial application of the fix-up I suggested in\n>> https://lore.kernel.org/git/220120.86mtjqks1b.gmgdl@evledraar.gmail.com/\n>> leaves the now-unused \"blank-template\"\n>\n> Good point; no need to remove something that isn't being created anymore.\n>\n>>\n>> >  - added attribution to Jose Lopes for finding and making the first\n>> >    draft of this patch (after confirming with them)\n>> >\n>> > Ævar mentioned \"git sparse-checkout add\" but I think that that is a\n>> > different problem - in the \"git sparse-checkout init\" case, we could get\n>> > into this case with a template that does not have .git/info, but in the\n>> > \"git sparse-checkout add\" case, the user would have had to explicitly\n>> > remove the info directory. So I'll limit this patch to the \"init\" case,\n>> > for now.\n> ...\n>>\n>> I agree that it's a slightly different problem, but I was just\n>> advocating for us testing what happened in these cases.\n>>\n>> The below fix-up does that.\n>\n> Different problem...addressed with a \"fix-up\"?  Why would we squash\n> extra testing of a different problem into the same commit?  I think\n> it'd at least deserve its own commit message.\n\nSure, or split up, or with an amended commit message etc.\n\n>> I think we should use warning_errno() there\n>> instead of some specutalite \"file may not exist\", but with/without this\n>> patch these tests show that only the \"init\" case was broken.\n>>\n>> As a more general issue I don't understand why \"add\" and \"init\" need to\n>> be conceptually different operations. If what defines a sparse checkout\n>> is just that it has that file and the 2 default patterns, which unless\n>> I'm missing something is the case.\n>>\n>> Why isn't \"add\" merely an \"init\"\n>> that'll optimistically add whatever pattern you asked for, in addition\n>> to doing an \"init\" if you didn't already?\n>>\n>> Then \"add\" and \"init\" will share the same error recovery behavior, and\n>> you won't needlessly have to run \"init/add x\" just to start using\n>> sparse-checkout with a pattern of \"x\".\n>\n> The high level idea you propose (\" 'add' can initialize sparsity state\n> if not currently in a sparse checkout\") could make sense.  It isn't\n> just a straightforward switchover with the various other config\n> settings and such, and would also necessitate adding additional\n> command line flags to the 'add' subcommand, but it could be done.\n> Didn't occur to me before; I'm not sure if such a change in UI would\n> be better or worse.  I'm inclined to leave it as it is, though,\n> especially since...\n>\n> The low level idea you propose (sharing code down to error paths) does\n> not make sense in this particular case, for reasons that are rather\n> non-obvious.  In particular, we need to be careful to avoid sharing\n> some code with \"init\".  The \"init\" subcommand is deprecated as of\n> ba2f3f58ac (\"git-sparse-checkout.txt: update to document\n> init/set/reapply changes\", 2021-12-14).  I initially wanted to remove\n> the separate \"init\" codepath, and just forward \"git sparse-checkout\n> init\" calls over to the \"git sparse-checkout set\" codepath (the only\n> difference being that \"init\" always comes with an empty set of\n> directories/patterns).  However, \"init\" had some problematic behavior\n> that I didn't want to copy to \"set\".  I also didn't want to kill that\n> behavior in \"init\" for backwards compatibility reasons.  And, this\n> xfopen call was tied up in that tangle, which means that it will\n> definitely remain separate, and thus needs to be fixed in isolation.\n\n....\n\n>> But I've never *actually* used this command, so maybe I'm just missing\n>> something obvious...\n>>\n>> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\n>> index 3189d3da965..6b56d9d177f 100755\n>> --- a/t/t1091-sparse-checkout-builtin.sh\n>> +++ b/t/t1091-sparse-checkout-builtin.sh\n>> @@ -80,11 +80,37 @@ test_expect_success 'git sparse-checkout init' '\n>>  '\n>>\n>>  test_expect_success 'git sparse-checkout init in empty repo' '\n>> -       test_when_finished rm -rf empty-repo blank-template &&\n>> +       test_when_finished rm -rf empty-repo &&\n>\n> This hunk looks like a good fixup to Jonathan's patch.\n>\n>>         git init --template= empty-repo &&\n>>         git -C empty-repo sparse-checkout init\n>>  '\n>>\n>> +test_expect_success 'git sparse-checkout add -- info/sparse-checkout missing' '\n>> +       test_when_finished \"rm -rf empty\" &&\n>> +       git init --template= empty &&\n>> +       git -C empty sparse-checkout init &&\n>> +       rm -rf empty/.git/info &&\n>> +\n>> +       cat >expect <<-\\EOF &&\n>> +       fatal: unable to load existing sparse-checkout patterns\n>> +       EOF\n>> +       test_expect_code 128 git -C empty sparse-checkout add bar 2>actual &&\n>> +       test_cmp expect actual\n>> +'\n>> +\n>> +test_expect_success 'git sparse-checkout list -- info/sparse-checkout missing' '\n>> +       test_when_finished \"rm -rf empty\" &&\n>> +       git init --template= empty &&\n>> +       git -C empty sparse-checkout init &&\n>> +       rm -rf empty/.git/info &&\n>> +\n>> +       cat >expect <<-\\EOF &&\n>> +       warning: this worktree is not sparse (sparse-checkout file may not exist)\n>> +       EOF\n>> +       git -C empty sparse-checkout list 2>actual &&\n>> +       test_cmp expect actual\n>> +'\n>> +\n>\n> So...you're trying to test what happens when a user intentionally\n> bricks their repository?\n\nI'm just saying that it's cheap to add a regression test for this\nmissing bit of related coverage, so why not add it?\n\nWe need to deal with the real world, a repo might be in all sorts of odd\nstates, including because of a user mistake.\n\n> (Note that `sparse-checkout init` sets core.sparseCheckout=true, as\n> explicitly documented.  core.sparseCheckout=true instructs git to pay\n> attention to $GIT_DIR/info/sparse-checkout for every unpack_trees()\n> call that updates the working tree, which basically means nearly any\n> significant Git operation involving a worktree update now needs that\n> file in order to function.  So, your commands told Git that this\n> directory is mandatory, and then you nuked the directory.)\n\n*nod*. But in that case shouldn't the errors say that you've configured\ncore.sparseCheckout=true but you're missing XYZ file?\n\n> Now, if you could find a testcase based on `git worktree add ...`\n> (which doesn't create an \"info\" directory) and then triggers problems\n> somehow without the intentional bricking, then what you'd have would\n> be more in line with what Jonathan is addressing here, but as it\n> stands it's hard to even call your testcases related.  There may be\n> some merit to testing deliberately broken repositories, but I'm just\n> not sure if that's what you really intended and were suggesting.  Was\n> it?  Or am I just missing something here?\n\nDoesn't the worktree case just use the \"main\" info/*, e.g. for\ninfo/excludes (didn't have time to test it now).\n"},{"id":"446693","messageId":"CABPp-BG5WzgcEwxnjraEN-SEKxUUvz-wD68afjQd_sWFe6R=Vg@mail.gmail.com","threadId":"57279","inReplyTo":"220122.86tudwht17.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2] sparse-checkout: create leading directory","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-01-22T17:29:03Z","receivedAt":"2022-01-22T17:29:19Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sat, Jan 22, 2022 at 4:08 AM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n>\n> On Fri, Jan 21 2022, Elijah Newren wrote:\n>\n> > On Fri, Jan 21, 2022 at 6:12 PM Ævar Arnfjörð Bjarmason\n> > <avarab@gmail.com> wrote:\n> >>\n> >> On Fri, Jan 21 2022, Jonathan Tan wrote:\n> >>\n...\n> >> > Ævar mentioned \"git sparse-checkout add\" but I think that that is a\n> >> > different problem - in the \"git sparse-checkout init\" case, we could get\n> >> > into this case with a template that does not have .git/info, but in the\n> >> > \"git sparse-checkout add\" case, the user would have had to explicitly\n> >> > remove the info directory. So I'll limit this patch to the \"init\" case,\n> >> > for now.\n> > ...\n> >>\n> >> I agree that it's a slightly different problem, but I was just\n> >> advocating for us testing what happened in these cases.\n> >>\n> >> The below fix-up does that.\n> >\n> > Different problem...addressed with a \"fix-up\"?  Why would we squash\n> > extra testing of a different problem into the same commit?  I think\n> > it'd at least deserve its own commit message.\n>\n> Sure, or split up, or with an amended commit message etc.\n\nI think it's a totally different kind of thing and wouldn't belong in\nthe same commit even with an amended commit message.  I'm curious why\nwe're so far apart on this and whether I'm missing something.\n\n...\n> >> +test_expect_success 'git sparse-checkout add -- info/sparse-checkout missing' '\n> >> +       test_when_finished \"rm -rf empty\" &&\n> >> +       git init --template= empty &&\n> >> +       git -C empty sparse-checkout init &&\n> >> +       rm -rf empty/.git/info &&\n> >> +\n> >> +       cat >expect <<-\\EOF &&\n> >> +       fatal: unable to load existing sparse-checkout patterns\n> >> +       EOF\n> >> +       test_expect_code 128 git -C empty sparse-checkout add bar 2>actual &&\n> >> +       test_cmp expect actual\n> >> +'\n> >> +\n> >> +test_expect_success 'git sparse-checkout list -- info/sparse-checkout missing' '\n> >> +       test_when_finished \"rm -rf empty\" &&\n> >> +       git init --template= empty &&\n> >> +       git -C empty sparse-checkout init &&\n> >> +       rm -rf empty/.git/info &&\n> >> +\n> >> +       cat >expect <<-\\EOF &&\n> >> +       warning: this worktree is not sparse (sparse-checkout file may not exist)\n> >> +       EOF\n> >> +       git -C empty sparse-checkout list 2>actual &&\n> >> +       test_cmp expect actual\n> >> +'\n> >> +\n> >\n> > So...you're trying to test what happens when a user intentionally\n> > bricks their repository?\n>\n> I'm just saying that it's cheap to add a regression test for this\n> missing bit of related coverage, so why not add it?\n\nSo, this is a slightly different issue than I was getting at before,\nbut this feels slightly obstructionist to me.  Now, suggesting related\nimprovements and ideas sounds totally fine; we should point those out\n-- once.  But pushing it again as though it needs to be addressed as\npart of the submission just doesn't feel right.  Jonathan's patch\nfixes a real problem and feels complete to me.  There are always\nadditional things that could also be fixed or addressed for any patch\nor series.  Expecting folks submitting a series to address every \"next\nstep improvement along these lines\" that any reviewer can think of\nseems unfriendly, especially as it has a snowball effect.\n\nGranted, if there's a bug with the patch, or it doesn't fully solve\nthe stated problem, then it's a different situation, but I don't think\nthat's the case here.  (Well, modulo the leftover removing of\n\"blank-template\" which is a real issue you identified with the patch.)\n\nAnd I think I'd say the same thing even if I saw your tests as being\nmuch more closely related to what Jonathan was checking.\n\nThat's my $0.02 on \"why not?\".  The story totally changes if you want\nto submit these tests separate from Jonathan's series.  If that's the\nscenario, then I fully agree with you on \"it's cheap to add more test\ncoverage so why not include it?\"\n\nAnyway...back to my curiosity about why we're so far apart on the\nrelatedness of your tests...\n\n> We need to deal with the real world, a repo might be in all sorts of odd\n> states, including because of a user mistake.\n\nSo, Jonathan was fixing behavior seen when the user hadn't even made a\nmistake.  Opening up to all possible user mistakes seems to widening\nscope pretty dramatically and feels like a different kind of thing to\nme.  But even that scope doesn't seem to include the tests you've\nproposed, at least not to me.  Under what circumstances would your\ntests model a user mistake?  A user mistake to me looks like one of\nthe following:\n\n  * \"I hit Ctrl+C while a `git switch` or `git sparse-checkout set\n...`  just happened to be furiously writing files\"\n  * \"I ran `git sparse-checkout (add|list|reapply)` without first\nrunning `git sparse-checkout (init|set)` as per the docs\"\n\nYour tests look roughly the same class as the following kinds of things:\n\n  * echo garbage >.git/refs/heads/master\n  * rm .git/objects/${random_loose_object_or_pack}\n\nI know users can attempt surgery on $GIT_DIR, and that perhaps curious\nones will do that to see how things break in order to help them\nunderstand how things work, but that seems to me to be a different\nrealm.\n\nNote that I'm not saying we shouldn't test what happens when the repo\nis intentionally corrupted; there's probably merit in that.  I'm just\ncurious why we're so far apart on this.  You view your tests as a\n\"slightly different problem\" and feel these tests could be included in\nJonathan's commit with an \"amended commit message\".  I think they're\nnot only different classes of problems, but separated by a third class\n(\"user mistake\") between the two types of problems.  If Jonathan had\nincluded your tests in his commit, I think I'd ask that they at\n_least_ be split into different commits.\n\nAm I missing the boat entirely somewhere?\n\n> > (Note that `sparse-checkout init` sets core.sparseCheckout=true, as\n> > explicitly documented.  core.sparseCheckout=true instructs git to pay\n> > attention to $GIT_DIR/info/sparse-checkout for every unpack_trees()\n> > call that updates the working tree, which basically means nearly any\n> > significant Git operation involving a worktree update now needs that\n> > file in order to function.  So, your commands told Git that this\n> > directory is mandatory, and then you nuked the directory.)\n>\n> *nod*. But in that case shouldn't the errors say that you've configured\n> core.sparseCheckout=true but you're missing XYZ file?\n\nYeah, it probably should.\n\nI just did a little more testing and it looks like commands like\n\"switch\" don't even error out; they just treat the missing\n$GIT_DIR/info/sparse-checkout files the same as all files being\nincluded.  Weird.  It seems to come from dir.c:add_patterns(), which\nappears to have perhaps gotten that way due to assuming the code was\nexclusively about .gitignore files rather than the sparse-checkout\nfile.  I think this may be yet another example of why it was a mistake\nto use gitignore-style patterns for the sparse-checkout feature,\nthough I'm more than a decade too late with my complaints.\n\nUgh.\n\nBut yeah, you're right to suggest we should have tests for this, and\nperhaps some fixes as well...in a separate submission from this one.\n;-)\n\n> > Now, if you could find a testcase based on `git worktree add ...`\n> > (which doesn't create an \"info\" directory) and then triggers problems\n> > somehow without the intentional bricking, then what you'd have would\n> > be more in line with what Jonathan is addressing here, but as it\n> > stands it's hard to even call your testcases related.  There may be\n> > some merit to testing deliberately broken repositories, but I'm just\n> > not sure if that's what you really intended and were suggesting.  Was\n> > it?  Or am I just missing something here?\n>\n> Doesn't the worktree case just use the \"main\" info/*, e.g. for\n> info/excludes (didn't have time to test it now).\n\nMaybe for info/excludes, I don't know.  But it certainly won't use the\nmain info/sparse-checkout; that'd be _horribly_ broken.  Each worktree\nshould be allowed to have its own sparsity rules.  And having multiple\nworktrees where one worktree is not sparse, while others are sparse\nbut with different sparsity patterns from each other (often only\nslightly different, but sometimes completely unrelated) is actually a\ncommon usecase.  We do that a lot at $DAYJOB.\n"},{"id":"446747","messageId":"220124.86zgnlgyjr.gmgdl@evledraar.gmail.com","threadId":"57279","inReplyTo":"CABPp-BG5WzgcEwxnjraEN-SEKxUUvz-wD68afjQd_sWFe6R=Vg@mail.gmail.com","subject":"Re: [PATCH v2] sparse-checkout: create leading directory","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-01-24T11:17:38Z","receivedAt":"2022-01-24T11:31:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Jan 22 2022, Elijah Newren wrote:\n\n> On Sat, Jan 22, 2022 at 4:08 AM Ævar Arnfjörð Bjarmason\n> <avarab@gmail.com> wrote:\n>>\n>> On Fri, Jan 21 2022, Elijah Newren wrote:\n>>\n>> > On Fri, Jan 21, 2022 at 6:12 PM Ævar Arnfjörð Bjarmason\n>> > <avarab@gmail.com> wrote:\n>> >>\n>> >> On Fri, Jan 21 2022, Jonathan Tan wrote:\n>> >>\n> ...\n>> >> > Ævar mentioned \"git sparse-checkout add\" but I think that that is a\n>> >> > different problem - in the \"git sparse-checkout init\" case, we could get\n>> >> > into this case with a template that does not have .git/info, but in the\n>> >> > \"git sparse-checkout add\" case, the user would have had to explicitly\n>> >> > remove the info directory. So I'll limit this patch to the \"init\" case,\n>> >> > for now.\n>> > ...\n>> >>\n>> >> I agree that it's a slightly different problem, but I was just\n>> >> advocating for us testing what happened in these cases.\n>> >>\n>> >> The below fix-up does that.\n>> >\n>> > Different problem...addressed with a \"fix-up\"?  Why would we squash\n>> > extra testing of a different problem into the same commit?  I think\n>> > it'd at least deserve its own commit message.\n>>\n>> Sure, or split up, or with an amended commit message etc.\n>\n> I think it's a totally different kind of thing and wouldn't belong in\n> the same commit even with an amended commit message.  I'm curious why\n> we're so far apart on this and whether I'm missing something.\n\nI think I'm biased by my initial look at this problem[1], which was to look\nat the various \"sparse_filename\" callers and their non-coverage. In my\nmind fixing & testing for that general problem would make for a nice\natomic change.\n\n> ...\n>> >> +test_expect_success 'git sparse-checkout add -- info/sparse-checkout missing' '\n>> >> +       test_when_finished \"rm -rf empty\" &&\n>> >> +       git init --template= empty &&\n>> >> +       git -C empty sparse-checkout init &&\n>> >> +       rm -rf empty/.git/info &&\n>> >> +\n>> >> +       cat >expect <<-\\EOF &&\n>> >> +       fatal: unable to load existing sparse-checkout patterns\n>> >> +       EOF\n>> >> +       test_expect_code 128 git -C empty sparse-checkout add bar 2>actual &&\n>> >> +       test_cmp expect actual\n>> >> +'\n>> >> +\n>> >> +test_expect_success 'git sparse-checkout list -- info/sparse-checkout missing' '\n>> >> +       test_when_finished \"rm -rf empty\" &&\n>> >> +       git init --template= empty &&\n>> >> +       git -C empty sparse-checkout init &&\n>> >> +       rm -rf empty/.git/info &&\n>> >> +\n>> >> +       cat >expect <<-\\EOF &&\n>> >> +       warning: this worktree is not sparse (sparse-checkout file may not exist)\n>> >> +       EOF\n>> >> +       git -C empty sparse-checkout list 2>actual &&\n>> >> +       test_cmp expect actual\n>> >> +'\n>> >> +\n>> >\n>> > So...you're trying to test what happens when a user intentionally\n>> > bricks their repository?\n>>\n>> I'm just saying that it's cheap to add a regression test for this\n>> missing bit of related coverage, so why not add it?\n>\n> So, this is a slightly different issue than I was getting at before,\n> but this feels slightly obstructionist to me.  Now, suggesting related\n> improvements and ideas sounds totally fine; we should point those out\n> -- once.  But pushing it again as though it needs to be addressed as\n> part of the submission just doesn't feel right.  Jonathan's patch\n> fixes a real problem and feels complete to me.  There are always\n> additional things that could also be fixed or addressed for any patch\n> or series.  Expecting folks submitting a series to address every \"next\n> step improvement along these lines\" that any reviewer can think of\n> seems unfriendly, especially as it has a snowball effect.\n>\n> Granted, if there's a bug with the patch, or it doesn't fully solve\n> the stated problem, then it's a different situation, but I don't think\n> that's the case here.  (Well, modulo the leftover removing of\n> \"blank-template\" which is a real issue you identified with the patch.)\n\nThe first thing I said in this thread is \"Thanks. This fix looks good to\nme.\". I'd be happy to have just this fix in. This patch resolves a\nblocked of an earlier series of mine.\n\nThe rest of the feedback here (aside from the trivial \"rm -rf\" fix) was\nan attempt to bridge the gap between this & my earlier look in [1].\n\n> And I think I'd say the same thing even if I saw your tests as being\n> much more closely related to what Jonathan was checking.\n>\n> That's my $0.02 on \"why not?\".  The story totally changes if you want\n> to submit these tests separate from Jonathan's series.  If that's the\n> scenario, then I fully agree with you on \"it's cheap to add more test\n> coverage so why not include it?\"\n\nSure, or maybe he'd be interested, or not. I'd rather try to suggest\nsome small proposed changes than submit a patch of my own as an initial\napproach.\n\n> Anyway...back to my curiosity about why we're so far apart on the\n> relatedness of your tests...\n>\n>> We need to deal with the real world, a repo might be in all sorts of odd\n>> states, including because of a user mistake.\n>\n> So, Jonathan was fixing behavior seen when the user hadn't even made a\n> mistake.  Opening up to all possible user mistakes seems to widening\n> scope pretty dramatically and feels like a different kind of thing to\n> me.  But even that scope doesn't seem to include the tests you've\n> proposed, at least not to me.  Under what circumstances would your\n> tests model a user mistake?  A user mistake to me looks like one of\n> the following:\n>\n>   * \"I hit Ctrl+C while a `git switch` or `git sparse-checkout set\n> ...`  just happened to be furiously writing files\"\n>   * \"I ran `git sparse-checkout (add|list|reapply)` without first\n> running `git sparse-checkout (init|set)` as per the docs\"\n>\n> Your tests look roughly the same class as the following kinds of things:\n>\n>   * echo garbage >.git/refs/heads/master\n>   * rm .git/objects/${random_loose_object_or_pack}\n>\n> I know users can attempt surgery on $GIT_DIR, and that perhaps curious\n> ones will do that to see how things break in order to help them\n> understand how things work, but that seems to me to be a different\n> realm.\n>\n> Note that I'm not saying we shouldn't test what happens when the repo\n> is intentionally corrupted; there's probably merit in that.  I'm just\n> curious why we're so far apart on this.  You view your tests as a\n> \"slightly different problem\" and feel these tests could be included in\n> Jonathan's commit with an \"amended commit message\".  I think they're\n> not only different classes of problems, but separated by a third class\n> (\"user mistake\") between the two types of problems.  If Jonathan had\n> included your tests in his commit, I think I'd ask that they at\n> _least_ be split into different commits.\n>\n> Am I missing the boat entirely somewhere?\n\nI was using \"fix-up\" rather loosely upthread. FWIW I never meant that\nthis needed to be in the same commit, but was going for \"this solves\nsome test blind spots in this adjacent area in nearby functions\", or\nsomething like that.\n\nI'm all for splitting extra tests into another commit or whatever,\nhowever that eventually lands in tree.\n\n[...]\n\n>> > (Note that `sparse-checkout init` sets core.sparseCheckout=true, as\n>> > explicitly documented.  core.sparseCheckout=true instructs git to pay\n>> > attention to $GIT_DIR/info/sparse-checkout for every unpack_trees()\n>> > call that updates the working tree, which basically means nearly any\n>> > significant Git operation involving a worktree update now needs that\n>> > file in order to function.  So, your commands told Git that this\n>> > directory is mandatory, and then you nuked the directory.)\n>>\n>> *nod*. But in that case shouldn't the errors say that you've configured\n>> core.sparseCheckout=true but you're missing XYZ file?\n>\n> Yeah, it probably should.\n>\n> I just did a little more testing and it looks like commands like\n> \"switch\" don't even error out; they just treat the missing\n> $GIT_DIR/info/sparse-checkout files the same as all files being\n> included.  Weird.  It seems to come from dir.c:add_patterns(), which\n> appears to have perhaps gotten that way due to assuming the code was\n> exclusively about .gitignore files rather than the sparse-checkout\n> file.  I think this may be yet another example of why it was a mistake\n> to use gitignore-style patterns for the sparse-checkout feature,\n> though I'm more than a decade too late with my complaints.\n>\n> Ugh.\n>\n> But yeah, you're right to suggest we should have tests for this, and\n> perhaps some fixes as well...in a separate submission from this one.\n> ;-)\n\nFWIW I don't think it's all that important to focus too much on how\nusers would get into this scenario. Sure, for the \"init\" case it's a\nthing that's broken with --template=, so that's more urgent.\n\nBut for the rest we're already carrying code for those edge cases (errno\nhandling and all), we just don't test for it.\n\nSo just adding tests seems prudent.\n\n>> > Now, if you could find a testcase based on `git worktree add ...`\n>> > (which doesn't create an \"info\" directory) and then triggers problems\n>> > somehow without the intentional bricking, then what you'd have would\n>> > be more in line with what Jonathan is addressing here, but as it\n>> > stands it's hard to even call your testcases related.  There may be\n>> > some merit to testing deliberately broken repositories, but I'm just\n>> > not sure if that's what you really intended and were suggesting.  Was\n>> > it?  Or am I just missing something here?\n>>\n>> Doesn't the worktree case just use the \"main\" info/*, e.g. for\n>> info/excludes (didn't have time to test it now).\n>\n> Maybe for info/excludes, I don't know.  But it certainly won't use the\n> main info/sparse-checkout; that'd be _horribly_ broken.  Each worktree\n> should be allowed to have its own sparsity rules.  And having multiple\n> worktrees where one worktree is not sparse, while others are sparse\n> but with different sparsity patterns from each other (often only\n> slightly different, but sometimes completely unrelated) is actually a\n> common usecase.  We do that a lot at $DAYJOB.\n\n1. https://lore.kernel.org/git/211220.86zgovt9bi.gmgdl@evledraar.gmail.com/\n\nFor what it's worth I rebased my earlier --no-template series locally on\nthis, and tried removing the creation of the \"info\" dir under\n--no-template.\n\nThe below diff shows the fixes that were needed for info/sparse-checkout\nas a result. Now I'm *not* saying this needs to be in this series or\nwhatever, it's just an FYI sanity check, i.e. this didn't reveal any\nremaining cases where \"git sparse-checkout\" in whatever mode broke\nunexpectedly without a .git/info.\n\nThese cases are just ones where we're manually setting up the sparse\ncheckout. Of course we may have missing coverage etc., but I think this\nis one more datapoint in favor of this proposed change being a good one.\n\ndiff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh\nindex 24092c09a95..822d753bd95 100755\n--- a/t/t1011-read-tree-sparse-checkout.sh\n+++ b/t/t1011-read-tree-sparse-checkout.sh\n@@ -53,6 +53,7 @@ test_expect_success 'read-tree without .git/info/sparse-checkout' '\n '\n \n test_expect_success 'read-tree with .git/info/sparse-checkout but disabled' '\n+\tmkdir .git/info &&\n \techo >.git/info/sparse-checkout &&\n \tread_tree_u_must_succeed -m -u HEAD &&\n \tgit ls-files -t >result &&\ndiff --git a/t/t1090-sparse-checkout-scope.sh b/t/t1090-sparse-checkout-scope.sh\nindex 3deb4901874..669b114db03 100755\n--- a/t/t1090-sparse-checkout-scope.sh\n+++ b/t/t1090-sparse-checkout-scope.sh\n@@ -25,6 +25,7 @@ test_expect_success 'create feature branch' '\n \n test_expect_success 'perform sparse checkout of main' '\n \tgit config --local --bool core.sparsecheckout true &&\n+\tmkdir .git/info &&\n \techo \"!/*\" >.git/info/sparse-checkout &&\n \techo \"/a\" >>.git/info/sparse-checkout &&\n \techo \"/c\" >>.git/info/sparse-checkout &&\n@@ -66,6 +67,7 @@ test_expect_success 'in partial clone, sparse checkout only fetches needed blobs\n \tgit -C server commit -m message &&\n \n \ttest_config -C client core.sparsecheckout 1 &&\n+\tmkdir client/.git/info &&\n \techo \"!/*\" >client/.git/info/sparse-checkout &&\n \techo \"/a\" >>client/.git/info/sparse-checkout &&\n \tgit -C client fetch --filter=blob:none origin &&\ndiff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\nindex 3e93506c045..e1d9d0d3c7a 100755\n--- a/t/t2018-checkout-branch.sh\n+++ b/t/t2018-checkout-branch.sh\n@@ -249,6 +249,7 @@ test_expect_success 'checkout -b to a new branch preserves mergeable changes des\n \ttest_commit file2 &&\n \n \techo stuff >>file1 &&\n+\tmkdir .git/info &&\n \techo file2 >.git/info/sparse-checkout &&\n \tgit config core.sparseCheckout true &&\n \ndiff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\nindex 979e843c65a..dbd37147c63 100755\n--- a/t/t3507-cherry-pick-conflict.sh\n+++ b/t/t3507-cherry-pick-conflict.sh\n@@ -558,6 +558,7 @@ test_expect_success 'cherry-pick preserves sparse-checkout' '\n \t\techo \\\"/*\\\" >.git/info/sparse-checkout\n \t\tgit read-tree --reset -u HEAD\n \t\trm .git/info/sparse-checkout\" &&\n+\tmkdir .git/info &&\n \techo /unrelated >.git/info/sparse-checkout &&\n \tgit read-tree --reset -u HEAD &&\n \ttest_must_fail git cherry-pick -Xours picked>actual &&\ndiff --git a/t/t6435-merge-sparse.sh b/t/t6435-merge-sparse.sh\nindex 74562e12356..6fd0bdf38ac 100755\n--- a/t/t6435-merge-sparse.sh\n+++ b/t/t6435-merge-sparse.sh\n@@ -26,6 +26,7 @@ test_expect_success 'setup' '\n \tgit rm modify_delete &&\n \ttest_commit_this ours &&\n \tgit config core.sparseCheckout true &&\n+\tmkdir .git/info &&\n \techo \"/checked-out\" >.git/info/sparse-checkout &&\n \tgit reset --hard &&\n \ttest_must_fail git merge theirs\ndiff --git a/t/t7418-submodule-sparse-gitmodules.sh b/t/t7418-submodule-sparse-gitmodules.sh\nindex f87e524d6d4..5cc0b7e700f 100755\n--- a/t/t7418-submodule-sparse-gitmodules.sh\n+++ b/t/t7418-submodule-sparse-gitmodules.sh\n@@ -33,6 +33,7 @@ test_expect_success 'sparse checkout setup which hides .gitmodules' '\n \t) &&\n \tgit clone upstream super &&\n \t(cd super &&\n+\t\tmkdir .git/info &&\n \t\tcat >.git/info/sparse-checkout <<-\\EOF &&\n \t\t/*\n \t\t!/.gitmodules\n"},{"id":"446768","messageId":"20220124182231.2439029-1-jonathantanmy@google.com","threadId":"57279","inReplyTo":"CABPp-BG9rxJsA9XwgUxpny5TDW4W7Z85PAkBdzKJXrSJxAim1g@mail.gmail.com","subject":"Re: [PATCH v2] sparse-checkout: create leading directory","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-01-24T18:22:31Z","receivedAt":"2022-01-24T18:22:35Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Elijah Newren <newren@gmail.com> writes:\n> On Fri, Jan 21, 2022 at 6:05 PM Jonathan Tan <jonathantanmy@google.com> wro=\n> te:\n> >\n> > When creating the sparse-checkout file, Git does not create the leading\n> > directory, \"$GIT_DIR/info\", if it does not exist. This causes problems\n> > if the repository does not have that directory. Therefore, ensure that\n> > the leading directory is created.\n> >\n> > This is the only \"open\" in builtin/sparse-checkout.c that does not have\n> > a leading directory check. (The other one in write_patterns_and_update()\n> > does.)\n> >\n> > Note that the test needs to explicitly specify a template when running\n> > \"git init\" because the default template used in the tests has the\n> > \"info/\" directory included.\n> \n> If wanted, you could avoid that by using `git worktree add ...`;\n> git-worktree will create a $GIT_DIR that does not contain an info\n> subdirectory.\n> \n> (No need to resubmit or anything, mostly I'm just mentioning another\n> way folks might have triggered this issue.)\n\nAh, thanks for the information.\n\n> Patch looks good to me:\n> \n> Reviewed-by: Elijah Newren <newren@gmail.com>\n\nThanks for taking a look!\n"},{"id":"446769","messageId":"20220124182500.2439823-1-jonathantanmy@google.com","threadId":"57279","inReplyTo":"220124.86zgnlgyjr.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2] sparse-checkout: create leading directory","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-01-24T18:25:00Z","receivedAt":"2022-01-24T18:25:13Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n> The first thing I said in this thread is \"Thanks. This fix looks good to\n> me.\". I'd be happy to have just this fix in. This patch resolves a\n> blocked of an earlier series of mine.\n> \n> The rest of the feedback here (aside from the trivial \"rm -rf\" fix) was\n> an attempt to bridge the gap between this & my earlier look in [1].\n> \n> > And I think I'd say the same thing even if I saw your tests as being\n> > much more closely related to what Jonathan was checking.\n> >\n> > That's my $0.02 on \"why not?\".  The story totally changes if you want\n> > to submit these tests separate from Jonathan's series.  If that's the\n> > scenario, then I fully agree with you on \"it's cheap to add more test\n> > coverage so why not include it?\"\n> \n> Sure, or maybe he'd be interested, or not. I'd rather try to suggest\n> some small proposed changes than submit a patch of my own as an initial\n> approach.\n\nIf my patch is OK going in on its own, I'd rather do that, and leave\nadditional changes to other patch sets.\n"},{"id":"446830","messageId":"CABPp-BFdv6HsD+YebquoCBNp7KBoZ8i-aVjo9aisx9Qp68VWjQ@mail.gmail.com","threadId":"57279","inReplyTo":"220124.86zgnlgyjr.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2] sparse-checkout: create leading directory","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-01-25T05:37:59Z","receivedAt":"2022-01-25T07:03:48Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Jan 24, 2022 at 3:31 AM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n>\n> On Sat, Jan 22 2022, Elijah Newren wrote:\n>\n> > On Sat, Jan 22, 2022 at 4:08 AM Ævar Arnfjörð Bjarmason\n> > <avarab@gmail.com> wrote:\n> >>\n> >> On Fri, Jan 21 2022, Elijah Newren wrote:\n> >>\n> >> > On Fri, Jan 21, 2022 at 6:12 PM Ævar Arnfjörð Bjarmason\n> >> > <avarab@gmail.com> wrote:\n> >> >>\n...\n> > I think it's a totally different kind of thing and wouldn't belong in\n> > the same commit even with an amended commit message.  I'm curious why\n> > we're so far apart on this and whether I'm missing something.\n>\n> I think I'm biased by my initial look at this problem[1], which was to look\n> at the various \"sparse_filename\" callers and their non-coverage. In my\n> mind fixing & testing for that general problem would make for a nice\n> atomic change.\n\nAh, ok, that link and your other comments below help; I think I'm\nstarting to see now.  Sorry for being slow.  I was focusing\n(pigeonholing?) on the likelihood and feasibility of a user entering\ncertain states, you were doing whatever was necessary to force the\ncode into certain states and then testing them.  The forcing into\ncertain states was tripping me up.  (Perhaps if we had a unit test\nframework instead of only a regression test framework, then you could\nhave suggested multiple unit tests and I wouldn't have been so\nconfused.)\n\nThanks for explaining.\n\n....\n> >> I'm just saying that it's cheap to add a regression test for this\n> >> missing bit of related coverage, so why not add it?\n> >\n> > So, this is a slightly different issue than I was getting at before,\n> > but this feels slightly obstructionist to me.  Now, suggesting related\n> > improvements and ideas sounds totally fine; we should point those out\n> > -- once.  But pushing it again as though it needs to be addressed as\n> > part of the submission just doesn't feel right.  Jonathan's patch\n> > fixes a real problem and feels complete to me.  There are always\n> > additional things that could also be fixed or addressed for any patch\n> > or series.  Expecting folks submitting a series to address every \"next\n> > step improvement along these lines\" that any reviewer can think of\n> > seems unfriendly, especially as it has a snowball effect.\n> >\n> > Granted, if there's a bug with the patch, or it doesn't fully solve\n> > the stated problem, then it's a different situation, but I don't think\n> > that's the case here.  (Well, modulo the leftover removing of\n> > \"blank-template\" which is a real issue you identified with the patch.)\n>\n> The first thing I said in this thread is \"Thanks. This fix looks good to\n> me.\". I'd be happy to have just this fix in. This patch resolves a\n> blocked of an earlier series of mine.\n>\n> The rest of the feedback here (aside from the trivial \"rm -rf\" fix) was\n> an attempt to bridge the gap between this & my earlier look in [1].\n\nIt helps to know your intent.  The \"I'd be happy to have just this fix\nin\" doesn't appear in your emails until here; to me, you communicated\nthe opposite by not only bringing up the additional changes but also\nfollowing up to the next email by also including a patch when you saw\nthey weren't included.\n\nAnyway, I think we're good here; we both agree Jonathan's patch\ndoesn't need the additions, and you've patiently explained to me what\nI was missing about your view on the tests.  Sorry for being slow on\nthat.\n\nOne other side point, though, since it came up in this thread...\n\n> >> > (Note that `sparse-checkout init` sets core.sparseCheckout=true, as\n> >> > explicitly documented.  core.sparseCheckout=true instructs git to pay\n> >> > attention to $GIT_DIR/info/sparse-checkout for every unpack_trees()\n> >> > call that updates the working tree, which basically means nearly any\n> >> > significant Git operation involving a worktree update now needs that\n> >> > file in order to function.  So, your commands told Git that this\n> >> > directory is mandatory, and then you nuked the directory.)\n> >>\n> >> *nod*. But in that case shouldn't the errors say that you've configured\n> >> core.sparseCheckout=true but you're missing XYZ file?\n> >\n> > Yeah, it probably should.\n> >\n> > I just did a little more testing and it looks like commands like\n> > \"switch\" don't even error out; they just treat the missing\n> > $GIT_DIR/info/sparse-checkout files the same as all files being\n> > included.  Weird.  It seems to come from dir.c:add_patterns(), which\n> > appears to have perhaps gotten that way due to assuming the code was\n> > exclusively about .gitignore files rather than the sparse-checkout\n> > file.\n...\n> FWIW I don't think it's all that important to focus too much on how\n> users would get into this scenario. Sure, for the \"init\" case it's a\n> thing that's broken with --template=, so that's more urgent.\n\nJust to check, \"this scenario\" means core.sparseCheckout=true with\n$GIT_DIR/info missing (and thus $GIT_DIR/info/sparse-checkout\nmissing), right?\n\n> But for the rest we're already carrying code for those edge cases (errno\n> handling and all), we just don't test for it.\n>\n> So just adding tests seems prudent.\n\nOkay, but if we're ignoring how users get into this scenario, then\nthere are more affected code paths than just the sparse-checkout\nsubcommand.  And it appears that at least some of those do not behave\nwell (they do weird-ish things and pretend success instead of showing\na reasonable error message, or even any error message).  So there's\nprobably some work involved to identify the relevant subcommands or\ncodepaths, so that we even know what to add tests for.\n"}]}