Re: [PATCH v2 4/4] tests for repack --filter mode

[John Cai <johncai86@gmail.com>]

Hi Rob,

On 17 Feb 2022, at 11:14, Robert Coup wrote:

> Hi John,
>
> Minor, but should we use oid rather than sha1 in the list.sh/upload.sh
> scripts? wrt sha256 slowly coming along the pipe.

good point, I'll make those adjustments.

>
>> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
>> index e489869dd94..78cc1858cb6 100755
>> --- a/t/t7700-repack.sh
>> +++ b/t/t7700-repack.sh
>> @@ -237,6 +237,26 @@ test_expect_success 'auto-bitmaps do not complain if unavailable' '
>>         test_must_be_empty actual
>>  '
>>
>> +test_expect_success 'repack with filter does not fetch from remote' '
>> +       rm -rf server client &&
>> +       test_create_repo server &&
>> +       git -C server config uploadpack.allowFilter true &&
>> +       git -C server config uploadpack.allowAnySHA1InWant true &&
>> +       echo content1 >server/file1 &&
>> +       git -C server add file1 &&
>> +       git -C server commit -m initial_commit &&
>> +       expected="?$(git -C server rev-parse :file1)" &&
>> +       git clone --bare --no-local server client &&
>> +       git -C client config remote.origin.promisor true &&
>> +       git -C client -c repack.writebitmaps=false repack -a -d --filter=blob:none &&
>
> Does writing bitmaps have any effect/interaction here?

Currently writing bitmaps don't play well with promisor objects. If I'm reading
the code correctly, it seems that when we build a bitmap with
bitmap_writer_build(), find_object_pos() gets called and will complain if an
object is missing from the pack.

We probably need to do the work to allow bitmaps to play well with promisor
objects.

>
>> +       git -C client rev-list --objects --all --missing=print >objects &&
>> +       grep "$expected" objects &&
>
> This is testing the object that was cloned initially is gone after the
> repack, ok.
>
>> +       git -C client repack -a -d &&
>> +       expected="$(git -C server rev-parse :file1)" &&
>> +       git -C client rev-list --objects --all --missing=print >objects &&
>> +       grep "$expected" objects
>
> But I'm not sure what you're testing here? A repack wouldn't fetch
> missing objects for a promisor pack anyway... and because there's no
> '^' in the pattern the grep will succeed regardless of whether the
> object is missing/present.

Good point. I overlooked the fact that by this point in the test, repack has
already written a promisor file. I think I'll just remove these last couple of
lines.

>
> Rob :)