{"thread":{"id":"59234","subject":"[PATCH] Use correct default remote for fetching in submodule.","startedAt":"2023-02-13T13:16:01Z","lastAt":"2023-02-13T13:45:33Z","messageCount":2,"participants":["Mara","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"472006","messageId":"623b0b8a-a5b3-408c-b924-9f88d9763b0e@app.fastmail.com","threadId":"59234","inReplyTo":null,"subject":"[PATCH] Use correct default remote for fetching in submodule.","fromName":"Mara","fromEmail":"mara@marabos.nl","sentAt":"2023-02-13T13:14:21Z","receivedAt":"2023-02-13T13:16:01Z","isPatch":true,"sender":{"key":"mara@marabos.nl","avatar":null},"body":"\"git submodule update\" first tries a regular \"git fetch\"\nto fetch the commit, but when that doesn't retrieve the commit\nit wants, it tries \"git fetch <remote> <commit>\".\nFor <remote>, it used the wrong default remote: the default\nremote of the outer repository, rather than the default remote\nof the submodule.\n\nSigned-off-by: Mara Bos <mara@marabos.nl>\n---\n builtin/submodule--helper.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 4c173d8b37..50b96e0b9d 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2225,7 +2225,10 @@ static int fetch_in_submodule(const char *module_path, int depth, int quiet,\n \t\tstrvec_pushf(&cp.args, \"--depth=%d\", depth);\n \tif (oid) {\n \t\tchar *hex = oid_to_hex(oid);\n-\t\tchar *remote = get_default_remote();\n+\t\tchar *remote;\n+\t\tint code = get_default_remote_submodule(module_path, &remote);\n+\t\tif (code)\n+\t\t\treturn code;\n\n \t\tstrvec_pushl(&cp.args, remote, hex, NULL);\n \t\tfree(remote);\n--\n2.39.0\n"},{"id":"472008","messageId":"CAPig+cRotsJZHnRMUqWaYz=nw1sBRsW5ms7WWuApviG2be2GTQ@mail.gmail.com","threadId":"59234","inReplyTo":"623b0b8a-a5b3-408c-b924-9f88d9763b0e@app.fastmail.com","subject":"Re: [PATCH] Use correct default remote for fetching in submodule.","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-02-13T13:45:15Z","receivedAt":"2023-02-13T13:45:33Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Feb 13, 2023 at 8:31 AM Mara <mara@marabos.nl> wrote:\n> \"git submodule update\" first tries a regular \"git fetch\"\n> to fetch the commit, but when that doesn't retrieve the commit\n> it wants, it tries \"git fetch <remote> <commit>\".\n> For <remote>, it used the wrong default remote: the default\n> remote of the outer repository, rather than the default remote\n> of the submodule.\n>\n> Signed-off-by: Mara Bos <mara@marabos.nl>\n> ---\n\nI'm not a submodule user and I don't have any particular familiarity\nwith this code, so I may be wrong, but...\n\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 4c173d8b37..50b96e0b9d 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -2225,7 +2225,10 @@ static int fetch_in_submodule(const char *module_path, int depth, int quiet,\n> -               char *remote = get_default_remote();\n> +               char *remote;\n> +               int code = get_default_remote_submodule(module_path, &remote);\n> +               if (code)\n> +                       return code;\n>                 strvec_pushl(&cp.args, remote, hex, NULL);\n>                 free(remote);\n\n... it looks like this change may introduce a memory leak. Digging\ndown through get_default_remote_submodule() and the functions it\ncalls, it appears that repo_get_default_remote() can return a non-zero\ncode _after_ it has allocated memory for `remote`. If I'm reading this\ncorrectly, then the above should probably be:\n\n    char *remote = NULL;\n    int code = get_default_remote_submodule(module_path, &remote);\n    if (code) {\n        free(remote);\n        return code;\n    }\n\nAlso, if possible, add a new test, perhaps to\nt/t7406-submodule-update.sh, demonstrating that this change fixes the\nproblem and to ensure that it doesn't get broken again. If you have a\nminimal-reproduction recipe which exhibits the problem, then you may\nbe able to turn it into an actual test.\n"}]}