Re: [PATCH] submodule: resolve insteadof-aliases when matching remote

Éric NICOLAS <[email protected]> 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.

> 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 <[email protected]>
> ---
>  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.