* 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).