Re: [PATCH 09/11] submodule--helper: free "char *" in "struct update_data"

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, Jul 14 2022, Glen Choo wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> Make the update_data_release() function free the "recursive_prefix"
>> and "displaypath" members when appropriate. For the former it could
>> come from either "argv" or from our own allocation, so we need to keep
>> track of a "to_free" sibling seperately.
>
> Obsolete message probably? "recursive_prefix" no longer exists as of
> gc/submodule-use-super-prefix ;)

Thanks, will fix!

>> For "displaypath" it's always ours, so the "const char *" type was
>> wrong to begin with, it should be a "char *" instead.
>
> Ok.
>
>> For update_submodule() we'll free() these as we go along, it's called
>> in a loop by update_submodules(), and we'll need to free the "last"
>> one.
>
> Hm, I don't follow this part. Does "as we go along" mean "as we go along
> freeing things in update_submodules()", or "we'll do this later on"?

You know what? After looking at this again I have no idea what I was
talking about here, and I think this makes no sense... :)

> I'm assuming it's the latter since this patch only frees the "last" one
> and doesn't free inside of update_submodule(), but maybe it's not so
> hard to do? I think it's just:
>
>   diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>   index 0bac39880d..34b54e97d1 100644
>   --- a/builtin/submodule--helper.c
>   +++ b/builtin/submodule--helper.c
>   @@ -2560,6 +2560,7 @@ static int update_submodule(struct update_data *update_data)
>   {
>     ensure_core_worktree(update_data->sm_path);
>
>   +	free(update_data->displaypath);
>     update_data->displaypath = get_submodule_displaypath(
>       update_data->sm_path, update_data->prefix);

Thanks, I'll fix this case in a re-roll, fixing it generally turned out
to be a lot trickier though, but in the process I fixed a lot of the
control flow issues. Am currently running extensive tests on it.

Thanks a lot for this & other reviews, it's been really useful, as
always.