Re: [BUG] push resends common history after repack during pre-push (2.54.0, 2.56.0)
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Oct 7, 2026, 09:03 UTC
- Message-ID
- <asYK6ld53e8lJ4Ir@pks.im>
- In-Reply-To
- <CA+tGzvYYKm=Yo88knZb4oavG9dH5smUCXnoqa-RR9-7YEBycVA@mail.gmail.com>
On Tue, Oct 06, 2026 at 07:37:47PM +0200, Jens Röcker wrote: [snip]
Show 11 quoted lines
> 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; }