Junio C Hamano <gitster@xxxxxxxxx> writes:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@xxxxxxxxx>
> writes:
>
>> From: Johannes Schindelin <johannes.schindelin@xxxxxx>
>>
>> Reported by Coverity.
>>
>> Signed-off-by: Johannes Schindelin <johannes.schindelin@xxxxxx>
>> ---
>> 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.
Good eye, I hadn't noticed this.
In this case, we're computing the same thing twice in the function -
first unconditionally, then conditionally. So we can actually just reuse
the first result by doing something like
next.recursive_prefix = xstrfmt("%s/", update_data->displaypath);
(note that we need to append a "/" at the end)
> 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.
Even better, I recently noticed that .recursive_prefix can probably just
be replaced with "--super-prefix", and that will let us dispatch to
get_submodule_displaypath() without this extra dance here.
I'll flesh that out into a series and send it soon.
> 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 ;-)
cc-ed Ævar, who knows even more than I do :)
git-submodule.sh is definitely going away soon \o/ but I don't think we
have plans to get rid of submodule--helper.c just yet, so I think we
should probably still keep this patch.
>
> Thanks.
>
>> next.prefix = NULL;
>> oidcpy(&next.oid, null_oid());
>> oidcpy(&next.suboid, null_oid());
