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 <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/config.c b/builtin/config.c
index 0015620ddeb2ab31a1bbb41a85eac70c7c548bfb..40456c077027b182173d613dfca333a8ec43f407 100644 (file)
--- 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 ae3652b08fa6f36af6c085e36b89efdd46d72389..13cf9eeb163c2663db8cea149fc4285740fe434b 100644 (file)
--- 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 f4966e374948a59cbe95af6f999f0e70d44aa1e9..db8b608064a881901f2f2a3f527f516a61e075d8 100644 (file)
--- 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: