* Re: [BUG] push resends common history after repack during pre-push (2.54.0, 2.56.0)
2026-10-06 17:37 [BUG] push resends common history after repack during pre-push (2.54.0, 2.56.0) Jens Röcker
@ 2026-10-06 17:47 ` Jens Röcker
2026-10-06 20:03 ` D. Ben Knoble
2026-10-07 9:03 ` Patrick Steinhardt
2 siblings, 0 replies; 4+ messages in thread
From: Jens Röcker @ 2026-10-06 17:47 UTC (permalink / raw)
To: git
[-- Attachment #1: Type: text/plain, Size: 9482 bytes --]
Hello Git developers,
Gmail hard-wrapped several long lines in the inline Python script in my
previous report. Please use the attached reproduce.txt instead. It is
the same tested script, supplied as a text/plain attachment to preserve
its exact contents.
Run it with:
python3 reproduce.txt --git /path/to/git
For an uninstalled Git build, also pass:
--exec-path /path/to/git/build/directory
All reported measurements remain unchanged: 300 bytes without repacking,
and 4,196,026 bytes with repacking, with or without push.negotiate=true,
for both Apple Git 2.54.0 and upstream Git 2.56.0.
Sorry for the formatting issue.
Attachment SHA-256:
932b10a426aca11eb5831cd338ad5f61d83db1301e334764a2d9df17ab4e6b76
Am Di., 6. Okt. 2026 um 19:37 Uhr schrieb Jens Röcker <jens.roecker@gmail.com>:
>
> Hello Git developers,
>
> A push can resend common history if its pre-push hook repacks the local
> object database and removes previously loose common objects. I reproduced
> this with Apple Git 2.54.0 (Apple Git-157) and an unmodified build of the
> current upstream Git 2.56.0 release on macOS 27.0 / arm64.
>
> The attached inline Python script creates fresh local repositories, seeds
> a bare receiver with a deterministic, incompressible 4-MiB historical blob,
> and pushes one tiny text-file commit. The common base is initially loose.
> The receiver uses receive.unpackLimit=1 so the added pack is measurable.
> Each case starts from a separate fresh repository pair. All pushes succeed
> and the receiver ends at the expected tip.
>
> Observed added receiver pack sizes, in bytes:
>
> Apple Git 2.54.0 upstream Git 2.56.0
> no hook 300 300
> repack in pre-push 4,196,026 4,196,026
> repack + negotiate 4,196,026 4,196,026
>
> The repacking hook is simply:
>
> #!/bin/sh
> set -eu
> cat >/dev/null
> git repack -adq
> git prune-packed
>
> Expected: the already-advertised common history should still be excluded
> when its storage moves from loose objects to a newly created pack.
> Actual: the historical blob is transmitted again. The receiver stores a
> new pack roughly the size of the historical blob. Enabling
> push.negotiate=true does not prevent the redundant transfer in this test.
>
> Possible mechanism, based on source inspection:
>
> In 2.54.0, send-pack.c:feed_object() drops negative OIDs when
> odb_has_object(..., 0) returns false. In 2.56.0, the same quick check is in
> append_negative_object(). In both versions, odb_has_object() uses
> OBJECT_INFO_QUICK unless ODB_HAS_OBJECT_RECHECK_PACKED is set. A parent
> process with a stale pack catalogue may therefore miss the base after the
> hook removes the loose copy; the fresh pack generator then sees the new
> pack and walks history without that excluded base. This is a proposed
> explanation of the measured effect, not an instrumented proof of the
> parent process's in-memory state.
>
> Relevant release source:
> https://github.com/git/git/blob/v2.54.0/send-pack.c
> https://github.com/git/git/blob/v2.56.0/send-pack.c
> https://github.com/git/git/blob/v2.56.0/odb.c
>
> The upstream 2.56.0 binary was built from the kernel.org release tarball
> with optional gettext, curl, Tcl/Tk, Perl, Python and Rust components
> disabled. Neither global Git configuration nor the installed system Git
> was changed. The script isolates system/global Git configuration and uses
> only local transport. Measurements are receiver pack-file sizes, rather
> than network-byte counters. A separate large-repository incident motivated
> this test, but this report includes only synthetic fixtures.
>
> To reproduce, save the inline script as reproduce.py and run:
>
> python3 reproduce.py --git /path/to/git
>
> For a Git binary built in-place, add:
>
> --exec-path /path/to/git/build/directory
>
> The script requires Python 3 and Git; its default mode removes only its
> own temporary test repositories when the run completes. To retain all
> fixture repositories and push logs, pass --output with a new directory.
>
> Minimal reproducer follows:
>
> #!/usr/bin/env python3
> """Reproduce redundant push history after loose objects move into a new pack.
>
> Uses fresh local test repositories only. Requires Python 3 and Git.
> Example: python3 reproduce.py --git /path/to/git --output /new/results/path
> For an uninstalled Git build, add --exec-path /path/to/build/directory.
> """
>
> import argparse
> import hashlib
> import json
> import os
> from pathlib import Path
> import platform
> import shutil
> import subprocess
> import tempfile
> import time
>
>
> def main():
> parser = argparse.ArgumentParser(description=__doc__)
> parser.add_argument("--git", default=shutil.which("git"))
> parser.add_argument("--exec-path")
> parser.add_argument("--output", type=Path)
> args = parser.parse_args()
> binary = str(Path(args.git).resolve())
> temporary = None
> if args.output:
> root = args.output.resolve()
> root.mkdir(parents=True, exist_ok=False)
> else:
> temporary = tempfile.TemporaryDirectory(prefix="git-push-repack-repro-")
> root = Path(temporary.name)
>
> env = {key: value for key, value in os.environ.items()
> if not key.startswith("GIT_")}
> env.update(GIT_CONFIG_NOSYSTEM="1", GIT_CONFIG_GLOBAL=os.devnull,
> GIT_AUTHOR_DATE="2001-01-01T00:00:00+0000",
> GIT_COMMITTER_DATE="2001-01-01T00:00:00+0000")
> env["PATH"] = str(Path(binary).parent) + os.pathsep + env.get("PATH", "")
> if args.exec_path:
> env["GIT_EXEC_PATH"] = str(Path(args.exec_path).resolve())
>
> def git(cwd, *words):
> return subprocess.run([binary, *words], cwd=cwd, env=env,
> text=True, capture_output=True, check=True,
> timeout=60)
>
> results = []
> for mode in ("no-hook", "repack", "repack-negotiate"):
> case = root / mode
> case.mkdir()
> repo, remote = case / "repo", case / "remote.git"
> git(case, "init", "-q", "-b", "main", str(repo))
> git(case, "init", "-q", "--bare", "-b", "main", str(remote))
> for key, value in (("user.name", "Git bug reproduction"),
> ("user.email", "test@example.invalid"),
> ("commit.gpgsign", "false"), ("gc.auto", "0"),
> ("core.hooksPath", str(repo / ".git/hooks"))):
> git(repo, "config", key, value)
> git(remote, "config", "gc.auto", "0")
> git(remote, "config", "receive.unpackLimit", "1")
> (repo / "history.bin").write_bytes(
> hashlib.shake_256(b"historical fixture").digest(4 * 1024 * 1024))
> git(repo, "add", "--", "history.bin")
> git(repo, "commit", "-q", "-m", "historical seed", "--", "history.bin")
> base = git(repo, "rev-parse", "HEAD").stdout.strip()
> git(repo, "push", "-q", str(remote), "HEAD:refs/heads/main")
> (repo / "change.txt").write_text("tiny change\n")
> git(repo, "add", "--", "change.txt")
> git(repo, "commit", "-q", "-m", "tiny change", "--", "change.txt")
> tip = git(repo, "rev-parse", "HEAD").stdout.strip()
> assert (repo / ".git/objects" / base[:2] / base[2:]).is_file()
> if mode != "no-hook":
> hook = repo / ".git/hooks/pre-push"
> hook.parent.mkdir(parents=True, exist_ok=True)
> hook.write_text("#!/bin/sh\nset -eu\ncat >/dev/null\n"
> "git repack -adq\ngit prune-packed\n")
> hook.chmod(0o700)
> before = set((remote / "objects/pack").glob("*.pack"))
> config = ["-c", "push.negotiate=true"] if mode ==
> "repack-negotiate" else []
> start = time.monotonic()
> result = git(repo, *config, "push", "--progress", str(remote),
> "HEAD:refs/heads/main")
> elapsed = time.monotonic() - start
> (case / "push.log").write_text(result.stdout + result.stderr)
> packs = set((remote / "objects/pack").glob("*.pack")) - before
> remote_tip = git(remote, "rev-parse", "refs/heads/main").stdout.strip()
> assert remote_tip == tip
> results.append({"case": mode, "push_rc": result.returncode,
> "new_remote_pack_bytes": sum(p.stat().st_size
> for p in packs),
> "elapsed_seconds": round(elapsed, 6),
> "remote_tip_matches": True})
> report = {"git_version": git(root, "version",
> "--build-options").stdout.strip(),
> "platform": {"system": platform.system(), "machine":
> platform.machine(),
> "macos": platform.mac_ver()[0]},
> "fixture_bytes": 4 * 1024 * 1024,
> "transport": "local bare repository", "results": results}
> encoded = json.dumps(report, indent=2) + "\n"
> (root / "results.json").write_text(encoded)
> print(encoded, end="")
> if temporary:
> temporary.cleanup()
>
>
> if __name__ == "__main__":
> main()
>
> Thank you.
--
Mit freundlichen Grüßen
Jens Röcker
[-- Attachment #2: reproduce.txt --]
[-- Type: text/plain, Size: 4848 bytes --]
#!/usr/bin/env python3
"""Reproduce redundant push history after loose objects move into a new pack.
Uses fresh local test repositories only. Requires Python 3 and Git.
Example: python3 reproduce.py --git /path/to/git --output /new/results/path
For an uninstalled Git build, add --exec-path /path/to/build/directory.
"""
import argparse
import hashlib
import json
import os
from pathlib import Path
import platform
import shutil
import subprocess
import tempfile
import time
def main():
parser = argparse.ArgumentParser(description=__doc__)
parser.add_argument("--git", default=shutil.which("git"))
parser.add_argument("--exec-path")
parser.add_argument("--output", type=Path)
args = parser.parse_args()
binary = str(Path(args.git).resolve())
temporary = None
if args.output:
root = args.output.resolve()
root.mkdir(parents=True, exist_ok=False)
else:
temporary = tempfile.TemporaryDirectory(prefix="git-push-repack-repro-")
root = Path(temporary.name)
env = {key: value for key, value in os.environ.items()
if not key.startswith("GIT_")}
env.update(GIT_CONFIG_NOSYSTEM="1", GIT_CONFIG_GLOBAL=os.devnull,
GIT_AUTHOR_DATE="2001-01-01T00:00:00+0000",
GIT_COMMITTER_DATE="2001-01-01T00:00:00+0000")
env["PATH"] = str(Path(binary).parent) + os.pathsep + env.get("PATH", "")
if args.exec_path:
env["GIT_EXEC_PATH"] = str(Path(args.exec_path).resolve())
def git(cwd, *words):
return subprocess.run([binary, *words], cwd=cwd, env=env,
text=True, capture_output=True, check=True,
timeout=60)
results = []
for mode in ("no-hook", "repack", "repack-negotiate"):
case = root / mode
case.mkdir()
repo, remote = case / "repo", case / "remote.git"
git(case, "init", "-q", "-b", "main", str(repo))
git(case, "init", "-q", "--bare", "-b", "main", str(remote))
for key, value in (("user.name", "Git bug reproduction"),
("user.email", "test@example.invalid"),
("commit.gpgsign", "false"), ("gc.auto", "0"),
("core.hooksPath", str(repo / ".git/hooks"))):
git(repo, "config", key, value)
git(remote, "config", "gc.auto", "0")
git(remote, "config", "receive.unpackLimit", "1")
(repo / "history.bin").write_bytes(
hashlib.shake_256(b"historical fixture").digest(4 * 1024 * 1024))
git(repo, "add", "--", "history.bin")
git(repo, "commit", "-q", "-m", "historical seed", "--", "history.bin")
base = git(repo, "rev-parse", "HEAD").stdout.strip()
git(repo, "push", "-q", str(remote), "HEAD:refs/heads/main")
(repo / "change.txt").write_text("tiny change\n")
git(repo, "add", "--", "change.txt")
git(repo, "commit", "-q", "-m", "tiny change", "--", "change.txt")
tip = git(repo, "rev-parse", "HEAD").stdout.strip()
assert (repo / ".git/objects" / base[:2] / base[2:]).is_file()
if mode != "no-hook":
hook = repo / ".git/hooks/pre-push"
hook.parent.mkdir(parents=True, exist_ok=True)
hook.write_text("#!/bin/sh\nset -eu\ncat >/dev/null\n"
"git repack -adq\ngit prune-packed\n")
hook.chmod(0o700)
before = set((remote / "objects/pack").glob("*.pack"))
config = ["-c", "push.negotiate=true"] if mode == "repack-negotiate" else []
start = time.monotonic()
result = git(repo, *config, "push", "--progress", str(remote),
"HEAD:refs/heads/main")
elapsed = time.monotonic() - start
(case / "push.log").write_text(result.stdout + result.stderr)
packs = set((remote / "objects/pack").glob("*.pack")) - before
remote_tip = git(remote, "rev-parse", "refs/heads/main").stdout.strip()
assert remote_tip == tip
results.append({"case": mode, "push_rc": result.returncode,
"new_remote_pack_bytes": sum(p.stat().st_size for p in packs),
"elapsed_seconds": round(elapsed, 6),
"remote_tip_matches": True})
report = {"git_version": git(root, "version", "--build-options").stdout.strip(),
"platform": {"system": platform.system(), "machine": platform.machine(),
"macos": platform.mac_ver()[0]},
"fixture_bytes": 4 * 1024 * 1024,
"transport": "local bare repository", "results": results}
encoded = json.dumps(report, indent=2) + "\n"
(root / "results.json").write_text(encoded)
print(encoded, end="")
if temporary:
temporary.cleanup()
if __name__ == "__main__":
main()
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [BUG] push resends common history after repack during pre-push (2.54.0, 2.56.0)
2026-10-06 17:37 [BUG] push resends common history after repack during pre-push (2.54.0, 2.56.0) Jens Röcker
2026-10-06 17:47 ` Jens Röcker
2026-10-06 20:03 ` D. Ben Knoble
@ 2026-10-07 9:03 ` Patrick Steinhardt
2 siblings, 0 replies; 4+ messages in thread
From: Patrick Steinhardt @ 2026-10-07 9:03 UTC (permalink / raw)
To: Jens Röcker; +Cc: git
On Tue, Oct 06, 2026 at 07:37:47PM +0200, Jens Röcker wrote:
[snip]
> Possible mechanism, based on source inspection:
>
> In 2.54.0, send-pack.c:feed_object() drops negative OIDs when
> odb_has_object(..., 0) returns false. In 2.56.0, the same quick check is in
> append_negative_object(). In both versions, odb_has_object() uses
> OBJECT_INFO_QUICK unless ODB_HAS_OBJECT_RECHECK_PACKED is set. A parent
> process with a stale pack catalogue may therefore miss the base after the
> hook removes the loose copy; the fresh pack generator then sees the new
> pack and walks history without that excluded base. This is a proposed
> explanation of the measured effect, not an instrumented proof of the
> parent process's in-memory state.
Right, that makes sense, `odb_has_object()` can have false negatives by
default. So in case the object database has been concurrently repacked
we'll potentially end up thinking that the object does not exist at all.
And in `append_negative_object()` (which is the modern equivalent to
`feed_object()`) we'll then silently skip such objects:
static void append_negative_object(struct repository *r,
struct oid_array *haves,
const struct object_id *oid)
{
/*
* The remote end may have advertised objects that we do not have in
* our object database. Skip those, as we cannot use them as boundary.
*/
if (!odb_has_object(r->objects, oid, 0))
return;
oid_array_append(haves, oid);
}
Consequently, we won't mark the object as negative boundary for the graph
walk and thus end up pushing too many objects.
The question is how to fix this. The obvious fix is of course to just
pass `ODB_HAS_OBJECT_RECHECK_PACKED`. But as the comment above explains,
it is expected that we will receive potentially-many object IDs that we
don't even have. And we certainly don't want to reload the object
database every single time we see an object that we truly don't have at
all, as that may be somewhat expensive.
I wonder whether we could maybe batch this check: instead of checking
each negative object separately, we could gather all of them and then
check them for existence. And if any of them are missing, we reload the
object database once and then re-check only those.
That'd be more efficient for sure compared to potentially reloading on
every single missing object. We still have the chance of racing with a
concurrent repack in that case. But maybe that's good enough?
Something like the below (untested) patch.
Thanks!
Patrick
diff --git a/send-pack.c b/send-pack.c
index f20460fbf4..aecc73209e 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -42,17 +42,46 @@ int option_parse_push_signed(const struct option *opt,
die("bad %s argument: %s", opt->long_name, arg);
}
-static void append_negative_object(struct repository *r,
- struct oid_array *haves,
- const struct object_id *oid)
+static void append_negative_objects(struct repository *r,
+ struct oid_array *haves,
+ const struct oidset *oids)
{
+ struct oidset missing = OIDSET_INIT;
+ const struct object_id *oid;
+ struct oidset_iter it;
+
+ oidset_iter_init(oids, &it);
+ while ((oid = oidset_iter_next(&it))) {
+ /*
+ * The remote end may have advertised objects that we do not have in
+ * our object database. Skip those, as we cannot use them as boundary.
+ */
+ if (!odb_has_object(r->objects, oid, 0)) {
+ oidset_insert(&missing, oid);
+ continue;
+ }
+
+ oid_array_append(haves, oid);
+ }
+
+ if (!oidset_size(&missing))
+ return;
+
/*
- * The remote end may have advertised objects that we do not have in
- * our object database. Skip those, as we cannot use them as boundary.
+ * A concurrent process may have repacked objects. Reprepare the object
+ * database once and re-try. Note that we explicitly batch this check
+ * so that we don't reload the object database for every truly-missing
+ * object.
*/
- if (!odb_has_object(r->objects, oid, 0))
- return;
- oid_array_append(haves, oid);
+ odb_reprepare(r->objects);
+
+ oidset_iter_init(&missing, &it);
+ while ((oid = oidset_iter_next(&it))) {
+ if (odb_has_object(r->objects, oid, 0))
+ oid_array_append(haves, oid);
+ }
+
+ oidset_clear(&missing);
}
/*
@@ -64,6 +93,7 @@ static int pack_objects(struct repository *r,
struct send_pack_args *args)
{
struct odb_generate_pack_options opts = ODB_GENERATE_PACK_OPTIONS_INIT;
+ struct oidset negative_oids = OIDSET_INIT;
struct odb_pack_generator *generator;
int rc;
@@ -84,18 +114,20 @@ static int pack_objects(struct repository *r,
opts.pack_fd = args->stateless_rpc ? -1 : fd;
for (size_t i = 0; i < advertised->nr; i++)
- append_negative_object(r, &opts.haves, &advertised->oid[i]);
+ oidset_insert(&negative_oids, &advertised->oid[i]);
for (size_t i = 0; i < negotiated->nr; i++)
- append_negative_object(r, &opts.haves, &negotiated->oid[i]);
+ oidset_insert(&negative_oids, &negotiated->oid[i]);
while (refs) {
if (!is_null_oid(&refs->old_oid))
- append_negative_object(r, &opts.haves, &refs->old_oid);
+ oidset_insert(&negative_oids, &refs->old_oid);
if (!is_null_oid(&refs->new_oid))
oid_array_append(&opts.wants, &refs->new_oid);
refs = refs->next;
}
+ append_negative_objects(r, &opts.haves, &negative_oids);
+
if (odb_generate_pack(r->objects, &generator, &opts))
die("git pack-objects failed");
odb_generate_pack_options_release(&opts);
@@ -114,6 +146,7 @@ static int pack_objects(struct repository *r,
rc = odb_pack_generator_finish(generator);
trace2_region_leave("send_pack", "pack_objects", r);
+ oidset_clear(&negative_oids);
return rc;
}
^ permalink raw reply related [flat|nested] 4+ messages in thread