Re: [PATCH v3 30/32] submodule--helper: libify 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. Let's address the former
> of those cases.
>
> Change the functions to return an int exit code (non-zero on failure),
> while leaving the get_default_remote() function for the callers that
> still want the die() semantics.
>
> This change addresses 1/2 of the "die" issue in these two lines in
> update_submodule():
>
> 	char *remote_name = get_default_remote_submodule(update_data->sm_path);
> 	const char *branch = remote_submodule_branch(update_data->sm_path);
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/submodule--helper.c | 58 +++++++++++++++++++++++--------------
>  1 file changed, 37 insertions(+), 21 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 88fc01320f3..9e4069b36cb 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -31,48 +31,57 @@
>  typedef void (*each_submodule_fn)(const struct cache_entry *list_item,
>  				  void *cb_data);
>  
> -static char *repo_get_default_remote(struct repository *repo)
> +static int repo_get_default_remote(struct repository *repo, char **default_remote)
>  {
> -	char *dest = NULL, *ret;
> +	char *dest = NULL;
>  	struct strbuf sb = STRBUF_INIT;
>  	struct ref_store *store = get_main_ref_store(repo);
>  	const char *refname = refs_resolve_ref_unsafe(store, "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"))
> -		return xstrdup("origin");
> +	if (!strcmp(refname, "HEAD")) {
> +		*default_remote = xstrdup("origin");
> +		return 0;
> +	}
>  
>  	if (!skip_prefix(refname, "refs/heads/", &refname))
> -		die(_("Expecting a full ref name, got %s"), refname);
> +		return die_message(_("Expecting a full ref name, got %s"),
> +				   refname);
>  
>  	strbuf_addf(&sb, "branch.%s.remote", refname);
>  	if (repo_config_get_string(repo, sb.buf, &dest))
> -		ret = xstrdup("origin");
> +		*default_remote = xstrdup("origin");
>  	else
> -		ret = dest;
> +		*default_remote = dest;
>  
>  	strbuf_release(&sb);
> -	return ret;
> +	return 0;
>  }

If we fail to look up the default remote for the branch, we assume it is
"origin", otherwise, it is the default remote. So in the preimage this
function would never return NULL, right? This matters in
sync_submodule()..

>  
> -static char *get_default_remote_submodule(const char *module_path)
> +static int get_default_remote_submodule(const char *module_path, char **default_remote)
>  {
>  	struct repository subrepo;
>  
>  	if (repo_submodule_init(&subrepo, the_repository, module_path,
>  				null_oid()) < 0)
> -		die(_("could not get a repository handle for submodule '%s'"),
> -		    module_path);
> -	return repo_get_default_remote(&subrepo);
> +		return die_message(_("could not get a repository handle for submodule '%s'"),
> +				   module_path);
> +	return repo_get_default_remote(&subrepo, default_remote);
>  }
>  
>  static char *get_default_remote(void)
>  {
> -	return repo_get_default_remote(the_repository);
> +	char *default_remote;
> +	int code = repo_get_default_remote(the_repository, &default_remote);
> +
> +	if (code)
> +		exit(code);
> +
> +	return default_remote;
>  }

repo_* usually denotes a difference between "the_repository" and "struct
repository", so I don't really like having [repo_]get_default_remote()
die/return differently. But it makes some sense to special case
"the_repository" since it is the top level, so if it fails, something is
very wrong. So on the whole, I'm ok with doing this to avoid disrupting
other callers.

> @@ -1159,6 +1168,7 @@ static void sync_submodule(const char *path, const char *prefix,
>  	char *sub_origin_url, *super_config_url, *displaypath, *default_remote;
>  	struct strbuf sb = STRBUF_INIT;
>  	char *sub_config_path = NULL;
> +	int code;
>  
>  	if (!is_submodule_active(the_repository, path))
>  		return;
> @@ -1198,10 +1208,9 @@ static void sync_submodule(const char *path, const char *prefix,
>  		goto cleanup;
>  
>  	strbuf_reset(&sb);
> -	default_remote = get_default_remote_submodule(path);
> -	if (!default_remote)
> -		die(_("failed to get the default remote for submodule '%s'"),
> -		      path);
> +	code = get_default_remote_submodule(path, &default_remote);
> +	if (code)
> +		exit(code);

In the preimage, we used to check that default_remote is not NULL, but
we don't check this any more. As we noted earlier, default_remote could
never be NULL (because we used to die() on failure), so I think we never
needed this check and it's ok to drop it.

Is this the same reasoning you had?