Æ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".
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
> ---
> builtin/submodule--helper.c | 38 +++++++++++++++++++------------------
> 1 file changed, 20 insertions(+), 18 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 29641690c8c..7d5ee6a6261 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -1650,20 +1650,22 @@ 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)) {
> 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);
> } else {
> - clone_data->path = xstrdup(clone_data->path);
> + clone_data_path = xstrdup(clone_data_path);
> }
>
> if (validate_submodule_git_dir(sm_gitdir, clone_data->name) < 0)
Hm, the leak in the preimage comes from the fact that we assign back to
clone_data->path, which is a "const char *" that initially comes from
argv. So we didn't free() it even though it always pointing to
free()-able memory past this point.
So now that we're introducing and assigning to a new variable,
clone_data_path, wouldn't it be simpler to just make it "char *" and
free it (instead of adding a separate "char *to_free" in the next
patch)?
