Re: [PATCH v3 31/32] submodule--helper: libify even more "die" paths for module_update()

[Glen Choo <chooglen@google.com>]

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

> As noted in a preceding commit the get_default_remote_submodule() and
> remote_submodule_branch() functions would invoke die(), and thus leave
> update_submodule() only partially lib-ified. We've addressed the
> former of those in a preceding commit, let's now address the latter.
>
> In addition to lib-ifying the function this fixes a potential (but
> obscure) segfault introduced by a logic error in
> 1012a5cbc3f (submodule--helper run-update-procedure: learn --remote,
> 2022-03-04):

Ah, good catch.

>
> We were assuming that remote_submodule_branch() would always return
> on-NULL, but if the submodule_from_path() call in that function fails

s/on-NULL/non-NULL I assume?

> we'll return NULL. See its introduction in
> 92bbe7ccf1f (submodule--helper: add remote-branch helper, 2016-08-03).
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/submodule--helper.c | 41 ++++++++++++++++++++++---------------
>  1 file changed, 25 insertions(+), 16 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 9e4069b36cb..65909255760 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2267,42 +2267,49 @@ static int run_update_procedure(struct update_data *ud)
>  	return run_update_command(ud, subforce);
>  }
>  
> -static const char *remote_submodule_branch(const char *path)
> +static int remote_submodule_branch(const char *path, const char **branch)
>  {
>  	const struct submodule *sub;
> -	const char *branch = NULL;
>  	char *key;
> +	*branch = NULL;
>  
>  	sub = submodule_from_path(the_repository, null_oid(), path);
>  	if (!sub)
> -		return NULL;
> +		return die_message(_("could not initialize submodule at path '%s'"),
> +				   path);
>  
>  	key = xstrfmt("submodule.%s.branch", sub->name);
> -	if (repo_config_get_string_tmp(the_repository, key, &branch))
> -		branch = sub->branch;
> +	if (repo_config_get_string_tmp(the_repository, key, branch))
> +		*branch = sub->branch;
>  	free(key);
>  
> -	if (!branch)
> -		return "HEAD";
> +	if (!*branch) {
> +		*branch = "HEAD";
> +		return 0;
> +	}
>  
> -	if (!strcmp(branch, ".")) {
> +	if (!strcmp(*branch, ".")) {
>  		const char *refname = resolve_ref_unsafe("HEAD", 0, NULL, NULL);
>  
>  		if (!refname)
> -			die(_("No such ref: %s"), "HEAD");
> +			return die_message(_("No such ref: %s"), "HEAD");
>  
>  		/* detached HEAD */
>  		if (!strcmp(refname, "HEAD"))
> -			die(_("Submodule (%s) branch configured to inherit "
> -			      "branch from superproject, but the superproject "
> -			      "is not on any branch"), sub->name);
> +			return die_message(_("Submodule (%s) branch configured to inherit "
> +					     "branch from superproject, but the superproject "
> +					     "is not on any branch"), sub->name);
>  
>  		if (!skip_prefix(refname, "refs/heads/", &refname))
> -			die(_("Expecting a full ref name, got %s"), refname);
> -		return refname;
> +			return die_message(_("Expecting a full ref name, got %s"),
> +					   refname);
> +
> +		*branch = refname;
> +		return 0;
>  	}
>  
> -	return branch;
> +	/* Our "branch" is coming from repo_config_get_string_tmp() */
> +	return 0;
>  }
>  
>  static int ensure_core_worktree(const char *path)
> @@ -2437,7 +2444,9 @@ static int update_submodule(struct update_data *update_data)
>  		code = get_default_remote_submodule(update_data->sm_path, &remote_name);
>  		if (code)
>  			return code;
> -		branch = remote_submodule_branch(update_data->sm_path);
> +		code = remote_submodule_branch(update_data->sm_path, &branch);
> +		if (code)
> +			return code;
>  		remote_ref = xstrfmt("refs/remotes/%s/%s", remote_name, branch);
This line is the one that would segfault, I assume?

Looks good.
>  
>  		if (!update_data->nofetch) {
> -- 
> 2.37.2.1279.g64dec4e13cf