git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* bug: submodule update fails to fetch
@ 2023-06-22 11:09 Sergei Golubchik
  2023-06-22 13:07 ` Taylor Blau
  2024-10-01  7:24 ` [PATCH] submodule: correct remote name with fetch Daniel Black
  0 siblings, 2 replies; 12+ messages in thread
From: Sergei Golubchik @ 2023-06-22 11:09 UTC (permalink / raw)
  To: git

Hi,

Sometimes (my local repository has lots of branches) after switching
branches

  git submodule update --init --recursive

fails with something like

  fatal: transport 'file' not allowed
  fatal: Fetched in submodule path 'wsrep-lib', but it did not contain e238c0d240c2557229b0523a4a032f3cf8b41639. Direct fetching of that commit failed.

the submodule transport is not 'file' (it's https) and the direct
fetching of the commit actually works:

  cd wsrep-lib
  git fetch origin e238c0d240c2557229b0523a4a032f3cf8b41639
  git checkout e238c0d240c2557229b0523a4a032f3cf8b41639
  cd ..

after that

  git submodule update --init --recursive

succeeds. This happens deterministically, but depends on the old and new
commits in the last checkout. As a workaround we've had to change our CI to do

  git submodule foreach --recursive 'git fetch origin $sha1;git checkout --force FETCH_HEAD'

This is the bit from `git bugreport`:

[System Info]
git version:
git version 2.39.3
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
uname: Linux 5.15.88-gentoo #1 SMP Wed Feb 15 16:42:45 CET 2023 x86_64
compiler info: gnuc: 11.3
libc info: glibc: 2.36
$SHELL (typically, interactive shell): /bin/bash

[Enabled Hooks]

Regards,
Sergei

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: bug: submodule update fails to fetch
  2023-06-22 11:09 bug: submodule update fails to fetch Sergei Golubchik
@ 2023-06-22 13:07 ` Taylor Blau
  2023-06-22 16:39   ` Sergei Golubchik
  2024-10-01  7:24 ` [PATCH] submodule: correct remote name with fetch Daniel Black
  1 sibling, 1 reply; 12+ messages in thread
From: Taylor Blau @ 2023-06-22 13:07 UTC (permalink / raw)
  To: Sergei Golubchik; +Cc: git

On Thu, Jun 22, 2023 at 01:09:07PM +0200, Sergei Golubchik wrote:
> Hi,
>
> Sometimes (my local repository has lots of branches) after switching
> branches
>
>   git submodule update --init --recursive
>
> fails with something like
>
>   fatal: transport 'file' not allowed
>   fatal: Fetched in submodule path 'wsrep-lib', but it did not contain e238c0d240c2557229b0523a4a032f3cf8b41639. Direct fetching of that commit failed.
>
> the submodule transport is not 'file' (it's https) and the direct
> fetching of the commit actually works:
>
>   cd wsrep-lib
>   git fetch origin e238c0d240c2557229b0523a4a032f3cf8b41639
>   git checkout e238c0d240c2557229b0523a4a032f3cf8b41639
>   cd ..
>
> after that
>
>   git submodule update --init --recursive
>
> succeeds. This happens deterministically, but depends on the old and new
> commits in the last checkout. As a workaround we've had to change our CI to do

It makes sense that after manually fetching the desired tip that the
submodule update goes through OK, because there is nothing to do (the
checked-out state matches what's in .gitmodules), so we don't have to
use any transport mechanism.

I recently changed the submodule update rules to disallow file-based
submodules when not directly executed by the user. See a1d4f67c12
(transport: make `protocol.file.allow` be "user" by default, 2022-07-29)
for more of the details there.

So in the short term, you should able to work around what you're seeing
by setting `protocol.file.allow` to "always" with something like

    $ git config --global protocol.file.allow always

But in the short-term, I am curious why we are complaining about needing
to use the file transport when you claim that the submodule actually
needs the HTTPS transport.

Are you able to share a copy of your repository, and/or its .gitmodules
file, and your repository-local .gitconfig, as well? Do you have some
`url.<base>.insteadOf` value configured elsewhere that would be
rewriting those paths for you?

Thanks,
Taylor

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: bug: submodule update fails to fetch
  2023-06-22 13:07 ` Taylor Blau
@ 2023-06-22 16:39   ` Sergei Golubchik
  2023-06-23  7:26     ` Jacob Keller
  0 siblings, 1 reply; 12+ messages in thread
From: Sergei Golubchik @ 2023-06-22 16:39 UTC (permalink / raw)
  To: Taylor Blau; +Cc: git

Hi, Taylor,

On Jun 22, Taylor Blau wrote:
> On Thu, Jun 22, 2023 at 01:09:07PM +0200, Sergei Golubchik wrote:
> > Hi,
> >
> > Sometimes (my local repository has lots of branches) after switching
> > branches
> >
> >   git submodule update --init --recursive
> >
> > fails with something like
> >
> >   fatal: transport 'file' not allowed
> >   fatal: Fetched in submodule path 'wsrep-lib', but it did not contain e238c0d240c2557229b0523a4a032f3cf8b41639. Direct fetching of that commit failed.
> >
> > the submodule transport is not 'file' (it's https) and the direct
> > fetching of the commit actually works:
> >
> >   cd wsrep-lib
> >   git fetch origin e238c0d240c2557229b0523a4a032f3cf8b41639
> >   git checkout e238c0d240c2557229b0523a4a032f3cf8b41639
> >   cd ..
> >
> > after that
> >
> >   git submodule update --init --recursive
> >
> > succeeds.
> 
> It makes sense that after manually fetching the desired tip that the
> submodule update goes through OK, because there is nothing to do (the
> checked-out state matches what's in .gitmodules), so we don't have to
> use any transport mechanism.

Right. I just used it to show that git thinks the submodule was updated
correctly and doesn't try to do anything after that.

> I recently changed the submodule update rules to disallow file-based
> submodules when not directly executed by the user. See a1d4f67c12
> (transport: make `protocol.file.allow` be "user" by default, 2022-07-29)
> for more of the details there.

Yes, I've seen it. That submodule shouldn't be affected:

  $ git remote -v
  origin  https://github.com/codership/wsrep-lib.git (fetch)
  origin  https://github.com/codership/wsrep-lib.git (push)

so I wouldn't want to circumvent your fix and allow the file transport
that we aren't using.

> But in the short-term, I am curious why we are complaining about needing
> to use the file transport when you claim that the submodule actually
> needs the HTTPS transport.
> 
> Are you able to share a copy of your repository, and/or its .gitmodules
> file, and your repository-local .gitconfig, as well? Do you have some
> `url.<base>.insteadOf` value configured elsewhere that would be
> rewriting those paths for you?

No insteadOf. Let me try to...
Okay, here's the bug. In submodule--helper.c, fetch_in_submodule()
function, there're lines:
---------------------------
2211         if (oid) {
2212                 char *hex = oid_to_hex(oid);
2213                 char *remote = get_default_remote();
2214
2215                 strvec_pushl(&cp.args, remote, hex, NULL);
2216                 free(remote);
2217         }
---------------------------

this get_default_remote() appears to be getting the default remote name
for the main repository and then uses it to fetch from the submodule.

It happens that my default remote isn't "origin" (long story), it's
"github", but in the submodule it's of course "origin", there's no
"github" remote there. As a result, `git submodule update` runs the
command

  git fetch github ${commit_hash}

in the submodule, and that's interpreted as 'file' transport.

To repeat this you need a repository where the default remote isn't
"origin" and a submodule where the commit cannot be fetched by simply
`git fetch` and needs a direct fetch.

Hope this helps.

Regards,
Sergei

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: bug: submodule update fails to fetch
  2023-06-22 16:39   ` Sergei Golubchik
@ 2023-06-23  7:26     ` Jacob Keller
  0 siblings, 0 replies; 12+ messages in thread
From: Jacob Keller @ 2023-06-23  7:26 UTC (permalink / raw)
  To: Sergei Golubchik; +Cc: Taylor Blau, git

On Thu, Jun 22, 2023 at 9:57 AM Sergei Golubchik <vuvova@gmail.com> wrote:
>
> Hi, Taylor,
>
> On Jun 22, Taylor Blau wrote:
> > On Thu, Jun 22, 2023 at 01:09:07PM +0200, Sergei Golubchik wrote:
> > > Hi,
> > >
> > > Sometimes (my local repository has lots of branches) after switching
> > > branches
> > >
> > >   git submodule update --init --recursive
> > >
> > > fails with something like
> > >
> > >   fatal: transport 'file' not allowed
> > >   fatal: Fetched in submodule path 'wsrep-lib', but it did not contain e238c0d240c2557229b0523a4a032f3cf8b41639. Direct fetching of that commit failed.
> > >
> > > the submodule transport is not 'file' (it's https) and the direct
> > > fetching of the commit actually works:
> > >
> > >   cd wsrep-lib
> > >   git fetch origin e238c0d240c2557229b0523a4a032f3cf8b41639
> > >   git checkout e238c0d240c2557229b0523a4a032f3cf8b41639
> > >   cd ..
> > >
> > > after that
> > >
> > >   git submodule update --init --recursive
> > >
> > > succeeds.
> >
> > It makes sense that after manually fetching the desired tip that the
> > submodule update goes through OK, because there is nothing to do (the
> > checked-out state matches what's in .gitmodules), so we don't have to
> > use any transport mechanism.
>
> Right. I just used it to show that git thinks the submodule was updated
> correctly and doesn't try to do anything after that.
>
> > I recently changed the submodule update rules to disallow file-based
> > submodules when not directly executed by the user. See a1d4f67c12
> > (transport: make `protocol.file.allow` be "user" by default, 2022-07-29)
> > for more of the details there.
>
> Yes, I've seen it. That submodule shouldn't be affected:
>
>   $ git remote -v
>   origin  https://github.com/codership/wsrep-lib.git (fetch)
>   origin  https://github.com/codership/wsrep-lib.git (push)
>
> so I wouldn't want to circumvent your fix and allow the file transport
> that we aren't using.
>
> > But in the short-term, I am curious why we are complaining about needing
> > to use the file transport when you claim that the submodule actually
> > needs the HTTPS transport.
> >
> > Are you able to share a copy of your repository, and/or its .gitmodules
> > file, and your repository-local .gitconfig, as well? Do you have some
> > `url.<base>.insteadOf` value configured elsewhere that would be
> > rewriting those paths for you?
>
> No insteadOf. Let me try to...
> Okay, here's the bug. In submodule--helper.c, fetch_in_submodule()
> function, there're lines:
> ---------------------------
> 2211         if (oid) {
> 2212                 char *hex = oid_to_hex(oid);
> 2213                 char *remote = get_default_remote();
> 2214
> 2215                 strvec_pushl(&cp.args, remote, hex, NULL);
> 2216                 free(remote);
> 2217         }
> ---------------------------
>
> this get_default_remote() appears to be getting the default remote name
> for the main repository and then uses it to fetch from the submodule.
>
> It happens that my default remote isn't "origin" (long story), it's
> "github", but in the submodule it's of course "origin", there's no
> "github" remote there. As a result, `git submodule update` runs the
> command
>
>   git fetch github ${commit_hash}
>
> in the submodule, and that's interpreted as 'file' transport.
>
> To repeat this you need a repository where the default remote isn't
> "origin" and a submodule where the commit cannot be fetched by simply
> `git fetch` and needs a direct fetch.
>
> Hope this helps.
>
> Regards,
> Sergei

I recently experienced something similar when changing the default
remote name for some of my repositories. I had wondered what causes
problems because submodule update would sometimes fail but other times
succeed.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH] submodule: correct remote name with fetch
  2023-06-22 11:09 bug: submodule update fails to fetch Sergei Golubchik
  2023-06-22 13:07 ` Taylor Blau
@ 2024-10-01  7:24 ` Daniel Black
  2024-10-01 17:27   ` Junio C Hamano
  1 sibling, 1 reply; 12+ messages in thread
From: Daniel Black @ 2024-10-01  7:24 UTC (permalink / raw)
  To: git; +Cc: Daniel Black

The fetching of submodules erroniously used
the main repository remote name instead of the
submodule remote name[1].

Correct this by using the correct function
to reteive the remote name.

1. https://www.spinics.net/lists/git/msg462320.html

Signed-off-by: Daniel Black <daniel@mariadb.org>
---
 builtin/submodule--helper.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index a1ada86952..210ae2570a 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2322,7 +2322,12 @@ static int fetch_in_submodule(const char *module_path, int depth, int quiet,
 		strvec_pushf(&cp.args, "--depth=%d", depth);
 	if (oid) {
 		char *hex = oid_to_hex(oid);
-		char *remote = get_default_remote();
+		char *remote;
+		int code;
+
+		code = get_default_remote_submodule(module_path, &remote);
+		if (code)
+			return code;
 
 		strvec_pushl(&cp.args, remote, hex, NULL);
 		free(remote);
-- 
2.46.2


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] submodule: correct remote name with fetch
  2024-10-01  7:24 ` [PATCH] submodule: correct remote name with fetch Daniel Black
@ 2024-10-01 17:27   ` Junio C Hamano
  2024-10-01 17:34     ` Junio C Hamano
  0 siblings, 1 reply; 12+ messages in thread
From: Junio C Hamano @ 2024-10-01 17:27 UTC (permalink / raw)
  To: Daniel Black; +Cc: git

Daniel Black <daniel@mariadb.org> writes:

> The fetching of submodules erroniously used
> the main repository remote name instead of the
> submodule remote name[1].

Please write the problem description in the present tense, i.e. "the
code does this, which is incorrect in this way".

> Correct this by using the correct function
> to reteive the remote name.

That's gives the same as "fix it" ;-).

Can we phrase it in end-user observable terms, for example, "instead
of grabbing the default remote of the superproject repository, ask
the default remote of the submodule we are going to run 'git fetch'
in".  It is a mere implementation detail of doing so, to call
get_default_remote_submodule() instead of get_default_remote().

> 1. https://www.spinics.net/lists/git/msg462320.html

Please use the URL that shows the Message-Id when referring to a
message in the list archive.  E.g.,

    https://lore.kernel.org/git/ZJR5SPDj4Wt_gmRO@pweza/
    https://public-inbox.org/git/ZJR5SPDj4Wt_gmRO@pweza/

> Signed-off-by: Daniel Black <daniel@mariadb.org>
> ---
>  builtin/submodule--helper.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index a1ada86952..210ae2570a 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2322,7 +2322,12 @@ static int fetch_in_submodule(const char *module_path, int depth, int quiet,
>  		strvec_pushf(&cp.args, "--depth=%d", depth);
>  	if (oid) {
>  		char *hex = oid_to_hex(oid);
> -		char *remote = get_default_remote();
> +		char *remote;
> +		int code;
> +
> +		code = get_default_remote_submodule(module_path, &remote);
> +		if (code)
> +			return code;

We never failed in this function to return without calling
run_command().  Now we do.

Shouldn't we clean up the child_process structure we allocated and
prepared before returning like this?  It would just be the matter of
calling child_process_clear(&cp), perhaps?

>  		strvec_pushl(&cp.args, remote, hex, NULL);
>  		free(remote);

Thanks.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] submodule: correct remote name with fetch
  2024-10-01 17:27   ` Junio C Hamano
@ 2024-10-01 17:34     ` Junio C Hamano
  2024-10-08  1:49       ` Daniel Black
  0 siblings, 1 reply; 12+ messages in thread
From: Junio C Hamano @ 2024-10-01 17:34 UTC (permalink / raw)
  To: Daniel Black; +Cc: git

Junio C Hamano <gitster@pobox.com> writes:

Sorry, forgot to say one more thing before pressing [SEND].

>>  builtin/submodule--helper.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)

Please add a test or two, that fail without this change (to
demonstrate the existing bug) and succeed with this change (to make
sure if somebody breaks the code you fixed again in the future, such
a breakage is caught).

Thanks.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* (no subject)
  2024-10-01 17:34     ` Junio C Hamano
@ 2024-10-08  1:49       ` Daniel Black
  2024-10-08  1:49         ` [RFC PATCH v2] submodule: correct remote name with fetch Daniel Black
  0 siblings, 1 reply; 12+ messages in thread
From: Daniel Black @ 2024-10-08  1:49 UTC (permalink / raw)
  To: git

I have this so far, but I need a hand with the test case.

With a manual hack of the code:

--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2450,7 +2450,7 @@ static int run_update_procedure(const struct update_data *ud)
                 * Now we tried the usual fetch, but `oid` may
                 * not be reachable from any of the refs.
                 */
-               if (!is_tip_reachable(ud->sm_path, &ud->oid) &&
+               if (is_tip_reachable(ud->sm_path, &ud->oid) &&
                    fetch_in_submodule(ud->sm_path, ud->depth, ud->quiet, &ud->oid))
                        return die_message(_("Fetched in submodule path '%s', but it did not "
                                             "contain %s. Direct fetching of that commit failed."),

I'm able to using the GIT_TRACE= of git submodule update arrive at the
correct fetching from origin in the test case where before the code fix
it was fetching from remote o1.

trace: run_command: cd sub; unset GIT_PREFIX; GIT_DIR=.git git rev-list -n 1 27fe1b65df6f55a58636afcba364dfcb64916cd6 --not --all
trace: start_command: /home/dan/repos/git/git rev-list -n 1 27fe1b65df6f55a58636afcba364dfcb64916cd6 --not --all
trace: built-in: git rev-list -n 1 27fe1b65df6f55a58636afcba364dfcb64916cd6 --not --all
trace: run_command: cd sub; unset GIT_PREFIX; GIT_DIR=.git git rev-list -n 1 27fe1b65df6f55a58636afcba364dfcb64916cd6 --not --all
trace: start_command: /home/dan/repos/git/git rev-list -n 1 27fe1b65df6f55a58636afcba364dfcb64916cd6 --not --all
trace: built-in: git rev-list -n 1 27fe1b65df6f55a58636afcba364dfcb64916cd6 --not --all
trace: run_command: cd sub; unset GIT_PREFIX; GIT_DIR=.git git fetch origin 27fe1b65df6f55a58636afcba364dfcb64916cd6
trace: start_command: /home/dan/repos/git/git fetch origin 27fe1b65df6f55a58636afcba364dfcb64916cd6

As Sergei said in the original referenced message "a submodule where the commit cannot be fetched by simply
`git fetch` and needs a direct fetch".

So this is the test case that I'm having trouble generating.


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [RFC PATCH v2] submodule: correct remote name with fetch
  2024-10-08  1:49       ` Daniel Black
@ 2024-10-08  1:49         ` Daniel Black
  2024-10-08 19:23           ` Junio C Hamano
  0 siblings, 1 reply; 12+ messages in thread
From: Daniel Black @ 2024-10-08  1:49 UTC (permalink / raw)
  To: git; +Cc: Daniel Black

The code fetches the submodules remote based on the superproject remote name
instead of the submodule remote name[1].

Instead of grabbing the default remote of the superproject repository, ask
the default remote of the submodule we are going to run 'git fetch' in.

1. https://lore.kernel.org/git/ZJR5SPDj4Wt_gmRO@pweza/

Signed-off-by: Daniel Black <daniel@mariadb.org>
---
 builtin/submodule--helper.c |  9 ++++-
 t/t5568-fetch-submodule.sh  | 65 +++++++++++++++++++++++++++++++++++++
 2 files changed, 73 insertions(+), 1 deletion(-)
 create mode 100755 t/t5568-fetch-submodule.sh

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index a1ada86952..567d21d330 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2322,7 +2322,14 @@ static int fetch_in_submodule(const char *module_path, int depth, int quiet,
 		strvec_pushf(&cp.args, "--depth=%d", depth);
 	if (oid) {
 		char *hex = oid_to_hex(oid);
-		char *remote = get_default_remote();
+		char *remote;
+		int code;
+
+		code = get_default_remote_submodule(module_path, &remote);
+		if (code) {
+			child_process_clear(&cp);
+			return code;
+		}
 
 		strvec_pushl(&cp.args, remote, hex, NULL);
 		free(remote);
diff --git a/t/t5568-fetch-submodule.sh b/t/t5568-fetch-submodule.sh
new file mode 100755
index 0000000000..56978bcfd7
--- /dev/null
+++ b/t/t5568-fetch-submodule.sh
@@ -0,0 +1,65 @@
+#!/bin/sh
+
+test_description='fetch can handle submodules origin names'
+
+GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB=1
+export GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB
+
+TEST_PASSES_SANITIZE_LEAK=true
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-httpd.sh
+start_httpd
+
+SUPER="$HTTPD_DOCUMENT_ROOT_PATH/super"
+SUB="$HTTPD_DOCUMENT_ROOT_PATH/sub"
+SUPER_URI="$HTTPD_URL/smart/super"
+SUB_URI="$HTTPD_URL/smart/sub"
+
+setup() {
+	SERVER="$1"
+	git init "$SERVER" &&
+	test_when_finished 'rm -rf "$SERVER"' &&
+	test_config -C "$SERVER" http.receivepack true
+}
+
+test_expect_success 'fetch submodule remote of different name from superproject' '
+	setup "$SUPER" &&
+	test_create_repo super &&
+	test_commit -C super bar &&
+	git -C super remote add upstream "$SUPER_URI" &&
+	test_config -C super push.default upstream &&
+	git -C super push --set-upstream upstream master:main &&
+
+	setup "$SUB" &&
+	test_create_repo sub &&
+	test_commit -C sub foo &&
+	git -C sub branch newmain &&
+	test_commit -C sub morefoo &&
+	test_commit -C sub moremorefoo &&
+	git -C sub remote add upstream "$SUB_URI" &&
+	test_config -C sub push.default upstream &&
+	git -C sub push --set-upstream upstream master:main &&
+
+	git -C super submodule add --branch main -- "$SUB_URI" sub &&
+	git -C super commit -am "add submodule" &&
+	git -C super push &&
+
+	# Needs to create unreachable commit from current master branch.
+	git -C sub checkout newmain &&
+	test_commit -C sub echo &&
+	test_commit -C sub moreecho &&
+	git -C sub push  --set-upstream upstream newmain:newmain &&
+
+	git clone --origin o1 --branch main -- "$SUPER_URI" superproject &&
+	git -C superproject submodule update --init &&
+
+	git -C super/sub fetch &&
+	git -C super/sub checkout origin/newmain &&
+	git -C super commit -m "update submodule" sub &&
+	git -C super push && 
+
+	git -C superproject pull --no-recurse-submodules &&
+	git -C superproject submodule update
+'
+
+test_done
-- 
2.46.2


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [RFC PATCH v2] submodule: correct remote name with fetch
  2024-10-08  1:49         ` [RFC PATCH v2] submodule: correct remote name with fetch Daniel Black
@ 2024-10-08 19:23           ` Junio C Hamano
  2024-10-09  3:32             ` [PATCH v3] " Daniel Black
  0 siblings, 1 reply; 12+ messages in thread
From: Junio C Hamano @ 2024-10-08 19:23 UTC (permalink / raw)
  To: Daniel Black; +Cc: git

Daniel Black <daniel@mariadb.org> writes:

> The code fetches the submodules remote based on the superproject remote name
> instead of the submodule remote name[1].
>
> Instead of grabbing the default remote of the superproject repository, ask
> the default remote of the submodule we are going to run 'git fetch' in.
>
> 1. https://lore.kernel.org/git/ZJR5SPDj4Wt_gmRO@pweza/
>
> Signed-off-by: Daniel Black <daniel@mariadb.org>
> ---
>  builtin/submodule--helper.c |  9 ++++-

The proposed log message is very well written.

>  t/t5568-fetch-submodule.sh  | 65 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 73 insertions(+), 1 deletion(-)
>  create mode 100755 t/t5568-fetch-submodule.sh

Hmph,

	$ git grep "submodule update" t/

gives quite a many hits in existing tests.  Didn't any of them have
sufficient preparation steps that testing of this bugfix can reuse?

A test on "submodule update" behaviour tends to need quite a lot of
preparation.  Preparing the superproject, addition of a submodule to
it, cloning of these two projects, and then half-preparing a clone
of these super-sub arrangement.  All of that needs to happen before
we can say "submodule update" and observe the outcome to see if the
bug still exists.  If we can piggy-back on a test script that already
has such preparation, it would be far more preferrable than having to
do another set of preparation.

Another thing.  If this is not about a bug that only manifests when
the HTTP transport is in use, it is strongly preferred to avoid
turning it into an httpd test.  Some developers and/or environments
skip them.


> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index a1ada86952..567d21d330 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2322,7 +2322,14 @@ static int fetch_in_submodule(const char *module_path, int depth, int quiet,
>  		strvec_pushf(&cp.args, "--depth=%d", depth);
>  	if (oid) {
>  		char *hex = oid_to_hex(oid);
> -		char *remote = get_default_remote();
> +		char *remote;
> +		int code;
> +
> +		code = get_default_remote_submodule(module_path, &remote);
> +		if (code) {
> +			child_process_clear(&cp);
> +			return code;
> +		}

The get_default_remote_submodule() helper eventually calls into
repo_get_default_remote() that returns an allocated string in
remote, but it only does so when it succeeds, so this early return
does not have to worry about leaking "remote" here.  Good.

The code change looks quite straight-forward and looking good.

>  		strvec_pushl(&cp.args, remote, hex, NULL);
>  		free(remote);

Thanks.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v3] submodule: correct remote name with fetch
  2024-10-08 19:23           ` Junio C Hamano
@ 2024-10-09  3:32             ` Daniel Black
  2024-10-09 17:51               ` Junio C Hamano
  0 siblings, 1 reply; 12+ messages in thread
From: Daniel Black @ 2024-10-09  3:32 UTC (permalink / raw)
  To: git; +Cc: Daniel Black

The code fetches the submodules remote based on the superproject remote name
instead of the submodule remote name[1].

Instead of grabbing the default remote of the superproject repository, ask
the default remote of the submodule we are going to run 'git fetch' in.

1. https://lore.kernel.org/git/ZJR5SPDj4Wt_gmRO@pweza/

Signed-off-by: Daniel Black <daniel@mariadb.org>
---
 builtin/submodule--helper.c |  9 ++++++++-
 t/t5572-pull-submodule.sh   | 20 ++++++++++++++++++++
 2 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index dc89488a7d..b6b5f1ebde 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2333,7 +2333,14 @@ static int fetch_in_submodule(const char *module_path, int depth, int quiet,
 		strvec_pushf(&cp.args, "--depth=%d", depth);
 	if (oid) {
 		char *hex = oid_to_hex(oid);
-		char *remote = get_default_remote();
+		char *remote;
+		int code;
+
+		code = get_default_remote_submodule(module_path, &remote);
+		if (code) {
+			child_process_clear(&cp);
+			return code;
+		}
 
 		strvec_pushl(&cp.args, remote, hex, NULL);
 		free(remote);
diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh
index 916e58c166..9b6cf8d88b 100755
--- a/t/t5572-pull-submodule.sh
+++ b/t/t5572-pull-submodule.sh
@@ -230,6 +230,7 @@ test_expect_success 'branch has no merge base with remote-tracking counterpart'
 
 	test_create_repo a-submodule &&
 	test_commit -C a-submodule foo &&
+	test_commit -C a-submodule bar &&
 
 	test_create_repo parent &&
 	git -C parent submodule add "$(pwd)/a-submodule" &&
@@ -246,4 +247,23 @@ test_expect_success 'branch has no merge base with remote-tracking counterpart'
 	git -C child pull --recurse-submodules --rebase
 '
 
+test_expect_success 'fetch submodule remote of different name from superproject' '
+	git -C child remote rename origin o1 &&
+	git -C child submodule update --init &&
+
+	# Needs to create unreachable commit from current master branch.
+	git -C a-submodule checkout -b newmain HEAD^ &&
+	test_commit -C a-submodule echo &&
+	test_commit -C a-submodule moreecho &&
+	subc=$(git -C a-submodule rev-parse --short HEAD) &&
+
+	git -C parent/a-submodule fetch &&
+	git -C parent/a-submodule checkout "$subc" &&
+	git -C parent commit -m "update submodule" a-submodule &&
+	git -C a-submodule reset --hard HEAD^^ &&
+
+	git -C child pull --no-recurse-submodules &&
+	git -C child submodule update
+'
+
 test_done
-- 
2.46.2


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v3] submodule: correct remote name with fetch
  2024-10-09  3:32             ` [PATCH v3] " Daniel Black
@ 2024-10-09 17:51               ` Junio C Hamano
  0 siblings, 0 replies; 12+ messages in thread
From: Junio C Hamano @ 2024-10-09 17:51 UTC (permalink / raw)
  To: Daniel Black; +Cc: git

Daniel Black <daniel@mariadb.org> writes:

> The code fetches the submodules remote based on the superproject remote name
> instead of the submodule remote name[1].
>
> Instead of grabbing the default remote of the superproject repository, ask
> the default remote of the submodule we are going to run 'git fetch' in.
>
> 1. https://lore.kernel.org/git/ZJR5SPDj4Wt_gmRO@pweza/
>
> Signed-off-by: Daniel Black <daniel@mariadb.org>
> ---
>  builtin/submodule--helper.c |  9 ++++++++-
>  t/t5572-pull-submodule.sh   | 20 ++++++++++++++++++++
>  2 files changed, 28 insertions(+), 1 deletion(-)

Excellent.  Will queue.

Thanks.

> diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh
> index 916e58c166..9b6cf8d88b 100755
> --- a/t/t5572-pull-submodule.sh
> +++ b/t/t5572-pull-submodule.sh
> @@ -230,6 +230,7 @@ test_expect_success 'branch has no merge base with remote-tracking counterpart'
>  
>  	test_create_repo a-submodule &&
>  	test_commit -C a-submodule foo &&
> +	test_commit -C a-submodule bar &&
>  
>  	test_create_repo parent &&
>  	git -C parent submodule add "$(pwd)/a-submodule" &&
> @@ -246,4 +247,23 @@ test_expect_success 'branch has no merge base with remote-tracking counterpart'
>  	git -C child pull --recurse-submodules --rebase
>  '
>  
> +test_expect_success 'fetch submodule remote of different name from superproject' '
> +	git -C child remote rename origin o1 &&
> +	git -C child submodule update --init &&
> +
> +	# Needs to create unreachable commit from current master branch.
> +	git -C a-submodule checkout -b newmain HEAD^ &&
> +	test_commit -C a-submodule echo &&
> +	test_commit -C a-submodule moreecho &&
> +	subc=$(git -C a-submodule rev-parse --short HEAD) &&
> +
> +	git -C parent/a-submodule fetch &&
> +	git -C parent/a-submodule checkout "$subc" &&
> +	git -C parent commit -m "update submodule" a-submodule &&
> +	git -C a-submodule reset --hard HEAD^^ &&
> +
> +	git -C child pull --no-recurse-submodules &&
> +	git -C child submodule update
> +'
> +
>  test_done

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2024-10-09 17:51 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-06-22 11:09 bug: submodule update fails to fetch Sergei Golubchik
2023-06-22 13:07 ` Taylor Blau
2023-06-22 16:39   ` Sergei Golubchik
2023-06-23  7:26     ` Jacob Keller
2024-10-01  7:24 ` [PATCH] submodule: correct remote name with fetch Daniel Black
2024-10-01 17:27   ` Junio C Hamano
2024-10-01 17:34     ` Junio C Hamano
2024-10-08  1:49       ` Daniel Black
2024-10-08  1:49         ` [RFC PATCH v2] submodule: correct remote name with fetch Daniel Black
2024-10-08 19:23           ` Junio C Hamano
2024-10-09  3:32             ` [PATCH v3] " Daniel Black
2024-10-09 17:51               ` Junio C Hamano

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).