From: Junio C Hamano <gitster@pobox.com>
Date: Fri, 10 Apr 2026 23:30:40 GMT
Subject: Re: [GSoC PATCH v5 2/6] repack-promisor add helper to fill promisor file after repack
Message-ID: <xmqqo6jqmlzz.fsf@gitster.g>
In-Reply-To: <3558bb38956b522c91057598db645eb42ffb48b2.1775861047.git.lorenzo.pegorari2002@gmail.com>

LorenzoPegorari <lorenzo.pegorari2002@gmail.com> writes:

> +	dest_pack = parse_pack_index(repo, dest_oid.hash, dest_idx_name);

parse_pack_index() has this comment:

        /*
         * Parse the pack idx file found at idx_path and create a packed_git struct
         * which can be used with find_pack_entry_one().
         *
         * You probably don't want to use this function! It skips most of the normal
         * sanity checks (including whether we even have the matching .pack file),
         * and does not add the resulting packed_git struct to the internal list of
         * packs. You probably want add_packed_git() instead.
         */
        struct packed_git *parse_pack_index(struct repository *r, unsigned char *sha1,
                                            const char *idx_path);

The function can return NULL, but this caller does not seem to be
prepared for it to return NULL (i.e., the loop introduced by the
repo_for_each_pack() macro we see below, nobody assumes dest_pack
could be NULL).

But what pack index file are we parsing here?  Isn't it already part
of the running system that we should be able to find on the list of
packfiles in the packfile store?  Is this because we lack "find a
packfile on this packfile store by name" API, because what we want
to find if each of <oid>s we have appear in the particular packfile
or not, and packfile_list_find_oid() is not sufficiently precise (i.e.
"the object appears in one of the packfile on the list" is not what
we want to know, "the object appears in this particular packfile" is)?

	Patrick CC'ed primarily because this part of the API and the
	data structures have been reshuffled to add quite a lot of
	abstraction since I last looked at the area.

As close_pack_index(dest_pack) does not release resources held by
dest_pack itself (even though the region of mmaped memory that is
pointed at by its index_data member is unmapped), I think that is
where the memory leak is breaking the CI jobs (see my other message).

But I am not sure if the use of parse_pack_index() - close_pack_index()
API is the right thing to use here.



> +	/* 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);
> +
> +	repo_for_each_pack(repo, p) {
> +		FILE *source;
> +		struct stat source_stat;
> +
> +		if (!p->pack_promisor)
> +			continue;
> +
> +		if (not_repacked_basenames &&
> +			strset_contains(not_repacked_basenames, pack_basename(p)))
> +			continue;
> +
> +		strbuf_reset(&source_promisor_name);
> +		strbuf_addstr(&source_promisor_name, p->pack_name);
> +		strbuf_strip_suffix(&source_promisor_name, ".pack");
> +		strbuf_addstr(&source_promisor_name, ".promisor");
> +
> +		if (stat(source_promisor_name.buf, &source_stat))
> +			die(_("File not found: %s"), source_promisor_name.buf);
> +
> +		source = xfopen(source_promisor_name.buf, "r");
> +
> +		while (strbuf_getline(&line, source) != EOF) {
> +			struct string_list line_sections = STRING_LIST_INIT_DUP;
> +			struct object_id oid;
> +
> +			/* Split line into <oid>, <ref> and <time> (if <time> exists) */
> +			string_list_split(&line_sections, line.buf, " ", 3);
> +
> +			/* Ignore the lines where <oid> doesn't appear in the dest_pack */
> +			get_oid_hex_algop(line_sections.items[0].string, &oid, repo->hash_algo);
> +			if (!find_pack_entry_one(&oid, dest_pack)) {
> +				string_list_clear(&line_sections, 0);
> +				continue;
> +			}
> +
> +			/* If <time> doesn't exist, retrieve it and add it to line */
> +			if (line_sections.nr < 3)
> +				strbuf_addf(&line, " %" PRItime, (timestamp_t)source_stat.st_mtime);
> +
> +			/*
> +			 * Add the finalized line to dest_to_write and dest_content if it
> +			 * wasn't already present inside dest_content
> +			 */
> +			if (strset_add(&dest_content, line.buf)) {
> +				strbuf_addbuf(&dest_to_write, &line);
> +				strbuf_addch(&dest_to_write, '\n');
> +			}
> +
> +			string_list_clear(&line_sections, 0);
> +		}
> +
> +		err = ferror(source);
> +		err |= fclose(source);
> +		if (err)
> +			die(_("Could not read '%s' promisor file"), source_promisor_name.buf);
> +	}
> +
> +	/* If dest_to_write is not empty, then there are new lines to append */
> +	if (dest_to_write.len) {
> +		if (fseek(dest, 0L, SEEK_END))
> +			die_errno(_("fseek failed"));
> +		fprintf(dest, "%s", dest_to_write.buf);
> +	}
> +
> +	err = ferror(dest);
> +	err |= fclose(dest);
> +	if (err)
> +		die(_("Could not write '%s' promisor file"), dest_promisor_name);
> +
> +	close_pack_index(dest_pack);
> +	free(dest_idx_name);
> +	free(dest_promisor_name);
> +	strset_clear(&dest_content);
> +	strbuf_release(&dest_to_write);
> +	strbuf_release(&source_promisor_name);
> +	strbuf_release(&line);
> +}
> +
>  static void finish_repacking_promisor_objects(struct repository *repo,
>  					      struct child_process *cmd,
>  					      struct string_list *names,