On Tue, Aug 02 2022, Glen Choo wrote:

Ævar Arnfjörð Bjarmason [off-list ref] writes:

quoted

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