Subject: Re: [PATCH 1/7] bundle-uri: create bundle_list struct and helpers

From: Junio C Hamano <[email-masked]>
Date: Mon, 22 Aug 2022 10:57:48 -0700

"Derrick Stolee via GitGitGadget" <[email-masked]> writes:

> +/**
> + * The remote_bundle_info struct contains information for a single bundle
> + * URI. This may be initialized simply by a given URI or might have
> + * additional metadata associated with it if the bundle was advertised by
> + * a bundle list.
> + */
> +struct remote_bundle_info {
> +	struct hashmap_entry ent;
> +
> +	/**
> +	 * The 'id' is a name given to the bundle for reference
> +	 * by other bundle infos.
> +	 */
> +	char *id;
> +
> +	/**
> +	 * The 'uri' is the location of the remote bundle so
> +	 * it can be downloaded on-demand. This will be NULL
> +	 * if there was no table of contents.
> +	 */
> +	char *uri;
> +
> +	/**
> +	 * If the bundle has been downloaded, then 'file' is a
> +	 * filename storing its contents. Otherwise, 'file' is
> +	 * an empty string.
> +	 */
> +	struct strbuf file;
> +};

Presumably the sequence of events are that first a bundle list is
obtained, with their .file member set to empty, then http worker(s)
download and deposit the contents to files at which time the .file
member is set to the resulting file.  The file downloader presumably
uses the usual "create a temporary file, download to it, and then
commit it by closing and then renaming" dance, and the downloading
http worker may want to have two strbufs somewhere it can access to
come up with the name of the temporary and the name of the final
file.  But once the result becomes a committed file, its name will
not change, or will it?

At this step without the code that actually uses the data, use of
strbuf, instead of "char *" like id and uri members do, smells like
a premature optimization, and it is unclear if the optimization is
even effective.

Other than that, looks good to me.