From f99c9478bcf622555fe3738b0407aebea1576923 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Fri, 15 Nov 2024 21:09:22 -0500 Subject: [PATCH] push: don't reuse deltas with path walk The --path-walk option in `git pack-objects` is implied by the pack.usePathWalk=true config value. This is intended to help the packfile generation within `git push` specifically. While this config does enable the path-walk feature, it does not lead to the expected levels of compression in the cases it was designed to handle. This is due to the default implication of the --reuse-delta option as well as auto-GC. In the performance tests used to evaluate the --path-walk option, such as those in p5313, the --no-reuse-delta option is used to ensure that deltas are recomputed according to the new object walk. However, it was assumed (I assumed this) that when the objects were loose from client-side operations that better deltas would be computed during this operation. This wasn't confirmed because the test process used data that was fetched from real repositories and thus existed in packed form only. I was able to confirm that this does not reproduce when the objects to push are loose. Careful use of making the pushed commit unreachable and loosening the objects via `git repack -Ad` helps to confirm my suspicions here. Independent of this change, I'm pushing for these pipeline agents to set `gc.auto=0` before creating their Git objects. In the current setup, the repo is adding objects and then incrementally repacking them and ending up with bad cross-path deltas. This approach can help scenarios where that makes sense, but will not cover all of our users without them choosing to opt-in to background maintenance (and even then, an incremental repack could cost them efficiency). In order to make sure we are getting the intended compression in `git push`, this change enforces the spawned `git pack-objects` process to use `--no-reuse-delta`. As far as I can tell, the main motivation for implying the --reuse-delta option by default is two-fold: 1. The code in send-pack.c that executes 'git pack-objects' is ignorant of whether the current process is a client pushing to a remote or a remote sending a fetch or clone to a client. 2. For servers, it is critical that they trust the previously computed deltas whenever possible, or they could overload their CPU resources. There's also the side that most servers use repacking logic that will replace any bad deltas that are sent by clients (or at least, that's the hope; we've seen that repacks can also pick bad deltas). This commit also adds a test case that demonstrates that `git -c pack.usePathWalk=true push` now avoids reusing deltas. To do this, the test case constructs a pack with a horrendously inefficient delta object, then verifies that the pack on the receiving side of the `push` fails to have such an inefficient delta. The test case would probably be a lot more readable if hex numbers were used instead of octal numbers, but alas, `printf "\x"` is not portable, only `printf "\"` is. For example, dash's built-in `printf` function simply prints `\x` verbatim while bash's built-in happily converts this construct to the corresponding byte. Signed-off-by: Derrick Stolee Signed-off-by: Johannes Schindelin --- builtin/push.c | 4 ++ send-pack.c | 2 + send-pack.h | 1 + t/t5590-push-path-walk.sh | 109 ++++++++++++++++++++++++++++++++++++++ transport.c | 1 + transport.h | 1 + 6 files changed, 118 insertions(+) create mode 100755 t/t5590-push-path-walk.sh diff --git a/builtin/push.c b/builtin/push.c index 90de3746b5229f..f49f436dd389b1 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -619,6 +619,10 @@ int cmd_push(int argc, else if (recurse_submodules == RECURSE_SUBMODULES_ONLY) flags |= TRANSPORT_RECURSE_SUBMODULES_ONLY; + prepare_repo_settings(the_repository); + if (the_repository->settings.pack_use_path_walk) + flags |= TRANSPORT_PUSH_NO_REUSE_DELTA; + if (tags) refspec_append(&rs, "refs/tags/*"); diff --git a/send-pack.c b/send-pack.c index cdb6dc11d1ea18..e66b1b5c6b4085 100644 --- a/send-pack.c +++ b/send-pack.c @@ -92,6 +92,8 @@ static int pack_objects(int fd, struct ref *refs, struct oid_array *advertised, strvec_push(&po.args, "--shallow"); if (args->disable_bitmaps) strvec_push(&po.args, "--no-use-bitmap-index"); + if (args->no_reuse_delta) + strvec_push(&po.args, "--no-reuse-delta"); po.in = -1; po.out = args->stateless_rpc ? -1 : fd; po.git_cmd = 1; diff --git a/send-pack.h b/send-pack.h index 7edb80596c7b0e..759a55c1bdab1c 100644 --- a/send-pack.h +++ b/send-pack.h @@ -22,6 +22,7 @@ struct send_pack_args { force_update:1, use_thin_pack:1, use_ofs_delta:1, + no_reuse_delta:1, dry_run:1, /* One of the SEND_PACK_PUSH_CERT_* constants. */ push_cert:2, diff --git a/t/t5590-push-path-walk.sh b/t/t5590-push-path-walk.sh new file mode 100755 index 00000000000000..7849ec337b0aa7 --- /dev/null +++ b/t/t5590-push-path-walk.sh @@ -0,0 +1,109 @@ +#!/bin/sh + +test_description='verify that push respects `pack.usePathWalk`' + +TEST_PASSES_SANITIZE_LEAK=true +. ./test-lib.sh +. "$TEST_DIRECTORY"/lib-pack.sh + +test_expect_success 'setup bare repository and clone' ' + git init --bare -b main bare.git && + git --git-dir=bare.git config receive.unpackLimit 0 && + git --git-dir bare.git commit-tree -m initial $EMPTY_TREE >head_oid && + git --git-dir bare.git update-ref refs/heads/main $(cat head_oid) && + git clone --bare bare.git clone.git +' +test_expect_success 'avoid reusing deltified objects' ' + # construct two commits, one containing a file with the hex digits + # repeated 16 times, the next reducing that to 8 times. The crucial + # part is that the blob of the second commit is deltified _really_ + # badly and it is therefore easy to detect if a `git push` reused that + # delta. + x="0123456789abcdef" && + printf "$x$x$x$x$x$x$x$x" >x128 && + printf "$x$x$x$x$x$x$x$x$x$x$x$x$x$x$x$x" >x256 && + + pack=clone.git/objects/pack/pack-tmp.pack && + pack_header 2 >$pack && + + # add x256 as a non-deltified object, using an uncompressed zlib stream + # for simplicity + # 060 = OBJ_BLOB << 4, 0200 = size larger than 15, + # 0 = lower 4 bits of size, 020 = bits 5-9 of size (size = 256) + printf "\260\020" >>$pack && + # Uncompressed zlib stream always starts with 0170 1 1, followed + # by two bytes encoding the size, little endian, then two bytes with + # the bitwise-complement of that size, then the payload, and then the + # Adler32 checksum. For some reason, the checksum is in big-endian + # format. + printf "\170\001\001\0\001\377\376" >>$pack && + cat x256 >>$pack && + # Manually-computed Adler32 checksum: 0xd7ae4621 + printf "\327\256\106\041" >>$pack && + + # add x128 as a very badly deltified object + # 0120 = OBJ_OFS_DELTA << 4, 0200 = total size larger than 15, + # 4 = lower 4 bits of size, 030 = bits 5-9 of size + # (size = 128 * 3 + 2 + 2) + printf "\344\030" >>$pack && + # 0415 = size (i.e. the relative negative offset) of the previous + # object (x256, used as base object) + # encoded as 0200 | ((0415 >> 7) - 1), 0415 & 0177 + printf "\201\015" >>$pack && + # Uncompressed zlib stream, as before, size = 2 + 2 + 128 * 3 (i.e. + # 0604) + printf "\170\001\001\204\001\173\376" >>$pack && + # base object size = 0400 (encoded as 0200 | (0400 & 0177), + # 0400 >> 7) + printf "\200\002" >>$pack && + # object size = 0200 (encoded as 0200 | (0200 & 0177), 0200 >> 7 + printf "\200\001" >>$pack && + # massively badly-deltified object: copy every single byte individually + # 0200 = copy, 1 = use 1 byte to encode the offset (counter), + # 020 = use 1 byte to encode the size (1) + printf "$(printf "\\\\221\\\\%03o\\\\001" $(test_seq 0 127))" >>$pack && + # Manually-computed Adler32 checksum: 0x99c369c4 + printf "\231\303\151\304" >>$pack && + + pack_trailer $pack && + git index-pack -v $pack && + + oid256=$(git hash-object x256) && + printf "100755 blob $oid256\thex\n" >tree && + tree_oid="$(git --git-dir=clone.git mktree tree && + tree_oid="$(git --git-dir=clone.git mktree verify && + size="$(sed -n "s/^$oid128 blob *\([^ ]*\).*/\1/p" verify && + size="$(sed -n "s/^$oid128 blob *\([^ ]*\).*/\1/p" push_options; args.url = transport->url; diff --git a/transport.h b/transport.h index 44100fa9b7fdd6..54767b5a051e10 100644 --- a/transport.h +++ b/transport.h @@ -158,6 +158,7 @@ struct transport { #define TRANSPORT_RECURSE_SUBMODULES_ONLY (1<<15) #define TRANSPORT_PUSH_FORCE_IF_INCLUDES (1<<16) #define TRANSPORT_PUSH_AUTO_UPSTREAM (1<<17) +#define TRANSPORT_PUSH_NO_REUSE_DELTA (1<<18) int transport_summary_width(const struct ref *refs);