On Wed, Mar 17 2021, Derrick Stolee wrote:
> On 3/17/2021 9:28 AM, Ævar Arnfjörð Bjarmason wrote:
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
>
> I want to learn from your suggested changes to the test, here,
> so forgive my questions here:
>
>> +test_index_entry_like () {
>> + dir=$1
>> + shift
>> + fmt=$1
>> + shift
>> + rev=$1
>> + shift
>> + entry=$1
>> + shift
>> + file=$1
>> + shift
>
> Why all the shifts? Why not just use $1, $2, $3,...? My
> guess is that you want to be able to insert a new parameter
> in the middle in the future without changing the later
> numbers, but that seems unlikely, and we could just add
> the parameter at the end.
It's just crappy RFC-quality code. I probably copied some other function
and went with it. No good reason. Yeah it's ugly.
>> + hash=$(git -C "$dir" rev-parse "$rev") &&
>> + printf "$fmt\n" "$hash" "$entry" >expected &&
>> + if grep "$entry" "$file" >line
>> + then
>> + test_cmp expected line
>> + else
>> + cat cache &&
>> + false
>> + fi
>> +}
>> +
>> test_expect_success 'sparse-index contents' '
>> init_repos &&
>>
>> - test-tool -C sparse-index read-cache --table >cache &&
>> + git -C sparse-index ls-files --sparse >cache &&
>> for dir in folder1 folder2 x
>> do
>> - TREE=$(git -C sparse-index rev-parse HEAD:$dir) &&
>> - grep "040000 tree $TREE $dir/" cache \
>> - || return 1
>> + test_index_entry_like sparse-index "040000 %s 0\t%s" "HEAD:$dir" "$dir/" cache || return 1
>
> I see how this uses only one line, but it seems like the
> test_index_entry_like is too generic to make it not a
> complicated mess of format strings that need to copy
> over and over again.
>
> Perhaps instead it could be a "test_entry_is_tree"
> and it only passes "$dir" and "cache"? Then we could drop the loop and
> just have
>
> test_entry_is_tree cache folder1 &&
> test_entry_is_tree cache folder2 &&
> test_entry_is_tree cache x &&
>
> or we could still use the loop, especially when we test for four trees.
Yeah that sounds good. Personally I don't mind 4x similar lines
copy/pasted over a for-loop in the tests. You don't need to worry about
the || return doing the right thing, and just setting up the for-loop is
already 3 lines...
>> - test-tool -C sparse-index read-cache --table >cache &&
>> + git -C sparse-index ls-files --sparse >cache &&
>> for dir in deep/deeper2 folder1 folder2 x
>> do
>> - TREE=$(git -C sparse-index rev-parse HEAD:$dir) &&
>> - grep "040000 tree $TREE $dir/" cache \
>> - || return 1
>> + test_index_entry_like sparse-index "040000 %s 0\t%s" "HEAD:$dir" "$dir/" cache || return 1
>> done &&
>>
>> + grep 040000 cache >lines &&
>> + test_line_count = 4 lines &&
>> +
>
> The point here is to check that no other entries are trees? We know
> that this number will be _at least_ 4 based on the loop above.
It's exactly 4 because we have 4 folders we're checking. But you tell
me. I was just trying to refactor this dependence on the ls-tree format
while moving it over to ls-files without spending too much time on
understanding all the specifics.
