Re: [PATCH v3 1/5] submodule--helper: use submodule_name_to_gitdir in add_submodule

[Junio C Hamano <gitster@pobox.com>]

Adrian Ratiu <adrian.ratiu@collabora.com> writes:

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

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

> @@ -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...

> @@ -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.

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