| Message ID | 20200125230035.136348-12-sandals@crustytoothpaste.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SHA-256 test fixes, part 8 | expand |
Hi brian, On Sat, 25 Jan 2020, brian m. carlson wrote: > Compute the length of object IDs and pack offsets instead of hard-coding > constants. > > Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> > --- > t/t5302-pack-index.sh | 18 +++++++++++------- > 1 file changed, 11 insertions(+), 7 deletions(-) > > diff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh > index 91d51b35f9..93ac003639 100755 > --- a/t/t5302-pack-index.sh > +++ b/t/t5302-pack-index.sh > @@ -8,7 +8,8 @@ test_description='pack index with 64-bit offsets and object CRC' > > test_expect_success \ > 'setup' \ > - 'rm -rf .git && > + 'test_oid_init && > + rm -rf .git && Why not consolidate the `test_expect_success` line into the current convention at the same time ("while at it")? I.e. test_expect_success 'setup' ' > git init && > git config pack.threads 1 && > i=1 && > @@ -32,7 +33,9 @@ test_expect_success \ > echo $tree && > git ls-tree $tree | sed -e "s/.* \\([0-9a-f]*\\) .*/\\1/" > } >obj-list && > - git update-ref HEAD $commit' > + git update-ref HEAD $commit && > + rawsz=$(test_oid rawsz) Since the `rawsz` assignment has a lot to do with `test_oid_init`, I would coddle this added line with the `test_oid_init` line above instead of adding it here. > +' > > test_expect_success \ > 'pack-objects with index version 1' \ > @@ -152,6 +155,7 @@ test_expect_success \ > '[index v1] 2) create a stealth corruption in a delta base reference' \ > '# This test assumes file_101 is a delta smaller than 16 bytes. > # It should be against file_100 but we substitute its base for file_099 > + offset=$((rawsz + 4)) && > sha1_101=$(git hash-object file_101) && > sha1_099=$(git hash-object file_099) && > offs_101=$(index_obj_offset 1.idx $sha1_101) && > @@ -159,8 +163,8 @@ test_expect_success \ > chmod +w ".git/objects/pack/pack-${pack1}.pack" && > dd of=".git/objects/pack/pack-${pack1}.pack" seek=$(($offs_101 + 1)) \ > if=".git/objects/pack/pack-${pack1}.idx" \ > - skip=$((4 + 256 * 4 + $nr_099 * 24)) \ > - bs=1 count=20 conv=notrunc && > + skip=$((4 + 256 * 4 + $nr_099 * offset)) \ > + bs=1 count=$rawsz conv=notrunc && Similarly, the `offset` variable is only used here, so I would assign it just before the `dd` call. The name `offset` might be a bit to generic not to be reused, either, maybe `recordsz` or `index_entry_size` or `entrysz`? Apart from that, the patch looks obviously good to me. Ciao, Dscho P.S.: I'll stop reviewing here for now (It is not that I am tired of looking at your patches, it is that I am just tired). > git cat-file blob $sha1_101 > file_101_foo1' > > test_expect_success \ > @@ -200,8 +204,8 @@ test_expect_success \ > chmod +w ".git/objects/pack/pack-${pack1}.pack" && > dd of=".git/objects/pack/pack-${pack1}.pack" seek=$(($offs_101 + 1)) \ > if=".git/objects/pack/pack-${pack1}.idx" \ > - skip=$((8 + 256 * 4 + $nr_099 * 20)) \ > - bs=1 count=20 conv=notrunc && > + skip=$((8 + 256 * 4 + $nr_099 * rawsz)) \ > + bs=1 count=$rawsz conv=notrunc && > git cat-file blob $sha1_101 > file_101_foo2' > > test_expect_success \ > @@ -226,7 +230,7 @@ test_expect_success \ > nr=$(index_obj_nr ".git/objects/pack/pack-${pack1}.idx" $obj) && > chmod +w ".git/objects/pack/pack-${pack1}.idx" && > printf xxxx | dd of=".git/objects/pack/pack-${pack1}.idx" conv=notrunc \ > - bs=1 count=4 seek=$((8 + 256 * 4 + $(wc -l <obj-list) * 20 + $nr * 4)) && > + bs=1 count=4 seek=$((8 + 256 * 4 + $(wc -l <obj-list) * rawsz + $nr * 4)) && > ( while read obj > do git cat-file -p $obj >/dev/null || exit 1 > done <obj-list ) && >
On 2020-01-26 at 22:23:32, Johannes Schindelin wrote: > On Sat, 25 Jan 2020, brian m. carlson wrote: > > > Compute the length of object IDs and pack offsets instead of hard-coding > > constants. > > > > Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> > > --- > > t/t5302-pack-index.sh | 18 +++++++++++------- > > 1 file changed, 11 insertions(+), 7 deletions(-) > > > > diff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh > > index 91d51b35f9..93ac003639 100755 > > --- a/t/t5302-pack-index.sh > > +++ b/t/t5302-pack-index.sh > > @@ -8,7 +8,8 @@ test_description='pack index with 64-bit offsets and object CRC' > > > > test_expect_success \ > > 'setup' \ > > - 'rm -rf .git && > > + 'test_oid_init && > > + rm -rf .git && > > Why not consolidate the `test_expect_success` line into the current > convention at the same time ("while at it")? I.e. > > test_expect_success 'setup' ' Sounds good. > > git init && > > git config pack.threads 1 && > > i=1 && > > @@ -32,7 +33,9 @@ test_expect_success \ > > echo $tree && > > git ls-tree $tree | sed -e "s/.* \\([0-9a-f]*\\) .*/\\1/" > > } >obj-list && > > - git update-ref HEAD $commit' > > + git update-ref HEAD $commit && > > + rawsz=$(test_oid rawsz) > > Since the `rawsz` assignment has a lot to do with `test_oid_init`, I would > coddle this added line with the `test_oid_init` line above instead of > adding it here. Sure, that seems like a good idea. > > +' > > > > test_expect_success \ > > 'pack-objects with index version 1' \ > > @@ -152,6 +155,7 @@ test_expect_success \ > > '[index v1] 2) create a stealth corruption in a delta base reference' \ > > '# This test assumes file_101 is a delta smaller than 16 bytes. > > # It should be against file_100 but we substitute its base for file_099 > > + offset=$((rawsz + 4)) && > > sha1_101=$(git hash-object file_101) && > > sha1_099=$(git hash-object file_099) && > > offs_101=$(index_obj_offset 1.idx $sha1_101) && > > @@ -159,8 +163,8 @@ test_expect_success \ > > chmod +w ".git/objects/pack/pack-${pack1}.pack" && > > dd of=".git/objects/pack/pack-${pack1}.pack" seek=$(($offs_101 + 1)) \ > > if=".git/objects/pack/pack-${pack1}.idx" \ > > - skip=$((4 + 256 * 4 + $nr_099 * 24)) \ > > - bs=1 count=20 conv=notrunc && > > + skip=$((4 + 256 * 4 + $nr_099 * offset)) \ > > + bs=1 count=$rawsz conv=notrunc && > > Similarly, the `offset` variable is only used here, so I would assign it > just before the `dd` call. The name `offset` might be a bit to generic not > to be reused, either, maybe `recordsz` or `index_entry_size` or `entrysz`? Yeah, those would be better names. As I will freely admit, I'm bad at naming things. > P.S.: I'll stop reviewing here for now (It is not that I am tired of > looking at your patches, it is that I am just tired). Sure. It's late where you are. I appreciate your review.
diff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh index 91d51b35f9..93ac003639 100755 --- a/t/t5302-pack-index.sh +++ b/t/t5302-pack-index.sh @@ -8,7 +8,8 @@ test_description='pack index with 64-bit offsets and object CRC' test_expect_success \ 'setup' \ - 'rm -rf .git && + 'test_oid_init && + rm -rf .git && git init && git config pack.threads 1 && i=1 && @@ -32,7 +33,9 @@ test_expect_success \ echo $tree && git ls-tree $tree | sed -e "s/.* \\([0-9a-f]*\\) .*/\\1/" } >obj-list && - git update-ref HEAD $commit' + git update-ref HEAD $commit && + rawsz=$(test_oid rawsz) +' test_expect_success \ 'pack-objects with index version 1' \ @@ -152,6 +155,7 @@ test_expect_success \ '[index v1] 2) create a stealth corruption in a delta base reference' \ '# This test assumes file_101 is a delta smaller than 16 bytes. # It should be against file_100 but we substitute its base for file_099 + offset=$((rawsz + 4)) && sha1_101=$(git hash-object file_101) && sha1_099=$(git hash-object file_099) && offs_101=$(index_obj_offset 1.idx $sha1_101) && @@ -159,8 +163,8 @@ test_expect_success \ chmod +w ".git/objects/pack/pack-${pack1}.pack" && dd of=".git/objects/pack/pack-${pack1}.pack" seek=$(($offs_101 + 1)) \ if=".git/objects/pack/pack-${pack1}.idx" \ - skip=$((4 + 256 * 4 + $nr_099 * 24)) \ - bs=1 count=20 conv=notrunc && + skip=$((4 + 256 * 4 + $nr_099 * offset)) \ + bs=1 count=$rawsz conv=notrunc && git cat-file blob $sha1_101 > file_101_foo1' test_expect_success \ @@ -200,8 +204,8 @@ test_expect_success \ chmod +w ".git/objects/pack/pack-${pack1}.pack" && dd of=".git/objects/pack/pack-${pack1}.pack" seek=$(($offs_101 + 1)) \ if=".git/objects/pack/pack-${pack1}.idx" \ - skip=$((8 + 256 * 4 + $nr_099 * 20)) \ - bs=1 count=20 conv=notrunc && + skip=$((8 + 256 * 4 + $nr_099 * rawsz)) \ + bs=1 count=$rawsz conv=notrunc && git cat-file blob $sha1_101 > file_101_foo2' test_expect_success \ @@ -226,7 +230,7 @@ test_expect_success \ nr=$(index_obj_nr ".git/objects/pack/pack-${pack1}.idx" $obj) && chmod +w ".git/objects/pack/pack-${pack1}.idx" && printf xxxx | dd of=".git/objects/pack/pack-${pack1}.idx" conv=notrunc \ - bs=1 count=4 seek=$((8 + 256 * 4 + $(wc -l <obj-list) * 20 + $nr * 4)) && + bs=1 count=4 seek=$((8 + 256 * 4 + $(wc -l <obj-list) * rawsz + $nr * 4)) && ( while read obj do git cat-file -p $obj >/dev/null || exit 1 done <obj-list ) &&
Compute the length of object IDs and pack offsets instead of hard-coding constants. Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> --- t/t5302-pack-index.sh | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)