Re: [PATCH] submodule: resolve insteadof-aliases when matching remote
From: Junio C Hamano <hidden>
Date: 2026-07-22 19:49:13
Éric NICOLAS [off-list ref] writes:
When ca62f524c1 introduced a mechanism to identify which remote is to be used by a submodule, we had it compare the URL stored in the .gitmodules inventory to those of each available remote.
Please refer to an existing commit using this format:
When ca62f524c1 (submodule: look up remotes by URL first,
2025-06-23) introduced ...
However, when using URL aliasing via url.<base>.insteadOf, we store in .gitmodules the URL pre-resolution of the alias, whereas the corresponding remote set up in the submodule reports using the *resolved* URL. This mechanism therefore fails to find a match then,
Since anything involving the .gitmodules file is often security- sensitive, it is always a good idea to go beyond just saying 'X fails to do Y.' We should also explain why that failure is a bad thing (or perhaps a good thing) and for what reason. If this aliasing were controlled by a remote entity (for example, if an upstream project modified the .gitmodules file to redirect us somewhere unexpected), failing to find a match could actually be a safety feature, shielding us from bad actors trying to hijack the local repository. Since that is not the case here, adding 'fails to find a match, which is unfortunate because...' would make the commit message much stronger.
quoted hunk ↗ jump to hunk
and resorts to the fallback logic, which does use either the only configured remote if there is only one, or attempts using "origin" otherwise. Resolve the alias in the URL inventoried in .gitmodules before comparing it against those of the corresponding submodule's configured remotes. Signed-off-by: Éric NICOLAS <redacted> --- remote.c | 15 ++++++++++++--- t/t7406-submodule-update.sh | 21 +++++++++++++++++++++ 2 files changed, 33 insertions(+), 3 deletions(-)diff --git a/remote.c b/remote.c index b17648d6ef..ae187fb3d6 100644 --- a/remote.c +++ b/remote.c@@ -1821,17 +1821,26 @@ const char *repo_default_remote(struct repository *repo) const char *repo_remote_from_url(struct repository *repo, const char *url) { + char *rewritten_url; + const char *url_to_match; + const char *remote_name = NULL; + read_config(repo, 0); + rewritten_url = alias_url(url, &repo->remote_state->rewrites); + url_to_match = rewritten_url ? rewritten_url : url;
Being a bit lazy, I probably would have just reused 'url' directly: if ((rewritten_url = alias_url(url, &repo->remote_state->rewrites))) url = rewritten_url; This lets us avoid introducing a brand-new 'url_to_match' variable, whose lifetime is essentially just taking over for 'url' anyway.
for (int i = 0; i < repo->remote_state->remotes_nr; i++) {
struct remote *remote = repo->remote_state->remotes[i];
if (!remote)
continue;
- if (remote_has_url(remote, url))
- return remote->name;
+ if (remote_has_url(remote, url_to_match)) {
+ remote_name = remote->name;
+ break;
+ }While the new code preserves the original 'first one wins' behavior, it does make me wonder why we do not issue a warning or raise an error when multiple URLs match. Leaving such an ambiguous configuration unflagged feels like a silent bug waiting to happen. But it is of course outside the scope of this topic.
} - return NULL; + free(rewritten_url); + return remote_name; }
Thanks.