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

From: Jacob Keller <[email-masked]>
Date: Tue, 17 Jun 2025 10:53:10 -0700

On 6/16/2025 7:58 PM, Lidong Yan wrote:
> Jacob Keller <[email-masked]> writes：
>>
>> From: Jacob Keller <[email-masked]>
>>
>> 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
>> 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".
>>
>> To allow using this helper function, introduce a repo_default_remote()
>> helper to remote.c which takes a repository structure. This helper will
>> load the remote configuration and get the "HEAD" branch. Then it will
>> call remotes_remote_for_branch to find the default remote.
> 
> Just a thought: since repo_default_remote() is only used within
> repo_get_default_remote(), and the two have very similar names,
> do you think it might be clearer to inline the former into the latter?
> 
Probably, but I can't inline it within submodule--helper.c because it
needs access to static functions in remote.c which I don't think make
sense to export.

I could also either move repo_get_default_remote() from
submodule--helper.c into remote.c, or try to eliminate it entirely.

My original thought was that it does some submodule-specific checks
which I don't want to remove but which don't seem to make sense
in-context with how the other remote.c helpers were implemented. Thus,
it didn't make sense to move it into remote.c, but I can't put
repo_default_remote() contents into submodule--helper.c without exposing
the read_config() and other static functions in remote.c

I'll think about this while resolving the memory issues and working on
the next version.