From: Samuel Abraham <abrahamadekunle50@gmail.com>
Date: Tue, 31 Mar 2026 09:53:04 GMT
Subject: Re: [PATCH] repack-promisor: add fake paths to oids when repacking promisor objects
Message-ID: <CADYq+fbLFH-gc9=N9H63N74wr2CEZDxwRAxdvc1Jq6R0dCsOeQ@mail.gmail.com>
In-Reply-To: <actjaxIkDEXHJbyi@pks.im>

On Tue, Mar 31, 2026 at 7:02 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Fri, Mar 27, 2026 at 05:12:43PM +0100, Abraham Samuel Adekunle wrote:
> > 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?

Thank you Patrick for the review.
Okay I will look into this

>
> Before answering these questions we basically just claim it's going to
> be an improvement without actually verifying.

Yes I agree

>
> > 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.

Okay

>
> > +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.

Okay thank you

>
> 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.

Okay, I think not writing any hints makes sense.
Thanks

>
> > +}
> > +
> >  /*
> >   * 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?

Okay noted
Thanks

Abraham