Re: [PATCH v4 4/9] builtin/clone: refactor handling of "--reference{,-if-able}"

From: Karthik Nayak <karthik.188@gmail.com>
Date: Sep 10, 2026, 09:37 UTC
Message-ID: <CAOLa=ZQf3XcX_G1H+vNSQtpU-Oj56pda0WcJAORw-kQMEPvTTw@mail.gmail.com>
In-Reply-To: <20260909-pks-odb-write-alternates-at-creation-time-v4-4-d8a78ffc32e4@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 19 quoted lines

> Users can pass "--reference{,-if-able}" to git-clone(1) to instruct it
> to set up alternates for the newly created repository. This allows it to
> reuse objects from the source repository so that in the best case we
> don't have to clone all objects over.
>
> Those options are handled by the confusingly named `setup_reference()`
> function -- without the above context, one might rightfully believe that
> it was about refs, not about alternates. The function itself is rather
> simple: we loop through all provided alternate paths and then, if such
> an alternate is valid, we write it to the object database.
>
> In subsequent commits we're about to consolidate the complete setup of
> alternates into this function, and furthermore we'll refactor the setup
> of the object database to handle doing this for us instead of writing
> the alternates into it one by one.
>
> Prepare for this refactoring by collecting the alternates into a strvec.
> Rename the function to `collect_alternates()` to clarify its scope.
>

Yay!

Show 15 quoted lines

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/clone.c | 44 ++++++++++++++++++++++++++++----------------
>  1 file changed, 28 insertions(+), 16 deletions(-)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index ac5843d7b9..8786a49332 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -154,24 +154,26 @@ static char *get_repo_path(const char *repo, int *is_bundle)
>  	return canon;
>  }
>
> -static int add_one_reference(struct string_list_item *item, void *cb_data)
> +struct add_one_alternate_data {

So this is `add_one_alternate()`'s data, was a bit confusing cause I first read that this was information regarding a single alternate, but then it also has a field called `alternates`.

> +	struct strvec *alternates;
> +	int required;

Question: here and other places, I see some of the boolean-ish fields
being declared as `int`. It doesn't matter, but is there a reason?

Show 51 quoted lines

> +};
> +
> +static int add_one_alternate(struct string_list_item *item, void *cb_data)
>  {
> +	struct add_one_alternate_data *data = cb_data;
>  	struct strbuf err = STRBUF_INIT;
> -	int *required = cb_data;
>  	char *ref_git = compute_alternate_path(item->string, &err);
>
>  	if (!ref_git) {
> -		if (*required)
> +		if (data->required)
>  			die("%s", err.buf);
>  		else
>  			fprintf(stderr,
>  				_("info: Could not add alternate for '%s': %s\n"),
>  				item->string, err.buf);
>  	} else {
> -		struct strbuf sb = STRBUF_INIT;
> -		strbuf_addf(&sb, "%s/objects", ref_git);
> -		odb_add_to_alternates_file(the_repository->objects, sb.buf);
> -		strbuf_release(&sb);
> +		strvec_pushf(data->alternates, "%s/objects", ref_git);
>  	}
>
>  	strbuf_release(&err);
> @@ -218,14 +220,20 @@ static void copy_alternates(struct strbuf *src, const char *src_repo)
>  	fclose(in);
>  }
>
> -static void setup_reference(void)
> +static void collect_alternates(struct strvec *alternates)
>  {
> -	int required = 1;
> -	for_each_string_list(&option_required_reference,
> -			     add_one_reference, &required);
> -	required = 0;
> -	for_each_string_list(&option_optional_reference,
> -			     add_one_reference, &required);
> +	if (option_required_reference.nr || option_optional_reference.nr) {
> +		struct add_one_alternate_data data = {
> +			.alternates = alternates,
> +			.required = 1,
> +		};
> +
> +		for_each_string_list(&option_required_reference,
> +				     add_one_alternate, &data);
> +		data.required = 0;
> +		for_each_string_list(&option_optional_reference,
> +				     add_one_alternate, &data);
> +	}

Nit: might be nicer to read

     if (!option_required_reference.nr && !option_optional_reference.nr)
        return;

The rest of it doesn't have to be in the `if` block.

Show 22 quoted lines

>  }
>
>  static void mkdir_if_missing(const char *pathname, mode_t mode)
> @@ -999,6 +1007,7 @@ int cmd_clone(int argc,
>  			   N_("uri"), N_("a URI for downloading bundles before fetching from origin remote")),
>  		OPT_END()
>  	};
> +	struct strvec alternates = STRVEC_INIT;
>
>  	const char * const builtin_clone_usage[] = {
>  		N_("git clone [<options>] [--] <repo> [<dir>]"),
> @@ -1339,8 +1348,10 @@ int cmd_clone(int argc,
>  		warning(_("--local is ignored"));
>
>  	create_object_database(the_repository);
> -	if (option_required_reference.nr || option_optional_reference.nr)
> -		setup_reference();
> +	collect_alternates(&alternates);
> +
> +	for (size_t i = 0; i < alternates.nr; i++)
> +		odb_add_to_alternates_file(the_repository->objects, alternates.v[i]);
>

So now adding the alternates is moved out of `collect_alternates()`.
Nit: might be nice to mention this in the commit message.

Show 13 quoted lines

>  	transport = transport_get(remote, path ? path : remote->url.v[0]);
>  	transport_set_verbosity(transport, option_verbosity, option_progress);
> @@ -1638,6 +1649,7 @@ int cmd_clone(int argc,
>  	string_list_clear(&option_not, 0);
>  	string_list_clear(&option_config, 0);
>  	string_list_clear(&server_options, 0);
> +	strvec_clear(&alternates);
>
>  	free(remote_name);
>  	strbuf_release(&reflog_msg);
>
> --
> 2.55.0.1074.ge7621b4bad.dirty