Re: [PATCH v9 3/9] environment: move editor_program into repo_config_values

["Pablo Sabater" <pabloosabaterr@gmail.com>]

[Resending review against v9, I accidentally sent the review in-reply-to
v7. The patch seems to not have changed and it applies to v9 as well.]

On Thu Jul 9, 2026 at 6:11 PM CEST, Tian Yuchen wrote:
> The global variable 'editor_program' holds the path to the user's
> preferred editor. Move 'editor_program' into
> 'struct repo_config_values' to continue the libification effort.
>
> There have been discussions on whether external programs like
> editors truly need to be configured on a per-repository basis within
> the same process. While a single process might rarely invoke
> different editors, this migration is necessary for two reasons:
>
> 1. Developers frequently use different toolchains for different
>    projects. Per-repo configuration respects this.
>
> 2. Moving this string into 'repo_config_values' eliminates mutable
>    global state. As the codebase moves toward becoming a long-running
>    processes managing multiple repositories concurrently must
>    not overwrite each other's program configurations.

Nit:
"As the codebase moves toward becoming long-running processes, managing
multiple repositories concurrently must not overwrite each other's
program configurations"

>
> No standalone getter function is introduced. Callers directly access
> the field via 'repo_config_values()'. Heap memory is safely reclaimed
> in 'repo_config_values_clear()'.

Why no getter function here? in patch 2/9 repo_excludes_file() is
introduced and also access the field via repo_config_values().

Super nit: on a previous commit you used backquotes for
repo_config_values_clear() but now you're using single quotes.

>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com>
> Mentored-by: Olamide Caleb Bello <belkid98@gmail.com>
> Signed-off-by: Tian Yuchen <cat@malon.dev>
> ---
>  editor.c      | 4 ++--
>  environment.c | 7 ++++---
>  environment.h | 2 +-
>  3 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/editor.c b/editor.c
> index fd174e6a03..07d264cba0 100644
> --- a/editor.c
> +++ b/editor.c
> @@ -29,8 +29,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 && repo_config_values(the_repository)->editor_program)
> +		editor = repo_config_values(the_repository)->editor_program;

Same as the previous patch, we can store repo_config_values() and avoid
re-calling the function.
Also, do we need the right-side condition?

>  	if (!editor && !terminal_is_dumb)
>  		editor = getenv("VISUAL");
>  	if (!editor)
> diff --git a/environment.c b/environment.c
> index 5950592d63..0a01f4761a 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -55,7 +55,6 @@ int fsync_object_files = -1;
>  int use_fsync = -1;
>  enum fsync_method fsync_method = FSYNC_METHOD_DEFAULT;
>  enum fsync_component fsync_components = FSYNC_COMPONENTS_DEFAULT;
> -char *editor_program;
>  char *askpass_program;
>  enum auto_crlf auto_crlf = AUTO_CRLF_FALSE;
>  enum eol core_eol = EOL_UNSET;
> @@ -435,8 +434,8 @@ int git_default_core_config(const char *var, const char *value,
>  	}
>
>  	if (!strcmp(var, "core.editor")) {
> -		FREE_AND_NULL(editor_program);
> -		return git_config_string(&editor_program, var, value);
> +		FREE_AND_NULL(cfg->editor_program);
> +		return git_config_string(&cfg->editor_program, var, value);
>  	}
>
>  	if (!strcmp(var, "core.commentchar") ||
> @@ -723,6 +722,7 @@ void repo_config_values_init(struct repo_config_values *cfg)
>  {
>  	cfg->attributes_file = NULL;
>  	cfg->excludes_file = NULL;
> +	cfg->editor_program = NULL;
>  	cfg->apply_sparse_checkout = 0;
>  	cfg->branch_track = BRANCH_TRACK_REMOTE;
>  	cfg->trust_ctime = 1;
> @@ -753,4 +753,5 @@ void repo_config_values_clear(struct repository *repo)
>
>  	FREE_AND_NULL(cfg->attributes_file);
>  	FREE_AND_NULL(cfg->excludes_file);
> +	FREE_AND_NULL(cfg->editor_program);
>  }
> diff --git a/environment.h b/environment.h
> index 2e8352de7f..1ec19149cb 100644
> --- a/environment.h
> +++ b/environment.h
> @@ -91,6 +91,7 @@ struct repo_config_values {
>  	/* section "core" config values */
>  	char *attributes_file;
>  	char *excludes_file;
> +	char *editor_program;
>  	int apply_sparse_checkout;
>  	int trust_ctime;
>  	int check_stat;
> @@ -218,7 +219,6 @@ const char *get_commit_output_encoding(void);
>  extern char *git_commit_encoding;
>  extern char *git_log_output_encoding;
>
> -extern char *editor_program;
>  extern char *askpass_program;
>
>  /*

Regards,
Pablo