Re: [PATCH] sparse-checkout: disable advice in 'disable'

[Derrick Stolee <stolee@gmail.com>]

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.