config: clarify memory ownership when preparing comment strings

The ownership of memory returned when preparing a comment string is
quite intricate: when the returned value is different than the passed
value, then the caller is responsible to free the memory. This is quite
subtle, and it's even easier to miss because the returned value is in
fact a `const char *`.

Adapt the function to always return either `NULL` or a newly allocated
string. The function is called at most once per git-config(1), so it's
not like this micro-optimization really matters. Thus, callers are now
always responsible for freeing the value.

Signed-off-by: Patrick Steinhardt <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>

Author: pks-t

builtin/config.c

@@ -44,7 +44,7 @@ static struct config_options config_options;
 static int show_origin;
 static int show_scope;
 static int fixed_value;
-static const char *comment;
+static const char *comment_arg;
 
 #define ACTION_GET (1<<0)
 #define ACTION_GET_ALL (1<<1)
@@ -174,7 +174,7 @@ static struct option builtin_config_options[] = {
 	OPT_BOOL(0, "show-origin", &show_origin, N_("show origin of config (file, standard input, blob, command line)")),
 	OPT_BOOL(0, "show-scope", &show_scope, N_("show scope of config (worktree, local, global, system, command)")),
 	OPT_STRING(0, "default", &default_value, N_("value"), N_("with --get, use default value when missing entry")),
-	OPT_STRING(0, "comment", &comment, N_("value"), N_("human-readable comment string (# will be prepended as needed)")),
+	OPT_STRING(0, "comment", &comment_arg, N_("value"), N_("human-readable comment string (# will be prepended as needed)")),
 	OPT_END(),
 };
 
@@ -674,7 +674,7 @@ static char *default_user_config(void)
 int cmd_config(int argc, const char **argv, const char *prefix)
 {
 	int nongit = !startup_info->have_repository;
-	char *value = NULL;
+	char *value = NULL, *comment = NULL;
 	int flags = 0;
 	int ret = 0;
 	struct key_value_info default_kvi = KVI_INIT;
@@ -799,7 +799,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		usage_builtin_config();
 	}
 
-	if (comment &&
+	if (comment_arg &&
 	    !(actions & (ACTION_ADD|ACTION_SET|ACTION_SET_ALL|ACTION_REPLACE_ALL))) {
 		error(_("--comment is only applicable to add/set/replace operations"));
 		usage_builtin_config();
@@ -841,7 +841,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		flags |= CONFIG_FLAGS_FIXED_VALUE;
 	}
 
-	comment = git_config_prepare_comment_string(comment);
+	comment = git_config_prepare_comment_string(comment_arg);
 
 	if (actions & PAGING_ACTIONS)
 		setup_auto_pager("config", 1);
@@ -993,6 +993,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		return get_colorbool(argv[0], argc == 2);
 	}
 
+	free(comment);
 	free(value);
 	return ret;
 }

config.c

@@ -3182,14 +3182,10 @@ void git_config_set(const char *key, const char *value)
 	trace2_cmd_set_config(key, value);
 }
 
-/*
- * The ownership rule is that the caller will own the string
- * if it receives a piece of memory different from what it passed
- * as the parameter.
- */
-const char *git_config_prepare_comment_string(const char *comment)
+char *git_config_prepare_comment_string(const char *comment)
 {
 	size_t leading_blanks;
+	char *prepared;
 
 	if (!comment)
 		return NULL;
@@ -3210,13 +3206,13 @@ const char *git_config_prepare_comment_string(const char *comment)
 
 	leading_blanks = strspn(comment, " \t");
 	if (leading_blanks && comment[leading_blanks] == '#')
-		; /* use it as-is */
+		prepared = xstrdup(comment); /* use it as-is */
 	else if (comment[0] == '#')
-		comment = xstrfmt(" %s", comment);
+		prepared = xstrfmt(" %s", comment);
 	else
-		comment = xstrfmt(" # %s", comment);
+		prepared = xstrfmt(" # %s", comment);
 
-	return comment;
+	return prepared;
 }
 
 static void validate_comment_string(const char *comment)

config.h

@@ -338,7 +338,7 @@ void git_config_set_multivar(const char *, const char *, const char *, unsigned)
 int repo_config_set_multivar_gently(struct repository *, const char *, const char *, const char *, unsigned);
 int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, const char *, unsigned);
 
-const char *git_config_prepare_comment_string(const char *);
+char *git_config_prepare_comment_string(const char *);
 
 /**
  * takes four parameters: