From: Patrick Steinhardt <ps@pks.im>
Date: Thu, 10 Sep 2026 14:26:41 GMT
Subject: Re: [PATCH v4 4/9] builtin/clone: refactor handling of "--reference{,-if-able}"
Message-ID: <aqK-IYVKCKp9ExmY@pks.im>
In-Reply-To: <CAOLa=ZQf3XcX_G1H+vNSQtpU-Oj56pda0WcJAORw-kQMEPvTTw@mail.gmail.com>

On Thu, Sep 10, 2026 at 02:37:31AM -0700, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > 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`.

We can also rename this to `collect_one_alternate()` and the structure
`collect_alternates_data`.

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

No, there isn't. I'm probably just still getting used to new fancy
features like that.

> > @@ -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.

We'll extend this over the next couple patches, so I'll leave this
as-is.

> > @@ -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.

Will do.

Patrick