On Thu, Jul 14 2022, Glen Choo wrote:
> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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.
