On 6/17/2025 4:12 PM, Junio C Hamano wrote:
> 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".
>
Yep.
>> 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.
>
Yep, we're removing both callers.
>> 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.
>
Yea, I figured there's little reason to go poke all the existing callers
right away. Those can be updated if/when they have a repository
parameter passed in.
>> +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.
>
>
Yep, this matches how most of the other remote logic works.
