Re: [PATCH 03/11] submodule--helper: avoid memory leak in `update_submodule()`

[Junio C Hamano <gitster@pobox.com>]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> Reported by Coverity.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/submodule--helper.c | 2 ++
>  1 file changed, 2 insertions(+)


> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 5c77dfcffee..d7b8004b933 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2512,6 +2512,8 @@ static int update_submodule(struct update_data *update_data)
>  
>  		next.recursive_prefix = get_submodule_displaypath(prefixed_path,
>  								  update_data->prefix);
> +		free(prefixed_path);
> +

This function has two very similar code block that computes
prefixed_path depending on the same condition, and one frees the
variable correctly while the other one (i.e. this one) forgets to do
so, which is irritating to see.

Perhaps the whole "we have update_data structure, in which
recursive_prefix, sm_path and prefix members in it; please set the
displaypath member based on these values" should become a helper
function, e.g.

	static const char *displaypath_from_update_data(struct update_data *u)
	{
		char *pp, *ret;

		if (u->recursive_prefix)
			pp = xstrfmt("%s%s", u->recursive_prefix, u->sm_path);
		else
			pp = xstrdup(u->sm_path);

		ret = get_submodule_displaypath(pp, u->prefix);
		free(pp);
		return ret;
	}

to avoid duplicated computation.

But the whole thing may become moot, as there seems to be a move to
get rid of submodule--helper.c altogether?

I'll refrain from touching this patch and instead redirect it to
Glen; perhaps removal of submodule--helper.c involves moving the
code here to another file or something, in which case it is far
easier if I outsource that to somebody who is actually working on
the file ;-)

Thanks.

>  		next.prefix = NULL;
>  		oidcpy(&next.oid, null_oid());
>  		oidcpy(&next.suboid, null_oid());