From: Lorenzo Pegorari <lorenzo.pegorari2002@gmail.com>
Date: Fri, 17 Apr 2026 00:34:11 GMT
Subject: Re: [GSoC PATCH v5 3/6] repack-promisor: preserve content of promisor files after repack
Message-ID: <aeGAAx6_o6f_BN3A@lorenzo-VM>
In-Reply-To: <641a7a4c-f836-4811-bbeb-ef534716c3a9@malon.dev>

On Sun, Apr 12, 2026 at 02:25:50AM +0800, Tian Yuchen wrote:
> On 4/11/26 06:55, LorenzoPegorari wrote:
> 
> > @@ -171,19 +172,15 @@ static void finish_repacking_promisor_objects(struct repository *repo,
> >   		/*
> >   		 * pack-objects creates the .pack and .idx files, but not the
> > -		 * .promisor file. Create the .promisor file, which is empty.
> > -		 *
> > -		 * NEEDSWORK: fetch-pack sometimes generates non-empty
> > -		 * .promisor files containing the ref names and associated
> > -		 * hashes at the point of generation of the corresponding
> > -		 * packfile, but this would not preserve their contents. Maybe
> > -		 * concatenate the contents of all .promisor files instead of
> > -		 * just creating a new empty file.
> > +		 * .promisor file. Create the .promisor file.
> >   		 */
> >   		promisor_name = mkpathdup("%s-%s.promisor", packtmp,
> >   					  line.buf);
> >   		write_promisor_file(promisor_name, NULL, 0);
> > +		/* Now let's fill the content of the newly created .promisor file */
> > +		copy_promisor_content(repo, line.buf, packtmp, not_repacked_basenames);
> 
> Here, the file opened by copy_promisor_content() is an empty file. Is this
> line necessary? ;)
> 
> ...hold on. I recall you mentioning in one of the versions that you had
> downgraded this helper from a generic function to a static one. Since it now
> only serves this particular business logic, I think the implementation
> should be tweaked slightly as well.

Good point. It makes sense to modify the helper function to create the
empty ".promisor" file, and then fill it, so that we won't use
`write_promisor_file()` at all.

> > +	/* Open the .promisor dest file, and fill dest_content with its content */
> > +	dest_promisor_name = mkpathdup("%s-%s.promisor", packtmp, dest_hex);
> > +	dest = xfopen(dest_promisor_name, "r+");
> > +	while (strbuf_getline(&line, dest) != EOF)
> > +		strset_add(&dest_content, line.buf);
> 
> If file contains a large number of unique lines, dest_to_write, which is a
> strbuf, may keep realloc memory until the loop ends, at which point all the
> memory is released. I wonder if this might be wasting some heap.
> 
> If it were me, I might write it like this:
> 
> 	struct strset seen_lines = STRSET_INIT;
> 	dest = xfopen(dest_promisor_name, "w");
> 	while (strbuf_getline(&line, source) != EOF) {
>     		if (strset_add(&seen_lines, line.buf)) {
>         		fprintf(dest, "%s\n", line.buf);
>     		}	
> 	}
> 
> It also prevents file pointer misalignment.

Makes sense. Will use this. Thanks!

> (I think we still need to discuss what should ultimately become of this
> helper; at the moment, it seems a bit disjointed, doesn’t it?)

I feel like keeping it a static function only used to generate the
".promisor" files after a repack makes the most sense.

> Thank you, Yuchen

Thanks,
Lorenzo