[PATCH] repack-promisor: add fake paths to oids when repacking promisor objects

[PATCH] repack-promisor: add fake paths to oids when repacking promisor objects

[Abraham Samuel Adekunle]

This change addresses the NEEDSWORK comment added by commit
5d19e81 (repack: repack promisor objects if -a or -A is set).

When 'git-repack' repacks promisor objects, only the raw oids
are sent to 'git-pack-objects'.
This gives 'git-pack-objects' no information about the original
pack order of those objects in the packfile so it must
rely on its default strategy of sorting the objects by type and
then by size over again. This can produce suboptimal packfiles
because the objects that were previously stored in the same
packfile can become separated.

Provide a hint to 'git-pack-objects' when sorting, by using the
packfile basename, and the offset of the object in the existing
packfile as fake paths when writing the oids to 'git-pack-objects'.

This will ensure they can be grouped by the type and existing pack
order which will make them end up close together in the sort, improving
delta compression.

Signed-off-by: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>
---
 repack-promisor.c | 39 +++++++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 10 deletions(-)

diff --git a/repack-promisor.c b/repack-promisor.c
index 90318ce150..3f3034fb79 100644
--- a/repack-promisor.c
+++ b/repack-promisor.c
@@ -12,25 +12,51 @@ struct write_oid_context {
 	const struct git_hash_algo *algop;
 };
 
+/**
+ * Build fake path for the objects to give pack-objects
+ * an ordering hint.
+ * For the packed objects: pack-basename/offset-padded
+ */
+
+static void build_ordering_hint(struct object_info *oi, struct strbuf *hint)
+{
+	struct packed_git *pack;
+	unsigned long offset;
+
+	if (oi->whence == OI_PACKED) {
+		pack = oi->u.packed.pack;
+		offset = oi->u.packed.offset;
+		strbuf_addf(hint, "%s/%05lu", pack_basename(pack), (unsigned long)offset);
+	} else
+		strbuf_addstr(hint, "loose");
+}
+
 /*
  * Write oid to the given struct child_process's stdin, starting it first if
  * necessary.
  */
 static int write_oid(const struct object_id *oid,
-		     struct object_info *oi UNUSED,
+		     struct object_info *oi,
 		     void *data)
 {
 	struct write_oid_context *ctx = data;
 	struct child_process *cmd = ctx->cmd;
+	struct strbuf hint = STRBUF_INIT;
 
 	if (cmd->in == -1) {
 		if (start_command(cmd))
 			die(_("could not start pack-objects to repack promisor objects"));
 	}
 
+	build_ordering_hint(oi, &hint);
+
 	if (write_in_full(cmd->in, oid_to_hex(oid), ctx->algop->hexsz) < 0 ||
+	    write_in_full(cmd->in, " ", 1) < 0 ||
+	    write_in_full(cmd->in, hint.buf, hint.len) < 0 ||
 	    write_in_full(cmd->in, "\n", 1) < 0)
 		die(_("failed to feed promisor objects to pack-objects"));
+
+	strbuf_release(&hint);
 	return 0;
 }
 
@@ -85,20 +111,13 @@ void repack_promisor_objects(struct repository *repo,
 {
 	struct write_oid_context ctx;
 	struct child_process cmd = CHILD_PROCESS_INIT;
+	struct object_info request = OBJECT_INFO_INIT;
 
 	prepare_pack_objects(&cmd, args, packtmp);
 	cmd.in = -1;
-
-	/*
-	 * NEEDSWORK: Giving pack-objects only the OIDs without any ordering
-	 * hints may result in suboptimal deltas in the resulting pack. See if
-	 * the OIDs can be sent with fake paths such that pack-objects can use a
-	 * {type -> existing pack order} ordering when computing deltas instead
-	 * of a {type -> size} ordering, which may produce better deltas.
-	 */
 	ctx.cmd = &cmd;
 	ctx.algop = repo->hash_algo;
-	odb_for_each_object(repo->objects, NULL, write_oid, &ctx,
+	odb_for_each_object(repo->objects, &request, write_oid, &ctx,
 			    ODB_FOR_EACH_OBJECT_PROMISOR_ONLY);
 
 	if (cmd.in == -1) {
-- 
2.39.5 (Apple Git-154)