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

On Thu, Sep 10, 2026 at 02:37:31AM -0700, Karthik Nayak wrote:

Patrick Steinhardt [off-list ref] writes:

quoted

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

quoted

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

quoted

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

quoted

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