Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
> Add "const" to the "struct module_clone_data" that we pass to
> clone_submodule(), which makes the ownership clear, and stops us from
> clobbering the "clone_data->path".
>
> We still need to add to the "reference" member, which is a "struct
> string_list". Let's do this by having clone_submodule() create its
> own, and copy the contents over, allowing us to pass it as a
> separate parameter.
I can't help but think that this would be easier to review as part of
the leaks series since:
- Outside of leaks, I don't think we really care about ownership (though
please please correct me if I'm off base).
- The ownership of "reference" is still quite messy (downstream code
might append to it, but its members are sometimes free()-able and
sometimes not), so it's hard to visualize what we're getting out of
this change without seeing the corresponding leak fix.
and...
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 6cedcc5b239..e235acce985 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -1569,18 +1567,20 @@ static char *clone_submodule_sm_gitdir(const char *name)
> return sm_gitdir;
> }
>
> -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))
> - clone_data->path = xstrfmt("%s/%s", get_git_work_tree(),
> - clone_data->path);
> + clone_data_path = xstrfmt("%s/%s", get_git_work_tree(),
> + clone_data->path);
- (this is pretty minor) It feels weird to see that we're intentionally
leaking clone_data_path at its inception. We aren't introducing any
new leaks, but moving this to the leaks series makes it clearer that
we eventually do the right thing.
