Re: [GSoC PATCH 2/3] pack-write: add helper to fill promisor file after repack

[Lorenzo Pegorari <lorenzo.pegorari2002@gmail.com>]

On Sat, Mar 21, 2026 at 10:04:01PM -0400, Eric Sunshine wrote:
> On Sat, Mar 21, 2026 at 5:29 PM LorenzoPegorari
> <lorenzo.pegorari2002@gmail.com> wrote:
> > Create a `copy_all_promisor_files()` helper function used to copy the
> > contents of all ".promisor" files in a `repository` inside another
> > ".promisor" file.
> >
> > This function can be used to preserve the contents of all ".promisor"
> > files inside a new ".promisor" file, for example when a repack happens.
> >
> > This function is written in such a way so that it will read all the
> > ".promisor" files inside the given `repository` line by line, and copy
> > only the lines that are not already present in the destination file. This
> > is done to avoid copying the same lines multiple times that may come from
> > multiple (redundant) packfiles. A better way to achieve this might be (is
> > definitely) possible.
> >
> > Signed-off-by: LorenzoPegorari <lorenzo.pegorari2002@gmail.com>
> > ---
> > diff --git a/pack-write.c b/pack-write.c
> > @@ -621,3 +621,65 @@ void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_
> > +void copy_all_promisor_files(struct repository *repo, const char *promisor_name)
> > +{
> > +       struct strbuf promisor_source_name = STRBUF_INIT;
> > +       struct strbuf read_source = STRBUF_INIT, read_dest = STRBUF_INIT;
> > +       struct strbuf write_dest = STRBUF_INIT;
> 
> These strbufs don't seem to be released, thus are leaked.

Of course... trivial mistake. Will fix it in v2.

> > +       int err;
> > +
> > +       FILE *dest = xfopen(promisor_name, "r+");
> > +
> > +       struct packed_git *p;
> 
> Style nit: Place all the variable declarations together (without blank
> lines), followed by a blank line.

Ack.

> > +       repo_for_each_pack(repo, p) {
> > +               if (!p->pack_promisor)
> > +                       continue;
> > +
> > +               strbuf_reset(&promisor_source_name);
> > +               strbuf_addstr(&promisor_source_name, p->pack_name);
> > +               strbuf_strip_suffix(&promisor_source_name, ".pack");
> > +               strbuf_addstr(&promisor_source_name, ".promisor");
> > +               FILE *source = xfopen(promisor_source_name.buf, "r");
> 
> This project still frowns upon variable declaration after code. You
> will want to declare `FILE *source;` at the top of this loop body and
> then assign `source = xfopen(...)` here.

Ack.

> > +               /*
> > +                * For each line of the promisor source file, check if it already
> > +                * is in the promisor dest file. If not, add it to write_dest, so
> > +                * that it will be written in the dest file.
> > +                */
> > +               while (strbuf_getline(&read_source, source) != EOF) {
> > +                       if (fseek(dest, 0L, SEEK_SET))
> > +                               die_errno(_("fseek failed"));
> > +                       int is_source_in_dest = 0;
> 
> Ditto regarding variable declaration following code.

Ack.

> > +                       while (strbuf_getline(&read_dest, dest) != EOF) {
> > +                               if (!strbuf_cmp(&read_source, &read_dest)) {
> > +                                       is_source_in_dest = 1;
> > +                                       break;
> > +                               }
> > +                       }
> > +                       if (!is_source_in_dest) {
> > +                               strbuf_addbuf(&write_dest, &read_source);
> > +                               strbuf_addstr(&write_dest, "\n");
> > +                       }
> 
> The commit message talks about this, and it is indeed very ugly that
> this re-reads `dest` from the beginning for *every* `source` line. Is
> there a reason you can't simply read `dest` into a `strset` (see Git's
> `strmap.h`) in its entirety before entering the repo_for_each_pack()
> loop and then merely check the strset for existence using
> strset_add()?

No reason at all, except for me to knowing about `strset`! Thanks for
suggesting it to me. Will use it in v2.

> > +               }
> > +
> > +               if (write_dest.len) {
> > +                       strbuf_strip_suffix(&write_dest, "\n");
> > +                       if (fseek(dest, 0L, SEEK_END))
> > +                               die_errno(_("fseek failed"));
> > +                       fprintf(dest, "%s\n", write_dest.buf);
> > +                       fflush(dest);
> > +                       strbuf_reset(&write_dest);
> > +               }
> > +
> > +               err = ferror(source);
> > +               err |= fclose(source);
> > +               if (err)
> > +                       die(_("could not read '%s' promisor file"), promisor_source_name.buf);
> > +       }
> > +
> > +       err = ferror(dest);
> > +       err |= fclose(dest);
> > +       if (err)
> > +               die(_("could not write '%s' promisor file"), promisor_name);
> > +}