From: Lorenzo Pegorari Date: Sun, 22 Mar 2026 18:50:34 GMT Subject: Re: [GSoC PATCH 2/3] pack-write: add helper to fill promisor file after repack Message-ID: In-Reply-To: On Sat, Mar 21, 2026 at 10:04:01PM -0400, Eric Sunshine wrote: > On Sat, Mar 21, 2026 at 5:29 PM LorenzoPegorari > 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 > > --- > > 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); > > +}