Re: [PATCH v3 2/2] blame: enable and test the sparse index

From: Lessley Dennington <lessleydennington@gmail.com>
Date: Nov 5, 2021, 00:04 UTC
Message-ID: <49ada9fe-3cd5-622d-fdc7-92f95b040b47@gmail.com>
In-Reply-To: <xmqqh7ct89us.fsf@gitster.g>

On 11/3/21 9:47 AM, Junio C Hamano wrote:

Show 21 quoted lines

> "Lessley Dennington via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
>> We do not include paths outside the sparse checkout cone because blame
>> currently does not support blaming files outside of the sparse definition.
>> Attempting to do so fails with the following error:
>>
>>    fatal: no such path '<path outside sparse definition>' in HEAD
> 
> Does this indicate that we need to update how the command line
> safety in verify_working_tree_path() works in a sparsely checked out
> working tree?  If foo/bar is outside the sparse definition,
> 
>      git blame HEAD foo/bar
> 
> may get such a message, but shouldn't
> 
>      git blame HEAD -- foo/bar
> 
> make it work?
>

This is something we could consider for the future, and I've updated the comment you called out below to better reflect that in the next version. However, I don't know that this change belongs in this series, which aims to enable the sparse index. Perhaps even updating the 'blame with pathspec outside sparse definition' test was a bit too far outside the target scope of this change - I can remove my updates if that is the case.

Show 9 quoted lines

>> -# TODO: blame currently does not support blaming files outside of the
>> -# sparse definition. It complains that the file doesn't exist locally.
>> -test_expect_failure 'blame with pathspec outside sparse definition' '
>> +# Blame does not support blaming files outside of the sparse
>> +# definition, so we verify this scenario.
> 
> IOW, why is it a good idea to drop the "TODO" and "currently" and pretend
> as if the current behaviour is the desirable one?
>

Thank you, updated for v4.

Show 44 quoted lines

>> +test_expect_success 'blame with pathspec outside sparse definition' '
>>   	init_repos &&
>> +	test_sparse_match git sparse-checkout set &&
>>   
>> -	test_all_match git blame folder1/a &&
>> -	test_all_match git blame folder2/a &&
>> -	test_all_match git blame deep/deeper2/a &&
>> -	test_all_match git blame deep/deeper2/deepest/a
>> +	for file in a \
>> +			deep/a \
>> +			deep/deeper1/a \
>> +			deep/deeper1/deepest/a
>> +	do
>> +		test_sparse_match test_must_fail git blame $file &&
>> +		cat >expect <<-EOF &&
>> +		fatal: Cannot lstat '"'"'$file'"'"': No such file or directory
>> +		EOF
>> +		# We compare sparse-checkout-err and sparse-index-err in
>> +		# `test_sparse_match`. Given we know they are the same, we
>> +		# only check the content of sparse-index-err here.
>> +		test_cmp expect sparse-index-err
>> +	done
>>   '
>>   
>>   test_expect_success 'checkout and reset (mixed)' '
>> @@ -878,6 +892,18 @@ test_expect_success 'sparse index is not expanded: diff' '
>>   	ensure_not_expanded diff --staged
>>   '
>>   
>> +test_expect_success 'sparse index is not expanded: blame' '
>> +	init_repos &&
>> +
>> +	for file in a \
>> +			deep/a \
>> +			deep/deeper1/a \
>> +			deep/deeper1/deepest/a
>> +	do
>> +		ensure_not_expanded blame $file
>> +	done
>> +'
>> +
>>   # NEEDSWORK: a sparse-checkout behaves differently from a full checkout
>>   # in this scenario, but it shouldn't.
>>   test_expect_success 'reset mixed and checkout orphan' '

Best,

Lessley