Jacob Keller <jacob.e.keller@xxxxxxxxx> writes:
> From: Jacob Keller <jacob.keller@xxxxxxxxx>
>
> 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.
