Re: [PATCH 09/11] submodule--helper: free "char *" in "struct update_data"
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Jul 18, 2022, 16:10 UTC
- Message-ID
- <220718.86v8ruwgc6.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <kl6ltu7j4k98.fsf@chooglen-macbookpro.roam.corp.google.com>
On Thu, Jul 14 2022, Glen Choo wrote:
Show 9 quoted lines
> Æ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!
Show 11 quoted lines
>> 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... :)
Show 15 quoted lines
> 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.