Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
> 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.
> 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.
> Helped-by: Glen Choo <chooglen@xxxxxxxxxx>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
> ---
> 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
