sparse-checkout: disable advice in 'disable' #1800
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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. Signed-off-by: Derrick Stolee <[email protected]>
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Derrick Stolee via GitGitGadget" <[email protected]> writes:
> 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 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. |
|
This patch series was integrated into seen via git@ece9c44. |
|
On the Git mailing list, Derrick Stolee wrote (reply to this): On 9/23/24 4:27 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <[email protected]> writes:
>
>> 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 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. |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Derrick Stolee <[email protected]> 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. |
|
This branch is now known as |
|
This patch series was integrated into seen via git@7fde753. |
|
There was a status update in the "New Topics" section about the branch When "git sparse-checkout disable" turns a sparse checkout into a regular checkout, the index is fully expanded. This totally expected behaviour however had an "oops, we are expanding the index" advice message, which has been corrected. Comments? source: <[email protected]> |
|
This patch series was integrated into seen via git@3102ea6. |
|
There was a status update in the "Cooking" section about the branch When "git sparse-checkout disable" turns a sparse checkout into a regular checkout, the index is fully expanded. This totally expected behaviour however had an "oops, we are expanding the index" advice message, which has been corrected. Comments? source: <[email protected]> |
|
This patch series was integrated into seen via git@6db79b2. |
|
This patch series was integrated into next via git@e670bcc. |
|
This patch series was integrated into seen via git@77d3530. |
|
There was a status update in the "Cooking" section about the branch When "git sparse-checkout disable" turns a sparse checkout into a regular checkout, the index is fully expanded. This totally expected behaviour however had an "oops, we are expanding the index" advice message, which has been corrected. Will merge to 'master'. source: <[email protected]> |
|
This patch series was integrated into seen via git@3e64b1d. |
|
There was a status update in the "Cooking" section about the branch When "git sparse-checkout disable" turns a sparse checkout into a regular checkout, the index is fully expanded. This totally expected behaviour however had an "oops, we are expanding the index" advice message, which has been corrected. Will merge to 'master'. source: <[email protected]> |
|
This patch series was integrated into seen via git@9293a93. |
|
This patch series was integrated into master via git@9293a93. |
|
This patch series was integrated into next via git@9293a93. |
|
Closed via 9293a93. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
cc: [email protected]
cc: [email protected]