Re: [PATCH v3 1/5] submodule--helper: use submodule_name_to_gitdir in add_submodule
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 6, 2025, 16:37 UTC
- Message-ID
- <xmqqh5wcq94v.fsf@gitster.g>
- In-Reply-To
- <20251006112518.3764240-2-adrian.ratiu@collabora.com>
Adrian Ratiu <adrian.ratiu@collabora.com> writes:
Show 5 quoted lines
> While testing submodule gitdir path encoding, I noticed submodule--helper > is still using a hardcoded name-based path leading to test failures, so > convert it to the common helper function introduced by commit ce125d431a > (submodule: extract path to submodule gitdir func, 2021-09-15) and used > in other locations across the source tree.
OK. To me during my first reading, the above read as if you found an open coded logic here in add_submodule(), made it into a new common helper function, and made this part as well as other locations call that new common helper function. Of course that is not the case.
Perhaps replacing everything after ", so convert it" with something simpler like
... to test failures. Call submodule_name_to_gitdir() helper
instead, which was invented exactly for this purpose and
everybody else uses.
might have helped me to avoid such a confusion. I dunno.Show 16 quoted lines
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index fcd73abe53..2873b2780e 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -3187,13 +3187,13 @@ static void append_fetch_remotes(struct strbuf *msg, const char *git_dir_path)
>
> static int add_submodule(const struct add_data *add_data)
> {
> - char *submod_gitdir_path;
> struct module_clone_data clone_data = MODULE_CLONE_DATA_INIT;
> struct string_list reference = STRING_LIST_INIT_NODUP;
> int ret = -1;
>
> /* perhaps the path already exists and is already a git repo, else clone it */
> if (is_directory(add_data->sm_path)) {
> + char *submod_gitdir_path;This hunk is not related to the theme of the change and not explained? I think the variable becomes used only within this block after the patch that loses the use of it on the "else" side, so in that sense it is not strictly unrelated, but is a fallout of this change. If we were to mention the change in the log message, something like "Also narrow the scope of a variable that is no longer used in the updated code" would suffice.
Show 14 quoted lines
> @@ -3207,10 +3207,11 @@ static int add_submodule(const struct add_data *add_data)
> free(submod_gitdir_path);
> } else {
> struct child_process cp = CHILD_PROCESS_INIT;
> + struct strbuf submod_gitdir = STRBUF_INIT;
>
> - submod_gitdir_path = xstrfmt(".git/modules/%s", add_data->sm_name);
> + submodule_name_to_gitdir(&submod_gitdir, the_repository, add_data->sm_name);
>
> - if (is_directory(submod_gitdir_path)) {
> + if (is_directory(submod_gitdir.buf)) {
> if (!add_data->force) {
> struct strbuf msg = STRBUF_INIT;
> char *die_msg;So this is the crux of the change, which makes sense. Where do we release the resource aquired here? Let's see...
Show 8 quoted lines
> @@ -3219,8 +3220,8 @@ static int add_submodule(const struct add_data *add_data) > "locally with remote(s):\n"), > add_data->sm_name); > > - append_fetch_remotes(&msg, submod_gitdir_path); > - free(submod_gitdir_path); > + append_fetch_remotes(&msg, submod_gitdir.buf); > + strbuf_release(&submod_gitdir);
... OK, where the variable was freed in the original, so if this is not the right place to release the resources, then the original was already buggy.
Show 9 quoted lines
>
> strbuf_addf(&msg, _("If you want to reuse this local git "
> "directory instead of cloning again from\n"
> @@ -3238,7 +3239,7 @@ static int add_submodule(const struct add_data *add_data)
> "submodule '%s'\n"), add_data->sm_name);
> }
> }
> - free(submod_gitdir_path);
> + strbuf_release(&submod_gitdir);Ditto.
And the only nonlocal exit other than die() inside this "else" block appears _after_ this part, so we are OK.
Looks good to me. Thanks.