{"thread":{"id":"62177","subject":"[PATCH] sparse-checkout: disable advice in 'disable'","startedAt":"2024-09-23T19:31:26Z","lastAt":"2024-09-25T16:44:23Z","messageCount":4,"participants":["Derrick Stolee via GitGitGadget","Junio C Hamano","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"503282","messageId":"pull.1800.git.1727119882901.gitgitgadget@gmail.com","threadId":"62177","inReplyTo":null,"subject":"[PATCH] sparse-checkout: disable advice in 'disable'","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-09-23T19:31:22Z","receivedAt":"2024-09-23T19:31:26Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <stolee@gmail.com>\n\nWhen running 'git sparse-checkout disable' with the sparse index\nenabled, Git is expected to expand the index into a full index. However,\nit currently outputs the advice message saying that that is unexpected\nand likely due to an issue with the working directory.\n\nDisable this advice message when in this code path. Establish a pattern\nfor doing a similar removal in the future.\n\nSigned-off-by: Derrick Stolee <stolee@gmail.com>\n---\n    sparse-checkout: disable advice in 'disable'\n    \n    I found this while someone was demoing some behavior that included\n    disabling the sparse-checkout feature. Having this message pop up during\n    that interaction was embarassing.\n    \n    Thanks, -Stolee\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1800%2Fderrickstolee%2Fsparse-checkout-unset-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1800/derrickstolee/sparse-checkout-unset-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1800\n\n builtin/sparse-checkout.c                | 5 +++++\n sparse-index.c                           | 5 +++--\n sparse-index.h                           | 7 +++++++\n t/t1092-sparse-checkout-compatibility.sh | 5 ++++-\n 4 files changed, 19 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex 5ccf6968628..85b4fc06b35 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -924,6 +924,11 @@ static int sparse_checkout_disable(int argc, const char **argv,\n \t\t\t     builtin_sparse_checkout_disable_options,\n \t\t\t     builtin_sparse_checkout_disable_usage, 0);\n \n+\t/*\n+\t * Disable the advice message for expanding a sparse index, as we\n+\t * are expecting to do that when disabling sparse-checkout.\n+\t */\n+\tgive_advice_on_expansion = 0;\n \trepo_read_index(the_repository);\n \n \tmemset(&pl, 0, sizeof(pl));\ndiff --git a/sparse-index.c b/sparse-index.c\nindex 9958656ded1..0f9fb4df026 100644\n--- a/sparse-index.c\n+++ b/sparse-index.c\n@@ -19,9 +19,10 @@\n  * advice for advice.sparseIndexExpanded when expanding a sparse index to a full\n  * one. However, this is sometimes done on purpose, such as in the sparse-checkout\n  * builtin, even when index.sparse=false. This may be disabled in\n- * convert_to_sparse().\n+ * convert_to_sparse() or by commands that know they will lead to a full\n+ * expansion, but this message is not actionable.\n  */\n-static int give_advice_on_expansion = 1;\n+int give_advice_on_expansion = 1;\n #define ADVICE_MSG \\\n \t\"The sparse index is expanding to a full index, a slow operation.\\n\"   \\\n \t\"Your working directory likely has contents that are outside of\\n\"     \\\ndiff --git a/sparse-index.h b/sparse-index.h\nindex a16f3e67d75..727034be7ca 100644\n--- a/sparse-index.h\n+++ b/sparse-index.h\n@@ -1,6 +1,13 @@\n #ifndef SPARSE_INDEX_H__\n #define SPARSE_INDEX_H__\n \n+/*\n+ * If performing an operation where the index is supposed to expand to a\n+ * full index, then disable the advice message by setting this global to\n+ * zero.\n+ */\n+extern int give_advice_on_expansion;\n+\n struct index_state;\n #define SPARSE_INDEX_MEMORY_ONLY (1 << 0)\n int is_sparse_index_allowed(struct index_state *istate, int flags);\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex eb32da2a7f2..6e230b54876 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -2355,7 +2355,10 @@ test_expect_success 'advice.sparseIndexExpanded' '\n \tmkdir -p sparse-index/deep/deeper2/deepest &&\n \ttouch sparse-index/deep/deeper2/deepest/bogus &&\n \tgit -C sparse-index status 2>err &&\n-\tgrep \"The sparse index is expanding to a full index\" err\n+\tgrep \"The sparse index is expanding to a full index\" err &&\n+\n+\tgit -C sparse-index sparse-checkout disable 2>err &&\n+\ttest_line_count = 0 err\n '\n \n test_expect_success 'cat-file -p' '\n\nbase-commit: 94b60adee30619a05296cf5ed6addb0e6d4e25dc\n-- \ngitgitgadget\n"},{"id":"503287","messageId":"xmqqr09ayv2d.fsf@gitster.g","threadId":"62177","inReplyTo":"pull.1800.git.1727119882901.gitgitgadget@gmail.com","subject":"Re: [PATCH] sparse-checkout: disable advice in 'disable'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-23T20:27:38Z","receivedAt":"2024-09-23T20:27:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Derrick Stolee <stolee@gmail.com>\n>\n> When running 'git sparse-checkout disable' with the sparse index\n> enabled, Git is expected to expand the index into a full index. However,\n> it currently outputs the advice message saying that that is unexpected\n> and likely due to an issue with the working directory.\n> ...\n> +\t/*\n> +\t * Disable the advice message for expanding a sparse index, as we\n> +\t * are expecting to do that when disabling sparse-checkout.\n> +\t */\n> +\tgive_advice_on_expansion = 0;\n>  \trepo_read_index(the_repository);\n\nSounds sensible.\n\n> +/*\n> + * If performing an operation where the index is supposed to expand to a\n> + * full index, then disable the advice message by setting this global to\n> + * zero.\n> + */\n> +extern int give_advice_on_expansion;\n> +\n>  struct index_state;\n>  #define SPARSE_INDEX_MEMORY_ONLY (1 << 0)\n>  int is_sparse_index_allowed(struct index_state *istate, int flags);\n> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\n> index eb32da2a7f2..6e230b54876 100755\n> --- a/t/t1092-sparse-checkout-compatibility.sh\n> +++ b/t/t1092-sparse-checkout-compatibility.sh\n> @@ -2355,7 +2355,10 @@ test_expect_success 'advice.sparseIndexExpanded' '\n>  \tmkdir -p sparse-index/deep/deeper2/deepest &&\n>  \ttouch sparse-index/deep/deeper2/deepest/bogus &&\n>  \tgit -C sparse-index status 2>err &&\n> -\tgrep \"The sparse index is expanding to a full index\" err\n> +\tgrep \"The sparse index is expanding to a full index\" err &&\n> +\n> +\tgit -C sparse-index sparse-checkout disable 2>err &&\n> +\ttest_line_count = 0 err\n\nI am not a huge fun of insisting that other code in the code path in\nwhich I got rid of an unwanted error message must stay silent.  As\nwe are expanding to full, we are presumably rehydrating all the\ndirectories that was sparsified, so it might be reasonable if we\nwant to see a progress output during this operation, for example [*].\nWould it make more sense to look for the lack of specific advice\nmessage instead?\n\n\n[Footnote]\n\n * A mere example to illustrate the principle.  \"We disable progress\n   during test\", or \"this is so small that progress won't trigger\"\n   may both be a good reaction to the example, but the general point\n   still stands.\n"},{"id":"503467","messageId":"5f4290f6-7c9b-47ee-b867-c9904ce0ccab@gmail.com","threadId":"62177","inReplyTo":"xmqqr09ayv2d.fsf@gitster.g","subject":"Re: [PATCH] sparse-checkout: disable advice in 'disable'","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2024-09-25T13:48:16Z","receivedAt":"2024-09-25T13:48:19Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"\n\nOn 9/23/24 4:27 PM, Junio C Hamano wrote:\n > \"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n >\n >> From: Derrick Stolee <stolee@gmail.com>\n >>\n >> When running 'git sparse-checkout disable' with the sparse index\n >> enabled, Git is expected to expand the index into a full index. However,\n >> it currently outputs the advice message saying that that is unexpected\n >> and likely due to an issue with the working directory.\n >> ...\n >> +\t/*\n >> +\t * Disable the advice message for expanding a sparse index, as we\n >> +\t * are expecting to do that when disabling sparse-checkout.\n >> +\t */\n >> +\tgive_advice_on_expansion = 0;\n >>   \trepo_read_index(the_repository);\n >\n > Sounds sensible.\n >\n >> +/*\n >> + * If performing an operation where the index is supposed to expand to a\n >> + * full index, then disable the advice message by setting this global to\n >> + * zero.\n >> + */\n >> +extern int give_advice_on_expansion;\n >> +\n >>   struct index_state;\n >>   #define SPARSE_INDEX_MEMORY_ONLY (1 << 0)\n >>   int is_sparse_index_allowed(struct index_state *istate, int flags);\n >> diff --git a/t/t1092-sparse-checkout-compatibility.sh \nb/t/t1092-sparse-checkout-compatibility.sh\n >> index eb32da2a7f2..6e230b54876 100755\n >> --- a/t/t1092-sparse-checkout-compatibility.sh\n >> +++ b/t/t1092-sparse-checkout-compatibility.sh\n >> @@ -2355,7 +2355,10 @@ test_expect_success 'advice.sparseIndexExpanded' '\n >>   \tmkdir -p sparse-index/deep/deeper2/deepest &&\n >>   \ttouch sparse-index/deep/deeper2/deepest/bogus &&\n >>   \tgit -C sparse-index status 2>err &&\n >> -\tgrep \"The sparse index is expanding to a full index\" err\n >> +\tgrep \"The sparse index is expanding to a full index\" err &&\n >> +\n >> +\tgit -C sparse-index sparse-checkout disable 2>err &&\n >> +\ttest_line_count = 0 err\n >\n > I am not a huge fun of insisting that other code in the code path in\n > which I got rid of an unwanted error message must stay silent.  As\n > we are expanding to full, we are presumably rehydrating all the\n > directories that was sparsified, so it might be reasonable if we\n > want to see a progress output during this operation, for example [*].\n > Would it make more sense to look for the lack of specific advice\n > message instead?\n\nI would say that there are generally two reasons why I chose to check\nthat 'err' was empty here:\n\n  1. Using \"! grep\" feels flaky to me. If we changed the error message,\n     then we could accidentally cause the test to pass because we don't\n     see the old message. This is somewhat mitigated by having the same\n     test check for the existence of the message, so careful use of a\n     common variable might avoid this potential future. Something like:\n\n+  msg=\"The sparse index is expanding to a full index\" &&\n-\tgrep \"The sparse index is expanding to a full index\" err\n+\tgrep \"$msg\" err &&\n+\n+\tgit -C sparse-index sparse-checkout disable 2>err &&\n+\t! grep \"$msg\" err\n\n  2. If the output is currently empty, then testing that it stays empty\n     will be a more rigid test. It will help us notice a change of\n     behavior here, even if it is an intentional change.\n\nFor the progress motivation, I'm not too worried because the progress\nindicators depend on isatty(2)[*], so would not appear here even if the\ncommand was slow or somehow GIT_PROGRESS_DELAY=0 was set.\n\nThanks,\n-Stolee\n\n[*] #leftoverbits: 'git sparse-checkout' commands do not have --progress\noptions, but could. The 'unpack_trees_options' structs have a member\ncalled 'show_progress' that could be populated based on a user option.\n"},{"id":"503474","messageId":"xmqqmsjvptsy.fsf@gitster.g","threadId":"62177","inReplyTo":"5f4290f6-7c9b-47ee-b867-c9904ce0ccab@gmail.com","subject":"Re: [PATCH] sparse-checkout: disable advice in 'disable'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-25T16:44:13Z","receivedAt":"2024-09-25T16:44:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <stolee@gmail.com> writes:\n\n>>> +\tgrep \"The sparse index is expanding to a full index\" err &&\n>>> +\n>>> +\tgit -C sparse-index sparse-checkout disable 2>err &&\n>>> +\ttest_line_count = 0 err\n>>\n>> I am not a huge fun of insisting that other code in the code path in\n>> which I got rid of an unwanted error message must stay silent.\n> ...\n> I would say that there are generally two reasons why I chose to check\n> that 'err' was empty here:\n>\n>  1. Using \"! grep\" feels flaky to me. If we changed the error message,\n>     then we could accidentally cause the test to pass because we don't\n>     see the old message. This is somewhat mitigated by having the same\n>     test check for the existence of the message, so careful use of a\n>     common variable might avoid this potential future.\n\nYup.  Duplicating is probably OK in practice as the eyes of the\ndeveloper who changed the message will be pulled to this test when\nthey notice the failure from the positive \"grep\" to notice the\nnegated version you use to replace \"The err file must be absolutely\nempty\", but I agree that a common variable would be even safer.\n\n> +  msg=\"The sparse index is expanding to a full index\" &&\n> -\tgrep \"The sparse index is expanding to a full index\" err\n> +\tgrep \"$msg\" err &&\n> +\n> +\tgit -C sparse-index sparse-checkout disable 2>err &&\n> +\t! grep \"$msg\" err\n\nExcellent.  \"test_grep\" and \"test_grep !\" are better choices, though.\n\n>  2. If the output is currently empty, then testing that it stays empty\n>     will be a more rigid test. It will help us notice a change of\n>     behavior here, even if it is an intentional change.\n\nSuch a stricter position cuts both ways.\n\nIf we assume infinite engineering resource availablility on ongoing\nbasis, yes, it may lead to a good discipline.  But having millions\nof such tests that will _notice_ changes that are irrelevant to the\nthing the test is about (e.g., this thing is about sparse index\nexpansion advice), we'd be setting ourselves to adjust numerous\ntests whose primary purpose has nothing to do with what we are\nchanging.\n\nThe choice also largely depends more on preference and taste that do\nnot have one absolute and universal answer.  I would personally\nprefer avoiding overly specific tests, but I also find it attractive\nif we can afford to be more specific in tests at times.\n\n> For the progress motivation, I'm not too worried because the progress\n> indicators depend on isatty(2)[*], so would not appear here even if the\n> command was slow or somehow GIT_PROGRESS_DELAY=0 was set.\n\nI already said that \"progress\" was a mere example, didn't I?\n\nEven if we expect we will not ever see unwanted progress indicators\nin this code, the general point still stands (iow, progress and\n\"unsparsifying warning\" are not the only things that output to the\nstandard error stream).\n\n> [*] #leftoverbits: 'git sparse-checkout' commands do not have --progress\n> options, but could. The 'unpack_trees_options' structs have a member\n> called 'show_progress' that could be populated based on a user option.\n\nNice.\n\nThanks.\n"}]}