Re: [PATCH v2 4/6] submodule--helper: improve logic for fallback remote name

[Junio C Hamano <gitster@pobox.com>]

Jacob Keller <jacob.e.keller@intel.com> writes:

> From: Jacob Keller <jacob.keller@gmail.com>
>
> The repo_get_default_remote() function in submodule--helper currently
> tries to figure out the proper remote name to use for a submodule based
> on a few factors.
>
> First, it tries to find the remote for the currently checked out branch.
> This works if the submodule is configured to checkout to a branch
> instead of a detached HEAD state.
>
> In the detached HEAD state, the code calls back to using "origin", on

"calls back" -> "falls back".

> the assumption that this is the default remote name. Some users may
> change this, such as by setting clone.defaultRemoteName, or by changing
> the remote name manually within the submodule repository.
>
> As a first step to improving this situation, refactor to reuse the logic
> from remotes_remote_for_branch(). This function uses the remote from the
> branch if it has one. If it doesn't then it checks to see if there is
> exactly one remote. It uses this remote first before attempting to fall
> back to "origin".

Designed wtih good taste.

> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 9e8cdfe1b2a8c2985d9c1b8ad6f1b0d1f9401714..4aa237033a526fca29cce2926419462179d40ee3 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -41,61 +41,25 @@
>  typedef void (*each_submodule_fn)(const struct cache_entry *list_item,
>  				  void *cb_data);
>  
> -static int repo_get_default_remote(struct repository *repo, char **default_remote)
> -{
> -	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)
> -		return die_message(_("No such ref: %s"), "HEAD");
> -
> -	/* detached HEAD */
> -	if (!strcmp(refname, "HEAD")) {
> -		*default_remote = xstrdup("origin");
> -		return 0;
> -	}
> -
> -	if (!skip_prefix(refname, "refs/heads/", &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))
> -		*default_remote = xstrdup("origin");
> -	else
> -		*default_remote = dest;
> -
> -	strbuf_release(&sb);
> -	return 0;
> -}

We will lose two callers of this function, so we can safely remove
it.

>  static int get_default_remote_submodule(const char *module_path, char **default_remote)
>  static char *get_default_remote(void)

These callers that used to call the removed helper now call
repo_default_remote() instread.  Good.


> diff --git a/remote.c b/remote.c
> index b3a9881a6eacf90bee71d6760858b37d68263502..94b31f4c23057a247a968fc0ebe2e5170e99614d 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1767,20 +1767,35 @@ static void set_merge(struct repository *repo, struct branch *ret)
>  	}
>  }
>  
> -struct branch *branch_get(const char *name)
> +static struct branch *repo_branch_get(struct repository *repo, const char *name)
>  {
>  	struct branch *ret;
>  
> -	read_config(the_repository, 0);
> +	read_config(repo, 0);
>  	if (!name || !*name || !strcmp(name, "HEAD"))
> -		ret = the_repository->remote_state->current_branch;
> +		ret = repo->remote_state->current_branch;
>  	else
> -		ret = make_branch(the_repository->remote_state, name,
> +		ret = make_branch(repo->remote_state, name,
>  				  strlen(name));
> -	set_merge(the_repository, ret);
> +	set_merge(repo, ret);
>  	return ret;
>  }
>  
> +struct branch *branch_get(const char *name)
> +{
> +	return repo_branch_get(the_repository, name);
> +}

Nice to see how the dependency to the_repository is lifted for new
callers while retaining the same interface for existing ones.

> +const char *repo_default_remote(struct repository *repo)
> +{
> +	struct branch *branch;
> +
> +	read_config(repo, 0);
> +	branch = repo_branch_get(repo, "HEAD");
> +
> +	return remotes_remote_for_branch(repo->remote_state, branch, NULL);
> +}

OK.  read_config() is a safe no-op if repo has already been
initialized, so this would give us what we want.  Nicely done.