Re: [GSoC PATCH v5 2/6] repack-promisor add helper to fill promisor file after repack

[Junio C Hamano <gitster@pobox.com>]

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

> +/*
> + * Go through all .promisor files contained in repo (excluding those whose name
> + * appears in not_repacked_basenames, which acts as a ignorelist), and copies
> + * their content inside the destination file "<packtmp>-<dest_hex>.promisor".
> + * Each line of a never repacked .promisor file is: "<oid> <ref>" (as described
> + * in the write_promisor_file() function).
> + * After a repack, the copied lines will be: "<oid> <ref> <time>", where <time>
> + * is the time (in Unix time) at which the .promisor file was last modified.
> + * Only the lines whose <oid> is present inside "<packtmp>-<dest_hex>.idx" will
> + * be copied.
> + * The contents of all .promisor files are assumed to be correctly formed.
> + */
> +static void copy_promisor_content(struct repository *repo,
> +					      const char *dest_hex,
> +					      const char *packtmp,
> +					      struct strset *not_repacked_basenames)
> +{
> +	char *dest_idx_name;
> +	char *dest_promisor_name;
> +	FILE *dest;
> +	struct strset dest_content = STRSET_INIT;
> +	struct strbuf dest_to_write = STRBUF_INIT;
> +	struct strbuf source_promisor_name = STRBUF_INIT;
> +	struct strbuf line = STRBUF_INIT;
> +	struct object_id dest_oid;
> +	struct packed_git *dest_pack, *p;
> +	int err;
> +
> +	dest_idx_name = mkpathdup("%s-%s.idx", packtmp, dest_hex);
> +	get_oid_hex_algop(dest_hex, &dest_oid, repo->hash_algo);

This needs to prepare for a corrupt input in dest_hex, which would
result in garbage dest_oid.  The helper function should signal a
failure with its return value, right?

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

As you earlier mentioned, this use of parse_pack_index() is
perfectly fine.  The call chains that reach here are both from
cmd_repack() that calls either repack_promisor_objects() or
pack_geometry_repack_promisors(), and both ran "pack-objects" to
create a new pack and called finish_repacking_promisor_objects(),
which in turn calls us, so the dest_hex/packtmp we are dealing with
point newly created packfile that is about to become but not yet
completed as a part of this repository.  We know we created it, and
we know "pack-objects" did not fail, so parse_pack_index() being
loose in validation does not pose a practical problem.

This still needs to prepare for parse_pack_index() to return NULL,
though.  

In the above two cases, we should make sure that dest_idx_name gets
freed before we return control to the caller (possibly signaling an
error by returning -1, but the current caller is not expecting to
hear a failure from us and that may be OK).

> +	/* 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);

The strbuf's contents line.buf[] is read/write, so we could use
line_sections that is initialized with NODUP and call
split_in_place() to avoid unnecessary small allocations and
deallocations, no?

More importantly, we say "split into up to 3 pieces".  What happens
if this is totally malformed and there is only one word?  Should we
still trust this line and try to carry it forward?  I doubt it.

> +			/* 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);

Or the first word split is not a sane hexadecimal string that
get_oid_hex() fails?

It would be the simplest to ignore/skip the line, just like what you
do to a correctly formated line about an irrelevant <oid> (iow, the
if() statement immediately below).

> +			if (!find_pack_entry_one(&oid, dest_pack)) {

Assuming that the object name was read correctly, if the pack we
just created does not have the <oid> we read from the existing
.promisor file, this line we just read has nothing to do with the
repacked result, so we ignore it, which sounds fine.

> +				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);

Should we also validate line_sections[1] in some way?  I am not sure
if we want to call check_ref_format() on it.

If we insist that .nr is at least 2 immediately after we split the
string, and make sure the line begins with <oid> (i.e., parsable as
hex object name) that might be sufficient.  I dunno.

> +			/*
> +			 * 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);

As we discussed, 

	free(dest_pack);

is missing.

> +	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,