sparse-checkout: disable advice in 'disable'
PATCH 1/1 sparse-checkout: disable advice in 'disable'
From: Derrick Stolee <[email protected]>
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 this advice message when in this code path. Establish a pattern for doing a similar removal in the future.
---
sparse-checkout: disable advice in 'disable'
I found this while someone was demoing some behavior that included disabling the sparse-checkout feature. Having this message pop up during that interaction was embarassing.
Thanks, -Stolee
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1800%2Fderrickstolee%2Fsparse-checkout-unset-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1800/derrickstolee/sparse-checkout-unset-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1800
builtin/sparse-checkout.c | 5 +++++ sparse-index.c | 5 +++-- sparse-index.h | 7 +++++++ t/t1092-sparse-checkout-compatibility.sh | 5 ++++- 4 files changed, 19 insertions(+), 3 deletions(-)
builtin/sparse-checkout.c +5 -0
@@ -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));
sparse-index.c +3 -2
@@ -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" \
sparse-index.h +7 -0
@@ -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);
t/t1092-sparse-checkout-compatibility.sh +4 -1
@@ -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' '
base-commit: 94b60adee30619a05296cf5ed6addb0e6d4e25dc
Signature · 1 line
gitgitgadget
builtin/sparse-checkout.c static int sparse_checkout_disable(int argc, const char **argv, · line 932
Quoted 13 lines · Derrick Stolee via GitGitGadget
From: Derrick Stolee <[email protected]>
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.
...builtin/sparse-checkout.c +5
-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);
Sounds sensible.
Quoted 15 lines
sparse-index.h +7
-0+/* +* 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);t/t1092-sparse-checkout-compatibility.sh +0
-0
t/t1092-sparse-checkout-compatibility.sh test_expect_success 'advice.sparseIndexExpanded' ' · line 2361
Quoted 8 lines
t/t1092-sparse-checkout-compatibility.sh +4
-1@@ -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.
Quoted 42 lines · Junio C Hamano
builtin/sparse-checkout.c static int sparse_checkout_disable(int argc, const char **argv, · line 932
Quoted 13 lines · Derrick Stolee via GitGitGadget
From: Derrick Stolee <[email protected]>
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.
...builtin/sparse-checkout.c +5
-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);Sounds sensible.
Quoted 15 lines
sparse-index.h +7
-0+/* +* 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);t/t1092-sparse-checkout-compatibility.sh +0
-0t/t1092-sparse-checkout-compatibility.sh test_expect_success 'advice.sparseIndexExpanded' ' · line 2361
Quoted 8 lines
t/t1092-sparse-checkout-compatibility.sh +4
-1@@ -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
Quoted 7 lines
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.
Quoted 11 lines · Derrick Stolee
Quoted 7 lines
t/t1092-sparse-checkout-compatibility.sh test_expect_success 'advice.sparseIndexExpanded' ' · line 2361
Quoted 4 lines
t/t1092-sparse-checkout-compatibility.sh +4
-0+ grep "The sparse index is expanding to a full index" err && + + git -C sparse-index sparse-checkout disable 2>err && + test_line_count = 0 errI 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:
Quoted 5 lines
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.
t/t1092-sparse-checkout-compatibility.sh test_expect_success 'advice.sparseIndexExpanded' '
Quoted 6 lines
t/t1092-sparse-checkout-compatibility.sh +5
-1+ 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.
Quoted 3 lines
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.
Quoted 3 lines
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).
Quoted 3 lines
[*] #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.