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

[Jacob Keller <jacob.e.keller@intel.com>]

On 6/17/2025 4:12 PM, Junio C Hamano wrote:
> 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".
> 

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.