{"thread":{"id":"55601","subject":"[PATCH 0/5] pack-objects: better handling of negative options","startedAt":"2021-05-01T14:02:22Z","lastAt":"2021-05-05T16:19:24Z","messageCount":15,"participants":["Jeff King","Junio C Hamano","Derrick Stolee","René Scharfe","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"423399","messageId":"YI1fbERSuS7Y3LKh@coredump.intra.peff.net","threadId":"55601","inReplyTo":null,"subject":"[PATCH 0/5] pack-objects: better handling of negative options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-01T14:02:20Z","receivedAt":"2021-05-01T14:02:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This series fixes a few problems discussed on the security list, but I\ndon't think the security implications are that interesting (an attacker\nwould need to control config or command-line options, which are already\npretty dangerous, and can mostly just cause the process to abort). But\nstill worth addressing.\n\nPatches 3 and 5 are the interesting ones. The rest are just cleaning up\nor improving test coverage.\n\n  [1/5]: t5300: modernize basic tests\n  [2/5]: t5300: check that we produced expected number of deltas\n  [3/5]: pack-objects: clamp negative window size to 0\n  [4/5]: t5316: check behavior of pack-objects --depth=0\n  [5/5]: pack-objects: clamp negative depth to 0\n\n builtin/pack-objects.c      |   4 +\n t/t5300-pack-object.sh      | 265 +++++++++++++++---------------------\n t/t5316-pack-delta-depth.sh |  15 ++\n 3 files changed, 126 insertions(+), 158 deletions(-)\n\n-Peff\n"},{"id":"423400","messageId":"YI1fk3nE6AJRK8qS@coredump.intra.peff.net","threadId":"55601","inReplyTo":"YI1fbERSuS7Y3LKh@coredump.intra.peff.net","subject":"[PATCH 1/5] t5300: modernize basic tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-01T14:02:59Z","receivedAt":"2021-05-01T14:03:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The first set of tests in t5300 goes back to 2005, and doesn't use some\nof our customary style and tools these days. In preparation for touching\nthem, let's modernize a few things:\n\n  - titles go on the line with test_expect_success, with a hanging\n    open-quote to start the test body\n\n  - test bodies should be indented with tabs\n\n  - opening braces for shell blocks in &&-chains go on their own line\n\n  - no space between redirect operators and files (\">foo\", not \"> foo\")\n\n  - avoid doing work outside of test blocks; in this case, we can stick\n    the setup of \".git2\" into the appropriate blocks\n\n  - avoid modifying and then cleaning up the environment or current\n    directory by using subshells and \"git -C\"\n\n  - this test does a curious thing when testing the unpacking: it sets\n    GIT_OBJECT_DIRECTORY, and then does a \"git init\" in the _original_\n    directory, creating a weird mixed situation. Instead, it's much\n    simpler to just \"git init --bare\" a new repository to unpack into,\n    and check the results there. I renamed this \"git2\" instead of\n    \".git2\" to make it more clear it's a separate repo.\n\n  - we can observe that the bodies of the no-delta, ref_delta, and\n    ofs_delta cases are all virtually identical except for the pack\n    creation, and factor out shared helper functions. I collapsed \"do\n    the unpack\" and \"check the results of the unpack\" into a single\n    test, since that makes the expected lifetime of the \"git2\" temporary\n    directory more clear (that also lets us use test_when_finished to\n    clean it up). This does make the \"-v\" output slightly less useful,\n    but the improvement in reading the actual test code makes it worth\n    it.\n\n  - I dropped the \"pwd\" calls from some tests. These don't do anything\n    functional, and I suspect may have been an aid for debugging when\n    the script was more cavalier about leaving the working directory\n    changed between tests.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5300-pack-object.sh | 243 ++++++++++++++---------------------------\n 1 file changed, 85 insertions(+), 158 deletions(-)\n\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex 2fc5e68250..1e10c832a6 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -8,125 +8,74 @@ test_description='git pack-object\n '\n . ./test-lib.sh\n \n-TRASH=$(pwd)\n-\n-test_expect_success \\\n-    'setup' \\\n-    'rm -f .git/index* &&\n-     perl -e \"print \\\"a\\\" x 4096;\" > a &&\n-     perl -e \"print \\\"b\\\" x 4096;\" > b &&\n-     perl -e \"print \\\"c\\\" x 4096;\" > c &&\n-     test-tool genrandom \"seed a\" 2097152 > a_big &&\n-     test-tool genrandom \"seed b\" 2097152 > b_big &&\n-     git update-index --add a a_big b b_big c &&\n-     cat c >d && echo foo >>d && git update-index --add d &&\n-     tree=$(git write-tree) &&\n-     commit=$(git commit-tree $tree </dev/null) && {\n-\t echo $tree &&\n-\t echo $commit &&\n-\t git ls-tree $tree | sed -e \"s/.* \\\\([0-9a-f]*\\\\)\t.*/\\\\1/\"\n-     } >obj-list && {\n-\t git diff-tree --root -p $commit &&\n-\t while read object\n-\t do\n-\t    t=$(git cat-file -t $object) &&\n-\t    git cat-file $t $object || return 1\n-\t done <obj-list\n-     } >expect'\n-\n-test_expect_success \\\n-    'pack without delta' \\\n-    'packname_1=$(git pack-objects --window=0 test-1 <obj-list)'\n-\n-test_expect_success \\\n-    'pack-objects with bogus arguments' \\\n-    'test_must_fail git pack-objects --window=0 test-1 blah blah <obj-list'\n-\n-rm -fr .git2\n-mkdir .git2\n-\n-test_expect_success \\\n-    'unpack without delta' \\\n-    \"GIT_OBJECT_DIRECTORY=.git2/objects &&\n-     export GIT_OBJECT_DIRECTORY &&\n-     git init &&\n-     git unpack-objects -n <test-1-${packname_1}.pack &&\n-     git unpack-objects <test-1-${packname_1}.pack\"\n-\n-unset GIT_OBJECT_DIRECTORY\n-cd \"$TRASH/.git2\"\n+test_expect_success 'setup' '\n+\trm -f .git/index* &&\n+\tperl -e \"print \\\"a\\\" x 4096;\" >a &&\n+\tperl -e \"print \\\"b\\\" x 4096;\" >b &&\n+\tperl -e \"print \\\"c\\\" x 4096;\" >c &&\n+\ttest-tool genrandom \"seed a\" 2097152 >a_big &&\n+\ttest-tool genrandom \"seed b\" 2097152 >b_big &&\n+\tgit update-index --add a a_big b b_big c &&\n+\tcat c >d && echo foo >>d && git update-index --add d &&\n+\ttree=$(git write-tree) &&\n+\tcommit=$(git commit-tree $tree </dev/null) &&\n+\t{\n+\t\techo $tree &&\n+\t\techo $commit &&\n+\t\tgit ls-tree $tree | sed -e \"s/.* \\\\([0-9a-f]*\\\\)\t.*/\\\\1/\"\n+\t} >obj-list &&\n+\t{\n+\t\tgit diff-tree --root -p $commit &&\n+\t\twhile read object\n+\t\tdo\n+\t\t\tt=$(git cat-file -t $object) &&\n+\t\t\tgit cat-file $t $object || return 1\n+\t\tdone <obj-list\n+\t} >expect\n+'\n \n-test_expect_success \\\n-    'check unpack without delta' \\\n-    '(cd ../.git && find objects -type f -print) |\n-     while read path\n-     do\n-         cmp $path ../.git/$path || {\n-\t     echo $path differs.\n-\t     return 1\n-\t }\n-     done'\n-cd \"$TRASH\"\n+test_expect_success 'pack without delta' '\n+\tpackname_1=$(git pack-objects --window=0 test-1 <obj-list)\n+'\n \n-test_expect_success \\\n-    'pack with REF_DELTA' \\\n-    'pwd &&\n-     packname_2=$(git pack-objects test-2 <obj-list)'\n+test_expect_success 'pack-objects with bogus arguments' '\n+\ttest_must_fail git pack-objects --window=0 test-1 blah blah <obj-list\n+'\n \n-rm -fr .git2\n-mkdir .git2\n+check_unpack () {\n+\ttest_when_finished \"rm -rf git2\" &&\n+\tgit init --bare git2 &&\n+\tgit -C git2 unpack-objects -n <\"$1\".pack &&\n+\tgit -C git2 unpack-objects <\"$1\".pack &&\n+\t(cd .git && find objects -type f -print) |\n+\twhile read path\n+\tdo\n+\t\tcmp git2/$path .git/$path || {\n+\t\t\techo $path differs.\n+\t\t\treturn 1\n+\t\t}\n+\tdone\n+}\n+\n+test_expect_success 'unpack without delta' '\n+\tcheck_unpack test-1-${packname_1}\n+'\n \n-test_expect_success \\\n-    'unpack with REF_DELTA' \\\n-    'GIT_OBJECT_DIRECTORY=.git2/objects &&\n-     export GIT_OBJECT_DIRECTORY &&\n-     git init &&\n-     git unpack-objects -n <test-2-${packname_2}.pack &&\n-     git unpack-objects <test-2-${packname_2}.pack'\n-\n-unset GIT_OBJECT_DIRECTORY\n-cd \"$TRASH/.git2\"\n-test_expect_success \\\n-    'check unpack with REF_DELTA' \\\n-    '(cd ../.git && find objects -type f -print) |\n-     while read path\n-     do\n-         cmp $path ../.git/$path || {\n-\t     echo $path differs.\n-\t     return 1\n-\t }\n-     done'\n-cd \"$TRASH\"\n+test_expect_success 'pack with REF_DELTA' '\n+\tpackname_2=$(git pack-objects test-2 <obj-list)\n+'\n \n-test_expect_success \\\n-    'pack with OFS_DELTA' \\\n-    'pwd &&\n-     packname_3=$(git pack-objects --delta-base-offset test-3 <obj-list)'\n+test_expect_success 'unpack with REF_DELTA' '\n+\tcheck_unpack test-2-${packname_2}\n+'\n \n-rm -fr .git2\n-mkdir .git2\n+test_expect_success 'pack with OFS_DELTA' '\n+\tpackname_3=$(git pack-objects --delta-base-offset test-3 <obj-list)\n+'\n \n-test_expect_success \\\n-    'unpack with OFS_DELTA' \\\n-    'GIT_OBJECT_DIRECTORY=.git2/objects &&\n-     export GIT_OBJECT_DIRECTORY &&\n-     git init &&\n-     git unpack-objects -n <test-3-${packname_3}.pack &&\n-     git unpack-objects <test-3-${packname_3}.pack'\n-\n-unset GIT_OBJECT_DIRECTORY\n-cd \"$TRASH/.git2\"\n-test_expect_success \\\n-    'check unpack with OFS_DELTA' \\\n-    '(cd ../.git && find objects -type f -print) |\n-     while read path\n-     do\n-         cmp $path ../.git/$path || {\n-\t     echo $path differs.\n-\t     return 1\n-\t }\n-     done'\n-cd \"$TRASH\"\n+test_expect_success 'unpack with OFS_DELTA' '\n+\tcheck_unpack test-3-${packname_3}\n+'\n \n test_expect_success 'compare delta flavors' '\n \tperl -e '\\''\n@@ -135,55 +84,33 @@ test_expect_success 'compare delta flavors' '\n \t'\\'' test-2-$packname_2.pack test-3-$packname_3.pack\n '\n \n-rm -fr .git2\n-mkdir .git2\n+check_use_objects () {\n+\ttest_when_finished \"rm -rf git2\" &&\n+\tgit init --bare git2 &&\n+\tcp \"$1\".pack \"$1\".idx git2/objects/pack &&\n+\t(\n+\t\tcd git2 &&\n+\t\tgit diff-tree --root -p $commit &&\n+\t\twhile read object\n+\t\tdo\n+\t\t\tt=$(git cat-file -t $object) &&\n+\t\t\tgit cat-file $t $object || exit 1\n+\t\tdone\n+\t) <obj-list >current &&\n+\tcmp expect current\n+}\n \n-test_expect_success \\\n-    'use packed objects' \\\n-    'GIT_OBJECT_DIRECTORY=.git2/objects &&\n-     export GIT_OBJECT_DIRECTORY &&\n-     git init &&\n-     cp test-1-${packname_1}.pack test-1-${packname_1}.idx .git2/objects/pack && {\n-\t git diff-tree --root -p $commit &&\n-\t while read object\n-\t do\n-\t    t=$(git cat-file -t $object) &&\n-\t    git cat-file $t $object || return 1\n-\t done <obj-list\n-    } >current &&\n-    cmp expect current'\n+test_expect_success 'use packed objects' '\n+\tcheck_use_objects test-1-${packname_1}\n+'\n \n-test_expect_success \\\n-    'use packed deltified (REF_DELTA) objects' \\\n-    'GIT_OBJECT_DIRECTORY=.git2/objects &&\n-     export GIT_OBJECT_DIRECTORY &&\n-     rm -f .git2/objects/pack/test-* &&\n-     cp test-2-${packname_2}.pack test-2-${packname_2}.idx .git2/objects/pack && {\n-\t git diff-tree --root -p $commit &&\n-\t while read object\n-\t do\n-\t    t=$(git cat-file -t $object) &&\n-\t    git cat-file $t $object || return 1\n-\t done <obj-list\n-    } >current &&\n-    cmp expect current'\n+test_expect_success 'use packed deltified (REF_DELTA) objects' '\n+\tcheck_use_objects test-2-${packname_2}\n+'\n \n-test_expect_success \\\n-    'use packed deltified (OFS_DELTA) objects' \\\n-    'GIT_OBJECT_DIRECTORY=.git2/objects &&\n-     export GIT_OBJECT_DIRECTORY &&\n-     rm -f .git2/objects/pack/test-* &&\n-     cp test-3-${packname_3}.pack test-3-${packname_3}.idx .git2/objects/pack && {\n-\t git diff-tree --root -p $commit &&\n-\t while read object\n-\t do\n-\t    t=$(git cat-file -t $object) &&\n-\t    git cat-file $t $object || return 1\n-\t done <obj-list\n-    } >current &&\n-    cmp expect current'\n-\n-unset GIT_OBJECT_DIRECTORY\n+test_expect_success 'use packed deltified (OFS_DELTA) objects' '\n+\tcheck_use_objects test-3-${packname_3}\n+'\n \n test_expect_success 'survive missing objects/pack directory' '\n \t(\n-- \n2.31.1.875.g5dccece0aa\n\n"},{"id":"423401","messageId":"YI1frTcgfGb6GA3N@coredump.intra.peff.net","threadId":"55601","inReplyTo":"YI1fbERSuS7Y3LKh@coredump.intra.peff.net","subject":"[PATCH 2/5] t5300: check that we produced expected number of deltas","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-01T14:03:25Z","receivedAt":"2021-05-01T14:03:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We pack a set of objects both with and without --window=0, assuming that\nthe 0-length window will cause us not to produce any deltas. Let's\nconfirm that this is the case.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5300-pack-object.sh | 23 ++++++++++++++++++++---\n 1 file changed, 20 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex 1e10c832a6..887e2d8d88 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -34,8 +34,22 @@ test_expect_success 'setup' '\n \t} >expect\n '\n \n+# usage: check_deltas <stderr_from_pack_objects> <cmp_op> <nr_deltas>\n+# e.g.: check_deltas stderr -gt 0\n+check_deltas() {\n+\tdeltas=$(perl -lne '/delta (\\d+)/ and print $1' \"$1\") &&\n+\tshift &&\n+\tif ! test \"$deltas\" \"$@\"\n+\tthen\n+\t\techo >&2 \"unexpected number of deltas (compared $delta $*)\"\n+\t\treturn 1\n+\tfi\n+}\n+\n test_expect_success 'pack without delta' '\n-\tpackname_1=$(git pack-objects --window=0 test-1 <obj-list)\n+\tpackname_1=$(git pack-objects --progress --window=0 test-1 \\\n+\t\t\t<obj-list 2>stderr) &&\n+\tcheck_deltas stderr = 0\n '\n \n test_expect_success 'pack-objects with bogus arguments' '\n@@ -62,15 +76,18 @@ test_expect_success 'unpack without delta' '\n '\n \n test_expect_success 'pack with REF_DELTA' '\n-\tpackname_2=$(git pack-objects test-2 <obj-list)\n+\tpackname_2=$(git pack-objects --progress test-2 <obj-list 2>stderr) &&\n+\tcheck_deltas stderr -gt 0\n '\n \n test_expect_success 'unpack with REF_DELTA' '\n \tcheck_unpack test-2-${packname_2}\n '\n \n test_expect_success 'pack with OFS_DELTA' '\n-\tpackname_3=$(git pack-objects --delta-base-offset test-3 <obj-list)\n+\tpackname_3=$(git pack-objects --progress --delta-base-offset test-3 \\\n+\t\t\t<obj-list 2>stderr) &&\n+\tcheck_deltas stderr -gt 0\n '\n \n test_expect_success 'unpack with OFS_DELTA' '\n-- \n2.31.1.875.g5dccece0aa\n\n"},{"id":"423402","messageId":"YI1fubjvQQlrPz9D@coredump.intra.peff.net","threadId":"55601","inReplyTo":"YI1fbERSuS7Y3LKh@coredump.intra.peff.net","subject":"[PATCH 3/5] pack-objects: clamp negative window size to 0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-01T14:03:37Z","receivedAt":"2021-05-01T14:03:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"A negative window size makes no sense, and the code in find_deltas() is\nnot prepared to handle it. If you pass \"-1\", for example, we end up\ngenerate a 0-length array of \"struct unpacked\", but our loop assumes it\nhas at least one entry in it (and we end up reading garbage memory).\n\nWe could complain to the user about this, but it's more forgiving to\njust clamp it to 0, which means \"do not find any deltas at all\". The\n0-case is already tested earlier in the script, so we'll make sure this\ndoes the same thing.\n\nReported-by: Yiyuan guo <yguoaz@gmail.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/pack-objects.c | 2 ++\n t/t5300-pack-object.sh | 5 +++++\n 2 files changed, 7 insertions(+)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 6d13cd3e1a..ea7a5b3ba5 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3871,6 +3871,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\t\t(1U << OE_Z_DELTA_BITS) - 1);\n \t\tcache_max_small_delta_size = (1U << OE_Z_DELTA_BITS) - 1;\n \t}\n+\tif (window < 0)\n+\t\twindow = 0;\n \n \tstrvec_push(&rp, \"pack-objects\");\n \tif (thin) {\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex 887e2d8d88..5c5e53f0be 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -613,4 +613,9 @@ test_expect_success '--stdin-packs with broken links' '\n \t)\n '\n \n+test_expect_success 'negative window clamps to 0' '\n+\tgit pack-objects --progress --window=-1 neg-window <obj-list 2>stderr &&\n+\tcheck_deltas stderr = 0\n+'\n+\n test_done\n-- \n2.31.1.875.g5dccece0aa\n\n"},{"id":"423403","messageId":"YI1f0NesWZFqh9O2@coredump.intra.peff.net","threadId":"55601","inReplyTo":"YI1fbERSuS7Y3LKh@coredump.intra.peff.net","subject":"[PATCH 4/5] t5316: check behavior of pack-objects --depth=0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-01T14:04:00Z","receivedAt":"2021-05-01T14:04:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We'd expect this to cleanly produce no deltas at all (as opposed to\ngetting confused by an out-of-bounds value), and it does.\n\nNote we have to adjust our max_chain test helper, which expected to find\nat least one delta.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5316-pack-delta-depth.sh | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/t/t5316-pack-delta-depth.sh b/t/t5316-pack-delta-depth.sh\nindex a8c1bc0f66..3e84df4137 100755\n--- a/t/t5316-pack-delta-depth.sh\n+++ b/t/t5316-pack-delta-depth.sh\n@@ -69,6 +69,7 @@ test_expect_success 'create series of packs' '\n max_chain() {\n \tgit index-pack --verify-stat-only \"$1\" >output &&\n \tperl -lne '\n+\t  BEGIN { $len = 0 }\n \t  /chain length = (\\d+)/ and $len = $1;\n \t  END { print $len }\n \t' output\n@@ -94,4 +95,11 @@ test_expect_success '--depth limits depth' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--depth=0 disables deltas' '\n+\tpack=$(git pack-objects --all --depth=0 </dev/null pack) &&\n+\techo 0 >expect &&\n+\tmax_chain pack-$pack.pack >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.31.1.875.g5dccece0aa\n\n"},{"id":"423404","messageId":"YI1f8jKReuE3LXVH@coredump.intra.peff.net","threadId":"55601","inReplyTo":"YI1fbERSuS7Y3LKh@coredump.intra.peff.net","subject":"[PATCH 5/5] pack-objects: clamp negative depth to 0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-01T14:04:34Z","receivedAt":"2021-05-01T14:04:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"A negative delta depth makes no sense, and the code is not prepared to\nhandle it. If passed \"--depth=-1\" on the command line, then this line\nfrom break_delta_chains():\n\n\tcur->depth = (total_depth--) % (depth + 1);\n\ntriggers a divide-by-zero. This is undefined behavior according to the C\nstandard, but on POSIX systems results in SIGFPE killing the process.\nThis is certainly one way to inform the use that the command was\ninvalid, but it's a bit friendlier to just treat it as \"don't allow any\ndeltas\", which we already do for --depth=0.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/pack-objects.c      | 2 ++\n t/t5316-pack-delta-depth.sh | 7 +++++++\n 2 files changed, 9 insertions(+)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex ea7a5b3ba5..da5e0700f9 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3861,6 +3861,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \tif (pack_to_stdout != !base_name || argc)\n \t\tusage_with_options(pack_usage, pack_objects_options);\n \n+\tif (depth < 0)\n+\t\tdepth = 0;\n \tif (depth >= (1 << OE_DEPTH_BITS)) {\n \t\twarning(_(\"delta chain depth %d is too deep, forcing %d\"),\n \t\t\tdepth, (1 << OE_DEPTH_BITS) - 1);\ndiff --git a/t/t5316-pack-delta-depth.sh b/t/t5316-pack-delta-depth.sh\nindex 3e84df4137..759169d074 100755\n--- a/t/t5316-pack-delta-depth.sh\n+++ b/t/t5316-pack-delta-depth.sh\n@@ -102,4 +102,11 @@ test_expect_success '--depth=0 disables deltas' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'negative depth disables deltas' '\n+\tpack=$(git pack-objects --all --depth=-1 </dev/null pack) &&\n+\techo 0 >expect &&\n+\tmax_chain pack-$pack.pack >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.31.1.875.g5dccece0aa\n"},{"id":"423466","messageId":"xmqqk0og8k09.fsf@gitster.g","threadId":"55601","inReplyTo":"YI1fk3nE6AJRK8qS@coredump.intra.peff.net","subject":"Re: [PATCH 1/5] t5300: modernize basic tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-03T05:27:18Z","receivedAt":"2021-05-03T05:27:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> The first set of tests in t5300 goes back to 2005, and doesn't use some\n> of our customary style and tools these days. In preparation for touching\n> them, let's modernize a few things:\n> ...\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  t/t5300-pack-object.sh | 243 ++++++++++++++---------------------------\n>  1 file changed, 85 insertions(+), 158 deletions(-)\n\nThanks.  That was an impressive list of a \"few\" things ;-)  The\nresult certainly is much easier to follow.\n\n"},{"id":"423482","messageId":"3d77d70b-2cc5-4ca9-8753-fa9af5111842@gmail.com","threadId":"55601","inReplyTo":"YI1fubjvQQlrPz9D@coredump.intra.peff.net","subject":"Re: [PATCH 3/5] pack-objects: clamp negative window size to 0","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-05-03T12:10:24Z","receivedAt":"2021-05-03T12:10:32Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/1/2021 10:03 AM, Jeff King wrote:\n> A negative window size makes no sense, and the code in find_deltas() is\n> not prepared to handle it. If you pass \"-1\", for example, we end up\n> generate a 0-length array of \"struct unpacked\", but our loop assumes it\n> has at least one entry in it (and we end up reading garbage memory).\n> \n> We could complain to the user about this, but it's more forgiving to\n> just clamp it to 0, which means \"do not find any deltas at all\". The\n> 0-case is already tested earlier in the script, so we'll make sure this\n> does the same thing.\n\nThis seems like a reasonable approach. It takes existing \"undefined\"\nbehavior and turns it into well-understood, \"defined\" behavior.\n\nThanks,\n-Stolee\n"},{"id":"423484","messageId":"dd080fd4-00cc-0e53-2cbd-30871c598b4d@gmail.com","threadId":"55601","inReplyTo":"YI1f8jKReuE3LXVH@coredump.intra.peff.net","subject":"Re: [PATCH 5/5] pack-objects: clamp negative depth to 0","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-05-03T12:11:45Z","receivedAt":"2021-05-03T12:11:52Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/1/2021 10:04 AM, Jeff King wrote:\n> A negative delta depth makes no sense, and the code is not prepared to\n> handle it. If passed \"--depth=-1\" on the command line, then this line\n> from break_delta_chains():\n> \n> \tcur->depth = (total_depth--) % (depth + 1);\n> \n> triggers a divide-by-zero. This is undefined behavior according to the C\n> standard, but on POSIX systems results in SIGFPE killing the process.\n> This is certainly one way to inform the use that the command was\n> invalid, but it's a bit friendlier to just treat it as \"don't allow any\n> deltas\", which we already do for --depth=0.\n\nMakes sense to me. This whole series LGTM.\n\nThanks,\n-Stolee\n"},{"id":"423494","messageId":"YJAOgrZVt2oSGCfw@coredump.intra.peff.net","threadId":"55601","inReplyTo":"xmqqk0og8k09.fsf@gitster.g","subject":"Re: [PATCH 1/5] t5300: modernize basic tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-03T14:53:54Z","receivedAt":"2021-05-03T14:53:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 03, 2021 at 02:27:18PM +0900, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > The first set of tests in t5300 goes back to 2005, and doesn't use some\n> > of our customary style and tools these days. In preparation for touching\n> > them, let's modernize a few things:\n> > ...\n> > Signed-off-by: Jeff King <peff@peff.net>\n> > ---\n> >  t/t5300-pack-object.sh | 243 ++++++++++++++---------------------------\n> >  1 file changed, 85 insertions(+), 158 deletions(-)\n> \n> Thanks.  That was an impressive list of a \"few\" things ;-)  The\n> result certainly is much easier to follow.\n\nSometimes these things get bigger than you expected. :)\n\n-Peff\n"},{"id":"423495","messageId":"YJAOzTIXkO2lhxSs@coredump.intra.peff.net","threadId":"55601","inReplyTo":"3d77d70b-2cc5-4ca9-8753-fa9af5111842@gmail.com","subject":"Re: [PATCH 3/5] pack-objects: clamp negative window size to 0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-03T14:55:09Z","receivedAt":"2021-05-03T14:55:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 03, 2021 at 08:10:24AM -0400, Derrick Stolee wrote:\n\n> On 5/1/2021 10:03 AM, Jeff King wrote:\n> > A negative window size makes no sense, and the code in find_deltas() is\n> > not prepared to handle it. If you pass \"-1\", for example, we end up\n> > generate a 0-length array of \"struct unpacked\", but our loop assumes it\n> > has at least one entry in it (and we end up reading garbage memory).\n> > \n> > We could complain to the user about this, but it's more forgiving to\n> > just clamp it to 0, which means \"do not find any deltas at all\". The\n> > 0-case is already tested earlier in the script, so we'll make sure this\n> > does the same thing.\n> \n> This seems like a reasonable approach. It takes existing \"undefined\"\n> behavior and turns it into well-understood, \"defined\" behavior.\n\nI was on the fence on doing that, or just:\n\n  if (window < 0)\n\tdie(\"sorry dude, negative windows are nonsense\");\n\nSo if anybody has a strong preference, I could be easily persuaded. Part\nof what led me to being forgiving was that we similarly clamp too-large\ndepth values (with a warning; I didn't think it was really necessary\nhere, though).\n\n-Peff\n"},{"id":"423604","messageId":"02a66bfb-aac0-c05e-dab3-366bc312d900@web.de","threadId":"55601","inReplyTo":"YJAOzTIXkO2lhxSs@coredump.intra.peff.net","subject":"Re: [PATCH 3/5] pack-objects: clamp negative window size to 0","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-05-04T18:47:56Z","receivedAt":"2021-05-04T18:48:06Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.05.21 um 16:55 schrieb Jeff King:\n> On Mon, May 03, 2021 at 08:10:24AM -0400, Derrick Stolee wrote:\n>\n>> On 5/1/2021 10:03 AM, Jeff King wrote:\n>>> A negative window size makes no sense, and the code in find_deltas() is\n>>> not prepared to handle it. If you pass \"-1\", for example, we end up\n>>> generate a 0-length array of \"struct unpacked\", but our loop assumes it\n>>> has at least one entry in it (and we end up reading garbage memory).\n>>>\n>>> We could complain to the user about this, but it's more forgiving to\n>>> just clamp it to 0, which means \"do not find any deltas at all\". The\n>>> 0-case is already tested earlier in the script, so we'll make sure this\n>>> does the same thing.\n>>\n>> This seems like a reasonable approach. It takes existing \"undefined\"\n>> behavior and turns it into well-understood, \"defined\" behavior.\n>\n> I was on the fence on doing that, or just:\n>\n>   if (window < 0)\n> \tdie(\"sorry dude, negative windows are nonsense\");\n>\n> So if anybody has a strong preference, I could be easily persuaded. Part\n> of what led me to being forgiving was that we similarly clamp too-large\n> depth values (with a warning; I didn't think it was really necessary\n> here, though).\n\nThere's another option: Mapping -1 or all negative values to the\nmaximum:\n\n\tif (window < 0)\n\t\twindow = INT_MAX;\n\tif (depth < 0)\n\t\tdepth = (1 << OE_DEPTH_BITS) - 1;\n\nThat's allows saying \"gimme all you got\" without knowing or being\nwilling to type out the exact maximum value, which may change between\nversions.  Not all that useful for --window, I guess.  That convention\nhas been used elsewhere I'm sure, but can't point out a concrete\nexample.  $arr[-1] get the last item of the array $arr in PowerShell,\nthough, which is kind of similar.\n\nSure, you get the same effect in both cases by typing 2147483647, but\n-1 is more convenient.\n\nNot a strong preference, but I thought it was at least worth\nmentioning that particular bike shed color. :)\n\nRené\n"},{"id":"423618","messageId":"YJG+xp2b1rUzBaws@coredump.intra.peff.net","threadId":"55601","inReplyTo":"02a66bfb-aac0-c05e-dab3-366bc312d900@web.de","subject":"Re: [PATCH 3/5] pack-objects: clamp negative window size to 0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-04T21:38:14Z","receivedAt":"2021-05-04T21:38:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 04, 2021 at 08:47:56PM +0200, René Scharfe wrote:\n\n> >> This seems like a reasonable approach. It takes existing \"undefined\"\n> >> behavior and turns it into well-understood, \"defined\" behavior.\n> >\n> > I was on the fence on doing that, or just:\n> >\n> >   if (window < 0)\n> > \tdie(\"sorry dude, negative windows are nonsense\");\n> >\n> > So if anybody has a strong preference, I could be easily persuaded. Part\n> > of what led me to being forgiving was that we similarly clamp too-large\n> > depth values (with a warning; I didn't think it was really necessary\n> > here, though).\n> \n> There's another option: Mapping -1 or all negative values to the\n> maximum:\n> \n> \tif (window < 0)\n> \t\twindow = INT_MAX;\n> \tif (depth < 0)\n> \t\tdepth = (1 << OE_DEPTH_BITS) - 1;\n> \n> That's allows saying \"gimme all you got\" without knowing or being\n> willing to type out the exact maximum value, which may change between\n> versions.  Not all that useful for --window, I guess.  That convention\n> has been used elsewhere I'm sure, but can't point out a concrete\n> example.  $arr[-1] get the last item of the array $arr in PowerShell,\n> though, which is kind of similar.\n> \n> Sure, you get the same effect in both cases by typing 2147483647, but\n> -1 is more convenient.\n> \n> Not a strong preference, but I thought it was at least worth\n> mentioning that particular bike shed color. :)\n\nThanks for thinking about this. In general, yeah, allowing \"-1\" as\n\"unlimited\" is a sensible thing for many options. But for these two\nvariables, I don't think \"unlimited\" is ever a good idea:\n\n  - for --window, it makes the amount of work quadratic in the number of\n    objects in the repository (and likewise, memory usage equivalent to\n    the uncompressed contents of the repo). Going beyond about 250 or so\n    gives diminishing returns.\n\n  - for --depth, going beyond about 50 provides diminishing space\n    returns, and makes the run-time performance of accessing the deltas\n    horrible.\n\nSo certainly INT_MAX or the max allowable by OE_DEPTH_BITS is a very bad\nidea in both cases, and the use would probably be happier if we just hit\na die() instead. :) We _could_ make \"-1\" mean \"the maximum sensible\nvaluable\", but I think there's a lot of wiggle room for \"sensible\"\nthere. I much prefer using and advertising the actual values there (as\nwe do for \"gc --aggressive\").\n\n-Peff\n"},{"id":"423656","messageId":"874kfhwfwz.fsf@evledraar.gmail.com","threadId":"55601","inReplyTo":"02a66bfb-aac0-c05e-dab3-366bc312d900@web.de","subject":"Re: [PATCH 3/5] pack-objects: clamp negative window size to 0","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-05-05T11:53:06Z","receivedAt":"2021-05-05T11:58:40Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, May 04 2021, René Scharfe wrote:\n\n> Am 03.05.21 um 16:55 schrieb Jeff King:\n>> On Mon, May 03, 2021 at 08:10:24AM -0400, Derrick Stolee wrote:\n>>\n>>> On 5/1/2021 10:03 AM, Jeff King wrote:\n>>>> A negative window size makes no sense, and the code in find_deltas() is\n>>>> not prepared to handle it. If you pass \"-1\", for example, we end up\n>>>> generate a 0-length array of \"struct unpacked\", but our loop assumes it\n>>>> has at least one entry in it (and we end up reading garbage memory).\n>>>>\n>>>> We could complain to the user about this, but it's more forgiving to\n>>>> just clamp it to 0, which means \"do not find any deltas at all\". The\n>>>> 0-case is already tested earlier in the script, so we'll make sure this\n>>>> does the same thing.\n>>>\n>>> This seems like a reasonable approach. It takes existing \"undefined\"\n>>> behavior and turns it into well-understood, \"defined\" behavior.\n>>\n>> I was on the fence on doing that, or just:\n>>\n>>   if (window < 0)\n>> \tdie(\"sorry dude, negative windows are nonsense\");\n>>\n>> So if anybody has a strong preference, I could be easily persuaded. Part\n>> of what led me to being forgiving was that we similarly clamp too-large\n>> depth values (with a warning; I didn't think it was really necessary\n>> here, though).\n>\n> There's another option: Mapping -1 or all negative values to the\n> maximum:\n>\n> \tif (window < 0)\n> \t\twindow = INT_MAX;\n> \tif (depth < 0)\n> \t\tdepth = (1 << OE_DEPTH_BITS) - 1;\n>\n> That's allows saying \"gimme all you got\" without knowing or being\n> willing to type out the exact maximum value, which may change between\n> versions.  Not all that useful for --window, I guess.  That convention\n> has been used elsewhere I'm sure, but can't point out a concrete\n> example.  $arr[-1] get the last item of the array $arr in PowerShell,\n> though, which is kind of similar.\n>\n> Sure, you get the same effect in both cases by typing 2147483647, but\n> -1 is more convenient.\n>\n> Not a strong preference, but I thought it was at least worth\n> mentioning that particular bike shed color. :)\n\nThat seems sensible to expose, but I think should really be\n--window=max, not --window=-1. The latter feels way too much like\nassuming that a user might know about C's \"set all bits\" semantics.\n\nThe one example of such a variable I could think of is core.abbrev=no,\nwhich could arguably benefit from a core.abbrev=max synonym.\n\nAnother one is *.threads, e.g. grep.threads, index.threads. We currently\nsay that \"auto\" is like \"max, but I can see how we'd eventually benefit\nfrom splitting those up. I sometimes run git on machines where that\n\"auto\" is 48 or whatever (I haven't benchmarked, but that's surely\ncounter-productive). Having max != auto in that case (but still having a\n\"max\") would be nice.\n\n"},{"id":"423696","messageId":"aa21321a-08d8-5a3a-0ccd-4f4ea4bf7a9c@web.de","threadId":"55601","inReplyTo":"874kfhwfwz.fsf@evledraar.gmail.com","subject":"Re: [PATCH 3/5] pack-objects: clamp negative window size to 0","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-05-05T16:19:06Z","receivedAt":"2021-05-05T16:19:24Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 05.05.21 um 13:53 schrieb Ævar Arnfjörð Bjarmason:\n>\n> On Tue, May 04 2021, René Scharfe wrote:\n>\n>> There's another option: Mapping -1 or all negative values to the\n>> maximum:\n\n> That seems sensible to expose, but I think should really be\n> --window=max, not --window=-1. The latter feels way too much like\n> assuming that a user might know about C's \"set all bits\" semantics.\n\nNitpicking: Setting all bits for -1 is done by two's complement, which\nis just one of the signed number representations supported by C.\n\nBut yeah: A non-numeric value would probably be better in general.  As\nPeff explained it's not a particularly good idea to specify the maximum\nvalues of --depth and --window, though, so no need to make it easier.\n\n> The one example of such a variable I could think of is core.abbrev=no,\n> which could arguably benefit from a core.abbrev=max synonym.\n\ncore.abbrev=no turns off abbreviation, i.e. you get the full hash\nsize (false and off work as well).\n\nFollowing that logic, core.abbrev=max would ask for a maximum of\nabbreviation, i.e. for the shortest unambiguous hash.  That would have\na length of at least 4.  This value is stored in a constant called\nminimum_abbrev -- which seems backwards.  The implied negation of abbrev\n(the more you abbreviate, the shorter the length) is a bit confusing.\n\n> Another one is *.threads, e.g. grep.threads, index.threads. We currently\n> say that \"auto\" is like \"max, but I can see how we'd eventually benefit\n> from splitting those up. I sometimes run git on machines where that\n> \"auto\" is 48 or whatever (I haven't benchmarked, but that's surely\n> counter-productive). Having max != auto in that case (but still having a\n> \"max\") would be nice.\n\nGood thinking.  What is the maximum number of threads?  Certainly higher\nthan the number of CPUs.  Would that be useful?  Maybe -- on a\nsingle-core VM with an SSD queue length of 32 we can probably benefit\nfrom running more than one thread.\n\nAre our threads CPU-bound or I/O-bound?  I guess the answer is \"yes\". :)\nHow do we even find out the disk queue length in a portable way?  And\nhow would we calculate the optimal number of threads?  Are these even\nthe right questions to ask?\n\nAn \"auto\" option might help with that.  I imagine it starting out with\nsome default value and then experimentally decreasing and and increasing\nthe number of threads to find out which one works best.  Downside: It\nwould need to have comparable workloads for that.  And these benchmarks\nneed an otherwise quiet system.  Similar battery level if running on a\nlaptop.  Same system temperature.  Impractical during normal use.\n\nPerhaps a \"git benchmark\" command that runs some synthetic speed tests\nto tune grep.threads etc. would be possible?\n\nRené\n"}]}