Re: [PATCH v3 07/24] multi-pack-index: expand test data
On 7/12/2018 2:02 PM, Eric Sunshine wrote:
On Thu, Jul 12, 2018 at 10:10 AM Derrick Stolee wrote:
On 7/6/2018 12:36 AM, Eric Sunshine wrote:
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?
In addition to the other small changes, this refactor in particular was
a big change (but a good one). I'm sending my current progress in this
direction, as I expect this can be improved.
I like the amount of code reduction. A couple minor comments...
+generate_objects () {
+ i=$1
+ iii=$(printf '%03i' $i)
+ {
+ test-tool genrandom "bar" 200 &&
+ test-tool genrandom "baz $iii" 50
+ } >wide_delta_$iii &&
+ {
+ test-tool genrandom "foo"$i 100 &&
+ test-tool genrandom "foo"$(( $i + 1 )) 100 &&
+ test-tool genrandom "foo"$(( $i + 2 )) 100
+ } >>deep_delta_$iii &&
I think this should be: s/>>/>/
It should!
+ echo $iii >file_$iii &&
+ test-tool genrandom "$iii" 8192 >>file_$iii &&
And this: s/>>/>/
In addition, I should wrap these two commands in { } like the files above.
Thanks,
-Stolee
Re: [PATCH v3 07/24] multi-pack-index: expand test data
On Thu, Jul 12, 2018 at 10:10 AM Derrick Stolee wrote:
> On 7/6/2018 12:36 AM, Eric Sunshine wrote:
> > 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?
>
> In addition to the other small changes, this refactor in particular was
> a big change (but a good one). I'm sending my current progress in this
> direction, as I expect this can be improved.
I like the amount of code reduction. A couple minor comments...
> +generate_objects () {
> + i=$1
> + iii=$(printf '%03i' $i)
> + {
> + test-tool genrandom "bar" 200 &&
> + test-tool genrandom "baz $iii" 50
> + } >wide_delta_$iii &&
> + {
> + test-tool genrandom "foo"$i 100 &&
> + test-tool genrandom "foo"$(( $i + 1 )) 100 &&
> + test-tool genrandom "foo"$(( $i + 2 )) 100
> + } >>deep_delta_$iii &&
I think this should be: s/>>/>/
> + echo $iii >file_$iii &&
> + test-tool genrandom "$iii" 8192 >>file_$iii &&
And this: s/>>/>/
> + git update-index --add file_$iii deep_delta_$iii wide_delta_$iii
> +}
Re: [PATCH v3 07/24] multi-pack-index: expand test data
On 7/6/2018 12:36 AM, Eric Sunshine wrote:
On Thu, Jul 5, 2018 at 8:54 PM Derrick Stolee wrote:
+ 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 HEADobj-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?
In addition to the other small changes, this refactor in particular was
a big change (but a good one). I'm sending my current progress in this
direction, as I expect this can be improved.
To make the commit_and_list_objects method more generic to all
situations, I had to add an extra commit, which will cause some of the
numbers to change in the later 'midx_read_expect' calls.
Thanks,
-Stolee
-->8--
From cb38bb284fd05cf2230725b6cb9ead5795c913f2 Mon Sep 17 00:00:00 2001
From: Derrick Stolee
Date: Thu, 31 May 2018 15:05:00 -0400
Subject: [PATCH] t5319: expand test data
As we build the multi-pack-index file format, we want to test the format
on real repositories. Add tests that 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
---
t/t5319-multi-pack-index.sh | 83 +
1 file changed, 83 insertions(+)
diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index 2ecc369529..a50be41bc0 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -13,9 +13,92 @@ midx_read_expect () {
}
test_expect_success 'write midx with no packs' '
+ test_when_finished rm -f pack/multi-pack-index &&
git multi-pack-index --object-dir=. write &&
test_path_is_file pack/multi-pack-index &&
midx_read_expect
'
+generate_objects () {
+ i=$1
+ iii=$(printf '%03i' $i)
+ {
+ test-tool genrandom "bar" 200 &&
+ test-tool genrandom "baz $iii" 50
+ } >wide_delta_$iii &&
+ {
+ test-tool genrandom "foo"$i 100 &&
+ test-tool genrandom "foo"$(( $i + 1 )) 100 &&
+ test-tool genrandom "foo"$(( $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
+}
+
+commit_and_list_objects () {
+ {
+ 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+ git ls-tree $tree | sed -e "s/.* \\([0-9a-f]*\\)
.*/\\1/"
+ } >obj-list &&
+ git reset --hard $commit
+}
+
+test_expect_success 'create objects' '
+ test_commit initial &&
+ for i in $(test_seq 1 5)
+ do
+ generate_objects $i
+ done &&
+ commit_and_list_objects
+'
+
+test_expect_success 'write midx with one v1 pack' '
+ pack=$(git pack-objects --index-version=1 pack/test + 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
+'
+
+test_expect_success 'write midx with one v2 pack' '
+ git pack-objects --index-version=2,0x40 pack/test
Re: [PATCH v3 07/24] multi-pack-index: expand test data
On Fri, Jul 6, 2018 at 12:36 AM Eric Sunshine wrote: > On Thu, Jul 5, 2018 at 8:54 PM Derrick Stolee wrote: > > +test_expect_success 'write midx with one v1 pack' ' > > + pack=$(git pack-objects --index-version=1 pack/test > + 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. Ignore this comment. As mentioned in my follow-up to 6/24, I missed the fact that midx_read_expect() is doing more than just creating the 'expect' file.
Re: [PATCH v3 07/24] multi-pack-index: expand test data
On Thu, Jul 5, 2018 at 8:54 PM Derrick Stolee 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...
> 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
> ---
> 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.
> 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.)
> + 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 + echo $tree &&
> + git ls-tree $tree | sed -e "s/.* \\([0-9a-f]*\\).*/\\1/"
> + } >obj-list &&
Perhaps indent the content of the {...} block?
> + git update-ref HEAD $commit
> +'
> +
> +test_expect_success 'write midx with one v1 pack' '
> + pack=$(git pack-objects --index-version=1 pack/test + 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/
> + 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 + 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?
> +test_expect_success 'write midx with two packs' '
> + git pack-objects --index-version=1 pack/test-2 + git multi-pack-index --object-dir=. write &&
> + midx_read_expect
> +'
> +
> +test_expect_success 'Add more packs' '
s/Add/add/
> + for j in $(test_seq 1 10)
> + do
> + [...]
> + done
> +'
