| Message ID | pull.1800.git.1727119882901.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | sparse-checkout: disable advice in 'disable' | expand |
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Derrick Stolee <stolee@gmail.com> > > When running 'git sparse-checkout disable' with the sparse index > enabled, Git is expected to expand the index into a full index. However, > it currently outputs the advice message saying that that is unexpected > and likely due to an issue with the working directory. > ... > + /* > + * Disable the advice message for expanding a sparse index, as we > + * are expecting to do that when disabling sparse-checkout. > + */ > + give_advice_on_expansion = 0; > repo_read_index(the_repository); Sounds sensible. > +/* > + * If performing an operation where the index is supposed to expand to a > + * full index, then disable the advice message by setting this global to > + * zero. > + */ > +extern int give_advice_on_expansion; > + > struct index_state; > #define SPARSE_INDEX_MEMORY_ONLY (1 << 0) > int is_sparse_index_allowed(struct index_state *istate, int flags); > diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh > index eb32da2a7f2..6e230b54876 100755 > --- a/t/t1092-sparse-checkout-compatibility.sh > +++ b/t/t1092-sparse-checkout-compatibility.sh > @@ -2355,7 +2355,10 @@ test_expect_success 'advice.sparseIndexExpanded' ' > mkdir -p sparse-index/deep/deeper2/deepest && > touch sparse-index/deep/deeper2/deepest/bogus && > git -C sparse-index status 2>err && > - grep "The sparse index is expanding to a full index" err > + grep "The sparse index is expanding to a full index" err && > + > + git -C sparse-index sparse-checkout disable 2>err && > + test_line_count = 0 err I am not a huge fun of insisting that other code in the code path in which I got rid of an unwanted error message must stay silent. As we are expanding to full, we are presumably rehydrating all the directories that was sparsified, so it might be reasonable if we want to see a progress output during this operation, for example [*]. Would it make more sense to look for the lack of specific advice message instead? [Footnote] * A mere example to illustrate the principle. "We disable progress during test", or "this is so small that progress won't trigger" may both be a good reaction to the example, but the general point still stands.
On 9/23/24 4:27 PM, Junio C Hamano wrote: > "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> From: Derrick Stolee <stolee@gmail.com> >> >> When running 'git sparse-checkout disable' with the sparse index >> enabled, Git is expected to expand the index into a full index. However, >> it currently outputs the advice message saying that that is unexpected >> and likely due to an issue with the working directory. >> ... >> + /* >> + * Disable the advice message for expanding a sparse index, as we >> + * are expecting to do that when disabling sparse-checkout. >> + */ >> + give_advice_on_expansion = 0; >> repo_read_index(the_repository); > > Sounds sensible. > >> +/* >> + * If performing an operation where the index is supposed to expand to a >> + * full index, then disable the advice message by setting this global to >> + * zero. >> + */ >> +extern int give_advice_on_expansion; >> + >> struct index_state; >> #define SPARSE_INDEX_MEMORY_ONLY (1 << 0) >> int is_sparse_index_allowed(struct index_state *istate, int flags); >> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh >> index eb32da2a7f2..6e230b54876 100755 >> --- a/t/t1092-sparse-checkout-compatibility.sh >> +++ b/t/t1092-sparse-checkout-compatibility.sh >> @@ -2355,7 +2355,10 @@ test_expect_success 'advice.sparseIndexExpanded' ' >> mkdir -p sparse-index/deep/deeper2/deepest && >> touch sparse-index/deep/deeper2/deepest/bogus && >> git -C sparse-index status 2>err && >> - grep "The sparse index is expanding to a full index" err >> + grep "The sparse index is expanding to a full index" err && >> + >> + git -C sparse-index sparse-checkout disable 2>err && >> + test_line_count = 0 err > > I am not a huge fun of insisting that other code in the code path in > which I got rid of an unwanted error message must stay silent. As > we are expanding to full, we are presumably rehydrating all the > directories that was sparsified, so it might be reasonable if we > want to see a progress output during this operation, for example [*]. > Would it make more sense to look for the lack of specific advice > message instead? I would say that there are generally two reasons why I chose to check that 'err' was empty here: 1. Using "! grep" feels flaky to me. If we changed the error message, then we could accidentally cause the test to pass because we don't see the old message. This is somewhat mitigated by having the same test check for the existence of the message, so careful use of a common variable might avoid this potential future. Something like: + msg="The sparse index is expanding to a full index" && - grep "The sparse index is expanding to a full index" err + grep "$msg" err && + + git -C sparse-index sparse-checkout disable 2>err && + ! grep "$msg" err 2. If the output is currently empty, then testing that it stays empty will be a more rigid test. It will help us notice a change of behavior here, even if it is an intentional change. For the progress motivation, I'm not too worried because the progress indicators depend on isatty(2)[*], so would not appear here even if the command was slow or somehow GIT_PROGRESS_DELAY=0 was set. Thanks, -Stolee [*] #leftoverbits: 'git sparse-checkout' commands do not have --progress options, but could. The 'unpack_trees_options' structs have a member called 'show_progress' that could be populated based on a user option.
Derrick Stolee <stolee@gmail.com> writes: >>> + grep "The sparse index is expanding to a full index" err && >>> + >>> + git -C sparse-index sparse-checkout disable 2>err && >>> + test_line_count = 0 err >> >> I am not a huge fun of insisting that other code in the code path in >> which I got rid of an unwanted error message must stay silent. > ... > I would say that there are generally two reasons why I chose to check > that 'err' was empty here: > > 1. Using "! grep" feels flaky to me. If we changed the error message, > then we could accidentally cause the test to pass because we don't > see the old message. This is somewhat mitigated by having the same > test check for the existence of the message, so careful use of a > common variable might avoid this potential future. Yup. Duplicating is probably OK in practice as the eyes of the developer who changed the message will be pulled to this test when they notice the failure from the positive "grep" to notice the negated version you use to replace "The err file must be absolutely empty", but I agree that a common variable would be even safer. > + msg="The sparse index is expanding to a full index" && > - grep "The sparse index is expanding to a full index" err > + grep "$msg" err && > + > + git -C sparse-index sparse-checkout disable 2>err && > + ! grep "$msg" err Excellent. "test_grep" and "test_grep !" are better choices, though. > 2. If the output is currently empty, then testing that it stays empty > will be a more rigid test. It will help us notice a change of > behavior here, even if it is an intentional change. Such a stricter position cuts both ways. If we assume infinite engineering resource availablility on ongoing basis, yes, it may lead to a good discipline. But having millions of such tests that will _notice_ changes that are irrelevant to the thing the test is about (e.g., this thing is about sparse index expansion advice), we'd be setting ourselves to adjust numerous tests whose primary purpose has nothing to do with what we are changing. The choice also largely depends more on preference and taste that do not have one absolute and universal answer. I would personally prefer avoiding overly specific tests, but I also find it attractive if we can afford to be more specific in tests at times. > For the progress motivation, I'm not too worried because the progress > indicators depend on isatty(2)[*], so would not appear here even if the > command was slow or somehow GIT_PROGRESS_DELAY=0 was set. I already said that "progress" was a mere example, didn't I? Even if we expect we will not ever see unwanted progress indicators in this code, the general point still stands (iow, progress and "unsparsifying warning" are not the only things that output to the standard error stream). > [*] #leftoverbits: 'git sparse-checkout' commands do not have --progress > options, but could. The 'unpack_trees_options' structs have a member > called 'show_progress' that could be populated based on a user option. Nice. Thanks.
diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c index 5ccf6968628..85b4fc06b35 100644 --- a/builtin/sparse-checkout.c +++ b/builtin/sparse-checkout.c @@ -924,6 +924,11 @@ static int sparse_checkout_disable(int argc, const char **argv, builtin_sparse_checkout_disable_options, builtin_sparse_checkout_disable_usage, 0); + /* + * Disable the advice message for expanding a sparse index, as we + * are expecting to do that when disabling sparse-checkout. + */ + give_advice_on_expansion = 0; repo_read_index(the_repository); memset(&pl, 0, sizeof(pl)); diff --git a/sparse-index.c b/sparse-index.c index 9958656ded1..0f9fb4df026 100644 --- a/sparse-index.c +++ b/sparse-index.c @@ -19,9 +19,10 @@ * advice for advice.sparseIndexExpanded when expanding a sparse index to a full * one. However, this is sometimes done on purpose, such as in the sparse-checkout * builtin, even when index.sparse=false. This may be disabled in - * convert_to_sparse(). + * convert_to_sparse() or by commands that know they will lead to a full + * expansion, but this message is not actionable. */ -static int give_advice_on_expansion = 1; +int give_advice_on_expansion = 1; #define ADVICE_MSG \ "The sparse index is expanding to a full index, a slow operation.\n" \ "Your working directory likely has contents that are outside of\n" \ diff --git a/sparse-index.h b/sparse-index.h index a16f3e67d75..727034be7ca 100644 --- a/sparse-index.h +++ b/sparse-index.h @@ -1,6 +1,13 @@ #ifndef SPARSE_INDEX_H__ #define SPARSE_INDEX_H__ +/* + * If performing an operation where the index is supposed to expand to a + * full index, then disable the advice message by setting this global to + * zero. + */ +extern int give_advice_on_expansion; + struct index_state; #define SPARSE_INDEX_MEMORY_ONLY (1 << 0) int is_sparse_index_allowed(struct index_state *istate, int flags); diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh index eb32da2a7f2..6e230b54876 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -2355,7 +2355,10 @@ test_expect_success 'advice.sparseIndexExpanded' ' mkdir -p sparse-index/deep/deeper2/deepest && touch sparse-index/deep/deeper2/deepest/bogus && git -C sparse-index status 2>err && - grep "The sparse index is expanding to a full index" err + grep "The sparse index is expanding to a full index" err && + + git -C sparse-index sparse-checkout disable 2>err && + test_line_count = 0 err ' test_expect_success 'cat-file -p' '