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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Aug 02 2022, Glen Choo wrote:

> Æ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").

Makes sense, will rename it.

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

I'll re-roll & leave it in submodule.[ch].