diff mbox series

[v4,01/14] config: clarify memory ownership when preparing comment strings

Message ID 3aa26d5bff21e0b93736b1502fa90694f6b2cedc.1714730169.git.ps@pks.im (mailing list archive)
State Superseded
Headers show
Series builtin/config: introduce subcommands | expand

Commit Message

Patrick Steinhardt May 3, 2024, 9:56 a.m. UTC 
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.
---
 builtin/config.c | 11 ++++++-----
 config.c         | 16 ++++++----------
 config.h         |  2 +-
 3 files changed, 13 insertions(+), 16 deletions(-)

Comments

Kristoffer Haugsbakk May 3, 2024, 10:13 a.m. UTC | #1 
On Fri, May 3, 2024, at 11:56, Patrick Steinhardt wrote:
> 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.
> ---

Missing a signoff on this one. The only one in the series.
diff mbox series

Patch

diff --git a/builtin/config.c b/builtin/config.c
index 0015620dde..40456c0770 100644
--- a/builtin/config.c
+++ b/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;
 }
diff --git a/config.c b/config.c
index ae3652b08f..13cf9eeb16 100644
--- a/config.c
+++ b/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)
diff --git a/config.h b/config.h
index f4966e3749..db8b608064 100644
--- a/config.h
+++ b/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: