From 54898998120ba5b317678427e5e5612a4b58792a Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 1 May 2021 10:02:59 -0400 Subject: t5300: modernize basic tests The first set of tests in t5300 goes back to 2005, and doesn't use some of our customary style and tools these days. In preparation for touching them, let's modernize a few things: - titles go on the line with test_expect_success, with a hanging open-quote to start the test body - test bodies should be indented with tabs - opening braces for shell blocks in &&-chains go on their own line - no space between redirect operators and files (">foo", not "> foo") - avoid doing work outside of test blocks; in this case, we can stick the setup of ".git2" into the appropriate blocks - avoid modifying and then cleaning up the environment or current directory by using subshells and "git -C" - this test does a curious thing when testing the unpacking: it sets GIT_OBJECT_DIRECTORY, and then does a "git init" in the _original_ directory, creating a weird mixed situation. Instead, it's much simpler to just "git init --bare" a new repository to unpack into, and check the results there. I renamed this "git2" instead of ".git2" to make it more clear it's a separate repo. - we can observe that the bodies of the no-delta, ref_delta, and ofs_delta cases are all virtually identical except for the pack creation, and factor out shared helper functions. I collapsed "do the unpack" and "check the results of the unpack" into a single test, since that makes the expected lifetime of the "git2" temporary directory more clear (that also lets us use test_when_finished to clean it up). This does make the "-v" output slightly less useful, but the improvement in reading the actual test code makes it worth it. - I dropped the "pwd" calls from some tests. These don't do anything functional, and I suspect may have been an aid for debugging when the script was more cavalier about leaving the working directory changed between tests. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh index 2fc5e68..1e10c83 100755 --- a/t/t5300-pack-object.sh +++ b/t/t5300-pack-object.sh @@ -8,125 +8,74 @@ test_description='git pack-object ' . ./test-lib.sh -TRASH=$(pwd) - -test_expect_success \ - 'setup' \ - 'rm -f .git/index* && - perl -e "print \"a\" x 4096;" > a && - perl -e "print \"b\" x 4096;" > b && - perl -e "print \"c\" x 4096;" > c && - test-tool genrandom "seed a" 2097152 > a_big && - test-tool genrandom "seed b" 2097152 > b_big && - git update-index --add a a_big b b_big c && - cat c >d && echo foo >>d && git update-index --add d && - tree=$(git write-tree) && - commit=$(git commit-tree $tree obj-list && { - git diff-tree --root -p $commit && - while read object - do - t=$(git cat-file -t $object) && - git cat-file $t $object || return 1 - done expect' - -test_expect_success \ - 'pack without delta' \ - 'packname_1=$(git pack-objects --window=0 test-1 a && + perl -e "print \"b\" x 4096;" >b && + perl -e "print \"c\" x 4096;" >c && + test-tool genrandom "seed a" 2097152 >a_big && + test-tool genrandom "seed b" 2097152 >b_big && + git update-index --add a a_big b b_big c && + cat c >d && echo foo >>d && git update-index --add d && + tree=$(git write-tree) && + commit=$(git commit-tree $tree obj-list && + { + git diff-tree --root -p $commit && + while read object + do + t=$(git cat-file -t $object) && + git cat-file $t $object || return 1 + done expect +' -test_expect_success \ - 'check unpack without delta' \ - '(cd ../.git && find objects -type f -print) | - while read path - do - cmp $path ../.git/$path || { - echo $path differs. - return 1 - } - done' -cd "$TRASH" +test_expect_success 'pack without delta' ' + packname_1=$(git pack-objects --window=0 test-1 current && + cmp expect current +} -test_expect_success \ - 'use packed objects' \ - 'GIT_OBJECT_DIRECTORY=.git2/objects && - export GIT_OBJECT_DIRECTORY && - git init && - cp test-1-${packname_1}.pack test-1-${packname_1}.idx .git2/objects/pack && { - git diff-tree --root -p $commit && - while read object - do - t=$(git cat-file -t $object) && - git cat-file $t $object || return 1 - done current && - cmp expect current' +test_expect_success 'use packed objects' ' + check_use_objects test-1-${packname_1} +' -test_expect_success \ - 'use packed deltified (REF_DELTA) objects' \ - 'GIT_OBJECT_DIRECTORY=.git2/objects && - export GIT_OBJECT_DIRECTORY && - rm -f .git2/objects/pack/test-* && - cp test-2-${packname_2}.pack test-2-${packname_2}.idx .git2/objects/pack && { - git diff-tree --root -p $commit && - while read object - do - t=$(git cat-file -t $object) && - git cat-file $t $object || return 1 - done current && - cmp expect current' +test_expect_success 'use packed deltified (REF_DELTA) objects' ' + check_use_objects test-2-${packname_2} +' -test_expect_success \ - 'use packed deltified (OFS_DELTA) objects' \ - 'GIT_OBJECT_DIRECTORY=.git2/objects && - export GIT_OBJECT_DIRECTORY && - rm -f .git2/objects/pack/test-* && - cp test-3-${packname_3}.pack test-3-${packname_3}.idx .git2/objects/pack && { - git diff-tree --root -p $commit && - while read object - do - t=$(git cat-file -t $object) && - git cat-file $t $object || return 1 - done current && - cmp expect current' - -unset GIT_OBJECT_DIRECTORY +test_expect_success 'use packed deltified (OFS_DELTA) objects' ' + check_use_objects test-3-${packname_3} +' test_expect_success 'survive missing objects/pack directory' ' ( -- cgit v0.10.2-6-g49f6 From 95356789ee5e65abbdf3f354b8be4c79b196e8a1 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 1 May 2021 10:03:25 -0400 Subject: t5300: check that we produced expected number of deltas We pack a set of objects both with and without --window=0, assuming that the 0-length window will cause us not to produce any deltas. Let's confirm that this is the case. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh index 1e10c83..887e2d8 100755 --- a/t/t5300-pack-object.sh +++ b/t/t5300-pack-object.sh @@ -34,8 +34,22 @@ test_expect_success 'setup' ' } >expect ' +# usage: check_deltas +# e.g.: check_deltas stderr -gt 0 +check_deltas() { + deltas=$(perl -lne '/delta (\d+)/ and print $1' "$1") && + shift && + if ! test "$deltas" "$@" + then + echo >&2 "unexpected number of deltas (compared $delta $*)" + return 1 + fi +} + test_expect_success 'pack without delta' ' - packname_1=$(git pack-objects --window=0 test-1 stderr) && + check_deltas stderr = 0 ' test_expect_success 'pack-objects with bogus arguments' ' @@ -62,7 +76,8 @@ test_expect_success 'unpack without delta' ' ' test_expect_success 'pack with REF_DELTA' ' - packname_2=$(git pack-objects test-2 stderr) && + check_deltas stderr -gt 0 ' test_expect_success 'unpack with REF_DELTA' ' @@ -70,7 +85,9 @@ test_expect_success 'unpack with REF_DELTA' ' ' test_expect_success 'pack with OFS_DELTA' ' - packname_3=$(git pack-objects --delta-base-offset test-3 stderr) && + check_deltas stderr -gt 0 ' test_expect_success 'unpack with OFS_DELTA' ' -- cgit v0.10.2-6-g49f6 From 953aa54e1a6a7571d48eddea3ee68745bf94e69d Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 1 May 2021 10:03:37 -0400 Subject: pack-objects: clamp negative window size to 0 A negative window size makes no sense, and the code in find_deltas() is not prepared to handle it. If you pass "-1", for example, we end up generate a 0-length array of "struct unpacked", but our loop assumes it has at least one entry in it (and we end up reading garbage memory). We could complain to the user about this, but it's more forgiving to just clamp it to 0, which means "do not find any deltas at all". The 0-case is already tested earlier in the script, so we'll make sure this does the same thing. Reported-by: Yiyuan guo Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index 6d13cd3..ea7a5b3 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -3871,6 +3871,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix) (1U << OE_Z_DELTA_BITS) - 1); cache_max_small_delta_size = (1U << OE_Z_DELTA_BITS) - 1; } + if (window < 0) + window = 0; strvec_push(&rp, "pack-objects"); if (thin) { diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh index 887e2d8..5c5e53f 100755 --- a/t/t5300-pack-object.sh +++ b/t/t5300-pack-object.sh @@ -613,4 +613,9 @@ test_expect_success '--stdin-packs with broken links' ' ) ' +test_expect_success 'negative window clamps to 0' ' + git pack-objects --progress --window=-1 neg-window stderr && + check_deltas stderr = 0 +' + test_done -- cgit v0.10.2-6-g49f6 From 49ac1d33bbd9d4be4ad8cb9730a07c086f688a5b Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 1 May 2021 10:04:00 -0400 Subject: t5316: check behavior of pack-objects --depth=0 We'd expect this to cleanly produce no deltas at all (as opposed to getting confused by an out-of-bounds value), and it does. Note we have to adjust our max_chain test helper, which expected to find at least one delta. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/t/t5316-pack-delta-depth.sh b/t/t5316-pack-delta-depth.sh index a8c1bc0..3e84df4 100755 --- a/t/t5316-pack-delta-depth.sh +++ b/t/t5316-pack-delta-depth.sh @@ -69,6 +69,7 @@ test_expect_success 'create series of packs' ' max_chain() { git index-pack --verify-stat-only "$1" >output && perl -lne ' + BEGIN { $len = 0 } /chain length = (\d+)/ and $len = $1; END { print $len } ' output @@ -94,4 +95,11 @@ test_expect_success '--depth limits depth' ' test_cmp expect actual ' +test_expect_success '--depth=0 disables deltas' ' + pack=$(git pack-objects --all --depth=0 expect && + max_chain pack-$pack.pack >actual && + test_cmp expect actual +' + test_done -- cgit v0.10.2-6-g49f6 From 6d52b6a5dfe0bee20dbfb2a3a7291bcf9a152672 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 1 May 2021 10:04:34 -0400 Subject: pack-objects: clamp negative depth to 0 A negative delta depth makes no sense, and the code is not prepared to handle it. If passed "--depth=-1" on the command line, then this line from break_delta_chains(): cur->depth = (total_depth--) % (depth + 1); triggers a divide-by-zero. This is undefined behavior according to the C standard, but on POSIX systems results in SIGFPE killing the process. This is certainly one way to inform the use that the command was invalid, but it's a bit friendlier to just treat it as "don't allow any deltas", which we already do for --depth=0. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index ea7a5b3..da5e070 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -3861,6 +3861,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix) if (pack_to_stdout != !base_name || argc) usage_with_options(pack_usage, pack_objects_options); + if (depth < 0) + depth = 0; if (depth >= (1 << OE_DEPTH_BITS)) { warning(_("delta chain depth %d is too deep, forcing %d"), depth, (1 << OE_DEPTH_BITS) - 1); diff --git a/t/t5316-pack-delta-depth.sh b/t/t5316-pack-delta-depth.sh index 3e84df4..759169d 100755 --- a/t/t5316-pack-delta-depth.sh +++ b/t/t5316-pack-delta-depth.sh @@ -102,4 +102,11 @@ test_expect_success '--depth=0 disables deltas' ' test_cmp expect actual ' +test_expect_success 'negative depth disables deltas' ' + pack=$(git pack-objects --all --depth=-1 expect && + max_chain pack-$pack.pack >actual && + test_cmp expect actual +' + test_done -- cgit v0.10.2-6-g49f6