Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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.
> @@ -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.
