From: Karthik Nayak Date: Thu, 10 Sep 2026 09:37:31 GMT Subject: Re: [PATCH v4 4/9] builtin/clone: refactor handling of "--reference{,-if-able}" Message-ID: In-Reply-To: <20260909-pks-odb-write-alternates-at-creation-time-v4-4-d8a78ffc32e4@pks.im> Patrick Steinhardt writes: > 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! > Signed-off-by: Patrick Steinhardt > --- > 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? > +}; > + > +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. > } > > 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 [] [--] []"), > @@ -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. > 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