Re: [PATCH v7 16/17] submodule--helper: free rest of "displaypath" in "struct update_data"

From: Glen Choo <chooglen@google.com>
Date: Sep 1, 2022, 21:20 UTC
Message-ID: <kl6ltu5qbxma.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <patch-v7-16.17-63ff554e306-20220831T231003Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Show 7 quoted lines

> A better fix for this would be to remove the "displaypath" member from
> the "struct update_data" entirely. Along with "oid", "suboid",
> "just_cloned" and "sm_path" it's managing members that mainly need to
> be passed between 1-3 stack frames of functions adjacent to this
> code. But doing so would be a much larger change (I have it locally,
> and fully untangling that in an incremental way is a 10 patch
> journey).

Yeah that does sound like a better fix _and_ too much churn right now.

Show 8 quoted lines

> So let's go for this much more isolated fix suggested by Glen. We
> FREE_AND_NULL() the "update_data->displaypath", the "AND_NULL()" part
> of that is needed due to the later "free(ud->displaypath)" in
> "update_data_release()" introduced in the preceding commit
>
> Moving ensure_core_worktree() out of update_submodule() may not be
> strictly required, but in doing so we are left with the exact same
> ordering as before, making this a smaller functional change.

Ok, it's nice that we don't have to reason about functional changes, though I doubt it will matter in this case.

I worry a bit about whether this is setting up an implicit contract where we always need to call ensure_core_worktree() before update_submodule(), but the contract has always been quite fuzzy here, e.g. the "struct update_data" at this point contains a mix of CLI args + clone result; we should fix that at some point, so doing this change in the meantime seems harmless.

Show 40 quoted lines

> Helped-by: Glen Choo <chooglen@google.com>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/submodule--helper.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 94ebd8ea38e..1650bf0070b 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2487,13 +2487,6 @@ static int update_submodule(struct update_data *update_data)
>  {
>  	int ret;
>  
> -	ret = ensure_core_worktree(update_data->sm_path);
> -	if (ret)
> -		return ret;
> -
> -	update_data->displaypath = get_submodule_displaypath(
> -		update_data->sm_path, update_data->prefix);
> -
>  	ret = determine_submodule_update_strategy(the_repository,
>  						  update_data->just_cloned,
>  						  update_data->sm_path,
> @@ -2599,7 +2592,15 @@ static int update_submodules(struct update_data *update_data)
>  		update_data->just_cloned = ucd.just_cloned;
>  		update_data->sm_path = ucd.sub->path;
>  
> +		code = ensure_core_worktree(update_data->sm_path);
> +		if (code)
> +			goto fail;
> +
> +		update_data->displaypath = get_submodule_displaypath(
> +			update_data->sm_path, update_data->prefix);
>  		code = update_submodule(update_data);
> +		FREE_AND_NULL(update_data->displaypath);
> +fail:
>  		if (!code)
>  			continue;
>  		ret = code;

Looks good.

> -- 
> 2.37.3.1420.g76f8a3d556c