From: Tian Yuchen <cat@malon.dev>
Date: Sat, 11 Apr 2026 18:25:50 GMT
Subject: Re: [GSoC PATCH v5 3/6] repack-promisor: preserve content of promisor files after repack
Message-ID: <641a7a4c-f836-4811-bbeb-ef534716c3a9@malon.dev>
In-Reply-To: <b483be7558f0efc1a6780b5cff13f4ccc3afd069.1775861047.git.lorenzo.pegorari2002@gmail.com>

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.

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

(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?)

Thank you, Yuchen