Re: [PATCH 13/20] submodule--helper: stop conflating "sb" in clone_submodule()

From: Glen Choo <chooglen@google.com>
Date: Jul 29, 2022, 17:08 UTC
Message-ID: <kl6l4jyzsv6o.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <patch-13.20-afe34d8b73b-20220728T161116Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Show 59 quoted lines

> Refactor the two uses of a "struct strbuf sb" such that each of them
> exists in its own scope. This makes the control flow clearer.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/submodule--helper.c | 25 +++++++++++++++++--------
>  1 file changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index f74957444e1..6cedcc5b239 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -1557,16 +1557,24 @@ static void prepare_possible_alternates(const char *sm_name,
>  	free(error_strategy);
>  }
>  
> -static int clone_submodule(struct module_clone_data *clone_data)
> +static char *clone_submodule_sm_gitdir(const char *name)
>  {
> -	char *p, *sm_gitdir;
> -	char *sm_alternate = NULL, *error_strategy = NULL;
>  	struct strbuf sb = STRBUF_INIT;
> -	struct child_process cp = CHILD_PROCESS_INIT;
> +	char *sm_gitdir;
>  
> -	submodule_name_to_gitdir(&sb, the_repository, clone_data->name);
> +	submodule_name_to_gitdir(&sb, the_repository, name);
>  	sm_gitdir = absolute_pathdup(sb.buf);
> -	strbuf_reset(&sb);
> +	strbuf_release(&sb);
> +
> +	return sm_gitdir;
> +}
> +
> +static int clone_submodule(struct module_clone_data *clone_data)
> +{
> +	char *p;
> +	char *sm_gitdir = clone_submodule_sm_gitdir(clone_data->name);
> +	char *sm_alternate = NULL, *error_strategy = NULL;
> +	struct child_process cp = CHILD_PROCESS_INIT;
>  
>  	if (!is_absolute_path(clone_data->path))
>  		clone_data->path = xstrfmt("%s/%s", get_git_work_tree(),
> @@ -1624,6 +1632,8 @@ static int clone_submodule(struct module_clone_data *clone_data)
>  			die(_("clone of '%s' into submodule path '%s' failed"),
>  			    clone_data->url, clone_data->path);
>  	} else {
> +		struct strbuf sb = STRBUF_INIT;
> +
>  		if (clone_data->require_init && !access(clone_data->path, X_OK) &&
>  		    !is_empty_dir(clone_data->path))
>  			die(_("directory not empty: '%s'"), clone_data->path);
> @@ -1631,7 +1641,7 @@ static int clone_submodule(struct module_clone_data *clone_data)
>  			die(_("could not create directory '%s'"), clone_data->path);
>  		strbuf_addf(&sb, "%s/index", sm_gitdir);
>  		unlink_or_warn(sb.buf);
> -		strbuf_reset(&sb);
> +		strbuf_release(&sb);
>  	}

As Junio mentioned in https://lore.kernel.org/git/xmqqlesmf9or.fsf@gitster.g, we could also replace this with xstrfmt(). I think that gets rid of all of the users of "sb", so I doubt we'll even need this patch once we make that change :)

Show 12 quoted lines

>  
>  	connect_work_tree_and_git_dir(clone_data->path, sm_gitdir, 0);
> @@ -1653,7 +1663,6 @@ static int clone_submodule(struct module_clone_data *clone_data)
>  	free(sm_alternate);
>  	free(error_strategy);
>  
> -	strbuf_release(&sb);
>  	free(sm_gitdir);
>  	free(p);
>  	return 0;
> -- 
> 2.37.1.1167.g38fda70d8c4