{"thread":{"id":"61727","subject":"[PATCH] advice: warn when sparse index expands","startedAt":"2024-07-03T15:14:42Z","lastAt":"2024-07-08T19:34:32Z","messageCount":10,"participants":["Derrick Stolee via GitGitGadget","Junio C Hamano","Derrick Stolee","rsbecker@nexbridge.com","Rubén Justo","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"498032","messageId":"pull.1756.git.1720019679517.gitgitgadget@gmail.com","threadId":"61727","inReplyTo":null,"subject":"[PATCH] advice: warn when sparse index expands","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-07-03T15:14:39Z","receivedAt":"2024-07-03T15:14:42Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <stolee@gmail.com>\n\nTypically, forcing a sparse index to expand to a full index means that\nGit could not determine the status of a file outside of the\nsparse-checkout and needed to expand sparse trees into the full list of\nsparse blobs. This operation can be very slow when the sparse-checkout\nis much smaller than the full tree at HEAD.\n\nWhen users are in this state, it is common that 'git status' will report\nthe problem. Usually there is a modified or untracked file outside of\nthe sparse-checkout mentioned by the 'git status' output. There are a\nnumber of reasons why this is insufficient:\n\n 1. Users may not have a full understanding of which files are inside or\n    outside of their sparse-checkout. This is more common in monorepos\n    that manage the sparse-checkout using custom tools that map build\n    dependencies into sparse-checkout definitions.\n\n 2. In some cases, an empty directory could exist outside the\n    sparse-checkout and these empty directories are not reported by 'git\n    status' and friends.\n\n 3. If the user has '.gitignore' or 'exclude' files, then 'git status'\n    will squelch the warnings and not demonstrate any problems.\n\nIn order to help users who are in this state, add a new advice message\nto indicate that a sparse index is expanded to a full index. This\nmessage should be written at most once per process, so add a static\nglobal 'give_advice_on_expansion' to sparse-index.c. Further, there is a\ncase in 'git sparse-checkout set' that uses the sparse index as an\nin-memory data structure (even when writing a full index) so we need to\ndisable the message in that kind of case.\n\nThe t1092-sparse-checkout-compatibility.sh test script compares the\nbehavior of several Git commands across full and sparse repositories,\nincluding sparse repositories with and without a sparse index. We need\nto disable the advice in the sparse-index repo to avoid differences in\nstderr. By leaving the advice on in the sparse-checkout repo (without\nthe sparse index), we can test the behavior of disabling the advice in\nconvert_to_sparse(). (Indeed, these tests are how that necessity was\ndiscovered.) Add a test that reenables the advice and demonstrates that\nthe message is output.\n\nThe advice message is defined outside of expand_index() to avoid super-\nwide lines. It is also defined as a macro to avoid compile issues with\n-Werror=format-security.\n\nSigned-off-by: Derrick Stolee <stolee@gmail.com>\n---\n    advice: warn when sparse index expands\n    \n    This is motivated by some cases I noticed while supporting users who\n    were in this bad state and hitting the performance issue noticed in [1].\n    These users had some of the example situations where 'git status' did\n    not provide helpful output. This idea has always been in the back of my\n    mind since the sparse index was created, but it didn't make sense\n    initially when only a few builtins could operate without immediately\n    expanding a sparse index to a full one.\n    \n    [1]\n    https://lore.kernel.org/git/pull.1754.v3.git.1719578605.gitgitgadget@gmail.com/\n    \n    Thanks, Stolee\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1756%2Fderrickstolee%2Fadvice-sparse-index-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1756/derrickstolee/advice-sparse-index-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1756\n\n Documentation/config/advice.txt          |  4 ++++\n advice.c                                 |  1 +\n advice.h                                 |  1 +\n sparse-index.c                           | 27 ++++++++++++++++++++++++\n t/t1092-sparse-checkout-compatibility.sh | 16 +++++++++++++-\n 5 files changed, 48 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt\nindex fa612417568..0ba89898207 100644\n--- a/Documentation/config/advice.txt\n+++ b/Documentation/config/advice.txt\n@@ -116,6 +116,10 @@ advice.*::\n \tskippedCherryPicks::\n \t\tShown when linkgit:git-rebase[1] skips a commit that has already\n \t\tbeen cherry-picked onto the upstream branch.\n+\tsparseIndexExpanded::\n+\t\tShown when a sparse index is expanded to a full index, which is likely\n+\t\tdue to an unexpected set of files existing outside of the\n+\t\tsparse-checkout.\n \tstatusAheadBehind::\n \t\tShown when linkgit:git-status[1] computes the ahead/behind\n \t\tcounts for a local ref compared to its remote tracking ref,\ndiff --git a/advice.c b/advice.c\nindex 558a46fc0b3..7845e427c89 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -77,6 +77,7 @@ static struct {\n \t[ADVICE_RM_HINTS]\t\t\t\t= { \"rmHints\" },\n \t[ADVICE_SEQUENCER_IN_USE]\t\t\t= { \"sequencerInUse\" },\n \t[ADVICE_SET_UPSTREAM_FAILURE]\t\t\t= { \"setUpstreamFailure\" },\n+\t[ADVICE_SPARSE_INDEX_EXPANDED]\t\t\t= { \"sparseIndexExpanded\" },\n \t[ADVICE_SKIPPED_CHERRY_PICKS]\t\t\t= { \"skippedCherryPicks\" },\n \t[ADVICE_STATUS_AHEAD_BEHIND_WARNING]\t\t= { \"statusAheadBehindWarning\" },\n \t[ADVICE_STATUS_HINTS]\t\t\t\t= { \"statusHints\" },\ndiff --git a/advice.h b/advice.h\nindex 5105d90129d..572272fa0da 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -44,6 +44,7 @@ enum advice_type {\n \tADVICE_RM_HINTS,\n \tADVICE_SEQUENCER_IN_USE,\n \tADVICE_SET_UPSTREAM_FAILURE,\n+\tADVICE_SPARSE_INDEX_EXPANDED,\n \tADVICE_SKIPPED_CHERRY_PICKS,\n \tADVICE_STATUS_AHEAD_BEHIND_WARNING,\n \tADVICE_STATUS_HINTS,\ndiff --git a/sparse-index.c b/sparse-index.c\nindex e48e40cae71..1e517f696dd 100644\n--- a/sparse-index.c\n+++ b/sparse-index.c\n@@ -12,6 +12,21 @@\n #include \"config.h\"\n #include \"dir.h\"\n #include \"fsmonitor-ll.h\"\n+#include \"advice.h\"\n+\n+/**\n+ * This global is used by expand_index() to determine if we should give the\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+ */\n+static 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\"This likely means that you have files in your working directory\\n\"  \\\n+\t\"that are outside of your sparse-checkout patterns. Remove them\\n\"   \\\n+\t\"to recover performance expectations, such as with 'git clean'.\"\n \n struct modify_index_context {\n \tstruct index_state *write;\n@@ -183,6 +198,12 @@ int convert_to_sparse(struct index_state *istate, int flags)\n \t    !is_sparse_index_allowed(istate, flags))\n \t\treturn 0;\n \n+\t/*\n+\t * If we are purposefully collapsing a full index, then don't give\n+\t * advice when it is expanded later.\n+\t */\n+\tgive_advice_on_expansion = 0;\n+\n \t/*\n \t * NEEDSWORK: If we have unmerged entries, then stay full.\n \t * Unmerged entries prevent the cache-tree extension from working.\n@@ -328,6 +349,12 @@ void expand_index(struct index_state *istate, struct pattern_list *pl)\n \t\t\tpl = NULL;\n \t}\n \n+\tif (!pl && give_advice_on_expansion) {\n+\t\tgive_advice_on_expansion = 0;\n+\t\tadvise_if_enabled(ADVICE_SPARSE_INDEX_EXPANDED,\n+\t\t\t\t  _(ADVICE_MSG));\n+\t}\n+\n \t/*\n \t * A NULL pattern set indicates we are expanding a full index, so\n \t * we use a special region name that indicates the full expansion.\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 2f1ae5fd3bc..a2c0e1b4dcc 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -159,7 +159,10 @@ init_repos () {\n \tgit -C sparse-checkout sparse-checkout set deep &&\n \tgit -C sparse-index sparse-checkout init --cone --sparse-index &&\n \ttest_cmp_config -C sparse-index true index.sparse &&\n-\tgit -C sparse-index sparse-checkout set deep\n+\tgit -C sparse-index sparse-checkout set deep &&\n+\n+\t# Disable this message to keep stderr the same.\n+\tgit -C sparse-index config advice.sparseIndexExpanded false\n }\n \n init_repos_as_submodules () {\n@@ -2331,4 +2334,15 @@ test_expect_success 'sparse-index is not expanded: check-attr' '\n \tensure_not_expanded check-attr -a --cached -- folder1/a\n '\n \n+test_expect_success 'advice.sparseIndexExpanded' '\n+\tinit_repos &&\n+\n+\tgit -C sparse-index config --unset advice.sparseIndexExpanded &&\n+\tgit -C sparse-index sparse-checkout set deep/deeper1 &&\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+'\n+\n test_done\n\nbase-commit: 06e570c0dfb2a2deb64d217db78e2ec21672f558\n-- \ngitgitgadget\n"},{"id":"498050","messageId":"xmqq8qyicpdd.fsf@gitster.g","threadId":"61727","inReplyTo":"pull.1756.git.1720019679517.gitgitgadget@gmail.com","subject":"Re: [PATCH] advice: warn when sparse index expands","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-03T18:16:46Z","receivedAt":"2024-07-03T18:16:48Z","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> Typically, forcing a sparse index to expand to a full index means that\n> Git could not determine the status of a file outside of the\n> sparse-checkout and needed to expand sparse trees into the full list of\n> sparse blobs. This operation can be very slow when the sparse-checkout\n> is much smaller than the full tree at HEAD.\n>\n> When users are in this state, it is common that 'git status' will report\n> the problem. Usually there is a modified or untracked file outside of\n> the sparse-checkout mentioned by the 'git status' output. There are a\n> number of reasons why this is insufficient:\n\nNicely written to explain why giving an advice message is a good\nidea to cover this situation.\n\nMaking it possible to squelch comes with no cost (once the code to\ndo so is written), so I do not have a huge problem with the use of\nadvise_if_enabled(), but I offhand do not know if the users would\never want to squelch it.  Is this something that users would choose\nto say \"yes, I know what I am doing is making my sparse working tree\nunusuably slow and I've heard how to whip my sparse working tree\ninto a better shape already---please do not tell it to me ever\nagain; because I need to leave these crufts outside the sparse cone\nanyway, I am willing to accept the unusually slow response,\noverhead, and wasted cycles and power\" to?\n\nOther than that, nicely done.\n\n"},{"id":"498054","messageId":"068752a3-4140-4b30-803a-1c409afb01e1@gmail.com","threadId":"61727","inReplyTo":"xmqq8qyicpdd.fsf@gitster.g","subject":"Re: [PATCH] advice: warn when sparse index expands","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2024-07-03T19:18:06Z","receivedAt":"2024-07-03T19:18:09Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/3/24 2:16 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n>> From: Derrick Stolee <stolee@gmail.com>\n>>\n>> Typically, forcing a sparse index to expand to a full index means that\n>> Git could not determine the status of a file outside of the\n>> sparse-checkout and needed to expand sparse trees into the full list of\n>> sparse blobs. This operation can be very slow when the sparse-checkout\n>> is much smaller than the full tree at HEAD.\n>>\n>> When users are in this state, it is common that 'git status' will report\n>> the problem. Usually there is a modified or untracked file outside of\n>> the sparse-checkout mentioned by the 'git status' output. There are a\n>> number of reasons why this is insufficient:\n> \n> Nicely written to explain why giving an advice message is a good\n> idea to cover this situation.\n> \n> Making it possible to squelch comes with no cost (once the code to\n> do so is written), so I do not have a huge problem with the use of\n> advise_if_enabled(), but I offhand do not know if the users would\n> ever want to squelch it.  Is this something that users would choose\n> to say \"yes, I know what I am doing is making my sparse working tree\n> unusuably slow and I've heard how to whip my sparse working tree\n> into a better shape already---please do not tell it to me ever\n> again; because I need to leave these crufts outside the sparse cone\n> anyway, I am willing to accept the unusually slow response,\n> overhead, and wasted cycles and power\" to?\n\nI currently can't imagine a case where a user would want to disable\nthis advice, but I defaulted to allowing it. I suppose it is more\ndifficult to remove that option later, so I should have defaulted\nto not having it removable via config.\n\nI can send a v2 without the config option present. (I'll wait a\nday or two to see if others have strong opinions.)\n\nThanks,\n-Stolee\n"},{"id":"498055","messageId":"084501dacd7f$38ac7d60$aa057820$@nexbridge.com","threadId":"61727","inReplyTo":"068752a3-4140-4b30-803a-1c409afb01e1@gmail.com","subject":"RE: [PATCH] advice: warn when sparse index expands","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2024-07-03T19:28:39Z","receivedAt":"2024-07-03T19:28:49Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On Wednesday, July 3, 2024 3:18 PM, Derrick Stolee wrote:\n>On 7/3/24 2:16 PM, Junio C Hamano wrote:\n>> \"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>>\n>>> From: Derrick Stolee <stolee@gmail.com>\n>>>\n>>> Typically, forcing a sparse index to expand to a full index means\n>>> that Git could not determine the status of a file outside of the\n>>> sparse-checkout and needed to expand sparse trees into the full list\n>>> of sparse blobs. This operation can be very slow when the\n>>> sparse-checkout is much smaller than the full tree at HEAD.\n>>>\n>>> When users are in this state, it is common that 'git status' will\n>>> report the problem. Usually there is a modified or untracked file\n>>> outside of the sparse-checkout mentioned by the 'git status' output.\n>>> There are a number of reasons why this is insufficient:\n>>\n>> Nicely written to explain why giving an advice message is a good idea\n>> to cover this situation.\n>>\n>> Making it possible to squelch comes with no cost (once the code to do\n>> so is written), so I do not have a huge problem with the use of\n>> advise_if_enabled(), but I offhand do not know if the users would ever\n>> want to squelch it.  Is this something that users would choose to say\n>> \"yes, I know what I am doing is making my sparse working tree\n>> unusuably slow and I've heard how to whip my sparse working tree into\n>> a better shape already---please do not tell it to me ever again;\n>> because I need to leave these crufts outside the sparse cone anyway, I\n>> am willing to accept the unusually slow response, overhead, and wasted\n>> cycles and power\" to?\n>\n>I currently can't imagine a case where a user would want to disable this advice, but I\n>defaulted to allowing it. I suppose it is more difficult to remove that option later, so I\n>should have defaulted to not having it removable via config.\n>\n>I can send a v2 without the config option present. (I'll wait a day or two to see if\n>others have strong opinions.)\n\nOne possible use case is during a CI test of user code where they need to do something that causes the advice, but don't want to see the message in their logs.\n--Randall\n\n"},{"id":"498056","messageId":"xmqqr0cab69r.fsf@gitster.g","threadId":"61727","inReplyTo":"068752a3-4140-4b30-803a-1c409afb01e1@gmail.com","subject":"Re: [PATCH] advice: warn when sparse index expands","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-03T19:54:40Z","receivedAt":"2024-07-03T19:54:43Z","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> I can send a v2 without the config option present. (I'll wait a\n> day or two to see if others have strong opinions.)\n\nFWIW, that wasn't the direction I meant to lead the topic into.  It\nis perfectly acceptable if some are much more often used than others\namong various advice.* configuration variables for squelching these\nmessages.\n\nI was merely making an observation that this is likely to be a less\nused ones.\n\nThanks.\n"},{"id":"498058","messageId":"57ef4a0b-f67a-42e8-83bf-6e4b6ad0faac@gmail.com","threadId":"61727","inReplyTo":"pull.1756.git.1720019679517.gitgitgadget@gmail.com","subject":"Re: [PATCH] advice: warn when sparse index expands","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-03T20:36:53Z","receivedAt":"2024-07-03T20:36:57Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Wed, Jul 03, 2024 at 03:14:39PM +0000, Derrick Stolee via GitGitGadget wrote:\n\n>  \t[ADVICE_SET_UPSTREAM_FAILURE]\t\t\t= { \"setUpstreamFailure\" },\n\n> +\t[ADVICE_SPARSE_INDEX_EXPANDED]\t\t\t= { \"sparseIndexExpanded\" },\n>  \t[ADVICE_SKIPPED_CHERRY_PICKS]\t\t\t= { \"skippedCherryPicks\" },\n\nMaybe this change will disappear in a possible v2, but if it remains\nyou'll probably want to swap these two lines. \n\n>  \tADVICE_SET_UPSTREAM_FAILURE,\n\n> +\tADVICE_SPARSE_INDEX_EXPANDED,\n>  \tADVICE_SKIPPED_CHERRY_PICKS,\n\nDitto.\n"},{"id":"498122","messageId":"CABPp-BG_udtdv1aCfJ7T_WmRExdD29ZVXD+cA6xOZYHofHDtTQ@mail.gmail.com","threadId":"61727","inReplyTo":"pull.1756.git.1720019679517.gitgitgadget@gmail.com","subject":"Re: [PATCH] advice: warn when sparse index expands","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2024-07-05T20:29:26Z","receivedAt":"2024-07-05T20:29:39Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi,\n\nOn Wed, Jul 3, 2024 at 8:14 AM Derrick Stolee via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> From: Derrick Stolee <stolee@gmail.com>\n>\n> Typically, forcing a sparse index to expand to a full index means that\n> Git could not determine the status of a file outside of the\n> sparse-checkout and needed to expand sparse trees into the full list of\n> sparse blobs. This operation can be very slow when the sparse-checkout\n> is much smaller than the full tree at HEAD.\n\nYep, I'm with you here.\n\n> When users are in this state, it is common that 'git status' will report\n> the problem.\n\nI struggled to understand this sentence in combination with your later\nstatements, though that may only be because I had some difficulty with\nlater parts of the commit message.  Perhaps addressing the later parts\nwill make this sentence fine as-is, but it's possible this sentence\ncould do with a bit more detail.\n\n> Usually there is a modified or untracked file outside of\n> the sparse-checkout mentioned by the 'git status' output. There are a\n> number of reasons why this is insufficient:\n\nFair enough; let's focus on why the output of git status is insufficient...\n\n>  1. Users may not have a full understanding of which files are inside or\n>     outside of their sparse-checkout. This is more common in monorepos\n>     that manage the sparse-checkout using custom tools that map build\n>     dependencies into sparse-checkout definitions.\n\nHaving sparse-checkout patterns managed by custom tools is a really\ngood point, but doesn't this statement of yours about needing to know\nparticular files or directories suggest that...\n\n> diff --git a/sparse-index.c b/sparse-index.c\n> index e48e40cae71..1e517f696dd 100644\n> --- a/sparse-index.c\n> +++ b/sparse-index.c\n> @@ -12,6 +12,21 @@\n>  #include \"config.h\"\n>  #include \"dir.h\"\n>  #include \"fsmonitor-ll.h\"\n> +#include \"advice.h\"\n> +\n> +/**\n> + * This global is used by expand_index() to determine if we should give the\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> + */\n> +static int give_advice_on_expansion = 1;\n> +#define ADVICE_MSG \\\n> +       \"The sparse index is expanding to a full index, a slow operation.\\n\" \\\n> +       \"This likely means that you have files in your working directory\\n\"  \\\n> +       \"that are outside of your sparse-checkout patterns. Remove them\\n\"   \\\n> +       \"to recover performance expectations, such as with 'git clean'.\"\n\n...this is an insufficient solution?\n\nI was a bit surprised you'd list your first reason for git status\nbeing insufficient, that users need to know which files/directories\nare the problem and then provide a solution that doesn't attempt to\nidentify any files or directories.\n\n>  2. In some cases, an empty directory could exist outside the\n>     sparse-checkout and these empty directories are not reported by 'git\n>     status' and friends.\n\nThis is a really good point too...but given this point, shouldn't your\nadded advice message also mention \"directories\" instead of just\nmentioning \"files\" so that users are aware they need to look for those\ntoo?\n\n>  3. If the user has '.gitignore' or 'exclude' files, then 'git status'\n>     will squelch the warnings and not demonstrate any problems.\n\nYour solution does help the user to know that there is a problem (even\nif they don't know which files -- or directories -- are the problem),\nso this patch is making things better.\n\nTwo other small comments...\n\n> diff --git a/advice.c b/advice.c\n> index 558a46fc0b3..7845e427c89 100644\n> --- a/advice.c\n> +++ b/advice.c\n> @@ -77,6 +77,7 @@ static struct {\n>         [ADVICE_RM_HINTS]                               = { \"rmHints\" },\n>         [ADVICE_SEQUENCER_IN_USE]                       = { \"sequencerInUse\" },\n>         [ADVICE_SET_UPSTREAM_FAILURE]                   = { \"setUpstreamFailure\" },\n> +       [ADVICE_SPARSE_INDEX_EXPANDED]                  = { \"sparseIndexExpanded\" },\n>         [ADVICE_SKIPPED_CHERRY_PICKS]                   = { \"skippedCherryPicks\" },\n\nThe rest of the list is in alphabetical order, so the new entry\nprobably should be too.\n\n>         [ADVICE_STATUS_AHEAD_BEHIND_WARNING]            = { \"statusAheadBehindWarning\" },\n>         [ADVICE_STATUS_HINTS]                           = { \"statusHints\" },\n> diff --git a/advice.h b/advice.h\n> index 5105d90129d..572272fa0da 100644\n> --- a/advice.h\n> +++ b/advice.h\n> @@ -44,6 +44,7 @@ enum advice_type {\n>         ADVICE_RM_HINTS,\n>         ADVICE_SEQUENCER_IN_USE,\n>         ADVICE_SET_UPSTREAM_FAILURE,\n> +       ADVICE_SPARSE_INDEX_EXPANDED,\n>         ADVICE_SKIPPED_CHERRY_PICKS,\n\nAgain, the rest of the entries are in alphabetical order.\n\n\nAnyway, I've nit-picked on the little things I saw, but overall this\nis a good patch that moves things in a good direction; with a few\nsmall touch-ups it'll probably be good to go.\n"},{"id":"498243","messageId":"8a0ea69d-861f-496f-84fd-4d62251c2771@gmail.com","threadId":"61727","inReplyTo":"CABPp-BG_udtdv1aCfJ7T_WmRExdD29ZVXD+cA6xOZYHofHDtTQ@mail.gmail.com","subject":"Re: [PATCH] advice: warn when sparse index expands","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2024-07-08T12:57:46Z","receivedAt":"2024-07-08T12:57:48Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/5/24 4:29 PM, Elijah Newren wrote:\n> On Wed, Jul 3, 2024 at 8:14 AM Derrick Stolee via GitGitGadget\n> <gitgitgadget@gmail.com> wrote:\n>>\n>> From: Derrick Stolee <stolee@gmail.com>\n>>\n>> Typically, forcing a sparse index to expand to a full index means that\n>> Git could not determine the status of a file outside of the\n>> sparse-checkout and needed to expand sparse trees into the full list of\n>> sparse blobs. This operation can be very slow when the sparse-checkout\n>> is much smaller than the full tree at HEAD.\n> \n> Yep, I'm with you here.\n> \n>> When users are in this state, it is common that 'git status' will report\n>> the problem.\n> \n> I struggled to understand this sentence in combination with your later\n> statements, though that may only be because I had some difficulty with\n> later parts of the commit message.  Perhaps addressing the later parts\n> will make this sentence fine as-is, but it's possible this sentence\n> could do with a bit more detail.\n\nI think also the issue with this sentence is that its thought isn't\ncomplete until also reading the next one. These can be combined to\nget to the point faster.\n\n>> Usually there is a modified or untracked file outside of\n>> the sparse-checkout mentioned by the 'git status' output. There are a\n>> number of reasons why this is insufficient:\n> \n> Fair enough; let's focus on why the output of git status is insufficient...\n> \n>>   1. Users may not have a full understanding of which files are inside or\n>>      outside of their sparse-checkout. This is more common in monorepos\n>>      that manage the sparse-checkout using custom tools that map build\n>>      dependencies into sparse-checkout definitions.\n> \n> Having sparse-checkout patterns managed by custom tools is a really\n> good point, but doesn't this statement of yours about needing to know\n> particular files or directories suggest that...\n>\n>> +static int give_advice_on_expansion = 1;\n>> +#define ADVICE_MSG \\\n>> +       \"The sparse index is expanding to a full index, a slow operation.\\n\" \\\n>> +       \"This likely means that you have files in your working directory\\n\"  \\\n>> +       \"that are outside of your sparse-checkout patterns. Remove them\\n\"   \\\n>> +       \"to recover performance expectations, such as with 'git clean'.\"\n> \n> ...this is an insufficient solution?\n> \n> I was a bit surprised you'd list your first reason for git status\n> being insufficient, that users need to know which files/directories\n> are the problem and then provide a solution that doesn't attempt to\n> identify any files or directories.\n> \n>>   2. In some cases, an empty directory could exist outside the\n>>      sparse-checkout and these empty directories are not reported by 'git\n>>      status' and friends.\n> \n> This is a really good point too...but given this point, shouldn't your\n> added advice message also mention \"directories\" instead of just\n> mentioning \"files\" so that users are aware they need to look for those\n> too?\n> \n>>   3. If the user has '.gitignore' or 'exclude' files, then 'git status'\n>>      will squelch the warnings and not demonstrate any problems.\n> \n> Your solution does help the user to know that there is a problem (even\n> if they don't know which files -- or directories -- are the problem),\n> so this patch is making things better.\n\nThanks for pushing for a more helpful message. I'm currently thinking\nthat the core issue is that the working directory has contents (files\nor directories) that disagree with the sparse-checkout definition. I\nwill update the language in v2 to point the user in the direction of\ncomparing the two.\n\nThanks,\n-Stolee\n"},{"id":"498260","messageId":"pull.1756.v2.git.1720448038745.gitgitgadget@gmail.com","threadId":"61727","inReplyTo":"pull.1756.git.1720019679517.gitgitgadget@gmail.com","subject":"[PATCH v2] advice: warn when sparse index expands","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-07-08T14:13:58Z","receivedAt":"2024-07-08T14:14:02Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <stolee@gmail.com>\n\nTypically, forcing a sparse index to expand to a full index means that\nGit could not determine the status of a file outside of the\nsparse-checkout and needed to expand sparse trees into the full list of\nsparse blobs. This operation can be very slow when the sparse-checkout\nis much smaller than the full tree at HEAD.\n\nWhen users are in this state, there is usually a modified or untracked\nfile outside of the sparse-checkout mentioned by the output of 'git\nstatus'. There are a number of reasons why this is insufficient:\n\n 1. Users may not have a full understanding of which files are inside or\n    outside of their sparse-checkout. This is more common in monorepos\n    that manage the sparse-checkout using custom tools that map build\n    dependencies into sparse-checkout definitions.\n\n 2. In some cases, an empty directory could exist outside the\n    sparse-checkout and these empty directories are not reported by 'git\n    status' and friends.\n\n 3. If the user has '.gitignore' or 'exclude' files, then 'git status'\n    will squelch the warnings and not demonstrate any problems.\n\nIn order to help users who are in this state, add a new advice message\nto indicate that a sparse index is expanded to a full index. This\nmessage should be written at most once per process, so add a static\nglobal 'give_advice_on_expansion' to sparse-index.c. Further, there is a\ncase in 'git sparse-checkout set' that uses the sparse index as an\nin-memory data structure (even when writing a full index) so we need to\ndisable the message in that kind of case.\n\nThe t1092-sparse-checkout-compatibility.sh test script compares the\nbehavior of several Git commands across full and sparse repositories,\nincluding sparse repositories with and without a sparse index. We need\nto disable the advice in the sparse-index repo to avoid differences in\nstderr. By leaving the advice on in the sparse-checkout repo (without\nthe sparse index), we can test the behavior of disabling the advice in\nconvert_to_sparse(). (Indeed, these tests are how that necessity was\ndiscovered.) Add a test that reenables the advice and demonstrates that\nthe message is output.\n\nThe advice message is defined outside of expand_index() to avoid super-\nwide lines. It is also defined as a macro to avoid compile issues with\n-Werror=format-security.\n\nSigned-off-by: Derrick Stolee <stolee@gmail.com>\n---\n    advice: warn when sparse index expands\n    \n    This is motivated by some cases I noticed while supporting users who\n    were in this bad state and hitting the performance issue noticed in [1].\n    These users had some of the example situations where 'git status' did\n    not provide helpful output. This idea has always been in the back of my\n    mind since the sparse index was created, but it didn't make sense\n    initially when only a few builtins could operate without immediately\n    expanding a sparse index to a full one.\n    \n    [1]\n    https://lore.kernel.org/git/pull.1754.v3.git.1719578605.gitgitgadget@gmail.com/\n    \n    \n    Updates in V2\n    =============\n    \n    Thanks for the discussion on this patch!\n    \n     * Alphabetical order is restored. Sorry for that mixup.\n     * The advice message is edited to hint towards ways for the user to\n       figure out what is happening.\n    \n    Thanks, Stolee\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1756%2Fderrickstolee%2Fadvice-sparse-index-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1756/derrickstolee/advice-sparse-index-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1756\n\nRange-diff vs v1:\n\n 1:  9d5fa2caabd ! 1:  0ba93568f28 advice: warn when sparse index expands\n     @@ Commit message\n          sparse blobs. This operation can be very slow when the sparse-checkout\n          is much smaller than the full tree at HEAD.\n      \n     -    When users are in this state, it is common that 'git status' will report\n     -    the problem. Usually there is a modified or untracked file outside of\n     -    the sparse-checkout mentioned by the 'git status' output. There are a\n     -    number of reasons why this is insufficient:\n     +    When users are in this state, there is usually a modified or untracked\n     +    file outside of the sparse-checkout mentioned by the output of 'git\n     +    status'. There are a number of reasons why this is insufficient:\n      \n           1. Users may not have a full understanding of which files are inside or\n              outside of their sparse-checkout. This is more common in monorepos\n     @@ Documentation/config/advice.txt: advice.*::\n      \n       ## advice.c ##\n      @@ advice.c: static struct {\n     - \t[ADVICE_RM_HINTS]\t\t\t\t= { \"rmHints\" },\n       \t[ADVICE_SEQUENCER_IN_USE]\t\t\t= { \"sequencerInUse\" },\n       \t[ADVICE_SET_UPSTREAM_FAILURE]\t\t\t= { \"setUpstreamFailure\" },\n     -+\t[ADVICE_SPARSE_INDEX_EXPANDED]\t\t\t= { \"sparseIndexExpanded\" },\n       \t[ADVICE_SKIPPED_CHERRY_PICKS]\t\t\t= { \"skippedCherryPicks\" },\n     ++\t[ADVICE_SPARSE_INDEX_EXPANDED]\t\t\t= { \"sparseIndexExpanded\" },\n       \t[ADVICE_STATUS_AHEAD_BEHIND_WARNING]\t\t= { \"statusAheadBehindWarning\" },\n       \t[ADVICE_STATUS_HINTS]\t\t\t\t= { \"statusHints\" },\n     + \t[ADVICE_STATUS_U_OPTION]\t\t\t= { \"statusUoption\" },\n      \n       ## advice.h ##\n      @@ advice.h: enum advice_type {\n     - \tADVICE_RM_HINTS,\n       \tADVICE_SEQUENCER_IN_USE,\n       \tADVICE_SET_UPSTREAM_FAILURE,\n     -+\tADVICE_SPARSE_INDEX_EXPANDED,\n       \tADVICE_SKIPPED_CHERRY_PICKS,\n     ++\tADVICE_SPARSE_INDEX_EXPANDED,\n       \tADVICE_STATUS_AHEAD_BEHIND_WARNING,\n       \tADVICE_STATUS_HINTS,\n     + \tADVICE_STATUS_U_OPTION,\n      \n       ## sparse-index.c ##\n      @@\n     @@ sparse-index.c\n      + */\n      +static 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\"This likely means that you have files in your working directory\\n\"  \\\n     -+\t\"that are outside of your sparse-checkout patterns. Remove them\\n\"   \\\n     -+\t\"to recover performance expectations, such as with 'git clean'.\"\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\"     \\\n     ++\t\"your sparse-checkout patterns. Use 'git sparse-checkout list' to\\n\"   \\\n     ++\t\"see your sparse-checkout definition and compare it to your working\\n\" \\\n     ++\t\"directory contents. Running 'git clean' may assist in this cleanup.\"\n       \n       struct modify_index_context {\n       \tstruct index_state *write;\n\n\n Documentation/config/advice.txt          |  4 ++++\n advice.c                                 |  1 +\n advice.h                                 |  1 +\n sparse-index.c                           | 28 ++++++++++++++++++++++++\n t/t1092-sparse-checkout-compatibility.sh | 16 +++++++++++++-\n 5 files changed, 49 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt\nindex fa612417568..0ba89898207 100644\n--- a/Documentation/config/advice.txt\n+++ b/Documentation/config/advice.txt\n@@ -116,6 +116,10 @@ advice.*::\n \tskippedCherryPicks::\n \t\tShown when linkgit:git-rebase[1] skips a commit that has already\n \t\tbeen cherry-picked onto the upstream branch.\n+\tsparseIndexExpanded::\n+\t\tShown when a sparse index is expanded to a full index, which is likely\n+\t\tdue to an unexpected set of files existing outside of the\n+\t\tsparse-checkout.\n \tstatusAheadBehind::\n \t\tShown when linkgit:git-status[1] computes the ahead/behind\n \t\tcounts for a local ref compared to its remote tracking ref,\ndiff --git a/advice.c b/advice.c\nindex 558a46fc0b3..6b879d805c0 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -78,6 +78,7 @@ static struct {\n \t[ADVICE_SEQUENCER_IN_USE]\t\t\t= { \"sequencerInUse\" },\n \t[ADVICE_SET_UPSTREAM_FAILURE]\t\t\t= { \"setUpstreamFailure\" },\n \t[ADVICE_SKIPPED_CHERRY_PICKS]\t\t\t= { \"skippedCherryPicks\" },\n+\t[ADVICE_SPARSE_INDEX_EXPANDED]\t\t\t= { \"sparseIndexExpanded\" },\n \t[ADVICE_STATUS_AHEAD_BEHIND_WARNING]\t\t= { \"statusAheadBehindWarning\" },\n \t[ADVICE_STATUS_HINTS]\t\t\t\t= { \"statusHints\" },\n \t[ADVICE_STATUS_U_OPTION]\t\t\t= { \"statusUoption\" },\ndiff --git a/advice.h b/advice.h\nindex 5105d90129d..d7466bc0ef2 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -45,6 +45,7 @@ enum advice_type {\n \tADVICE_SEQUENCER_IN_USE,\n \tADVICE_SET_UPSTREAM_FAILURE,\n \tADVICE_SKIPPED_CHERRY_PICKS,\n+\tADVICE_SPARSE_INDEX_EXPANDED,\n \tADVICE_STATUS_AHEAD_BEHIND_WARNING,\n \tADVICE_STATUS_HINTS,\n \tADVICE_STATUS_U_OPTION,\ndiff --git a/sparse-index.c b/sparse-index.c\nindex e48e40cae71..8f4a43c182a 100644\n--- a/sparse-index.c\n+++ b/sparse-index.c\n@@ -12,6 +12,22 @@\n #include \"config.h\"\n #include \"dir.h\"\n #include \"fsmonitor-ll.h\"\n+#include \"advice.h\"\n+\n+/**\n+ * This global is used by expand_index() to determine if we should give the\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+ */\n+static 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\"     \\\n+\t\"your sparse-checkout patterns. Use 'git sparse-checkout list' to\\n\"   \\\n+\t\"see your sparse-checkout definition and compare it to your working\\n\" \\\n+\t\"directory contents. Running 'git clean' may assist in this cleanup.\"\n \n struct modify_index_context {\n \tstruct index_state *write;\n@@ -183,6 +199,12 @@ int convert_to_sparse(struct index_state *istate, int flags)\n \t    !is_sparse_index_allowed(istate, flags))\n \t\treturn 0;\n \n+\t/*\n+\t * If we are purposefully collapsing a full index, then don't give\n+\t * advice when it is expanded later.\n+\t */\n+\tgive_advice_on_expansion = 0;\n+\n \t/*\n \t * NEEDSWORK: If we have unmerged entries, then stay full.\n \t * Unmerged entries prevent the cache-tree extension from working.\n@@ -328,6 +350,12 @@ void expand_index(struct index_state *istate, struct pattern_list *pl)\n \t\t\tpl = NULL;\n \t}\n \n+\tif (!pl && give_advice_on_expansion) {\n+\t\tgive_advice_on_expansion = 0;\n+\t\tadvise_if_enabled(ADVICE_SPARSE_INDEX_EXPANDED,\n+\t\t\t\t  _(ADVICE_MSG));\n+\t}\n+\n \t/*\n \t * A NULL pattern set indicates we are expanding a full index, so\n \t * we use a special region name that indicates the full expansion.\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 2f1ae5fd3bc..a2c0e1b4dcc 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -159,7 +159,10 @@ init_repos () {\n \tgit -C sparse-checkout sparse-checkout set deep &&\n \tgit -C sparse-index sparse-checkout init --cone --sparse-index &&\n \ttest_cmp_config -C sparse-index true index.sparse &&\n-\tgit -C sparse-index sparse-checkout set deep\n+\tgit -C sparse-index sparse-checkout set deep &&\n+\n+\t# Disable this message to keep stderr the same.\n+\tgit -C sparse-index config advice.sparseIndexExpanded false\n }\n \n init_repos_as_submodules () {\n@@ -2331,4 +2334,15 @@ test_expect_success 'sparse-index is not expanded: check-attr' '\n \tensure_not_expanded check-attr -a --cached -- folder1/a\n '\n \n+test_expect_success 'advice.sparseIndexExpanded' '\n+\tinit_repos &&\n+\n+\tgit -C sparse-index config --unset advice.sparseIndexExpanded &&\n+\tgit -C sparse-index sparse-checkout set deep/deeper1 &&\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+'\n+\n test_done\n\nbase-commit: 06e570c0dfb2a2deb64d217db78e2ec21672f558\n-- \ngitgitgadget\n"},{"id":"498309","messageId":"xmqqo777ekzj.fsf@gitster.g","threadId":"61727","inReplyTo":"pull.1756.v2.git.1720448038745.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] advice: warn when sparse index expands","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-08T19:34:24Z","receivedAt":"2024-07-08T19:34:32Z","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>     ... This idea has always been in the back of my\n>     mind since the sparse index was created, but it didn't make sense\n>     initially when only a few builtins could operate without immediately\n>     expanding a sparse index to a full one.\n\nYeah, if this triggered for a command whose operation does require\nexpanding, then that would be annoying to the users.\n\nThe phrasing that suggests \"git clean\" used in this version does\nlook better.  I wonder if \"git clean\" would also have to expand and\nthen trigger this warning, though ;-)\n\nThanks.\n"}]}