Re: [PATCH v2 22/28] submodule--helper: move submodule_strategy_to_string() to only user

[Glen Choo <chooglen@google.com>]

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

> Move the submodule_strategy_to_string() function added in
> 3604242f080 (submodule: port init from shell to C, 2016-04-15) to its
> only user.
>
> This function would return NULL on SM_UPDATE_UNSPECIFIED, so it wasn't
> safe to xstrdup() its return value in the general case, or to use it
> in a sprintf() format as the code removed in the preceding commit did.
>
> But its callers would never call it with either SM_UPDATE_UNSPECIFIED
> or SM_UPDATE_COMMAND. Let's move it to a "static" helper, and have its
> functionality reflect how it's used, and BUG() out on the rest.
>
> By doing this we can also stop needlessly xstrdup()-ing and free()-ing
> the memory for the config we're setting. We can instead always use
> constant strings. We can also use the *_tmp() variant of
> git_config_get_string().
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/submodule--helper.c | 29 ++++++++++++++++++++++++-----
>  submodule.c                 | 21 ---------------------
>  submodule.h                 |  1 -
>  3 files changed, 24 insertions(+), 27 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index b49528e1ba9..d787c0fead4 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -413,12 +413,32 @@ struct init_cb {
>  };
>  #define INIT_CB_INIT { 0 }
>  
> +static const char *submodule_strategy_to_string(enum submodule_update_type type)
> +{
> +	switch (type) {
> +	case SM_UPDATE_CHECKOUT:
> +		return "checkout";
> +	case SM_UPDATE_MERGE:
> +		return "merge";
> +	case SM_UPDATE_REBASE:
> +		return "rebase";
> +	case SM_UPDATE_NONE:
> +		return "none";
> +	case SM_UPDATE_UNSPECIFIED:
> +	case SM_UPDATE_COMMAND:
> +		BUG("init_submodule() should handle type %d", type);
> +	default:
> +		BUG("unexpected update strategy type: %d", type);
> +	}
> +}
> +

This function is meant to convert from an update strategy back to the
string that the user actually provided in their gitconfig.

The change in behavior makes this function BUG() out on types that
aren't "magic" tokens, i.e. "UNSPECIFIED" (which is obviously not
expressible) and "COMMAND" (which allows users to specify an arbitrary
command using "!", like "!cat"). It shouldn't be difficult to teach
callers to handle "COMMAND", so this seems like an ok change, though I
think we should probably amend the function name to
submodule_update_type_to_string() and change the UNSPECIFIED and COMMAND
arms to something like BUG("type %d has no corresponding string").

I'm not so convinced that this function should be static, though. I
think it's more natural for submodule_update_type_to_string() to have
the same visibility as enum submodule_update_type. Today, we only have
one other caller who uses this enum, and it doesn't even need this
_to_string() fn (fsck.c calls parse_submodule_update_type() and doesn't
need _to_string() because it has the raw config values). But it feels a
bit inevitable that this will get moved back to submodule.h.

>  static void init_submodule(const char *path, const char *prefix,
>  			   unsigned int flags)
>  {
>  	const struct submodule *sub;
>  	struct strbuf sb = STRBUF_INIT;
> -	char *upd = NULL, *url = NULL, *displaypath;
> +	const char *upd;
> +	char *url = NULL, *displaypath;
>  
>  	displaypath = get_submodule_displaypath(path, prefix);
>  
> @@ -474,14 +494,14 @@ static void init_submodule(const char *path, const char *prefix,
>  
>  	/* Copy "update" setting when it is not set yet */
>  	strbuf_addf(&sb, "submodule.%s.update", sub->name);
> -	if (git_config_get_string(sb.buf, &upd) &&
> +	if (git_config_get_string_tmp(sb.buf, &upd) &&
>  	    sub->update_strategy.type != SM_UPDATE_UNSPECIFIED) {
>  		if (sub->update_strategy.type == SM_UPDATE_COMMAND) {
>  			fprintf(stderr, _("warning: command update mode suggested for submodule '%s'\n"),
>  				sub->name);
> -			upd = xstrdup("none");
> +			upd = "none";
>  		} else {
> -			upd = xstrdup(submodule_strategy_to_string(&sub->update_strategy));
> +			upd = submodule_strategy_to_string(sub->update_strategy.type);
>  		}
>  
>  		if (git_config_set_gently(sb.buf, upd))
> @@ -490,7 +510,6 @@ static void init_submodule(const char *path, const char *prefix,
>  	strbuf_release(&sb);
>  	free(displaypath);
>  	free(url);
> -	free(upd);
>  }
>  
>  static void init_submodule_cb(const struct cache_entry *list_item, void *cb_data)
> diff --git a/submodule.c b/submodule.c
> index 3fa5db3ecdf..e297d94d17f 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -415,27 +415,6 @@ int parse_submodule_update_strategy(const char *value,
>  	return 0;
>  }
>  
> -const char *submodule_strategy_to_string(const struct submodule_update_strategy *s)
> -{
> -	struct strbuf sb = STRBUF_INIT;
> -	switch (s->type) {
> -	case SM_UPDATE_CHECKOUT:
> -		return "checkout";
> -	case SM_UPDATE_MERGE:
> -		return "merge";
> -	case SM_UPDATE_REBASE:
> -		return "rebase";
> -	case SM_UPDATE_NONE:
> -		return "none";
> -	case SM_UPDATE_UNSPECIFIED:
> -		return NULL;
> -	case SM_UPDATE_COMMAND:
> -		strbuf_addf(&sb, "!%s", s->command);
> -		return strbuf_detach(&sb, NULL);
> -	}
> -	return NULL;
> -}
> -

The rest of the changes look good. An alternative would be to _always_
return a non-const char * (e.g. return xstrdup("none")) and expect the
caller to free it, but that feels like bending over backwards to
accomodate xstrfmt("!%s").

>  void handle_ignore_submodules_arg(struct diff_options *diffopt,
>  				  const char *arg)
>  {
> diff --git a/submodule.h b/submodule.h
> index bfaa9da1868..949f67b8031 100644
> --- a/submodule.h
> +++ b/submodule.h
> @@ -72,7 +72,6 @@ void die_path_inside_submodule(struct index_state *istate,
>  enum submodule_update_type parse_submodule_update_type(const char *value);
>  int parse_submodule_update_strategy(const char *value,
>  				    struct submodule_update_strategy *dst);
> -const char *submodule_strategy_to_string(const struct submodule_update_strategy *s);
>  void handle_ignore_submodules_arg(struct diff_options *, const char *);
>  void show_submodule_diff_summary(struct diff_options *o, const char *path,
>  			    struct object_id *one, struct object_id *two,
> -- 
> 2.37.1.1233.ge8b09efaedc