Re: [PATCH v3 03/26] submodule--helper: pass a "const struct module_clone_data" to clone_submodule()
From: Junio C Hamano <hidden>
Date: 2022-07-21 21:26:42
Ævar Arnfjörð Bjarmason [off-list ref] writes:
-static int clone_submodule(struct module_clone_data *clone_data)
+static int clone_submodule(const struct module_clone_data *clone_data,
+ struct string_list *reference)
{
char *p;
char *sm_gitdir = clone_submodule_sm_gitdir(clone_data->name);
char *sm_alternate = NULL, *error_strategy = NULL;
struct child_process cp = CHILD_PROCESS_INIT;
+ const char *clone_data_path;
if (!is_absolute_path(clone_data->path)) {
struct strbuf sb = STRBUF_INIT;
strbuf_addf(&sb, "%s/%s", get_git_work_tree(), clone_data->path);
- clone_data->path = strbuf_detach(&sb, NULL);
+ clone_data_path = strbuf_detach(&sb, NULL);OK.
} else {
- clone_data->path = xstrdup(clone_data->path);
+ clone_data_path = xstrdup(clone_data_path);Is the variable we are duplicating by passing it to xstrdup() still uninitialized at this point? How could the compiler not catch this? Apparently there is no test coverage on this codepath; all tests pass when I replace this else clause with BUG(). Let's read on, pretending that you passed clone_data->path to xstrdup(), for now.
quoted hunk
@@ -1674,7 +1676,7 @@ static int clone_submodule(struct module_clone_data *clone_data) if (safe_create_leading_directories_const(sm_gitdir) < 0) die(_("could not create directory '%s'"), sm_gitdir); - prepare_possible_alternates(clone_data->name, &clone_data->reference); + prepare_possible_alternates(clone_data->name, reference); strvec_push(&cp.args, "clone"); strvec_push(&cp.args, "--no-checkout");@@ -1684,9 +1686,9 @@ static int clone_submodule(struct module_clone_data *clone_data) strvec_push(&cp.args, "--progress"); if (clone_data->depth && *(clone_data->depth)) strvec_pushl(&cp.args, "--depth", clone_data->depth, NULL); - if (clone_data->reference.nr) { + if (reference->nr) { struct string_list_item *item; - for_each_string_list_item(item, &clone_data->reference) + for_each_string_list_item(item, reference) strvec_pushl(&cp.args, "--reference", item->string, NULL); }
All the uses of reference (both above, so below) looks OK. I wonder if we can simply have a separate string list variable in module_clone() and add_submodule() and remove clone_data.reference member, which may make the end-result even cleaner, now that we pass it as a separate parameter anyway.