From 1d423c493e3fc067b8bf39b61529ea1a6bbf224e Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Sat, 30 May 2026 22:37:37 -0700 Subject: [PATCH 01/18] pack-objects: honor a ".baddeltas" marker in try_delta() When considering delta compression, try_delta() skips pairs of non-delta objects from the same pack, assuming an earlier delta search already rejected them. Packs written by fast-import, bulk check-in, or pack-objects --window=0 need not satisfy that assumption, as Jeff King discussed: https://lore.kernel.org/git/20231009202149.GA3281325@coredump.intra.peff.net/ Recognize a .baddeltas sidecar to disable this shortcut for marked packs. Existing deltas remain reusable. Assisted-by: Claude Opus 4.7 Signed-off-by: Elijah Newren --- Documentation/gitformat-pack.adoc | 26 ++++++ builtin/pack-objects.c | 6 ++ odb/source-packed.c | 3 +- packfile.c | 12 ++- packfile.h | 3 +- repack.c | 1 + t/meson.build | 1 + t/t5337-pack-baddeltas.sh | 136 ++++++++++++++++++++++++++++++ 8 files changed, 183 insertions(+), 5 deletions(-) create mode 100755 t/t5337-pack-baddeltas.sh diff --git a/Documentation/gitformat-pack.adoc b/Documentation/gitformat-pack.adoc index 3416edceab82e9..d1ab3ff923d1b3 100644 --- a/Documentation/gitformat-pack.adoc +++ b/Documentation/gitformat-pack.adoc @@ -12,6 +12,7 @@ SYNOPSIS $GIT_DIR/objects/pack/pack-*.{pack,idx} $GIT_DIR/objects/pack/pack-*.rev $GIT_DIR/objects/pack/pack-*.mtimes +$GIT_DIR/objects/pack/pack-*.baddeltas $GIT_DIR/objects/pack/multi-pack-index DESCRIPTION @@ -357,6 +358,31 @@ All 4-byte numbers are in network byte order. and a checksum of all of the above (each having length according to the specified hash function). +== pack-*.baddeltas files + +The optional `.baddeltas` file is an empty marker sitting alongside a +`pack-*.pack` (and its `.idx`). It signals to `git pack-objects` that +the delta layout of the pack should not be trusted: even when two +objects appear together in the same pack and neither is stored as a +delta, the next packing run should still call out to its delta search +routine for the pair instead of assuming a prior pack-objects already +considered (and rejected) the pair. + +This is intended for producers that intentionally skip delta search +when writing a pack (for example, processes that bulk-import objects +or aggregate multiple existing packs without recomputing deltas). +Without this marker, the same-pack delta skip in `git pack-objects` +would silently inherit those producers' lack of delta search into +future repacks. + +The contents of the file are currently ignored. Producers should +write an empty file; consumers must tolerate (and ignore) any +content. + +The marker only affects whether `git pack-objects` will attempt to +compute new deltas for object pairs that share the marked pack. It +does not disable reuse of existing on-disk deltas. + == multi-pack-index (MIDX) files have the following format: The multi-pack-index files refer to multiple pack-files and loose objects. diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index af9390a46b9a69..7c45167c29f21a 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -2827,9 +2827,15 @@ static int try_delta(struct unpacked *trg, struct unpacked *src, * be considered, as even if we produce a suboptimal delta against * it, we will still save the transfer cost, as we already know * the other side has it and we won't send src_entry at all. + * + * If the source pack carries a ".baddeltas" marker, we treat its + * existing delta layout as untrusted: even if the two objects are + * in the same pack and neither is a delta, we have no reason to + * believe a previous packing run actually considered the pair. */ if (reuse_delta && IN_PACK(trg_entry) && IN_PACK(trg_entry) == IN_PACK(src_entry) && + !IN_PACK(trg_entry)->has_bad_deltas && !src_entry->preferred_base && trg_entry->in_pack_type != OBJ_REF_DELTA && trg_entry->in_pack_type != OBJ_OFS_DELTA) diff --git a/odb/source-packed.c b/odb/source-packed.c index 61d68eca044442..2d3d4e1f8fdadc 100644 --- a/odb/source-packed.c +++ b/odb/source-packed.c @@ -775,7 +775,8 @@ static void prepare_pack(const char *full_name, size_t full_name_len, ends_with(file_name, ".bitmap") || ends_with(file_name, ".keep") || ends_with(file_name, ".promisor") || - ends_with(file_name, ".mtimes")) + ends_with(file_name, ".mtimes") || + ends_with(file_name, ".baddeltas")) string_list_append(data->garbage, full_name); else report_garbage(PACKDIR_FILE_GARBAGE, full_name); diff --git a/packfile.c b/packfile.c index 4fa5fd67c8497f..abb0a01e4408f7 100644 --- a/packfile.c +++ b/packfile.c @@ -367,7 +367,9 @@ void close_pack(struct packed_git *p) void unlink_pack_path(const char *pack_name, int force_delete) { - static const char *exts[] = {".idx", ".pack", ".rev", ".keep", ".bitmap", ".promisor", ".mtimes"}; + static const char *exts[] = {".idx", ".pack", ".rev", ".keep", + ".bitmap", ".promisor", ".mtimes", + ".baddeltas"}; int i; struct strbuf buf = STRBUF_INIT; size_t plen; @@ -723,10 +725,10 @@ struct packed_git *add_packed_git(struct repository *r, const char *path, return NULL; /* - * ".promisor" is long enough to hold any suffix we're adding (and + * ".baddeltas" is long enough to hold any suffix we're adding (and * the use xsnprintf double-checks that) */ - alloc = st_add3(path_len, strlen(".promisor"), 1); + alloc = st_add3(path_len, strlen(".baddeltas"), 1); p = alloc_packed_git(r, alloc); memcpy(p->pack_name, path, path_len); @@ -753,6 +755,10 @@ struct packed_git *add_packed_git(struct repository *r, const char *path, if (!access(p->pack_name, F_OK)) p->is_cruft = 1; + xsnprintf(p->pack_name + path_len, alloc - path_len, ".baddeltas"); + if (!access(p->pack_name, F_OK)) + p->has_bad_deltas = 1; + xsnprintf(p->pack_name + path_len, alloc - path_len, ".pack"); if (stat(p->pack_name, &st) || !S_ISREG(st.st_mode)) { free(p); diff --git a/packfile.h b/packfile.h index 6d30d15a0053b3..370b21c281e201 100644 --- a/packfile.h +++ b/packfile.h @@ -34,7 +34,8 @@ struct packed_git { do_not_close:1, pack_promisor:1, multi_pack_index:1, - is_cruft:1; + is_cruft:1, + has_bad_deltas:1; unsigned char hash[GIT_MAX_RAWSZ]; struct revindex_entry *revindex; const uint32_t *revindex_data; diff --git a/repack.c b/repack.c index d2aa58e13484fd..d17eaa06c167a4 100644 --- a/repack.c +++ b/repack.c @@ -330,6 +330,7 @@ static struct { {".mtimes", 1}, {".bitmap", 1}, {".promisor", 1}, + {".baddeltas", 1}, {".idx"}, }; diff --git a/t/meson.build b/t/meson.build index 7f53cca7d1f891..90d4c7c39708f8 100644 --- a/t/meson.build +++ b/t/meson.build @@ -639,6 +639,7 @@ integration_tests = [ 't5333-pseudo-merge-bitmaps.sh', 't5334-incremental-multi-pack-index.sh', 't5335-compact-multi-pack-index.sh', + 't5337-pack-baddeltas.sh', 't5351-unpack-large-objects.sh', 't5400-send-pack.sh', 't5401-update-hooks.sh', diff --git a/t/t5337-pack-baddeltas.sh b/t/t5337-pack-baddeltas.sh new file mode 100755 index 00000000000000..7f5af219b4a92c --- /dev/null +++ b/t/t5337-pack-baddeltas.sh @@ -0,0 +1,136 @@ +#!/bin/sh + +test_description='`.baddeltas` sidecar disables the same-pack try_delta() skip' + +. ./test-lib.sh + +# Two similar but distinct blobs. The contents are deliberately large +# enough and similar enough that a real delta search will find a useful +# delta between them. +generate_blobs () { + { + printf "header\n" && + i=0 && + while test $i -lt 200 + do + printf "line %d padding-padding-padding-padding\n" $i && + i=$((i + 1)) || return 1 + done + } >a && + cp a b && + printf "tail-line\n" >>b +} + +# Build a pack from the two blobs without doing any delta search, and +# echo the basename of the resulting .pack file. +build_input_pack () { + A=$(git hash-object -w a) && + B=$(git hash-object -w b) && + pack_hash=$(printf "%s\n%s\n" "$A" "$B" | + git pack-objects --window=0 .git/objects/pack/pack) && + git prune-packed && + echo "pack-$pack_hash.pack" +} + +# Count delta entries in a pack idx. "git verify-pack -v" prints one +# line per object with 5 fields for non-delta entries (oid, type, size, +# size-in-pack, offset) and 7 fields for delta entries (... depth +# base-oid). +count_deltas () { + git verify-pack -v "$1" | + awk 'NF == 7 { n++ } END { print n + 0 }' +} + +# Repack just the two blobs from the existing pack into a fresh pack +# with prefix "out". Echo the basename of the resulting .pack file. +repack_blobs () { + A=$(git hash-object a) && + B=$(git hash-object b) && + pack_hash=$(printf "%s\n%s\n" "$A" "$B" | + git pack-objects .git/objects/pack/out) && + echo "out-$pack_hash.pack" +} + +test_expect_success 'set up two similar blobs' ' + git init repo && + ( + cd repo && + generate_blobs + ) +' + +test_expect_success 'without .baddeltas, same-pack pair is skipped' ' + test_when_finished "rm -fr work" && + cp -R repo work && + ( + cd work && + input_pack=$(build_input_pack) && + test 0 -eq "$(count_deltas .git/objects/pack/${input_pack%.pack}.idx)" && + out_pack=$(repack_blobs) && + test 0 -eq "$(count_deltas .git/objects/pack/${out_pack%.pack}.idx)" + ) +' + +test_expect_success 'with .baddeltas, same-pack pair gets reconsidered' ' + test_when_finished "rm -fr work" && + cp -R repo work && + ( + cd work && + input_pack=$(build_input_pack) && + test 0 -eq "$(count_deltas .git/objects/pack/${input_pack%.pack}.idx)" && + >.git/objects/pack/${input_pack%.pack}.baddeltas && + out_pack=$(repack_blobs) && + test 1 -le "$(count_deltas .git/objects/pack/${out_pack%.pack}.idx)" + ) +' + +test_expect_success '.baddeltas does not trigger garbage warnings' ' + test_when_finished "rm -fr work" && + cp -R repo work && + ( + cd work && + input_pack=$(build_input_pack) && + >.git/objects/pack/${input_pack%.pack}.baddeltas && + git count-objects -v 2>warnings && + test_grep ! -i garbage warnings + ) +' + +test_expect_success 'repacking an unchanged pack removes .baddeltas' ' + test_when_finished "rm -fr work" && + cp -R repo work && + ( + cd work && + # Tiny blobs leave no useful deltas to find. + for i in 1 2 3 4 + do + oid=$(echo "marker-$i" | git hash-object -w --stdin) && + git update-ref "refs/tags/blob-$i" "$oid" || return 1 + done && + git repack -ad && + ls .git/objects/pack/pack-*.pack >before && + test_line_count = 1 before && + read pack "${pack%.pack}.baddeltas" && + git repack -ad && + ls .git/objects/pack/pack-*.pack >after && + test_cmp before after && + test_path_is_missing "${pack%.pack}.baddeltas" && + git fsck + ) +' + +test_expect_success '.baddeltas is removed by git repack -d' ' + test_when_finished "rm -fr work" && + cp -R repo work && + ( + cd work && + input_pack=$(build_input_pack) && + >.git/objects/pack/${input_pack%.pack}.baddeltas && + git repack -ad && + test_path_is_missing .git/objects/pack/${input_pack%.pack}.baddeltas && + test_path_is_missing .git/objects/pack/$input_pack + ) +' + +test_done From 2eb7a743b02df47916a8af87d3e914a1f9837663 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Mon, 21 Sep 2026 11:48:38 -0700 Subject: [PATCH 02/18] pack-write: share creation of pack marker files index-pack creates .keep and .promisor markers without overwriting existing files. Extract this writer into pack-write.c so other pack producers can reuse it, leaving filename selection and output reporting in index-pack. Signed-off-by: Elijah Newren --- builtin/index-pack.c | 20 ++------------------ pack-write.c | 19 +++++++++++++++++++ pack.h | 7 +++++++ 3 files changed, 28 insertions(+), 18 deletions(-) diff --git a/builtin/index-pack.c b/builtin/index-pack.c index 6b2a87e2d39355..9a3dd9ecc58d87 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -1561,30 +1561,14 @@ static void write_special_file(const char *suffix, const char *msg, { struct strbuf name_buf = STRBUF_INIT; const char *filename; - int fd; - int msg_len = strlen(msg); if (pack_name) filename = derive_filename(pack_name, "pack", suffix, &name_buf); else filename = odb_pack_name(the_repository, &name_buf, hash, suffix); - fd = safe_create_file_with_leading_directories(the_repository, filename); - if (fd < 0) { - if (errno != EEXIST) - die_errno(_("cannot write %s file '%s'"), - suffix, filename); - } else { - if (msg_len > 0) { - write_or_die(fd, msg, msg_len); - write_or_die(fd, "\n", 1); - } - if (close(fd) != 0) - die_errno(_("cannot close written %s file '%s'"), - suffix, filename); - if (report) - *report = suffix; - } + if (write_pack_marker_file(the_repository, filename, msg) && report) + *report = suffix; strbuf_release(&name_buf); } diff --git a/pack-write.c b/pack-write.c index 85674e4b726bab..d33a648a87f822 100644 --- a/pack-write.c +++ b/pack-write.c @@ -599,6 +599,25 @@ void stage_tmp_packfiles(struct repository *repo, free(mtimes_tmp_name); } +int write_pack_marker_file(struct repository *repo, const char *filename, + const char *msg) +{ + int fd = safe_create_file_with_leading_directories(repo, filename); + + if (fd < 0) { + if (errno == EEXIST) + return 0; + die_errno(_("cannot write file '%s'"), filename); + } + if (*msg) { + write_or_die(fd, msg, strlen(msg)); + write_or_die(fd, "\n", 1); + } + if (close(fd)) + die_errno(_("cannot close written file '%s'"), filename); + return 1; +} + void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_sought) { int i, err; diff --git a/pack.h b/pack.h index ada506b5c5d0db..bc42618aee2507 100644 --- a/pack.h +++ b/pack.h @@ -111,6 +111,13 @@ char *index_pack_lockfile(struct odb_source *source, int fd, struct ref; +/* + * Create a pack marker containing msg followed by a newline, or an empty + * marker if msg is empty. Leave existing files untouched. Return 1 if + * created, 0 if already present; die on other errors. + */ +int write_pack_marker_file(struct repository *repo, const char *filename, + const char *msg); void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_sought); char *write_rev_file(struct repository *repo, From 3bd9d62b9739bc8132ce08688c9e940951a44bab Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Sat, 30 May 2026 22:40:13 -0700 Subject: [PATCH 03/18] pack-objects: add a --mark-bad-deltas option Packs written without a full delta search need a way to request reconsideration during later repacks. Add --mark-bad-deltas to write an empty .baddeltas marker beside each output pack. This option is incompatible with --stdout. Publish the marker before the index so readers discover the pack with its marker already in place. Assisted-by: Claude Opus 4.7 Signed-off-by: Elijah Newren --- Documentation/git-pack-objects.adoc | 8 +++++++ builtin/pack-objects.c | 15 +++++++++++++ t/t5337-pack-baddeltas.sh | 34 +++++++++++++++++++++++++++++ 3 files changed, 57 insertions(+) diff --git a/Documentation/git-pack-objects.adoc b/Documentation/git-pack-objects.adoc index 65cd00c152f495..0196849510406b 100644 --- a/Documentation/git-pack-objects.adoc +++ b/Documentation/git-pack-objects.adoc @@ -143,6 +143,14 @@ options which imply `--revs`. have an mtime older than ``. If unspecified (and given `--cruft`), then no objects are eliminated. +--mark-bad-deltas:: + Write a `.baddeltas` marker file alongside each output pack. The + marker signals that objects within the pack have not been fully + delta-searched against other objects within the same pack and + that future repacking should consider them. Any deltas that do + exist within this pack can still be reused, however. + Incompatible with `--stdout`. + --window=:: --depth=:: These two options affect how the objects contained in diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index 7c45167c29f21a..ac881b0f5ed6df 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -44,6 +44,7 @@ #include "pack-mtimes.h" #include "parse-options.h" #include "pkt-line.h" +#include "path.h" #include "blob.h" #include "tree.h" #include "path-walk.h" @@ -212,6 +213,7 @@ static int keep_unreachable, unpack_unreachable, include_tag; static timestamp_t unpack_unreachable_expiration; static int pack_loose_unreachable; static int cruft; +static int mark_bad_deltas; static int shallow = 0; static timestamp_t cruft_expiration; static int local; @@ -1470,6 +1472,14 @@ static void write_pack_file(void) &pack_idx_opts, hash, &idx_tmp_name); + if (mark_bad_deltas) { + size_t tmpname_len = tmpname.len; + + strbuf_addstr(&tmpname, "baddeltas"); + write_pack_marker_file(the_repository, tmpname.buf, ""); + strbuf_setlen(&tmpname, tmpname_len); + } + if (write_bitmap_index) { size_t tmpname_len = tmpname.len; @@ -5207,6 +5217,8 @@ int cmd_pack_objects(int argc, N_("unpack unreachable objects newer than