{"thread":{"id":"65326","subject":"[GSoC PATCH 0/3] preserve promisor files content after repack","startedAt":"2026-03-21T21:28:24Z","lastAt":"2026-05-19T15:17:49Z","messageCount":79,"participants":["LorenzoPegorari","Eric Sunshine","Lorenzo Pegorari","Junio C Hamano","Tian Yuchen","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"539616","messageId":"cover.1774125871.git.lorenzo.pegorari2002@gmail.com","threadId":"65326","inReplyTo":null,"subject":"[GSoC PATCH 0/3] preserve promisor files content after repack","fromName":"LorenzoPegorari","fromEmail":"lorenzo.pegorari2002@gmail.com","sentAt":"2026-03-21T21:28:20Z","receivedAt":"2026-03-21T21:28:24Z","isPatch":true,"body":"The goal of this patch is to solve the NEEDSWORK comment added by\n5374a290 (fetch-pack: write fetched refs to .promisor, 14/10/2019). This\nis done by adding a helper function that takes the content of all\n.promisor files in the `repository`, and copies it inside the first\n.promisor file created by the repack.\n\nAlso, I added a comment explaining what is the purpose of the content of\nthe .promisor files, since this wasn't explained anywhere (I found\ninformation regarding this only in the message of the previously cited\ncommit).\n\nI am not satisfied at all with this patch, and I would love to have some\nfeedback for the v2. The issues/questions that I had while writing these\npatches are the following:\n * Is there a way to not have to check line by line if the content of\n .promisor files are already inside the destination .promisor file?\n * Does it make sense to copy everything inside the first .promisor file\n created by the repack? Is it worth the effort make sure to copy each\n ref (and associatde hash) inside the .promisor file of the packfile\n that contains that ref?\n\n\nLorenzoPegorari (3):\n pack-write: add explanation to promisor file content\n pack-write: add helper to fill promisor file after repack\n repack-promisor: preserve content of promisor files after repack\n\n Documentation/git-repack.adoc | 4 +-\n pack-write.c | 71 +++++++++++++++++++++++++++++++++++\n pack.h | 1 +\n repack-promisor.c | 23 ++++++++----\n 4 files changed, 89 insertions(+), 10 deletions(-)\n\n-- \n2.43.0\n\n"},{"id":"539617","messageId":"9bba49563e809634e223c3b9d75496cde94707db.1774125871.git.lorenzo.pegorari2002@gmail.com","threadId":"65326","inReplyTo":"cover.1774125871.git.lorenzo.pegorari2002@gmail.com","subject":"[GSoC PATCH 1/3] pack-write: add explanation to promisor file content","fromName":"LorenzoPegorari","fromEmail":"lorenzo.pegorari2002@gmail.com","sentAt":"2026-03-21T21:28:35Z","receivedAt":"2026-03-21T21:28:39Z","isPatch":true,"body":"In the entire codebase there is no explanation as to why the \".promisor\"\nfiles may contain the ref names (and their associated hashes) that were\nfetched at the time the corresponding packfile was downloaded.\n\nAdd comment explaining that these pieces of information are used only for\ndebugging reasons, and how they can be used while debugging.\n\nSigned-off-by: LorenzoPegorari \n---\n pack-write.c | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/pack-write.c b/pack-write.c\nindex 83eaf88541..6a2023327e 100644\n--- a/pack-write.c\n+++ b/pack-write.c\n@@ -603,6 +603,15 @@ void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_\n \tint i, err;\n \tFILE *output = xfopen(promisor_name, \"w\");\n \n+\t/*\n+\t* Write in the .promisor file the ref names and associated hashes,\n+\t* obtained by fetch-pack, at the point of generation of the\n+\t* corresponding packfile. These pieces of info are only used to make\n+\t* it easier to debug issues with partial clones, as we can identify\n+\t* what refs (and their associated hashes) were fetched at the time\n+\t* the packfile was downloaded, and if necessary, compare those hashes\n+\t* against what the promisor remote reports now.\n+\t*/\n \tfor (i = 0; i < nr_sought; i++)\n \t\tfprintf(output, \"%s %s\\n\", oid_to_hex(&sought[i]->old_oid),\n \t\t\tsought[i]->name);\n-- \n2.43.0\n\n"},{"id":"539618","messageId":"3c0702f81b3e4a57cd38284ab90f8277da3fcd40.1774125871.git.lorenzo.pegorari2002@gmail.com","threadId":"65326","inReplyTo":"cover.1774125871.git.lorenzo.pegorari2002@gmail.com","subject":"[GSoC PATCH 2/3] pack-write: add helper to fill promisor file after repack","fromName":"LorenzoPegorari","fromEmail":"lorenzo.pegorari2002@gmail.com","sentAt":"2026-03-21T21:28:51Z","receivedAt":"2026-03-21T21:28:55Z","isPatch":true,"body":"Create a `copy_all_promisor_files()` helper function used to copy the\ncontents of all \".promisor\" files in a `repository` inside another\n\".promisor\" file.\n\nThis function can be used to preserve the contents of all \".promisor\"\nfiles inside a new \".promisor\" file, for example when a repack happens.\n\nThis function is written in such a way so that it will read all the\n\".promisor\" files inside the given `repository` line by line, and copy\nonly the lines that are not already present in the destination file. This\nis done to avoid copying the same lines multiple times that may come from\nmultiple (redundant) packfiles. A better way to achieve this might be (is\ndefinitely) possible.\n\nSigned-off-by: LorenzoPegorari \n---\n pack-write.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++\n pack.h | 1 +\n 2 files changed, 63 insertions(+)\n\ndiff --git a/pack-write.c b/pack-write.c\nindex 6a2023327e..3620e6bd02 100644\n--- a/pack-write.c\n+++ b/pack-write.c\n@@ -621,3 +621,65 @@ void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_\n \tif (err)\n \t\tdie(_(\"could not write '%s' promisor file\"), promisor_name);\n }\n+\n+void copy_all_promisor_files(struct repository *repo, const char *promisor_name)\n+{\n+\tstruct strbuf promisor_source_name = STRBUF_INIT;\n+\tstruct strbuf read_source = STRBUF_INIT, read_dest = STRBUF_INIT;\n+\tstruct strbuf write_dest = STRBUF_INIT;\n+\tint err;\n+\n+\tFILE *dest = xfopen(promisor_name, \"r+\");\n+\n+\tstruct packed_git *p;\n+\trepo_for_each_pack(repo, p) {\n+\t\tif (!p->pack_promisor)\n+\t\t\tcontinue;\n+\n+\t\tstrbuf_reset(&promisor_source_name);\n+\t\tstrbuf_addstr(&promisor_source_name, p->pack_name);\n+\t\tstrbuf_strip_suffix(&promisor_source_name, \".pack\");\n+\t\tstrbuf_addstr(&promisor_source_name, \".promisor\");\n+\t\tFILE *source = xfopen(promisor_source_name.buf, \"r\");\n+\n+\t\t/*\n+\t\t * For each line of the promisor source file, check if it already\n+\t\t * is in the promisor dest file. If not, add it to write_dest, so\n+\t\t * that it will be written in the dest file.\n+\t\t */\n+\t\twhile (strbuf_getline(&read_source, source) != EOF) {\n+\t\t\tif (fseek(dest, 0L, SEEK_SET))\n+\t\t\t\tdie_errno(_(\"fseek failed\"));\n+\t\t\tint is_source_in_dest = 0;\n+\t\t\twhile (strbuf_getline(&read_dest, dest) != EOF) {\n+\t\t\t\tif (!strbuf_cmp(&read_source, &read_dest)) {\n+\t\t\t\t\tis_source_in_dest = 1;\n+\t\t\t\t\tbreak;\n+\t\t\t\t}\n+\t\t\t}\n+\t\t\tif (!is_source_in_dest) {\n+\t\t\t\tstrbuf_addbuf(&write_dest, &read_source);\n+\t\t\t\tstrbuf_addstr(&write_dest, \"\\n\");\n+\t\t\t}\n+\t\t}\n+\n+\t\tif (write_dest.len) {\n+\t\t\tstrbuf_strip_suffix(&write_dest, \"\\n\");\n+\t\t\tif (fseek(dest, 0L, SEEK_END))\n+\t\t\t\tdie_errno(_(\"fseek failed\"));\n+\t\t\tfprintf(dest, \"%s\\n\", write_dest.buf);\n+\t\t\tfflush(dest);\n+\t\t\tstrbuf_reset(&write_dest);\n+\t\t}\n+\n+\t\terr = ferror(source);\n+\t\terr |= fclose(source);\n+\t\tif (err)\n+\t\t\tdie(_(\"could not read '%s' promisor file\"), promisor_source_name.buf);\n+\t}\n+\n+\terr = ferror(dest);\n+\terr |= fclose(dest);\n+\tif (err)\n+\t\tdie(_(\"could not write '%s' promisor file\"), promisor_name);\n+}\ndiff --git a/pack.h b/pack.h\nindex ec76472e49..509e90edba 100644\n--- a/pack.h\n+++ b/pack.h\n@@ -105,6 +105,7 @@ char *index_pack_lockfile(struct repository *r, int fd, int *is_well_formed);\n struct ref;\n \n void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_sought);\n+void copy_all_promisor_files(struct repository *repo, const char *promisor_name);\n \n char *write_rev_file(struct repository *repo,\n \t\t const char *rev_name,\n-- \n2.43.0\n\n"},{"id":"539619","messageId":"6967066fe347cc2377d0f5dce14cfa7e569a8b66.1774125871.git.lorenzo.pegorari2002@gmail.com","threadId":"65326","inReplyTo":"cover.1774125871.git.lorenzo.pegorari2002@gmail.com","subject":"[GSoC PATCH 3/3] repack-promisor: preserve content of promisor files after repack","fromName":"LorenzoPegorari","fromEmail":"lorenzo.pegorari2002@gmail.com","sentAt":"2026-03-21T21:29:01Z","receivedAt":"2026-03-21T21:29:05Z","isPatch":true,"body":"When a repack involving promisor packfiles happens, the new \".promisor\"\nfile is created empty, losing all the debug info that might be present\ninside the \".promisor\" files before the repack.\n\nUse the \"copy_all_promisor_files()\" function created previously to\npreserve the contents of all \".promisor\" files inside the first\n\".promisor\" file created by the repack.\n\nAlso, update the documentation accordingly.\n\nSigned-off-by: LorenzoPegorari \n---\n Documentation/git-repack.adoc | 4 ++--\n repack-promisor.c | 23 +++++++++++++++--------\n 2 files changed, 17 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/git-repack.adoc b/Documentation/git-repack.adoc\nindex 673ce91083..33d3c8afbd 100644\n--- a/Documentation/git-repack.adoc\n+++ b/Documentation/git-repack.adoc\n@@ -45,8 +45,8 @@ other objects in that pack they already have locally.\n +\n Promisor packfiles are repacked separately: if there are packfiles that\n have an associated \".promisor\" file, these packfiles will be repacked\n-into another separate pack, and an empty \".promisor\" file corresponding\n-to the new separate pack will be written.\n+into another separate pack, and a \".promisor\" file corresponding to the\n+new separate pack will be written (with arbitrary contents).\n \n -A::\n \tSame as `-a`, unless `-d` is used. Then any unreachable\ndiff --git a/repack-promisor.c b/repack-promisor.c\nindex 90318ce150..6670728669 100644\n--- a/repack-promisor.c\n+++ b/repack-promisor.c\n@@ -40,6 +40,7 @@ static void finish_repacking_promisor_objects(struct repository *repo,\n \t\t\t\t\t const char *packtmp)\n {\n \tstruct strbuf line = STRBUF_INIT;\n+\tint is_first_promisor = 1;\n \tFILE *out;\n \n \tclose(cmd->in);\n@@ -55,19 +56,25 @@ static void finish_repacking_promisor_objects(struct repository *repo,\n \n \t\t/*\n \t\t * pack-objects creates the .pack and .idx files, but not the\n-\t\t * .promisor file. Create the .promisor file, which is empty.\n-\t\t *\n-\t\t * NEEDSWORK: fetch-pack sometimes generates non-empty\n-\t\t * .promisor files containing the ref names and associated\n-\t\t * hashes at the point of generation of the corresponding\n-\t\t * packfile, but this would not preserve their contents. Maybe\n-\t\t * concatenate the contents of all .promisor files instead of\n-\t\t * just creating a new empty file.\n+\t\t * .promisor file. Create the .promisor file.\n \t\t */\n \t\tpromisor_name = mkpathdup(\"%s-%s.promisor\", packtmp,\n \t\t\t\t\t line.buf);\n \t\twrite_promisor_file(promisor_name, NULL, 0);\n \n+\t\t/*\n+\t\t * Fetch-pack sometimes generates non-empty .promisor files\n+\t\t * containing the ref names and associated hashes at the point of\n+\t\t * generation of the corresponding packfile. These pieces of info\n+\t\t * are only used for debugging reasons. In order to preserve\n+\t\t * these, let's copy the contents of all .promisor files in the\n+\t\t * first promisor file created.\n+\t\t */\n+\t\tif (is_first_promisor) {\n+\t\t\tcopy_all_promisor_files(repo, promisor_name);\n+\t\t\tis_first_promisor = 0;\n+\t\t}\n+\n \t\titem->util = generated_pack_populate(item->string, packtmp);\n \n \t\tfree(promisor_name);\n-- \n2.43.0\n\n"},{"id":"539628","messageId":"CAPig+cQSsMfvHJnwuXGQ1Je8ekz=Rqbaibn-3shbya5y-5xTKg@mail.gmail.com","threadId":"65326","inReplyTo":"3c0702f81b3e4a57cd38284ab90f8277da3fcd40.1774125871.git.lorenzo.pegorari2002@gmail.com","subject":"Re: [GSoC PATCH 2/3] pack-write: add helper to fill promisor file after repack","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2026-03-22T02:04:01Z","receivedAt":"2026-03-22T02:04:18Z","isPatch":true,"body":"On Sat, Mar 21, 2026 at 5:29 PM LorenzoPegorari\n wrote:\n> Create a `copy_all_promisor_files()` helper function used to copy the\n> contents of all \".promisor\" files in a `repository` inside another\n> \".promisor\" file.\n>\n> This function can be used to preserve the contents of all \".promisor\"\n> files inside a new \".promisor\" file, for example when a repack happens.\n>\n> This function is written in such a way so that it will read all the\n> \".promisor\" files inside the given `repository` line by line, and copy\n> only the lines that are not already present in the destination file. This\n> is done to avoid copying the same lines multiple times that may come from\n> multiple (redundant) packfiles. A better way to achieve this might be (is\n> definitely) possible.\n>\n> Signed-off-by: LorenzoPegorari \n> ---\n> diff --git a/pack-write.c b/pack-write.c\n> @@ -621,3 +621,65 @@ void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_\n> +void copy_all_promisor_files(struct repository *repo, const char *promisor_name)\n> +{\n> + struct strbuf promisor_source_name = STRBUF_INIT;\n> + struct strbuf read_source = STRBUF_INIT, read_dest = STRBUF_INIT;\n> + struct strbuf write_dest = STRBUF_INIT;\n\nThese strbufs don't seem to be released, thus are leaked.\n\n> + int err;\n> +\n> + FILE *dest = xfopen(promisor_name, \"r+\");\n> +\n> + struct packed_git *p;\n\nStyle nit: Place all the variable declarations together (without blank\nlines), followed by a blank line.\n\n> + repo_for_each_pack(repo, p) {\n> + if (!p->pack_promisor)\n> + continue;\n> +\n> + strbuf_reset(&promisor_source_name);\n> + strbuf_addstr(&promisor_source_name, p->pack_name);\n> + strbuf_strip_suffix(&promisor_source_name, \".pack\");\n> + strbuf_addstr(&promisor_source_name, \".promisor\");\n> + FILE *source = xfopen(promisor_source_name.buf, \"r\");\n\nThis project still frowns upon variable declaration after code. You\nwill want to declare `FILE *source;` at the top of this loop body and\nthen assign `source = xfopen(...)` here.\n\n> + /*\n> + * For each line of the promisor source file, check if it already\n> + * is in the promisor dest file. If not, add it to write_dest, so\n> + * that it will be written in the dest file.\n> + */\n> + while (strbuf_getline(&read_source, source) != EOF) {\n> + if (fseek(dest, 0L, SEEK_SET))\n> + die_errno(_(\"fseek failed\"));\n> + int is_source_in_dest = 0;\n\nDitto regarding variable declaration following code.\n\n> + while (strbuf_getline(&read_dest, dest) != EOF) {\n> + if (!strbuf_cmp(&read_source, &read_dest)) {\n> + is_source_in_dest = 1;\n> + break;\n> + }\n> + }\n> + if (!is_source_in_dest) {\n> + strbuf_addbuf(&write_dest, &read_source);\n> + strbuf_addstr(&write_dest, \"\\n\");\n> + }\n\nThe commit message talks about this, and it is indeed very ugly that\nthis re-reads `dest` from the beginning for *every* `source` line. Is\nthere a reason you can't simply read `dest` into a `strset` (see Git's\n`strmap.h`) in its entirety before entering the repo_for_each_pack()\nloop and then merely check the strset for existence using\nstrset_add()?\n\n> + }\n> +\n> + if (write_dest.len) {\n> + strbuf_strip_suffix(&write_dest, \"\\n\");\n> + if (fseek(dest, 0L, SEEK_END))\n> + die_errno(_(\"fseek failed\"));\n> + fprintf(dest, \"%s\\n\", write_dest.buf);\n> + fflush(dest);\n> + strbuf_reset(&write_dest);\n> + }\n> +\n> + err = ferror(source);\n> + err |= fclose(source);\n> + if (err)\n> + die(_(\"could not read '%s' promisor file\"), promisor_source_name.buf);\n> + }\n> +\n> + err = ferror(dest);\n> + err |= fclose(dest);\n> + if (err)\n> + die(_(\"could not write '%s' promisor file\"), promisor_name);\n> +}\n"},{"id":"539662","messageId":"acA5-pIc9zVbqY1e@lorenzo-VM","threadId":"65326","inReplyTo":"CAPig+cQSsMfvHJnwuXGQ1Je8ekz=Rqbaibn-3shbya5y-5xTKg@mail.gmail.com","subject":"Re: [GSoC PATCH 2/3] pack-write: add helper to fill promisor file after repack","fromName":"Lorenzo Pegorari","fromEmail":"lorenzo.pegorari2002@gmail.com","sentAt":"2026-03-22T18:50:34Z","receivedAt":"2026-03-22T18:50:38Z","isPatch":true,"body":"On Sat, Mar 21, 2026 at 10:04:01PM -0400, Eric Sunshine wrote:\n> On Sat, Mar 21, 2026 at 5:29 PM LorenzoPegorari\n> wrote:\n> > Create a `copy_all_promisor_files()` helper function used to copy the\n> > contents of all \".promisor\" files in a `repository` inside another\n> > \".promisor\" file.\n> >\n> > This function can be used to preserve the contents of all \".promisor\"\n> > files inside a new \".promisor\" file, for example when a repack happens.\n> >\n> > This function is written in such a way so that it will read all the\n> > \".promisor\" files inside the given `repository` line by line, and copy\n> > only the lines that are not already present in the destination file. This\n> > is done to avoid copying the same lines multiple times that may come from\n> > multiple (redundant) packfiles. A better way to achieve this might be (is\n> > definitely) possible.\n> >\n> > Signed-off-by: LorenzoPegorari \n> > ---\n> > diff --git a/pack-write.c b/pack-write.c\n> > @@ -621,3 +621,65 @@ void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_\n> > +void copy_all_promisor_files(struct repository *repo, const char *promisor_name)\n> > +{\n> > + struct strbuf promisor_source_name = STRBUF_INIT;\n> > + struct strbuf read_source = STRBUF_INIT, read_dest = STRBUF_INIT;\n> > + struct strbuf write_dest = STRBUF_INIT;\n> \n> These strbufs don't seem to be released, thus are leaked.\n\nOf course... trivial mistake. Will fix it in v2.\n\n> > + int err;\n> > +\n> > + FILE *dest = xfopen(promisor_name, \"r+\");\n> > +\n> > + struct packed_git *p;\n> \n> Style nit: Place all the variable declarations together (without blank\n> lines), followed by a blank line.\n\nAck.\n\n> > + repo_for_each_pack(repo, p) {\n> > + if (!p->pack_promisor)\n> > + continue;\n> > +\n> > + strbuf_reset(&promisor_source_name);\n> > + strbuf_addstr(&promisor_source_name, p->pack_name);\n> > + strbuf_strip_suffix(&promisor_source_name, \".pack\");\n> > + strbuf_addstr(&promisor_source_name, \".promisor\");\n> > + FILE *source = xfopen(promisor_source_name.buf, \"r\");\n> \n> This project still frowns upon variable declaration after code. You\n> will want to declare `FILE *source;` at the top of this loop body and\n> then assign `source = xfopen(...)` here.\n\nAck.\n\n> > + /*\n> > + * For each line of the promisor source file, check if it already\n> > + * is in the promisor dest file. If not, add it to write_dest, so\n> > + * that it will be written in the dest file.\n> > + */\n> > + while (strbuf_getline(&read_source, source) != EOF) {\n> > + if (fseek(dest, 0L, SEEK_SET))\n> > + die_errno(_(\"fseek failed\"));\n> > + int is_source_in_dest = 0;\n> \n> Ditto regarding variable declaration following code.\n\nAck.\n\n> > + while (strbuf_getline(&read_dest, dest) != EOF) {\n> > + if (!strbuf_cmp(&read_source, &read_dest)) {\n> > + is_source_in_dest = 1;\n> > + break;\n> > + }\n> > + }\n> > + if (!is_source_in_dest) {\n> > + strbuf_addbuf(&write_dest, &read_source);\n> > + strbuf_addstr(&write_dest, \"\\n\");\n> > + }\n> \n> The commit message talks about this, and it is indeed very ugly that\n> this re-reads `dest` from the beginning for *every* `source` line. Is\n> there a reason you can't simply read `dest` into a `strset` (see Git's\n> `strmap.h`) in its entirety before entering the repo_for_each_pack()\n> loop and then merely check the strset for existence using\n> strset_add()?\n\nNo reason at all, except for me to knowing about `strset`! Thanks for\nsuggesting it to me. Will use it in v2.\n\n> > + }\n> > +\n> > + if (write_dest.len) {\n> > + strbuf_strip_suffix(&write_dest, \"\\n\");\n> > + if (fseek(dest, 0L, SEEK_END))\n> > + die_errno(_(\"fseek failed\"));\n> > + fprintf(dest, \"%s\\n\", write_dest.buf);\n> > + fflush(dest);\n> > + strbuf_reset(&write_dest);\n> > + }\n> > +\n> > + err = ferror(source);\n> > + err |= fclose(source);\n> > + if (err)\n> > + die(_(\"could not read '%s' promisor file\"), promisor_source_name.buf);\n> > + }\n> > +\n> > + err = ferror(dest);\n> > + err |= fclose(dest);\n> > + if (err)\n> > + die(_(\"could not write '%s' promisor file\"), promisor_name);\n> > +}\n"},{"id":"539663","messageId":"cover.1774205661.git.lorenzo.pegorari2002@gmail.com","threadId":"65326","inReplyTo":"cover.1774125871.git.lorenzo.pegorari2002@gmail.com","subject":"[GSoC PATCH v2 0/4] preserve promisor files content after repack","fromName":"LorenzoPegorari","fromEmail":"lorenzo.pegorari2002@gmail.com","sentAt":"2026-03-22T19:16:03Z","receivedAt":"2026-03-22T19:16:08Z","isPatch":true,"body":"The goal of this patch is to solve the NEEDSWORK comment added by\n5374a290 (fetch-pack: write fetched refs to .promisor, 14/10/2019). This\nis done by adding a helper function that takes the content of all\n.promisor files in the `repository`, and copies it inside the first\n.promisor file created by the repack.\n\nAlso, I added a comment explaining what is the purpose of the content of\nthe .promisor files, since this wasn't explained anywhere (I found\ninformation regarding this only in the message of the previously cited\ncommit).\n\nFinally, I added a test to \"t7700-repack.sh\" that checks if the content\nof .promisor files are correctly copied into the first .promisor file\ncreated by a repack.\n\n\nV2 DIFF:\n * changed how the `copy_all_promisor_files()` function works, so that it\n reads `dest` into a `strset` in its entirety before entering the\n `repo_for_each_pack()` loop, and then checks the `strset` for\n existence using `strset_add()` (as suggested by Eric Sunshine)\n * correctly release `strbuf`s\n * added test\n\n\nLorenzoPegorari (4):\n pack-write: add explanation to promisor file content\n pack-write: add helper to fill promisor file after repack\n repack-promisor: preserve content of promisor files after repack\n t7700: test for promisor file content after repack\n\n Documentation/git-repack.adoc | 4 +-\n pack-write.c | 70 +++++++++++++++++++++++++++++++++++\n pack.h | 1 +\n repack-promisor.c | 23 ++++++++----\n t/t7700-repack.sh | 12 ++++++\n 5 files changed, 100 insertions(+), 10 deletions(-)\n\nRange-diff against v1:\n1: 9bba49563e = 1: fec0c24897 pack-write: add explanation to promisor file content\n2: 3c0702f81b < -: ---------- pack-write: add helper to fill promisor file after repack\n-: ---------- > 2: 0bb031e744 pack-write: add helper to fill promisor file after repack\n3: 6967066fe3 = 3: 3dab969a39 repack-promisor: preserve content of promisor files after repack\n-: ---------- > 4: cb642d8225 t7700: test for promisor file content after repack\n-- \n2.43.0\n"},{"id":"539664","messageId":"fec0c24897092d19a718563ca4ef6e509ab104e6.1774205661.git.lorenzo.pegorari2002@gmail.com","threadId":"65326","inReplyTo":"cover.1774205661.git.lorenzo.pegorari2002@gmail.com","subject":"[GSoC PATCH v2 1/4] pack-write: add explanation to promisor file content","fromName":"LorenzoPegorari","fromEmail":"lorenzo.pegorari2002@gmail.com","sentAt":"2026-03-22T19:16:23Z","receivedAt":"2026-03-22T19:16:27Z","isPatch":true,"body":"In the entire codebase there is no explanation as to why the \".promisor\"\nfiles may contain the ref names (and their associated hashes) that were\nfetched at the time the corresponding packfile was downloaded.\n\nAdd comment explaining that these pieces of information are used only for\ndebugging reasons, and how they can be used while debugging.\n\nSigned-off-by: LorenzoPegorari \n---\n pack-write.c | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/pack-write.c b/pack-write.c\nindex 83eaf88541..6a2023327e 100644\n--- a/pack-write.c\n+++ b/pack-write.c\n@@ -603,6 +603,15 @@ void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_\n \tint i, err;\n \tFILE *output = xfopen(promisor_name, \"w\");\n \n+\t/*\n+\t* Write in the .promisor file the ref names and associated hashes,\n+\t* obtained by fetch-pack, at the point of generation of the\n+\t* corresponding packfile. These pieces of info are only used to make\n+\t* it easier to debug issues with partial clones, as we can identify\n+\t* what refs (and their associated hashes) were fetched at the time\n+\t* the packfile was downloaded, and if necessary, compare those hashes\n+\t* against what the promisor remote reports now.\n+\t*/\n \tfor (i = 0; i < nr_sought; i++)\n \t\tfprintf(output, \"%s %s\\n\", oid_to_hex(&sought[i]->old_oid),\n \t\t\tsought[i]->name);\n-- \n2.43.0\n\n"},{"id":"539665","messageId":"0bb031e7443bb53abbbb0afaa347285d6d8cf7b8.1774205661.git.lorenzo.pegorari2002@gmail.com","threadId":"65326","inReplyTo":"cover.1774205661.git.lorenzo.pegorari2002@gmail.com","subject":"[GSoC PATCH v2 2/4] pack-write: add helper to fill promisor file after repack","fromName":"LorenzoPegorari","fromEmail":"lorenzo.pegorari2002@gmail.com","sentAt":"2026-03-22T19:18:05Z","receivedAt":"2026-03-22T19:18:10Z","isPatch":true,"body":"Create a `copy_all_promisor_files()` helper function used to copy the\ncontents of all \".promisor\" files in a `repository` inside another\n\".promisor\" file.\n\nThis function can be used to preserve the contents of all \".promisor\"\nfiles inside a new \".promisor\" file, for example when a repack happens.\n\nThis function is written in such a way so that it will read all the\n\".promisor\" files inside the given `repository` line by line, and copy\nonly the lines that are not already present in the destination file. This\nis done to avoid copying the same lines multiple times that may come from\nmultiple (redundant) packfiles. There might be another better/cleaner way\nto achieve this.\n\nSigned-off-by: LorenzoPegorari \n---\n pack-write.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++\n pack.h | 1 +\n 2 files changed, 62 insertions(+)\n\ndiff --git a/pack-write.c b/pack-write.c\nindex 6a2023327e..583e40b423 100644\n--- a/pack-write.c\n+++ b/pack-write.c\n@@ -13,6 +13,7 @@\n #include \"path.h\"\n #include \"repository.h\"\n #include \"strbuf.h\"\n+#include \"strmap.h\"\n \n void reset_pack_idx_option(struct pack_idx_option *opts)\n {\n@@ -621,3 +622,63 @@ void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_\n \tif (err)\n \t\tdie(_(\"could not write '%s' promisor file\"), promisor_name);\n }\n+\n+void copy_all_promisor_files(struct repository *repo, const char *promisor_name)\n+{\n+\tstruct strset dest_content = STRSET_INIT;\n+\tstruct strbuf read_line = STRBUF_INIT;\n+\tstruct strbuf promisor_source_name = STRBUF_INIT;\n+\tstruct strbuf write_dest = STRBUF_INIT;\n+\tFILE *dest, *source;\n+\tstruct packed_git *p;\n+\tint err;\n+\n+\tdest = xfopen(promisor_name, \"r+\");\n+\twhile (strbuf_getline(&read_line, dest) != EOF)\n+\t\tstrset_add(&dest_content, read_line.buf);\n+\n+\trepo_for_each_pack(repo, p) {\n+\t\tif (!p->pack_promisor)\n+\t\t\tcontinue;\n+\n+\t\tstrbuf_reset(&promisor_source_name);\n+\t\tstrbuf_addstr(&promisor_source_name, p->pack_name);\n+\t\tstrbuf_strip_suffix(&promisor_source_name, \".pack\");\n+\t\tstrbuf_addstr(&promisor_source_name, \".promisor\");\n+\t\tsource = xfopen(promisor_source_name.buf, \"r\");\n+\n+\t\t/*\n+\t\t * For each line of the promisor source file, check if it already\n+\t\t * is in the promisor dest file. If not, add it to write_dest, so\n+\t\t * that it will be written in the dest file.\n+\t\t */\n+\t\twhile (strbuf_getline(&read_line, source) != EOF) {\n+\t\t\tif (strset_add(&dest_content, read_line.buf)) {\n+\t\t\t\tstrbuf_addbuf(&write_dest, &read_line);\n+\t\t\t\tstrbuf_addstr(&write_dest, \"\\n\");\n+\t\t\t}\n+\t\t}\n+\n+\t\terr = ferror(source);\n+\t\terr |= fclose(source);\n+\t\tif (err)\n+\t\t\tdie(_(\"could not read '%s' promisor file\"), promisor_source_name.buf);\n+\t}\n+\n+\tif (write_dest.len) {\n+\t\tstrbuf_strip_suffix(&write_dest, \"\\n\");\n+\t\tif (fseek(dest, 0L, SEEK_END))\n+\t\t\tdie_errno(_(\"fseek failed\"));\n+\t\tfprintf(dest, \"%s\\n\", write_dest.buf);\n+\t}\n+\n+\terr = ferror(dest);\n+\terr |= fclose(dest);\n+\tif (err)\n+\t\tdie(_(\"could not write '%s' promisor file\"), promisor_name);\n+\n+\tstrbuf_release(&read_line);\n+\tstrbuf_release(&promisor_source_name);\n+\tstrbuf_release(&write_dest);\n+\tstrset_clear(&dest_content);\n+}\ndiff --git a/pack.h b/pack.h\nindex ec76472e49..509e90edba 100644\n--- a/pack.h\n+++ b/pack.h\n@@ -105,6 +105,7 @@ char *index_pack_lockfile(struct repository *r, int fd, int *is_well_formed);\n struct ref;\n \n void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_sought);\n+void copy_all_promisor_files(struct repository *repo, const char *promisor_name);\n \n char *write_rev_file(struct repository *repo,\n \t\t const char *rev_name,\n-- \n2.43.0\n\n"},{"id":"539666","messageId":"3dab969a3942532f49f6f9cdcddb5fb2be11e232.1774205661.git.lorenzo.pegorari2002@gmail.com","threadId":"65326","inReplyTo":"cover.1774205661.git.lorenzo.pegorari2002@gmail.com","subject":"[GSoC PATCH v2 3/4] repack-promisor: preserve content of promisor files after repack","fromName":"LorenzoPegorari","fromEmail":"lorenzo.pegorari2002@gmail.com","sentAt":"2026-03-22T19:18:34Z","receivedAt":"2026-03-22T19:18:39Z","isPatch":true,"body":"When a repack involving promisor packfiles happens, the new \".promisor\"\nfile is created empty, losing all the debug info that might be present\ninside the \".promisor\" files before the repack.\n\nUse the \"copy_all_promisor_files()\" function created previously to\npreserve the contents of all \".promisor\" files inside the first\n\".promisor\" file created by the repack.\n\nAlso, update the documentation accordingly.\n\nSigned-off-by: LorenzoPegorari \n---\n Documentation/git-repack.adoc | 4 ++--\n repack-promisor.c | 23 +++++++++++++++--------\n 2 files changed, 17 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/git-repack.adoc b/Documentation/git-repack.adoc\nindex 673ce91083..33d3c8afbd 100644\n--- a/Documentation/git-repack.adoc\n+++ b/Documentation/git-repack.adoc\n@@ -45,8 +45,8 @@ other objects in that pack they already have locally.\n +\n Promisor packfiles are repacked separately: if there are packfiles that\n have an associated \".promisor\" file, these packfiles will be repacked\n-into another separate pack, and an empty \".promisor\" file corresponding\n-to the new separate pack will be written.\n+into another separate pack, and a \".promisor\" file corresponding to the\n+new separate pack will be written (with arbitrary contents).\n \n -A::\n \tSame as `-a`, unless `-d` is used. Then any unreachable\ndiff --git a/repack-promisor.c b/repack-promisor.c\nindex 90318ce150..6670728669 100644\n--- a/repack-promisor.c\n+++ b/repack-promisor.c\n@@ -40,6 +40,7 @@ static void finish_repacking_promisor_objects(struct repository *repo,\n \t\t\t\t\t const char *packtmp)\n {\n \tstruct strbuf line = STRBUF_INIT;\n+\tint is_first_promisor = 1;\n \tFILE *out;\n \n \tclose(cmd->in);\n@@ -55,19 +56,25 @@ static void finish_repacking_promisor_objects(struct repository *repo,\n \n \t\t/*\n \t\t * pack-objects creates the .pack and .idx files, but not the\n-\t\t * .promisor file. Create the .promisor file, which is empty.\n-\t\t *\n-\t\t * NEEDSWORK: fetch-pack sometimes generates non-empty\n-\t\t * .promisor files containing the ref names and associated\n-\t\t * hashes at the point of generation of the corresponding\n-\t\t * packfile, but this would not preserve their contents. Maybe\n-\t\t * concatenate the contents of all .promisor files instead of\n-\t\t * just creating a new empty file.\n+\t\t * .promisor file. Create the .promisor file.\n \t\t */\n \t\tpromisor_name = mkpathdup(\"%s-%s.promisor\", packtmp,\n \t\t\t\t\t line.buf);\n \t\twrite_promisor_file(promisor_name, NULL, 0);\n \n+\t\t/*\n+\t\t * Fetch-pack sometimes generates non-empty .promisor files\n+\t\t * containing the ref names and associated hashes at the point of\n+\t\t * generation of the corresponding packfile. These pieces of info\n+\t\t * are only used for debugging reasons. In order to preserve\n+\t\t * these, let's copy the contents of all .promisor files in the\n+\t\t * first promisor file created.\n+\t\t */\n+\t\tif (is_first_promisor) {\n+\t\t\tcopy_all_promisor_files(repo, promisor_name);\n+\t\t\tis_first_promisor = 0;\n+\t\t}\n+\n \t\titem->util = generated_pack_populate(item->string, packtmp);\n \n \t\tfree(promisor_name);\n-- \n2.43.0\n\n"},{"id":"539667","messageId":"cb642d82258c35ea85e4c18459d09473f23de373.1774205661.git.lorenzo.pegorari2002@gmail.com","threadId":"65326","inReplyTo":"cover.1774205661.git.lorenzo.pegorari2002@gmail.com","subject":"[GSoC PATCH v2 4/4] t7700: test for promisor file content after repack","fromName":"LorenzoPegorari","fromEmail":"lorenzo.pegorari2002@gmail.com","sentAt":"2026-03-22T19:18:54Z","receivedAt":"2026-03-22T19:18:58Z","isPatch":true,"body":"Add test that checks if the content of all \".promisor\" files are copied\ninside the first \".promisor\" file created by a repack.\n\nSigned-off-by: LorenzoPegorari \n---\n t/t7700-repack.sh | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex 63ef63fc50..10187d5954 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -904,4 +904,16 @@ test_expect_success 'pending objects are repacked appropriately' '\n \t)\n '\n \n+test_expect_success 'check .promisor file content after repack' '\n+\tgit init prom_test1 &&\n+\ttest_commit -C prom_test1 temp &&\n+\tgit clone prom_test1 prom_test2 --filter=blob:none --no-local &&\n+\n+\tcp $(ls prom_test2/.git/objects/pack/pack-*.promisor) prom_content_before &&\n+\tgit -C prom_test2 repack -a -d &&\n+\tcp $(ls prom_test2/.git/objects/pack/pack-*.promisor) prom_content_after &&\n+\n+\ttest_cmp prom_content_before prom_content_after\n+'\n+\n test_done\n-- \n2.43.0\n\n"},{"id":"539779","messageId":"CAPig+cR7-3rdkHvGbzk8O7P=83pxBTvqXTUgMxYpf+OK9jNCgg@mail.gmail.com","threadId":"65326","inReplyTo":"0bb031e7443bb53abbbb0afaa347285d6d8cf7b8.1774205661.git.lorenzo.pegorari2002@gmail.com","subject":"Re: [GSoC PATCH v2 2/4] pack-write: add helper to fill promisor file after repack","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2026-03-23T20:27:44Z","receivedAt":"2026-03-23T20:27:56Z","isPatch":true,"body":"On Sun, Mar 22, 2026 at 3:18 PM LorenzoPegorari\n wrote:\n> Create a `copy_all_promisor_files()` helper function used to copy the\n> contents of all \".promisor\" files in a `repository` inside another\n> \".promisor\" file.\n>\n> This function can be used to preserve the contents of all \".promisor\"\n> files inside a new \".promisor\" file, for example when a repack happens.\n>\n> This function is written in such a way so that it will read all the\n> \".promisor\" files inside the given `repository` line by line, and copy\n> only the lines that are not already present in the destination file. This\n> is done to avoid copying the same lines multiple times that may come from\n> multiple (redundant) packfiles. There might be another better/cleaner way\n> to achieve this.\n>\n> Signed-off-by: LorenzoPegorari \n> ---\n\nThanks, I think this version addresses all my review comments[*] and\nlooks much better overall. Use of `strset` makes a big difference over\nthe previous attempt. A couple minor comments below...\n\n[*]: https://lore.kernel.org/git/CAPig+cQSsMfvHJnwuXGQ1Je8ekz=Rqbaibn-3shbya5y-5xTKg@mail.gmail.com/\n\n> diff --git a/pack-write.c b/pack-write.c\n> @@ -621,3 +622,63 @@ void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_\n> +void copy_all_promisor_files(struct repository *repo, const char *promisor_name)\n> +{\n> + struct strset dest_content = STRSET_INIT;\n> + struct strbuf read_line = STRBUF_INIT;\n> + struct strbuf promisor_source_name = STRBUF_INIT;\n> + struct strbuf write_dest = STRBUF_INIT;\n> + FILE *dest, *source;\n> + struct packed_git *p;\n> + int err;\n\nNit: I probably would have declared `FILE *dest` within the scope of\nthe repo_for_each_pack() loop as suggested in the review, but it's not\nworth a reroll.\n\n> + dest = xfopen(promisor_name, \"r+\");\n> + while (strbuf_getline(&read_line, dest) != EOF)\n> + strset_add(&dest_content, read_line.buf);\n> +\n> + repo_for_each_pack(repo, p) {\n> + if (!p->pack_promisor)\n> + continue;\n> +\n> + strbuf_reset(&promisor_source_name);\n> + strbuf_addstr(&promisor_source_name, p->pack_name);\n> + strbuf_strip_suffix(&promisor_source_name, \".pack\");\n> + strbuf_addstr(&promisor_source_name, \".promisor\");\n> + source = xfopen(promisor_source_name.buf, \"r\");\n> +\n> + /*\n> + * For each line of the promisor source file, check if it already\n> + * is in the promisor dest file. If not, add it to write_dest, so\n> + * that it will be written in the dest file.\n> + */\n> + while (strbuf_getline(&read_line, source) != EOF) {\n> + if (strset_add(&dest_content, read_line.buf)) {\n> + strbuf_addbuf(&write_dest, &read_line);\n> + strbuf_addstr(&write_dest, \"\\n\");\n\nNot worth a reroll, but this could also be:\n\n strbuf_addch(&write_dest, '\\n');\n\n> + }\n> + }\n> +\n> + err = ferror(source);\n> + err |= fclose(source);\n> + if (err)\n> + die(_(\"could not read '%s' promisor file\"), promisor_source_name.buf);\n> + }\n> +\n> + if (write_dest.len) {\n> + strbuf_strip_suffix(&write_dest, \"\\n\");\n> + if (fseek(dest, 0L, SEEK_END))\n> + die_errno(_(\"fseek failed\"));\n> + fprintf(dest, \"%s\\n\", write_dest.buf);\n> + }\n\nCan you explain why you strip \"\\n\" and then re-add it via fprintf()?\nThe reason is not immediately obvious.\n\n> + err = ferror(dest);\n> + err |= fclose(dest);\n> + if (err)\n> + die(_(\"could not write '%s' promisor file\"), promisor_name);\n> +\n> + strbuf_release(&read_line);\n> + strbuf_release(&promisor_source_name);\n> + strbuf_release(&write_dest);\n> + strset_clear(&dest_content);\n> +}\n\nEverything appears to be released. Good.\n"},{"id":"539781","messageId":"xmqqmrzy45m4.fsf@gitster.g","threadId":"65326","inReplyTo":"fec0c24897092d19a718563ca4ef6e509ab104e6.1774205661.git.lorenzo.pegorari2002@gmail.com","subject":"Re: [GSoC PATCH v2 1/4] pack-write: add explanation to promisor file content","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-23T21:07:31Z","receivedAt":"2026-03-23T21:07:34Z","isPatch":true,"body":"LorenzoPegorari writes:\n\n> In the entire codebase there is no explanation as to why the \".promisor\"\n> files may contain the ref names (and their associated hashes) that were\n> fetched at the time the corresponding packfile was downloaded.\n>\n> Add comment explaining that these pieces of information are used only for\n> debugging reasons, and how they can be used while debugging.\n>\n> Signed-off-by: LorenzoPegorari \n\nA natural question any reader of the above (and below) would be\nasking is: Who told you that these are only to aid debugging?\n\nPlease refer to the commit that brought in the reasoning behind the\ncomment to make it more convincing. \n\nSomething like this replacing the second paragraph,\n\n As explained in the log message of the commit 5374a290\n (fetch-pack: write fetched refs to .promisor, 2019-10-14), where\n this loop originally came from, these ref values are not\n actually used for anything in the production, but are solely\n there to help debugging. Explain it in a new comment.\n\nperhaps?\n\n> +\t/*\n> +\t* Write in the .promisor file the ref names and associated hashes,\n> +\t* obtained by fetch-pack, at the point of generation of the\n> +\t* corresponding packfile. These pieces of info are only used to make\n> +\t* it easier to debug issues with partial clones, as we can identify\n> +\t* what refs (and their associated hashes) were fetched at the time\n> +\t* the packfile was downloaded, and if necessary, compare those hashes\n> +\t* against what the promisor remote reports now.\n> +\t*/\n\nI do not want to sound too pedantic, but we align '*' asterisks in\nour multi-line comments, assuming tabwidth=8 and monospace:\n\n\t/*\n\t * Write in the .promisor ...\n\t...\n\t * against what the promisor remote reports now.\n\t */\n\nYour second and subsequent lines lack a single whitespace after the\nleading tab used for indent.\n\n> \tfor (i = 0; i < nr_sought; i++)\n> \t\tfprintf(output, \"%s %s\\n\", oid_to_hex(&sought[i]->old_oid),\n> \t\t\tsought[i]->name);\n"},{"id":"539782","messageId":"xmqqfr5q44jx.fsf@gitster.g","threadId":"65326","inReplyTo":"0bb031e7443bb53abbbb0afaa347285d6d8cf7b8.1774205661.git.lorenzo.pegorari2002@gmail.com","subject":"Re: [GSoC PATCH v2 2/4] pack-write: add helper to fill promisor file after repack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-23T21:30:26Z","receivedAt":"2026-03-23T21:30:29Z","isPatch":true,"body":"LorenzoPegorari writes:\n\n> Create a `copy_all_promisor_files()` helper function used to copy the\n> contents of all \".promisor\" files in a `repository` inside another\n> \".promisor\" file.\n>\n> This function can be used to preserve the contents of all \".promisor\"\n> files inside a new \".promisor\" file, for example when a repack happens.\n>\n> This function is written in such a way so that it will read all the\n> \".promisor\" files inside the given `repository` line by line, and copy\n> only the lines that are not already present in the destination file. This\n> is done to avoid copying the same lines multiple times that may come from\n> multiple (redundant) packfiles. There might be another better/cleaner way\n> to achieve this.\n\nIn the previous step, we extablished that these \"back then their ref\nX used to point at object Y\" records are there so that we can\nidentify which refs were fetched at the time the packfile was\ndownloaded to help debugging. When repacking, losing these records\ncertainly would lose information.\n\nBut would concatenating all into a single file help preserve the\nuseful information? Don't we need do better than that?\n\nA NEEDSWORK comment, as was discussed in another thread or two in\nthe recent past, is not necessarily a well thought out fully\nfinished specification of an additional piece of work. \"We know\nthis has a problem, we may need to do something about it, like\nconcatenating to save the contents, perhaps? We do not know the\nanswer, and we do not bother thinking it through right at this\nmoment. It is left to the future developers to figure it out\" is\nwhat a NEEDSWORK comment is about.\n\nYour first response to such a comment may be \"yeah, I agree that it\nis bad to lose information we added to help debugging\", but the\nsecond one should be to wonder if the \"like concatenating...\" is the\nbest approach going forward.\n\nIn other words, we should take a NEEDSWORK comment as a mere\nstarting point, and what NEEDS your work begins at thinking what\nneeds to be done about the problem raised there.\n\nBy mixing them up all into a single list, you no longer can tell\nwhen their ref X was observed to be pointing at object Y anymore.\nYou may have two packs originally, with a record for \"ref X pointing\nat object Y\" in each of them, but by deduping them, you lose the\ninformation that you cloned at one time, and made an additional\nfetch on another day, and the fact the ref X was pointing at the\nsame value at both times. I am not sure if it is a good\nimplementation if the objective of this topic is to preserve\ninformation that is useful for debugging.\n\nI wonder if it helps to append to each line the file timestamp of\nthe .promisor file we took the record originally? For the sake of\ncompleteness, we could consider adding the filename as well, but we\ncan quickly dismiss it as not so useful ;-)\n\nIf repacking already repacked promisor packfile, the records would\nalready contain such a timestamp at the end, so the code to copy\nexisting records must be prepared to see if the records are the\n tuple, or tuple, and act\naccordingly.\n\nI am *not* saying that without such a \"preserve timestamp\" column in\nthe record, copying existing records to a new .promisor file is\nuseless. But we do not see any explanation why the author thinks\nthat it is sufficient to copy existing records while silently\ndeduping. We can implement only one choice backed by series of\ndecisions like \"timestamp might help\" and \"original filenames would\nprobably not help\", and the design should describe what was\nconsidered and rejected (as opposed to \"we didn't think things\nthrough---we just did what the original NEEDSWORK comment suggested\ndoing\").\n\nThanks.\n"},{"id":"539783","messageId":"xmqqa4vy43q2.fsf@gitster.g","threadId":"65326","inReplyTo":"3dab969a3942532f49f6f9cdcddb5fb2be11e232.1774205661.git.lorenzo.pegorari2002@gmail.com","subject":"Re: [GSoC PATCH v2 3/4] repack-promisor: preserve content of promisor files after repack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-23T21:48:21Z","receivedAt":"2026-03-23T21:48:24Z","isPatch":true,"body":"LorenzoPegorari writes:\n\n> @@ -40,6 +40,7 @@ static void finish_repacking_promisor_objects(struct repository *repo,\n> \t\t\t\t\t const char *packtmp)\n> {\n> \tstruct strbuf line = STRBUF_INIT;\n> +\tint is_first_promisor = 1;\n> \tFILE *out;\n> ... \n> +\t\t/*\n> +\t\t * Fetch-pack sometimes generates non-empty .promisor files\n> +\t\t * containing the ref names and associated hashes at the point of\n> +\t\t * generation of the corresponding packfile. These pieces of info\n> +\t\t * are only used for debugging reasons. In order to preserve\n> +\t\t * these, let's copy the contents of all .promisor files in the\n> +\t\t * first promisor file created.\n> +\t\t */\n> +\t\tif (is_first_promisor) {\n> +\t\t\tcopy_all_promisor_files(repo, promisor_name);\n> +\t\t\tis_first_promisor = 0;\n> +\t\t}\n> +\n\nHere the underlying assumption seems to be that whichever one of the\ntwo potential callers of this function, repack_promisor_objects()\nand pack_geometry_repack_promisors(), would handle all the existing\npacks with corresponding .promisor file so it is safe to coalesce\nall the debugging comments from all the existing .promisor files\ninto one?\n\nIs it really true, though? Especially with geometry repacking\nenabled, wouldn't a regular repack coalesce only the smallish ones\ninto a single pack while leaving an already largeish ones intact, or\nsomething?\n\nThanks.\n"},{"id":"540017","messageId":"acRUjAG7QOD5kVMI@lorenzo-VM","threadId":"65326","inReplyTo":"xmqqmrzy45m4.fsf@gitster.g","subject":"Re: [GSoC PATCH v2 1/4] pack-write: add explanation to promisor file content","fromName":"Lorenzo Pegorari","fromEmail":"lorenzo.pegorari2002@gmail.com","sentAt":"2026-03-25T21:33:00Z","receivedAt":"2026-03-25T21:33:06Z","isPatch":true,"body":"On Mon, Mar 23, 2026 at 02:07:31PM -0700, Junio C Hamano wrote:\n> LorenzoPegorari writes:\n> \n> > In the entire codebase there is no explanation as to why the \".promisor\"\n> > files may contain the ref names (and their associated hashes) that were\n> > fetched at the time the corresponding packfile was downloaded.\n> >\n> > Add comment explaining that these pieces of information are used only for\n> > debugging reasons, and how they can be used while debugging.\n> >\n> > Signed-off-by: LorenzoPegorari \n> \n> A natural question any reader of the above (and below) would be\n> asking is: Who told you that these are only to aid debugging?\n> \n> Please refer to the commit that brought in the reasoning behind the\n> comment to make it more convincing. \n> \n> Something like this replacing the second paragraph,\n> \n> As explained in the log message of the commit 5374a290\n> (fetch-pack: write fetched refs to .promisor, 2019-10-14), where\n> this loop originally came from, these ref values are not\n> actually used for anything in the production, but are solely\n> there to help debugging. Explain it in a new comment.\n> \n> perhaps?\n\nMakes perfect sense. I should have done this from the start. Thanks for\npointing that out.\n\n> > +\t/*\n> > +\t* Write in the .promisor file the ref names and associated hashes,\n> > +\t* obtained by fetch-pack, at the point of generation of the\n> > +\t* corresponding packfile. These pieces of info are only used to make\n> > +\t* it easier to debug issues with partial clones, as we can identify\n> > +\t* what refs (and their associated hashes) were fetched at the time\n> > +\t* the packfile was downloaded, and if necessary, compare those hashes\n> > +\t* against what the promisor remote reports now.\n> > +\t*/\n> \n> I do not want to sound too pedantic, but we align '*' asterisks in\n> our multi-line comments, assuming tabwidth=8 and monospace:\n> \n> \t/*\n> \t * Write in the .promisor ...\n> \t...\n> \t * against what the promisor remote reports now.\n> \t */\n> \n> Your second and subsequent lines lack a single whitespace after the\n> leading tab used for indent.\n\nYou are not too pedantic! Ack.\n\n> > \tfor (i = 0; i < nr_sought; i++)\n> > \t\tfprintf(output, \"%s %s\\n\", oid_to_hex(&sought[i]->old_oid),\n> > \t\t\tsought[i]->name);\n"},{"id":"540033","messageId":"acSTY_i4zAueN9jD@lorenzo-VM","threadId":"65326","inReplyTo":"xmqqfr5q44jx.fsf@gitster.g","subject":"Re: [GSoC PATCH v2 2/4] pack-write: add helper to fill promisor file after repack","fromName":"Lorenzo Pegorari","fromEmail":"lorenzo.pegorari2002@gmail.com","sentAt":"2026-03-26T02:01:07Z","receivedAt":"2026-03-26T02:01:11Z","isPatch":true,"body":"On Mon, Mar 23, 2026 at 02:30:26PM -0700, Junio C Hamano wrote:\n> LorenzoPegorari writes:\n> \n> > Create a `copy_all_promisor_files()` helper function used to copy the\n> > contents of all \".promisor\" files in a `repository` inside another\n> > \".promisor\" file.\n> >\n> > This function can be used to preserve the contents of all \".promisor\"\n> > files inside a new \".promisor\" file, for example when a repack happens.\n> >\n> > This function is written in such a way so that it will read all the\n> > \".promisor\" files inside the given `repository` line by line, and copy\n> > only the lines that are not already present in the destination file. This\n> > is done to avoid copying the same lines multiple times that may come from\n> > multiple (redundant) packfiles. There might be another better/cleaner way\n> > to achieve this.\n> \n> In the previous step, we extablished that these \"back then their ref\n> X used to point at object Y\" records are there so that we can\n> identify which refs were fetched at the time the packfile was\n> downloaded to help debugging. When repacking, losing these records\n> certainly would lose information.\n> \n> But would concatenating all into a single file help preserve the\n> useful information? Don't we need do better than that?\n\nYeah, we absolutely need to! That's why I said that I was not satisfied\nat all with the patch (in cover letter of v1). I really needed some\nfeedback, because I knew that I was doing things wrong.\n\n> A NEEDSWORK comment, as was discussed in another thread or two in\n> the recent past, is not necessarily a well thought out fully\n> finished specification of an additional piece of work. \"We know\n> this has a problem, we may need to do something about it, like\n> concatenating to save the contents, perhaps? We do not know the\n> answer, and we do not bother thinking it through right at this\n> moment. It is left to the future developers to figure it out\" is\n> what a NEEDSWORK comment is about.\n> \n> Your first response to such a comment may be \"yeah, I agree that it\n> is bad to lose information we added to help debugging\", but the\n> second one should be to wonder if the \"like concatenating...\" is the\n> best approach going forward.\n> \n> In other words, we should take a NEEDSWORK comment as a mere\n> starting point, and what NEEDS your work begins at thinking what\n> needs to be done about the problem raised there.\n\nI fully understand this! Honestly, my biggest weakness that I've\ndiscovered about myself as a dev (through past open-source experience,\ne.g. GSoC'25) is that I get hesitant when I have to work on and submit\na patch if I don't have a lot of experience with the codebase. This\nhappens particularly when I have to take a decision, and not only\ncomplete a task.\n\nIn fact, I decided to work on this specific NEEDSWORK issue to get more\nexperience on promisor remotes (the feature that I want to improve in my\nGSoC proposal) before the GSoC coding period... if I get selected, of\ncourse :-).\n\nI will try my best to improve!\n\n> By mixing them up all into a single list, you no longer can tell\n> when their ref X was observed to be pointing at object Y anymore.\n> You may have two packs originally, with a record for \"ref X pointing\n> at object Y\" in each of them, but by deduping them, you lose the\n> information that you cloned at one time, and made an additional\n> fetch on another day, and the fact the ref X was pointing at the\n> same value at both times. I am not sure if it is a good\n> implementation if the objective of this topic is to preserve\n> information that is useful for debugging.\n\nMy reasoning was based on the (wrong) assumption that it was impossible\nfor the same record fo \"ref X is pointing at object Y\" to appear\nmultiple times. Obviously then, deduping them is the wrong solution, as\nit will lose some debugging information.\n\n> I wonder if it helps to append to each line the file timestamp of\n> the .promisor file we took the record originally? For the sake of\n> completeness, we could consider adding the filename as well, but we\n> can quickly dismiss it as not so useful ;-)\n\nRelated to what I said before about getting hesitant... I actually\nthought about (pretty much) exactly this! My original idea was to add\na timestamp of the current time when the repack happened. I discarded it\nbecause I didn't want to add any new information (for no particular\nreason tbh) and because I didn't want \".promisor\" file content to\npotentially become too long if many repacks happen.\n\nYour solution is much cleaner compared to what I originally thought of.\n\n> If repacking already repacked promisor packfile, the records would\n> already contain such a timestamp at the end, so the code to copy\n> existing records must be prepared to see if the records are the\n> tuple, or tuple, and act\n> accordingly.\n\nMakes perfect sense.\n\n> I am *not* saying that without such a \"preserve timestamp\" column in\n> the record, copying existing records to a new .promisor file is\n> useless. But we do not see any explanation why the author thinks\n> that it is sufficient to copy existing records while silently\n> deduping. We can implement only one choice backed by series of\n> decisions like \"timestamp might help\" and \"original filenames would\n> probably not help\", and the design should describe what was\n> considered and rejected (as opposed to \"we didn't think things\n> through---we just did what the original NEEDSWORK comment suggested\n> doing\").\n\nI 100% agree.\n\n> Thanks.\n\nThank you Junio for your time and feedback,\n\nLorenzo\n"},{"id":"540034","messageId":"acSWG-7Kj67zgzvZ@lorenzo-VM","threadId":"65326","inReplyTo":"xmqqa4vy43q2.fsf@gitster.g","subject":"Re: [GSoC PATCH v2 3/4] repack-promisor: preserve content of promisor files after repack","fromName":"Lorenzo Pegorari","fromEmail":"lorenzo.pegorari2002@gmail.com","sentAt":"2026-03-26T02:12:43Z","receivedAt":"2026-03-26T02:12:46Z","isPatch":true,"body":"On Mon, Mar 23, 2026 at 02:48:21PM -0700, Junio C Hamano wrote:\n> LorenzoPegorari writes:\n> \n> > @@ -40,6 +40,7 @@ static void finish_repacking_promisor_objects(struct repository *repo,\n> > \t\t\t\t\t const char *packtmp)\n> > {\n> > \tstruct strbuf line = STRBUF_INIT;\n> > +\tint is_first_promisor = 1;\n> > \tFILE *out;\n> > ... \n> > +\t\t/*\n> > +\t\t * Fetch-pack sometimes generates non-empty .promisor files\n> > +\t\t * containing the ref names and associated hashes at the point of\n> > +\t\t * generation of the corresponding packfile. These pieces of info\n> > +\t\t * are only used for debugging reasons. In order to preserve\n> > +\t\t * these, let's copy the contents of all .promisor files in the\n> > +\t\t * first promisor file created.\n> > +\t\t */\n> > +\t\tif (is_first_promisor) {\n> > +\t\t\tcopy_all_promisor_files(repo, promisor_name);\n> > +\t\t\tis_first_promisor = 0;\n> > +\t\t}\n> > +\n> \n> Here the underlying assumption seems to be that whichever one of the\n> two potential callers of this function, repack_promisor_objects()\n> and pack_geometry_repack_promisors(), would handle all the existing\n> packs with corresponding .promisor file so it is safe to coalesce\n> all the debugging comments from all the existing .promisor files\n> into one?\n> \n> Is it really true, though? Especially with geometry repacking\n> enabled, wouldn't a regular repack coalesce only the smallish ones\n> into a single pack while leaving an already largeish ones intact, or\n> something?\n> \n> Thanks.\n\nI will look into this. I'm going to drastically rework this patch\nseries, so that the next version will be much better and better\nexplained.\n\nThank you so much for the time,\n\nLorenzo\n"},{"id":"540101","messageId":"acVbhqGEc2826Cii@lorenzo-VM","threadId":"65326","inReplyTo":"CAPig+cR7-3rdkHvGbzk8O7P=83pxBTvqXTUgMxYpf+OK9jNCgg@mail.gmail.com","subject":"Re: [GSoC PATCH v2 2/4] pack-write: add helper to fill promisor file after repack","fromName":"Lorenzo Pegorari","fromEmail":"lorenzo.pegorari2002@gmail.com","sentAt":"2026-03-26T16:15:02Z","receivedAt":"2026-03-26T16:15:07Z","isPatch":true,"body":"On Mon, Mar 23, 2026 at 04:27:44PM -0400, Eric Sunshine wrote:\n> On Sun, Mar 22, 2026 at 3:18 PM LorenzoPegorari\n> wrote:\n> > Create a `copy_all_promisor_files()` helper function used to copy the\n> > contents of all \".promisor\" files in a `repository` inside another\n> > \".promisor\" file.\n> >\n> > This function can be used to preserve the contents of all \".promisor\"\n> > files inside a new \".promisor\" file, for example when a repack happens.\n> >\n> > This function is written in such a way so that it will read all the\n> > \".promisor\" files inside the given `repository` line by line, and copy\n> > only the lines that are not already present in the destination file. This\n> > is done to avoid copying the same lines multiple times that may come from\n> > multiple (redundant) packfiles. There might be another better/cleaner way\n> > to achieve this.\n> >\n> > Signed-off-by: LorenzoPegorari \n> > ---\n> \n> Thanks, I think this version addresses all my review comments[*] and\n> looks much better overall. Use of `strset` makes a big difference over\n> the previous attempt. A couple minor comments below...\n> \n> [*]: https://lore.kernel.org/git/CAPig+cQSsMfvHJnwuXGQ1Je8ekz=Rqbaibn-3shbya5y-5xTKg@mail.gmail.com/\n> \n> > diff --git a/pack-write.c b/pack-write.c\n> > @@ -621,3 +622,63 @@ void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_\n> > +void copy_all_promisor_files(struct repository *repo, const char *promisor_name)\n> > +{\n> > + struct strset dest_content = STRSET_INIT;\n> > + struct strbuf read_line = STRBUF_INIT;\n> > + struct strbuf promisor_source_name = STRBUF_INIT;\n> > + struct strbuf write_dest = STRBUF_INIT;\n> > + FILE *dest, *source;\n> > + struct packed_git *p;\n> > + int err;\n> \n> Nit: I probably would have declared `FILE *dest` within the scope of\n> the repo_for_each_pack() loop as suggested in the review, but it's not\n> worth a reroll.\n\nAck. I will fix it. Thanks!\n\n> > + dest = xfopen(promisor_name, \"r+\");\n> > + while (strbuf_getline(&read_line, dest) != EOF)\n> > + strset_add(&dest_content, read_line.buf);\n> > +\n> > + repo_for_each_pack(repo, p) {\n> > + if (!p->pack_promisor)\n> > + continue;\n> > +\n> > + strbuf_reset(&promisor_source_name);\n> > + strbuf_addstr(&promisor_source_name, p->pack_name);\n> > + strbuf_strip_suffix(&promisor_source_name, \".pack\");\n> > + strbuf_addstr(&promisor_source_name, \".promisor\");\n> > + source = xfopen(promisor_source_name.buf, \"r\");\n> > +\n> > + /*\n> > + * For each line of the promisor source file, check if it already\n> > + * is in the promisor dest file. If not, add it to write_dest, so\n> > + * that it will be written in the dest file.\n> > + */\n> > + while (strbuf_getline(&read_line, source) != EOF) {\n> > + if (strset_add(&dest_content, read_line.buf)) {\n> > + strbuf_addbuf(&write_dest, &read_line);\n> > + strbuf_addstr(&write_dest, \"\\n\");\n> \n> Not worth a reroll, but this could also be:\n> \n> strbuf_addch(&write_dest, '\\n');\n\nAck.\n\n> > + }\n> > + }\n> > +\n> > + err = ferror(source);\n> > + err |= fclose(source);\n> > + if (err)\n> > + die(_(\"could not read '%s' promisor file\"), promisor_source_name.buf);\n> > + }\n> > +\n> > + if (write_dest.len) {\n> > + strbuf_strip_suffix(&write_dest, \"\\n\");\n> > + if (fseek(dest, 0L, SEEK_END))\n> > + die_errno(_(\"fseek failed\"));\n> > + fprintf(dest, \"%s\\n\", write_dest.buf);\n> > + }\n> \n> Can you explain why you strip \"\\n\" and then re-add it via fprintf()?\n> The reason is not immediately obvious.\n\nStripping it and then adding it again is actually not necessary. I think\nit was necessary in a previous iteration. Thanks for noticing, will fix!\n\n> > + err = ferror(dest);\n> > + err |= fclose(dest);\n> > + if (err)\n> > + die(_(\"could not write '%s' promisor file\"), promisor_name);\n> > +\n> > + strbuf_release(&read_line);\n> > + strbuf_release(&promisor_source_name);\n> > + strbuf_release(&write_dest);\n> > + strset_clear(&dest_content);\n> > +}\n> \n> Everything appears to be released. Good.\n\nThank you so much for your help Eric,\n\nLorenzo\n\n"},{"id":"540957","messageId":"cover.1775431990.git.lorenzo.pegorari2002@gmail.com","threadId":"65326","inReplyTo":"cover.1774205661.git.lorenzo.pegorari2002@gmail.com","subject":"[GSoC PATCH v3 0/5] preserve promisor files content after repack","fromName":"LorenzoPegorari","fromEmail":"lorenzo.pegorari2002@gmail.com","sentAt":"2026-04-06T00:23:48Z","receivedAt":"2026-04-06T00:23:53Z","isPatch":true,"body":"The goal of this patch is to solve the NEEDSWORK comment added by\n5374a290 (fetch-pack: write fetched refs to .promisor, 14/10/2019). This\nis done by adding a helper function that takes the content of all\n.promisor files in the `repository`, and copies it inside the first\n.promisor file created by the repack.\n\nAlso, I added a comment explaining what is the purpose of the content of\nthe .promisor files, since this wasn't explained anywhere (I found\ninformation regarding this only in the message of the previously cited\ncommit).\n\nFinally, I added some tests to \"t7700-repack.sh\" and\n\"t7703-repack-geometric.sh\" that check if the content of .promisor files\nare correctly copied into the .promisor files created by a repack.\n\nThis version is significantly different from the previous one. Maybe I\nshould have created a completely different patch series. Let me know for\nthe future.\n\n\nIMPORTANT:\nThe \"CodingGuidelines\" explicitly state that:\n \"A C file must directly include the header files that declare the\n functions and the types it uses, except for the functions and types\n that are made available to it by including one of the header files\n it must include by the previous rule\"\nwhere \"the previous rule\" is (if I understand correctly), the one related\nto \"\". From what I understand then, I should have\nadded an include for \"strmap.h\" (which is needed for `strset`), correct?\nAnd if I am correct, shouldn't \"strbuf.h\", \"hash.h\", \"odb.h\",\n\"string-list.h\" and \"strvec.h\" also be included?\n\n\nV3 DIFF:\n * Made the helper function \"copy_promisor_content()\" a static function,\n because, in my opinion, it is too specific to be used anywhere else\n (at least in the near future).\n * Modified the helper function to add a