Re: [BUG] push resends common history after repack during pre-push (2.54.0, 2.56.0)

[Patrick Steinhardt <ps@pks.im>]

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;
 }