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

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

[Éric NICOLAS]

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.

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,
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 <ccjmne@gmail.com>
---
 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;
 
 	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;
+		}
 	}
-	return NULL;
+	free(rewritten_url);
+	return remote_name;
 }
 
 int branch_has_merge_config(struct branch *branch)
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 9554720152..84e2cbbef9 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -256,6 +256,27 @@ test_expect_success 'submodule update --remote should fetch upstream changes' '
 	)
 '
 
+test_expect_success 'submodule update --remote resolves URL rewrites' '
+	test_config_global "url.$(pwd)/.insteadOf" local: &&
+	mkdir aliased-super aliased-submodule &&
+	(
+		cd aliased-submodule &&
+		git init &&
+		echo line >file &&
+		git add file &&
+		git commit -m "Initial commit"
+	) &&
+	(
+		cd aliased-super &&
+		git init &&
+		git submodule add local:aliased-submodule submodule &&
+		git submodule update --force submodule &&
+		git -C submodule remote rename origin upstream &&
+		git -C submodule remote add fork user@host &&
+		git submodule update --remote submodule
+	)
+'
+
 test_expect_success 'submodule update --remote should fetch upstream changes with .' '
 	(
 		cd super &&
-- 
2.55.0

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

[Junio C Hamano]

Éric NICOLAS <ccjmne@gmail.com> 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 <ccjmne@gmail.com>
> ---
>  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.

[PATCH v2] submodule: resolve insteadOf aliases when matching remote

[Éric NICOLAS]

When ca62f524c1 (submodule: look up remotes by URL first, 2025-06-23)
introduced a mechanism to identify which remote is to be used by a
submodule, it compared the URL stored in the .gitmodules inventory to
that of each available remote.

The URLs of remotes are rewritten according to url.<base>.insteadOf,
whereas those stored in the .gitmodules aren't.  When such aliasing
applies, no match can be made between the two corresponding sides, and
the procedure degrades to its fallback logic electing either the only
configured remote if there is only one, or "origin" otherwise.

That behaviour is unfortunate when no remote is called "origin",
because its last resort will have a submodule update command look for a
non-existent remote-tracking reference and fail to proceed, instead of
using the remote whose rewritten URL matches.

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 <ccjmne@gmail.com>
---
Thank you for your guidance.

Changes in v2:

- Reword the commit message more purposefully
- Adjust the implementation as suggested, avoiding a superfluous
  variable
- Tidy up the integration test

 remote.c                    | 14 +++++++++++---
 t/t7406-submodule-update.sh | 19 +++++++++++++++++++
 2 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/remote.c b/remote.c
index b17648d6ef..b1fed58e79 100644
--- a/remote.c
+++ b/remote.c
@@ -1821,17 +1821,25 @@ 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 *remote_name = NULL;
+
 	read_config(repo, 0);
+	if ((rewritten_url = alias_url(url, &repo->remote_state->rewrites)))
+		url = rewritten_url;
 
 	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)) {
+			remote_name = remote->name;
+			break;
+		}
 	}
-	return NULL;
+	free(rewritten_url);
+	return remote_name;
 }
 
 int branch_has_merge_config(struct branch *branch)
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 9554720152..10adeabf0f 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -256,6 +256,25 @@ test_expect_success 'submodule update --remote should fetch upstream changes' '
 	)
 '
 
+test_expect_success 'submodule update --remote resolves URL rewrites' '
+	test_config_global "url.$(pwd)/.insteadOf" local: &&
+	mkdir alias-super alias-submodule &&
+	(
+		cd alias-submodule &&
+		git init &&
+		git commit --allow-empty --message "Initial commit"
+	) &&
+	(
+		cd alias-super &&
+		git init &&
+		git submodule add local:alias-submodule submodule &&
+		git submodule update --force &&
+		git -C submodule remote rename origin upstream &&
+		git -C submodule remote add fork user@host &&
+		git submodule update --remote
+	)
+'
+
 test_expect_success 'submodule update --remote should fetch upstream changes with .' '
 	(
 		cd super &&

Range-diff against v1:
1:  ed507998b3 ! 1:  4363eb3cb1 submodule: resolve insteadof-aliases when matching remote
    @@ Metadata
     Author: Éric NICOLAS <ccjmne@gmail.com>
     
      ## Commit message ##
    -    submodule: resolve insteadof-aliases when matching remote
    +    submodule: resolve insteadOf aliases when matching remote
     
    -    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.
    +    When ca62f524c1 (submodule: look up remotes by URL first, 2025-06-23)
    +    introduced a mechanism to identify which remote is to be used by a
    +    submodule, it compared the URL stored in the .gitmodules inventory to
    +    that of each available remote.
     
    -    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,
    -    and resorts to the fallback logic, which does use either the only
    -    configured remote if there is only one, or attempts using "origin"
    -    otherwise.
    +    The URLs of remotes are rewritten according to url.<base>.insteadOf,
    +    whereas those stored in the .gitmodules aren't.  When such aliasing
    +    applies, no match can be made between the two corresponding sides, and
    +    the procedure degrades to its fallback logic electing either the only
    +    configured remote if there is only one, or "origin" otherwise.
    +
    +    That behaviour is unfortunate when no remote is called "origin",
    +    because its last resort will have a submodule update command look for a
    +    non-existent remote-tracking reference and fail to proceed, instead of
    +    using the remote whose rewritten URL matches.
     
         Resolve the alias in the URL inventoried in .gitmodules before comparing
         it against those of the corresponding submodule's configured remotes.
    @@ remote.c: 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;
    ++	if ((rewritten_url = alias_url(url, &repo->remote_state->rewrites)))
    ++		url = rewritten_url;
      
      	for (int i = 0; i < repo->remote_state->remotes_nr; i++) {
      		struct remote *remote = repo->remote_state->remotes[i];
    @@ remote.c: const char *repo_default_remote(struct repository *repo)
      
     -		if (remote_has_url(remote, url))
     -			return remote->name;
    -+		if (remote_has_url(remote, url_to_match)) {
    ++		if (remote_has_url(remote, url)) {
     +			remote_name = remote->name;
     +			break;
     +		}
    @@ t/t7406-submodule-update.sh: test_expect_success 'submodule update --remote shou
      
     +test_expect_success 'submodule update --remote resolves URL rewrites' '
     +	test_config_global "url.$(pwd)/.insteadOf" local: &&
    -+	mkdir aliased-super aliased-submodule &&
    ++	mkdir alias-super alias-submodule &&
     +	(
    -+		cd aliased-submodule &&
    ++		cd alias-submodule &&
     +		git init &&
    -+		echo line >file &&
    -+		git add file &&
    -+		git commit -m "Initial commit"
    ++		git commit --allow-empty --message "Initial commit"
     +	) &&
     +	(
    -+		cd aliased-super &&
    ++		cd alias-super &&
     +		git init &&
    -+		git submodule add local:aliased-submodule submodule &&
    -+		git submodule update --force submodule &&
    ++		git submodule add local:alias-submodule submodule &&
    ++		git submodule update --force &&
     +		git -C submodule remote rename origin upstream &&
     +		git -C submodule remote add fork user@host &&
    -+		git submodule update --remote submodule
    ++		git submodule update --remote
     +	)
     +'
     +
-- 
2.55.0