Re: [PATCH v3 07/24] multi-pack-index: expand test data

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jul 6, 2018, 04:36 UTC
Message-ID: <CAPig+cTO=UPuCRUAAWfBwaNhRD+=dgSwpgEB2fQJuWyFEd7Aew@mail.gmail.com>
In-Reply-To: <20180706005321.124643-8-dstolee@microsoft.com>

On Thu, Jul 5, 2018 at 8:54 PM Derrick Stolee <stolee@gmail.com> wrote:

> multi-pack-index: expand test data

Since this patch is touching only t5319, a more typical title would be:

    t5319: expand test data

> As we build the multi-pack-index file format, we want to test the format
> on real repoasitories. Add tests to t5319-multi-pack-index.sh that

s/repoasitories/repositories/

And, since the title now mentions t5319, this can become simply:

    Add tests that...

Show 13 quoted lines

> create repository data including multiple packfiles with both version 1
> and version 2 formats.
>
> The current 'git multi-pack-index write' command will always write the
> same file with no "real" data. This will be expanded in future commits,
> along with the test expectations.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
> @@ -13,9 +13,108 @@ midx_read_expect () {
>  test_expect_success 'write midx with no packs' '
> +       test_when_finished rm -f pack/multi-pack-index &&

Should this test_when_finished() have been added in an earlier patch? It seems out of place here.

Show 11 quoted lines

>         git multi-pack-index --object-dir=. write &&
>         test_path_is_file pack/multi-pack-index &&
>         midx_read_expect
>  '
>
> +test_expect_success 'create objects' '
> +       for i in $(test_seq 1 5)
> +       do
> +               iii=$(printf '%03i' $i)
> +               test-tool genrandom "bar" 200 >wide_delta_$iii &&
> +               test-tool genrandom "baz $iii" 50 >>wide_delta_$iii &&

Alternately:

    {
        test-tool genrandom "bar" 200 &&
         test-tool genrandom "baz $iii" 50
    } >wide_delta_$iii &&

which makes it easier to see at a glance that both commands are populating the same file. Same comment for the other files. (Not worth a re-roll.)

> +               test-tool genrandom "foo"$i 100 >deep_delta_$iii &&
> +               test-tool genrandom "foo"$(expr $i + 1) 100 >>deep_delta_$iii &&
> +               test-tool genrandom "foo"$(expr $i + 2) 100 >>deep_delta_$iii &&

Or, just use POSIX arithmetic expansion:

    $(( $i + 1 ))

> +               echo $iii >file_$iii &&
> +               test-tool genrandom "$iii" 8192 >>file_$iii &&
> +               git update-index --add file_$iii deep_delta_$iii wide_delta_$iii &&
> +               i=$(expr $i + 1) || return 1

Ditto, POSIX arithmetic expansion:

    i=$(( $i + 1 ))

(Not worth a re-roll.)

Show 8 quoted lines

> +       done &&
> +       { echo 101 && test-tool genrandom 100 8192; } >file_101 &&
> +       git update-index --add file_101 &&
> +       tree=$(git write-tree) &&
> +       commit=$(git commit-tree $tree </dev/null) && {
> +       echo $tree &&
> +       git ls-tree $tree | sed -e "s/.* \\([0-9a-f]*\\)        .*/\\1/"
> +       } >obj-list &&

Perhaps indent the content of the {...} block?

Show 9 quoted lines

> +       git update-ref HEAD $commit
> +'
> +
> +test_expect_success 'write midx with one v1 pack' '
> +       pack=$(git pack-objects --index-version=1 pack/test <obj-list) &&
> +       test_when_finished rm pack/test-$pack.pack pack/test-$pack.idx pack/multi-pack-index &&
> +       git multi-pack-index --object-dir=. write &&
> +       midx_read_expect
> +'

It's odd to see all these tests ending by creating an 'expect' file but not actually doing anything with that file.

> +test_expect_success 'Add more objects' '

s/Add/add/

Show 22 quoted lines

> +       for i in $(test_seq 6 10)
> +       do
> +               iii=$(printf '%03i' $i)
> +               test-tool genrandom "bar" 200 >wide_delta_$iii &&
> +               test-tool genrandom "baz $iii" 50 >>wide_delta_$iii &&
> +               test-tool genrandom "foo"$i 100 >deep_delta_$iii &&
> +               test-tool genrandom "foo"$(expr $i + 1) 100 >>deep_delta_$iii &&
> +               test-tool genrandom "foo"$(expr $i + 2) 100 >>deep_delta_$iii &&
> +               echo $iii >file_$iii &&
> +               test-tool genrandom "$iii" 8192 >>file_$iii &&
> +               git update-index --add file_$iii deep_delta_$iii wide_delta_$iii &&
> +               i=$(expr $i + 1) || return 1
> +       done &&
> +       { echo 101 && test-tool genrandom 100 8192; } >file_101 &&
> +       git update-index --add file_101 &&
> +       tree=$(git write-tree) &&
> +       commit=$(git commit-tree $tree -p HEAD</dev/null) && {
> +       echo $tree &&
> +       git ls-tree $tree | sed -e "s/.* \\([0-9a-f]*\\)        .*/\\1/"
> +       } >obj-list2 &&
> +       git update-ref HEAD $commit
> +'

There seems to be a fair bit of duplication in these tests which create objects. Is it possible to factor out some of this code into a shell function?

Show 7 quoted lines

> +test_expect_success 'write midx with two packs' '
> +       git pack-objects --index-version=1 pack/test-2 <obj-list2 &&
> +       git multi-pack-index --object-dir=. write &&
> +       midx_read_expect
> +'
> +
> +test_expect_success 'Add more packs' '

s/Add/add/

Show 5 quoted lines

> +       for j in $(test_seq 1 10)
> +       do
> +               [...]
> +       done
> +'