| Message ID | 20241008015835.41678-2-daniel@mariadb.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC,v2] submodule: correct remote name with fetch | expand |
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.
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
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