| Message ID | 20241001072423.1165932-1-daniel@mariadb.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | submodule: correct remote name with fetch | expand |
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.
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.
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);
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(-)