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

From: John Cai <johncai86@gmail.com>
Date: Feb 17, 2022, 20:36 UTC
Message-ID: <6BD2011A-9CD7-488C-8F17-F78FE59E93C7@gmail.com>
In-Reply-To: <CAFLLRp+9TmMKu5UpaN4sUr+o_9AGAVvtis0e87VMJsCva67q3w@mail.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.

Show 23 quoted lines

>
>> 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.

Show 16 quoted lines

>
>> +       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 :)