{"thread":{"id":"65369","subject":"[PATCH] repack-promisor: add fake paths to oids when repacking promisor objects","startedAt":"2026-03-27T16:12:41Z","lastAt":"2026-03-31T15:51:09Z","messageCount":4,"participants":["Abraham Samuel Adekunle","Patrick Steinhardt","Samuel Abraham","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"540192","messageId":"acasFS_UXC8NybtE@Adekunles-MacBook-Air.local","threadId":"65369","inReplyTo":null,"subject":"[PATCH] repack-promisor: add fake paths to oids when repacking promisor objects","fromName":"Abraham Samuel Adekunle","fromEmail":"abrahamadekunle50@gmail.com","sentAt":"2026-03-27T16:12:43Z","receivedAt":"2026-03-27T16:12:41Z","isPatch":true,"body":"This change addresses the NEEDSWORK comment added by commit\n5d19e81 (repack: repack promisor objects if -a or -A is set).\n\nWhen 'git-repack' repacks promisor objects, only the raw oids\nare sent to 'git-pack-objects'.\nThis gives 'git-pack-objects' no information about the original\npack order of those objects in the packfile so it must\nrely on its default strategy of sorting the objects by type and\nthen by size over again. This can produce suboptimal packfiles\nbecause the objects that were previously stored in the same\npackfile can become separated.\n\nProvide a hint to 'git-pack-objects' when sorting, by using the\npackfile basename, and the offset of the object in the existing\npackfile as fake paths when writing the oids to 'git-pack-objects'.\n\nThis will ensure they can be grouped by the type and existing pack\norder which will make them end up close together in the sort, improving\ndelta compression.\n\nSigned-off-by: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>\n---\n repack-promisor.c | 39 +++++++++++++++++++++++++++++----------\n 1 file changed, 29 insertions(+), 10 deletions(-)\n\ndiff --git a/repack-promisor.c b/repack-promisor.c\nindex 90318ce150..3f3034fb79 100644\n--- a/repack-promisor.c\n+++ b/repack-promisor.c\n@@ -12,25 +12,51 @@ struct write_oid_context {\n \tconst struct git_hash_algo *algop;\n };\n \n+/**\n+ * Build fake path for the objects to give pack-objects\n+ * an ordering hint.\n+ * For the packed objects: pack-basename/offset-padded\n+ */\n+\n+static void build_ordering_hint(struct object_info *oi, struct strbuf *hint)\n+{\n+\tstruct packed_git *pack;\n+\tunsigned long offset;\n+\n+\tif (oi->whence == OI_PACKED) {\n+\t\tpack = oi->u.packed.pack;\n+\t\toffset = oi->u.packed.offset;\n+\t\tstrbuf_addf(hint, \"%s/%05lu\", pack_basename(pack), (unsigned long)offset);\n+\t} else\n+\t\tstrbuf_addstr(hint, \"loose\");\n+}\n+\n /*\n  * Write oid to the given struct child_process's stdin, starting it first if\n  * necessary.\n  */\n static int write_oid(const struct object_id *oid,\n-\t\t     struct object_info *oi UNUSED,\n+\t\t     struct object_info *oi,\n \t\t     void *data)\n {\n \tstruct write_oid_context *ctx = data;\n \tstruct child_process *cmd = ctx->cmd;\n+\tstruct strbuf hint = STRBUF_INIT;\n \n \tif (cmd->in == -1) {\n \t\tif (start_command(cmd))\n \t\t\tdie(_(\"could not start pack-objects to repack promisor objects\"));\n \t}\n \n+\tbuild_ordering_hint(oi, &hint);\n+\n \tif (write_in_full(cmd->in, oid_to_hex(oid), ctx->algop->hexsz) < 0 ||\n+\t    write_in_full(cmd->in, \" \", 1) < 0 ||\n+\t    write_in_full(cmd->in, hint.buf, hint.len) < 0 ||\n \t    write_in_full(cmd->in, \"\\n\", 1) < 0)\n \t\tdie(_(\"failed to feed promisor objects to pack-objects\"));\n+\n+\tstrbuf_release(&hint);\n \treturn 0;\n }\n \n@@ -85,20 +111,13 @@ void repack_promisor_objects(struct repository *repo,\n {\n \tstruct write_oid_context ctx;\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\tstruct object_info request = OBJECT_INFO_INIT;\n \n \tprepare_pack_objects(&cmd, args, packtmp);\n \tcmd.in = -1;\n-\n-\t/*\n-\t * NEEDSWORK: Giving pack-objects only the OIDs without any ordering\n-\t * hints may result in suboptimal deltas in the resulting pack. See if\n-\t * the OIDs can be sent with fake paths such that pack-objects can use a\n-\t * {type -> existing pack order} ordering when computing deltas instead\n-\t * of a {type -> size} ordering, which may produce better deltas.\n-\t */\n \tctx.cmd = &cmd;\n \tctx.algop = repo->hash_algo;\n-\todb_for_each_object(repo->objects, NULL, write_oid, &ctx,\n+\todb_for_each_object(repo->objects, &request, write_oid, &ctx,\n \t\t\t    ODB_FOR_EACH_OBJECT_PROMISOR_ONLY);\n \n \tif (cmd.in == -1) {\n-- \n2.39.5 (Apple Git-154)\n\n"},{"id":"540466","messageId":"actjaxIkDEXHJbyi@pks.im","threadId":"65369","inReplyTo":"acasFS_UXC8NybtE@Adekunles-MacBook-Air.local","subject":"Re: [PATCH] repack-promisor: add fake paths to oids when repacking promisor objects","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-31T06:02:19Z","receivedAt":"2026-03-31T06:02:32Z","isPatch":true,"body":"On Fri, Mar 27, 2026 at 05:12:43PM +0100, Abraham Samuel Adekunle wrote:\n> This change addresses the NEEDSWORK comment added by commit\n> 5d19e81 (repack: repack promisor objects if -a or -A is set).\n> \n> When 'git-repack' repacks promisor objects, only the raw oids\n> are sent to 'git-pack-objects'.\n> This gives 'git-pack-objects' no information about the original\n> pack order of those objects in the packfile so it must\n> rely on its default strategy of sorting the objects by type and\n> then by size over again. This can produce suboptimal packfiles\n> because the objects that were previously stored in the same\n> packfile can become separated.\n> \n> Provide a hint to 'git-pack-objects' when sorting, by using the\n> packfile basename, and the offset of the object in the existing\n> packfile as fake paths when writing the oids to 'git-pack-objects'.\n> \n> This will ensure they can be grouped by the type and existing pack\n> order which will make them end up close together in the sort, improving\n> delta compression.\n\nI think the general idea may be sound, but ideally we would have some\nbenchmarks that demonstrate it actually is. Like, can you come up with\nscenarios where it will indeed improve the packfile size and show the\nadvantage of this change? Are there scenarios that are likely to have a\ndisadvantage because of this new ordering? Which of these scenarios do\nwe expect to be more likely?\n\nBefore answering these questions we basically just claim it's going to\nbe an improvement without actually verifying.\n\n> diff --git a/repack-promisor.c b/repack-promisor.c\n> index 90318ce150..3f3034fb79 100644\n> --- a/repack-promisor.c\n> +++ b/repack-promisor.c\n> @@ -12,25 +12,51 @@ struct write_oid_context {\n>  \tconst struct git_hash_algo *algop;\n>  };\n>  \n> +/**\n> + * Build fake path for the objects to give pack-objects\n> + * an ordering hint.\n> + * For the packed objects: pack-basename/offset-padded\n> + */\n> +\n\nNit: this empty line can be removed.\n\n> +static void build_ordering_hint(struct object_info *oi, struct strbuf *hint)\n> +{\n> +\tstruct packed_git *pack;\n> +\tunsigned long offset;\n> +\n> +\tif (oi->whence == OI_PACKED) {\n> +\t\tpack = oi->u.packed.pack;\n> +\t\toffset = oi->u.packed.offset;\n> +\t\tstrbuf_addf(hint, \"%s/%05lu\", pack_basename(pack), (unsigned long)offset);\n> +\t} else\n> +\t\tstrbuf_addstr(hint, \"loose\");\n\nNit: our coding guidelines say that once an if statement requires curly\nbraces in one branch, all branches should have them.\n\nI also have to wonder whether it's going to be a benefit to also specify\na hint for loose objects, or whether we should rather not write any hint\nat all for them.\n\n> +}\n> +\n>  /*\n>   * Write oid to the given struct child_process's stdin, starting it first if\n>   * necessary.\n>   */\n>  static int write_oid(const struct object_id *oid,\n> -\t\t     struct object_info *oi UNUSED,\n> +\t\t     struct object_info *oi,\n>  \t\t     void *data)\n>  {\n>  \tstruct write_oid_context *ctx = data;\n>  \tstruct child_process *cmd = ctx->cmd;\n> +\tstruct strbuf hint = STRBUF_INIT;\n>  \n>  \tif (cmd->in == -1) {\n>  \t\tif (start_command(cmd))\n>  \t\t\tdie(_(\"could not start pack-objects to repack promisor objects\"));\n>  \t}\n>  \n> +\tbuild_ordering_hint(oi, &hint);\n> +\n>  \tif (write_in_full(cmd->in, oid_to_hex(oid), ctx->algop->hexsz) < 0 ||\n> +\t    write_in_full(cmd->in, \" \", 1) < 0 ||\n> +\t    write_in_full(cmd->in, hint.buf, hint.len) < 0 ||\n>  \t    write_in_full(cmd->in, \"\\n\", 1) < 0)\n>  \t\tdie(_(\"failed to feed promisor objects to pack-objects\"));\n\nThis now translate into at least four write(3p) syscalls per object. Can\nwe maybe reuse a buffer so that we can reduce the number of syscalls?\n\nThanks!\n\nPatrick\n"},{"id":"540498","messageId":"CADYq+fbLFH-gc9=N9H63N74wr2CEZDxwRAxdvc1Jq6R0dCsOeQ@mail.gmail.com","threadId":"65369","inReplyTo":"actjaxIkDEXHJbyi@pks.im","subject":"Re: [PATCH] repack-promisor: add fake paths to oids when repacking promisor objects","fromName":"Samuel Abraham","fromEmail":"abrahamadekunle50@gmail.com","sentAt":"2026-03-31T09:53:04Z","receivedAt":"2026-03-31T09:53:04Z","isPatch":true,"body":"On Tue, Mar 31, 2026 at 7:02 AM Patrick Steinhardt <ps@pks.im> wrote:\n>\n> On Fri, Mar 27, 2026 at 05:12:43PM +0100, Abraham Samuel Adekunle wrote:\n> > This change addresses the NEEDSWORK comment added by commit\n> > 5d19e81 (repack: repack promisor objects if -a or -A is set).\n> >\n> > When 'git-repack' repacks promisor objects, only the raw oids\n> > are sent to 'git-pack-objects'.\n> > This gives 'git-pack-objects' no information about the original\n> > pack order of those objects in the packfile so it must\n> > rely on its default strategy of sorting the objects by type and\n> > then by size over again. This can produce suboptimal packfiles\n> > because the objects that were previously stored in the same\n> > packfile can become separated.\n> >\n> > Provide a hint to 'git-pack-objects' when sorting, by using the\n> > packfile basename, and the offset of the object in the existing\n> > packfile as fake paths when writing the oids to 'git-pack-objects'.\n> >\n> > This will ensure they can be grouped by the type and existing pack\n> > order which will make them end up close together in the sort, improving\n> > delta compression.\n>\n> I think the general idea may be sound, but ideally we would have some\n> benchmarks that demonstrate it actually is. Like, can you come up with\n> scenarios where it will indeed improve the packfile size and show the\n> advantage of this change? Are there scenarios that are likely to have a\n> disadvantage because of this new ordering? Which of these scenarios do\n> we expect to be more likely?\n\nThank you Patrick for the review.\nOkay I will look into this\n\n>\n> Before answering these questions we basically just claim it's going to\n> be an improvement without actually verifying.\n\nYes I agree\n\n>\n> > diff --git a/repack-promisor.c b/repack-promisor.c\n> > index 90318ce150..3f3034fb79 100644\n> > --- a/repack-promisor.c\n> > +++ b/repack-promisor.c\n> > @@ -12,25 +12,51 @@ struct write_oid_context {\n> >       const struct git_hash_algo *algop;\n> >  };\n> >\n> > +/**\n> > + * Build fake path for the objects to give pack-objects\n> > + * an ordering hint.\n> > + * For the packed objects: pack-basename/offset-padded\n> > + */\n> > +\n>\n> Nit: this empty line can be removed.\n\nOkay\n\n>\n> > +static void build_ordering_hint(struct object_info *oi, struct strbuf *hint)\n> > +{\n> > +     struct packed_git *pack;\n> > +     unsigned long offset;\n> > +\n> > +     if (oi->whence == OI_PACKED) {\n> > +             pack = oi->u.packed.pack;\n> > +             offset = oi->u.packed.offset;\n> > +             strbuf_addf(hint, \"%s/%05lu\", pack_basename(pack), (unsigned long)offset);\n> > +     } else\n> > +             strbuf_addstr(hint, \"loose\");\n>\n> Nit: our coding guidelines say that once an if statement requires curly\n> braces in one branch, all branches should have them.\n\nOkay thank you\n\n>\n> I also have to wonder whether it's going to be a benefit to also specify\n> a hint for loose objects, or whether we should rather not write any hint\n> at all for them.\n\nOkay, I think not writing any hints makes sense.\nThanks\n\n>\n> > +}\n> > +\n> >  /*\n> >   * Write oid to the given struct child_process's stdin, starting it first if\n> >   * necessary.\n> >   */\n> >  static int write_oid(const struct object_id *oid,\n> > -                  struct object_info *oi UNUSED,\n> > +                  struct object_info *oi,\n> >                    void *data)\n> >  {\n> >       struct write_oid_context *ctx = data;\n> >       struct child_process *cmd = ctx->cmd;\n> > +     struct strbuf hint = STRBUF_INIT;\n> >\n> >       if (cmd->in == -1) {\n> >               if (start_command(cmd))\n> >                       die(_(\"could not start pack-objects to repack promisor objects\"));\n> >       }\n> >\n> > +     build_ordering_hint(oi, &hint);\n> > +\n> >       if (write_in_full(cmd->in, oid_to_hex(oid), ctx->algop->hexsz) < 0 ||\n> > +         write_in_full(cmd->in, \" \", 1) < 0 ||\n> > +         write_in_full(cmd->in, hint.buf, hint.len) < 0 ||\n> >           write_in_full(cmd->in, \"\\n\", 1) < 0)\n> >               die(_(\"failed to feed promisor objects to pack-objects\"));\n>\n> This now translate into at least four write(3p) syscalls per object. Can\n> we maybe reuse a buffer so that we can reduce the number of syscalls?\n\nOkay noted\nThanks\n\nAbraham\n"},{"id":"540529","messageId":"xmqqqzp02e1h.fsf@gitster.g","threadId":"65369","inReplyTo":"actjaxIkDEXHJbyi@pks.im","subject":"Re: [PATCH] repack-promisor: add fake paths to oids when repacking promisor objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-31T15:51:06Z","receivedAt":"2026-03-31T15:51:09Z","isPatch":true,"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n>> This will ensure they can be grouped by the type and existing pack\n>> order which will make them end up close together in the sort, improving\n>> delta compression.\n>\n> I think the general idea may be sound, but ideally we would have some\n> benchmarks that demonstrate it actually is. Like, can you come up with\n> scenarios where it will indeed improve the packfile size and show the\n> advantage of this change? Are there scenarios that are likely to have a\n> disadvantage because of this new ordering? Which of these scenarios do\n> we expect to be more likely?\n>\n> Before answering these questions we basically just claim it's going to\n> be an improvement without actually verifying.\n\nWhile it is a very good point that a change that claims to improve\nperformance must come with verifyable data, because the packfile\nsize alone is not what you want to optimize for in the first place,\nit is quite hard to come up with a useful benchmark in this area.\n\nBack when I was working on packfile generation, we needed to\noptimize for two things (luckily they are not competing goals).  One\nis to choose a good delta-base, which will contribute to an overall\npack size that is smaller.  The ordering of the objects in a pack,\non the other hand, does not directly contribute to the size, but has\nimpact on runtime performance, by keeping related things closer\ntogether to reduce the need to \"seek\" in the pack stream.\n\nGenerally, two objects that appear next to each other in a well\noptimized packstream are not expected to be similar with each other.\nThey are more likely to be two unrelated files that appear in the\nsame tree object (i.e., they do not delta with each other well, but\nat runtime, they are often needed together).  So it may even be\ndetrimental to use the offset in packfile as a clue to choose among\npotential delta bases.\n\nDo we have name-hash data for the original pack somewhere available\nso that the repacker can take advantage of?  If so, it may be more\nrelevant thing to reuse.\n\nI agree with your other points in your review, too.  Thanks for\nhelping this topic.\n"}]}