Re: [PATCH 0/12] git_config_string() considered harmful

[Rubén Justo <rjusto@xxxxxxxxx>]

On Sat, Apr 06, 2024 at 08:56:56PM -0400, Jeff King wrote:

> And it's not just git_config_pathname(), but really git_config_string(),

Indeed.

After Junio's series and yours, I'm on the fence now, but my envision was
to introduce:

--- >8 ---
diff --git a/config.c b/config.c
index eebce8c7e0..7322bdfb94 100644
--- a/config.c
+++ b/config.c
@@ -1345,6 +1345,15 @@ int git_config_string(const char **dest, const char *var, const char *value)
 	return 0;
 }
 
+int git_config_strbuf(struct strbuf *dest, const char *var, const char *value)
+{
+	if (!value)
+		return config_error_nonbool(var);
+	strbuf_reset(dest);
+	strbuf_addstr(dest, value);
+	return 0;
+}
+
 int git_config_pathname(const char **dest, const char *var, const char *value)
 {
 	if (!value)
diff --git a/config.h b/config.h
index f4966e3749..46e3137612 100644
--- a/config.h
+++ b/config.h
@@ -282,6 +282,12 @@ int git_config_bool(const char *, const char *);
  */
 int git_config_string(const char **, const char *, const char *);
 
+/**
+ * Copies the value string into the `dest` parameter; if no
+ * string is given, prints an error message and returns -1.
+ */
+int git_config_strbuf(struct strbuf *, const char *, const char *);
+
 /**
  * Similar to `git_config_string`, but expands `~` or `~user` into the
  * user's home directory when found at the beginning of the path.
--- 8< ---

To allow uses like:

--- >8 ---
diff --git a/config.c b/config.c
index 7322bdfb94..03884fa782 100644
--- a/config.c
+++ b/config.c
@@ -1572,7 +1572,7 @@ static int git_default_core_config(const char *var, const char *value,
 	}
 
 	if (!strcmp(var, "core.editor"))
-		return git_config_string(&editor_program, var, value);
+		return git_config_strbuf(&editor_program, var, value);
 
 	if (!strcmp(var, "core.commentchar") ||
 	    !strcmp(var, "core.commentstring")) {
diff --git a/editor.c b/editor.c
index b67b802ddf..618c193249 100644
--- a/editor.c
+++ b/editor.c
@@ -27,8 +27,8 @@ const char *git_editor(void)
 	const char *editor = getenv("GIT_EDITOR");
 	int terminal_is_dumb = is_terminal_dumb();
 
-	if (!editor && editor_program)
-		editor = editor_program;
+	if (!editor && editor_program.len)
+		editor = editor_program.buf;
 	if (!editor && !terminal_is_dumb)
 		editor = getenv("VISUAL");
 	if (!editor)
diff --git a/environment.c b/environment.c
index a73ba9c12c..b5073ff972 100644
--- a/environment.c
+++ b/environment.c
@@ -58,7 +58,7 @@ size_t packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE;
 size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT;
 size_t delta_base_cache_limit = 96 * 1024 * 1024;
 unsigned long big_file_threshold = 512 * 1024 * 1024;
-const char *editor_program;
+struct strbuf editor_program = STRBUF_INIT;
 const char *askpass_program;
 const char *excludes_file;
 enum auto_crlf auto_crlf = AUTO_CRLF_FALSE;
diff --git a/environment.h b/environment.h
index 05fd94d7be..c20898345e 100644
--- a/environment.h
+++ b/environment.h
@@ -220,7 +220,7 @@ const char *get_commit_output_encoding(void);
 extern const char *git_commit_encoding;
 extern const char *git_log_output_encoding;
 
-extern const char *editor_program;
+extern struct strbuf editor_program;
 extern const char *askpass_program;
 extern const char *excludes_file;