Re: [PATCH] repack-promisor: add fake paths to oids when repacking promisor objects
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Mar 31, 2026, 06:02 UTC
- Message-ID
- <actjaxIkDEXHJbyi@pks.im>
- In-Reply-To
- <acasFS_UXC8NybtE@Adekunles-MacBook-Air.local>
On Fri, Mar 27, 2026 at 05:12:43PM +0100, Abraham Samuel Adekunle wrote:
Show 19 quoted lines
> 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.
I think the general idea may be sound, but ideally we would have some benchmarks that demonstrate it actually is. Like, can you come up with scenarios where it will indeed improve the packfile size and show the advantage of this change? Are there scenarios that are likely to have a disadvantage because of this new ordering? Which of these scenarios do we expect to be more likely?
Before answering these questions we basically just claim it's going to be an improvement without actually verifying.
Show 14 quoted lines
> 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
> + */
> +Nit: this empty line can be removed.
Show 11 quoted lines
> +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");Nit: our coding guidelines say that once an if statement requires curly braces in one branch, all branches should have them.
I also have to wonder whether it's going to be a benefit to also specify a hint for loose objects, or whether we should rather not write any hint at all for them.
Show 27 quoted lines
> +}
> +
> /*
> * 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"));This now translate into at least four write(3p) syscalls per object. Can we maybe reuse a buffer so that we can reduce the number of syscalls?
Thanks!
Patrick