public inbox for git@vger.kernel.org
 help / color / mirror / Atom feed
* [GSOC][PATCH 0/2] Remove global state from editor.c
@ 2026-03-01 10:42 Shreyansh Paliwal
  2026-03-01 10:42 ` [GSOC][PATCH 1/2] editor: make editor_program local to editor.c Shreyansh Paliwal
                   ` (3 more replies)
  0 siblings, 4 replies; 14+ messages in thread
From: Shreyansh Paliwal @ 2026-03-01 10:42 UTC (permalink / raw)
  To: git
  Cc: gitster, christian.couder, karthik.188, jltobler, ayu.chandekar,
	siddharthasthana31, lucasseikioshiro, Shreyansh Paliwal

This series reduces reliance on global states. Mainly there
are two such global states in editor.c,

* editor_program: defined in environment.c and populated during config
  parsing, but only used by editor.c via git_editor().

* the_repository: used in git_sequence_editor() to read the sequence.editor
  configuration.

In patch 1/2, localize editor_program to editor.c by introducing a helper
that allows git_default_core_config() to continue initializing the value
during initial config parsing.

In patch 2/2, remove the remaining use of the_repository in editor.c by
passing struct repository through git_sequence_editor() and its
callers. With this change, editor.c no longer requires
'USE_THE_REPOSITORY_VARIABLE'.

Shreyansh Paliwal (2):
  editor: make editor_program local to editor.c
  editor: remove the_repository usage

 builtin/var.c        |  2 +-
 editor.c             | 18 ++++++++++++------
 editor.h             |  6 ++++--
 environment.c        |  5 ++---
 environment.h        |  1 -
 rebase-interactive.c |  2 +-
 6 files changed, 20 insertions(+), 14 deletions(-)

--
2.53.0

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [GSOC][PATCH 1/2] editor: make editor_program local to editor.c
  2026-03-01 10:42 [GSOC][PATCH 0/2] Remove global state from editor.c Shreyansh Paliwal
@ 2026-03-01 10:42 ` Shreyansh Paliwal
  2026-03-01 13:19   ` Burak Kaan Karaçay
  2026-03-01 10:42 ` [GSOC][PATCH 2/2] editor: remove the_repository usage Shreyansh Paliwal
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 14+ messages in thread
From: Shreyansh Paliwal @ 2026-03-01 10:42 UTC (permalink / raw)
  To: git
  Cc: gitster, christian.couder, karthik.188, jltobler, ayu.chandekar,
	siddharthasthana31, lucasseikioshiro, Shreyansh Paliwal

editor_program is a global variable defined in environment.c, which is set
by git_default_core_config(), but is used only by editor.c in the function
git_editor().

Remove the global from the environment.c and localize it in editor.c.
Introduce a helper for setting the local editor_program variable by the
help of git_config_string(). Call this helper in the core.editor part of
the git_default_core_config().

This keeps the existing initialization timing and availability of the
variable, so invalid core.editor values are still reported early during
startup, causing no ux regression.

Signed-off-by: Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com>
---
 editor.c      | 8 ++++++++
 editor.h      | 2 ++
 environment.c | 5 ++---
 environment.h | 1 -
 4 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/editor.c b/editor.c
index fd174e6a03..b509d23f3b 100644
--- a/editor.c
+++ b/editor.c
@@ -18,6 +18,14 @@
 #define DEFAULT_EDITOR "vi"
 #endif

+static char *editor_program;
+
+int set_editor_program(const char *var, const char *value)
+{
+	FREE_AND_NULL(editor_program);
+	return git_config_string(&editor_program, var, value);
+}
+
 int is_terminal_dumb(void)
 {
 	const char *terminal = getenv("TERM");
diff --git a/editor.h b/editor.h
index f1c41df378..ced29046f8 100644
--- a/editor.h
+++ b/editor.h
@@ -8,6 +8,8 @@ const char *git_editor(void);
 const char *git_sequence_editor(void);
 int is_terminal_dumb(void);

+int set_editor_program(const char *var, const char *value);
+
 /**
  * Launch the user preferred editor to edit a file and fill the buffer
  * with the file's contents upon the user completing their editing. The
diff --git a/environment.c b/environment.c
index 0026eb2274..9aa9124328 100644
--- a/environment.c
+++ b/environment.c
@@ -37,6 +37,7 @@
 #include "setup.h"
 #include "ws.h"
 #include "write-or-die.h"
+#include "editor.h"

 static int pack_compression_seen;
 static int zlib_compression_seen;
@@ -61,7 +62,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;
 char *excludes_file;
 enum auto_crlf auto_crlf = AUTO_CRLF_FALSE;
@@ -437,8 +437,7 @@ 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);
+		return set_editor_program(var, value);
 	}

 	if (!strcmp(var, "core.commentchar") ||
diff --git a/environment.h b/environment.h
index 27f657af04..053b678786 100644
--- a/environment.h
+++ b/environment.h
@@ -199,7 +199,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;
 extern char *excludes_file;

--
2.53.0

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [GSOC][PATCH 2/2] editor: remove the_repository usage
  2026-03-01 10:42 [GSOC][PATCH 0/2] Remove global state from editor.c Shreyansh Paliwal
  2026-03-01 10:42 ` [GSOC][PATCH 1/2] editor: make editor_program local to editor.c Shreyansh Paliwal
@ 2026-03-01 10:42 ` Shreyansh Paliwal
  2026-03-09 10:37   ` Karthik Nayak
  2026-03-01 16:39 ` [GSOC][PATCH 0/2] Remove global state from editor.c Tian Yuchen
  2026-03-10 17:40 ` [GSOC][PATCH v2 " Shreyansh Paliwal
  3 siblings, 1 reply; 14+ messages in thread
From: Shreyansh Paliwal @ 2026-03-01 10:42 UTC (permalink / raw)
  To: git
  Cc: gitster, christian.couder, karthik.188, jltobler, ayu.chandekar,
	siddharthasthana31, lucasseikioshiro, Shreyansh Paliwal

git_sequence_editor() reads sequence.editor using the_repository. Pass
struct repository through the callers instead of relying on the global
state. It is called from,

* builtin/var.c: Mostly the_repository is used in all the functions and
  there is no proper local access to a repository, so pass the_repository.

* editor.c: The caller is inside launch_sequence_editor() function which is
  called from rebase-interactive.c:edit_todo_list(), which does have a
  local repository instance, so pass it down the caller.

With no remaining global states in editor.c remove '#define
USE_THE_REPOSITORY_VARIABLE'. This removes another dependency on
the_repository and keeps editor code consistent with the ongoing effort to
reduce global state.

Signed-off-by: Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com>
---
 builtin/var.c        |  2 +-
 editor.c             | 10 ++++------
 editor.h             |  4 ++--
 rebase-interactive.c |  2 +-
 4 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/builtin/var.c b/builtin/var.c
index cc3a43cde2..7da263b129 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -38,7 +38,7 @@ static char *editor(int ident_flag UNUSED)

 static char *sequence_editor(int ident_flag UNUSED)
 {
-	return xstrdup_or_null(git_sequence_editor());
+	return xstrdup_or_null(git_sequence_editor(the_repository));
 }

 static char *pager(int ident_flag UNUSED)
diff --git a/editor.c b/editor.c
index b509d23f3b..1f97c362c2 100644
--- a/editor.c
+++ b/editor.c
@@ -1,5 +1,3 @@
-#define USE_THE_REPOSITORY_VARIABLE
-
 #include "git-compat-util.h"
 #include "abspath.h"
 #include "advice.h"
@@ -53,12 +51,12 @@ const char *git_editor(void)
 	return editor;
 }

-const char *git_sequence_editor(void)
+const char *git_sequence_editor(struct repository *r)
 {
 	const char *editor = getenv("GIT_SEQUENCE_EDITOR");

 	if (!editor)
-		repo_config_get_string_tmp(the_repository, "sequence.editor", &editor);
+		repo_config_get_string_tmp(r, "sequence.editor", &editor);
 	if (!editor)
 		editor = git_editor();

@@ -138,9 +136,9 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
 }

 int launch_sequence_editor(const char *path, struct strbuf *buffer,
-			   const char *const *env)
+			   const char *const *env, struct repository *r)
 {
-	return launch_specified_editor(git_sequence_editor(), path, buffer, env);
+	return launch_specified_editor(git_sequence_editor(r), path, buffer, env);
 }

 int strbuf_edit_interactively(struct repository *r,
diff --git a/editor.h b/editor.h
index ced29046f8..bcd0cebc85 100644
--- a/editor.h
+++ b/editor.h
@@ -5,7 +5,7 @@ struct repository;
 struct strbuf;

 const char *git_editor(void);
-const char *git_sequence_editor(void);
+const char *git_sequence_editor(struct repository *r);
 int is_terminal_dumb(void);

 int set_editor_program(const char *var, const char *value);
@@ -21,7 +21,7 @@ int launch_editor(const char *path, struct strbuf *buffer,
 		  const char *const *env);

 int launch_sequence_editor(const char *path, struct strbuf *buffer,
-			   const char *const *env);
+			   const char *const *env, struct repository *r);

 /*
  * In contrast to `launch_editor()`, this function writes out the contents
diff --git a/rebase-interactive.c b/rebase-interactive.c
index 809f76a87b..405ef353af 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -132,7 +132,7 @@ int edit_todo_list(struct repository *r, struct replay_opts *opts,
 				    (flags | TODO_LIST_APPEND_TODO_HELP) & ~TODO_LIST_SHORTEN_IDS) < 0)
 		return error(_("could not write '%s'."), rebase_path_todo_backup());

-	if (launch_sequence_editor(todo_file, &new_todo->buf, NULL))
+	if (launch_sequence_editor(todo_file, &new_todo->buf, NULL, r))
 		return -2;

 	strbuf_stripspace(&new_todo->buf, comment_line_str);
--
2.53.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [GSOC][PATCH 1/2] editor: make editor_program local to editor.c
  2026-03-01 10:42 ` [GSOC][PATCH 1/2] editor: make editor_program local to editor.c Shreyansh Paliwal
@ 2026-03-01 13:19   ` Burak Kaan Karaçay
  2026-03-01 15:42     ` Shreyansh Paliwal
  2026-03-01 16:22     ` Phillip Wood
  0 siblings, 2 replies; 14+ messages in thread
From: Burak Kaan Karaçay @ 2026-03-01 13:19 UTC (permalink / raw)
  To: Shreyansh Paliwal
  Cc: git, gitster, christian.couder, karthik.188, jltobler,
	ayu.chandekar, siddharthasthana31, lucasseikioshiro

Hi Shreyansh,

I am a GSoC applicant like you. I just wanted to leave my two cents 
here.

On Sun, Mar 01, 2026 at 04:12:58PM +0530, Shreyansh Paliwal wrote:
>+static char *editor_program;
>+
>+int set_editor_program(const char *var, const char *value)
>+{
>+	FREE_AND_NULL(editor_program);
>+	return git_config_string(&editor_program, var, value);
>+}
>+

While moving the global variable from 'environment.c' to 'editor.c'
doesn't cause any behavior change, it still relies on global state.

I think passing a 'struct repository' and using the 'repo_config_get*'
helpers here might be a more robust approach. I know this means we would
catch config errors later (right before the editor start up). However,
since it doesn't seem like it would cause a data loss or serious issues,
this behavioral change feels like a reasonable trade-off.

Thanks again for the patches!

Best,
Burak Kaan Karaçay

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [GSOC][PATCH 1/2] editor: make editor_program local to editor.c
  2026-03-01 13:19   ` Burak Kaan Karaçay
@ 2026-03-01 15:42     ` Shreyansh Paliwal
  2026-03-01 16:22     ` Phillip Wood
  1 sibling, 0 replies; 14+ messages in thread
From: Shreyansh Paliwal @ 2026-03-01 15:42 UTC (permalink / raw)
  To: git
  Cc: gitster, christian.couder, karthik.188, jltobler, ayu.chandekar,
	siddharthasthana31, bkkaracay, lucasseikioshiro

> Hi Shreyansh,
>
> I am a GSoC applicant like you. I just wanted to leave my two cents
> here.
>
> On Sun, Mar 01, 2026 at 04:12:58PM +0530, Shreyansh Paliwal wrote:
> >+static char *editor_program;
> >+
> >+int set_editor_program(const char *var, const char *value)
> >+{
> >+	FREE_AND_NULL(editor_program);
> >+	return git_config_string(&editor_program, var, value);
> >+}
> >+
>
> While moving the global variable from 'environment.c' to 'editor.c'
> doesn't cause any behavior change, it still relies on global state.
>
> I think passing a 'struct repository' and using the 'repo_config_get*'
> helpers here might be a more robust approach. I know this means we would
> catch config errors later (right before the editor start up). However,
> since it doesn't seem like it would cause a data loss or serious issues,
> this behavioral change feels like a reasonable trade-off.
>
> Thanks again for the patches!

Hi Burak,

Thanks for the feedback on this, I appreciate you taking the time to look.

I did consider the approach you suggested. Currently, editor_program is
only used within editor.c, and it is I believe a process-wide setting rather
than something tied to a specific repository. Because of that, it did not
seem necessary to add it to struct repository or repo_settings at this stage.

More importantly, my intention for this was to keep original behavior as-is.
As noted in earlier discussions [1][2], maintaining early config validation
is important so that invalid core.editor values are caught early. Moving to
a repo-based lazy lookup would change that.

That said, I agree that there may be a better way to do this refactor,
so I'd be glad to hear more thoughts on this :)

Best,
Shreyansh

[1]- https://lore.kernel.org/git/1d43d1d0-bf6b-4806-834e-89f545fab766@gmail.com/
[2]- https://lore.kernel.org/git/xmqqpl63b2tm.fsf@gitster.g/

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [GSOC][PATCH 1/2] editor: make editor_program local to editor.c
  2026-03-01 13:19   ` Burak Kaan Karaçay
  2026-03-01 15:42     ` Shreyansh Paliwal
@ 2026-03-01 16:22     ` Phillip Wood
  2026-03-01 18:30       ` Burak Kaan Karaçay
  1 sibling, 1 reply; 14+ messages in thread
From: Phillip Wood @ 2026-03-01 16:22 UTC (permalink / raw)
  To: Burak Kaan Karaçay, Shreyansh Paliwal
  Cc: git, gitster, christian.couder, karthik.188, jltobler,
	ayu.chandekar, siddharthasthana31, lucasseikioshiro

Hi Burak

On 01/03/2026 13:19, Burak Kaan Karaçay wrote:
> Hi Shreyansh,
> 
> I am a GSoC applicant like you. I just wanted to leave my two cents here.
> 
> On Sun, Mar 01, 2026 at 04:12:58PM +0530, Shreyansh Paliwal wrote:
>> +static char *editor_program;
>> +
>> +int set_editor_program(const char *var, const char *value)
>> +{
>> +    FREE_AND_NULL(editor_program);
>> +    return git_config_string(&editor_program, var, value);
>> +}
>> +
> 
> While moving the global variable from 'environment.c' to 'editor.c'
> doesn't cause any behavior change, it still relies on global state.

That's true, but does it really make sense for this config setting 
per-repository? Why would I want to use different editors for different 
repositories in the same process?

Thanks

Phillip

> I think passing a 'struct repository' and using the 'repo_config_get*'
> helpers here might be a more robust approach. I know this means we would
> catch config errors later (right before the editor start up). However,
> since it doesn't seem like it would cause a data loss or serious issues,
> this behavioral change feels like a reasonable trade-off.
> 
> Thanks again for the patches!
> 
> Best,
> Burak Kaan Karaçay
> 


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [GSOC][PATCH 0/2] Remove global state from editor.c
  2026-03-01 10:42 [GSOC][PATCH 0/2] Remove global state from editor.c Shreyansh Paliwal
  2026-03-01 10:42 ` [GSOC][PATCH 1/2] editor: make editor_program local to editor.c Shreyansh Paliwal
  2026-03-01 10:42 ` [GSOC][PATCH 2/2] editor: remove the_repository usage Shreyansh Paliwal
@ 2026-03-01 16:39 ` Tian Yuchen
  2026-03-10 17:40 ` [GSOC][PATCH v2 " Shreyansh Paliwal
  3 siblings, 0 replies; 14+ messages in thread
From: Tian Yuchen @ 2026-03-01 16:39 UTC (permalink / raw)
  To: Shreyansh Paliwal, git
  Cc: gitster, christian.couder, karthik.188, jltobler, ayu.chandekar,
	siddharthasthana31, lucasseikioshiro

Hi Shreyansh and Burak,

Thanks for the patch.

Reading through discussion, I think both of you highlighted very valid 
constraints:

> While moving the global variable from 'environment.c' to 'editor.c'
> doesn't cause any behavior change, it still relies on global state.

Yes, changing an extern to a static variable doesn't truly remove the 
global state, right?

> More importantly, my intention for this was to keep original behavior as-is.
> As noted in earlier discussions [1][2], maintaining early config validation
> is important so that invalid core.editor values are caught early. Moving to
> a repo-based lazy lookup would change that.

This one also makes sense to me.

However,

> I believe a process-wide setting rather
> than something tied to a specific repository.

I have reservations about this, and I believe this is the most critical 
issue. For instance, we can run:

	git config --local core.editor "nvim"

where the configuration is written in the .git/config of the current 
repository. If core.editor is process-wide, git should not permit the 
existence of a "local" core.editor at all. Since it can be set for 
individual repositories, it should be tied to the specific struct 
repository, right?

A more intuitive case is:

	Repo A: core.editor = vim
	Repo B: core.editor = nvim

For users managing multiple repositories (submodules), it's perfectly 
reasonable to use different editors in different contexts. At least for 
me, I use different configurations for Vim and NVim, and I switch 
between different editors when writing with different languages. (like 
set textwidth=72 for Git? _(:3 ⌒゙)_)

I recently faced the same dilemma migrating git_commit_encoding and 
git_log_output_encoding. I personally believe that adding editor_program 
to repo-settings.c is the best approach.

By doing this:

- We truly eliminate the global state. Each struct repository gets its 
own editor setting.

- We maintain early validation. The config can still be parsed early 
(e.g., during prepare_repo_settings()?)

Thanks again for the patch.

Regards,

Yuchen



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [GSOC][PATCH 1/2] editor: make editor_program local to editor.c
  2026-03-01 16:22     ` Phillip Wood
@ 2026-03-01 18:30       ` Burak Kaan Karaçay
  2026-03-09 10:36         ` Karthik Nayak
  0 siblings, 1 reply; 14+ messages in thread
From: Burak Kaan Karaçay @ 2026-03-01 18:30 UTC (permalink / raw)
  To: phillip.wood
  Cc: Shreyansh Paliwal, git, gitster, christian.couder, karthik.188,
	jltobler, ayu.chandekar, siddharthasthana31, lucasseikioshiro

On Sun, Mar 01, 2026 at 04:22:38PM +0000, Phillip Wood wrote:
>>While moving the global variable from 'environment.c' to 'editor.c'
>>doesn't cause any behavior change, it still relies on global state.
>
>That's true, but does it really make sense for this config setting 
>per-repository? Why would I want to use different editors for 
>different repositories in the same process?
>
>Thanks
>
>Phillip

In practical sense, yes, it's true. Users generally don't use different
editors for different repositories. For repository dependent settings 
.editorconfig mostly cover all scenarios.

However, as far as I know git doesn't have a system-wide only
configuration settings. These changes mostly serve to libification
process of git. If we leave 'core.editor' setting as a global variable 
and user tries to interact with multiple repositories that have
different editor configurations using our libified git, it can mix up
the configs of two repositories.

If we really want to keep these variables independent from repositories,
we should probably prohibit 'core.editor' setting in local repository
configs. Otherwise, leaving it global seems like a weird behavioral
choice.

Thanks,
Burak Kaan Karaçay

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [GSOC][PATCH 1/2] editor: make editor_program local to editor.c
  2026-03-01 18:30       ` Burak Kaan Karaçay
@ 2026-03-09 10:36         ` Karthik Nayak
  0 siblings, 0 replies; 14+ messages in thread
From: Karthik Nayak @ 2026-03-09 10:36 UTC (permalink / raw)
  To: Burak Kaan Karaçay, phillip.wood
  Cc: Shreyansh Paliwal, git, gitster, christian.couder, jltobler,
	ayu.chandekar, siddharthasthana31, lucasseikioshiro

[-- Attachment #1: Type: text/plain, Size: 1731 bytes --]

Burak Kaan Karaçay <bkkaracay@gmail.com> writes:

> On Sun, Mar 01, 2026 at 04:22:38PM +0000, Phillip Wood wrote:
>>>While moving the global variable from 'environment.c' to 'editor.c'
>>>doesn't cause any behavior change, it still relies on global state.
>>
>>That's true, but does it really make sense for this config setting
>>per-repository? Why would I want to use different editors for
>>different repositories in the same process?
>>
>>Thanks
>>
>>Phillip
>
> In practical sense, yes, it's true. Users generally don't use different
> editors for different repositories. For repository dependent settings
> .editorconfig mostly cover all scenarios.
>
> However, as far as I know git doesn't have a system-wide only
> configuration settings. These changes mostly serve to libification
> process of git. If we leave 'core.editor' setting as a global variable
> and user tries to interact with multiple repositories that have
> different editor configurations using our libified git, it can mix up
> the configs of two repositories.
>
> If we really want to keep these variables independent from repositories,
> we should probably prohibit 'core.editor' setting in local repository
> configs. Otherwise, leaving it global seems like a weird behavioral
> choice.
>
> Thanks,
> Burak Kaan Karaçay

I do agree with the point you're making, true isolation for libification
would indeed require that this variable is not a global variable.

But, while libification is the destination, steps in that direction
should be welcome, and I think one step is to simply localize the global
variables.

Also bloating up `struct repository` without much thought might not be a
good decision either.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [GSOC][PATCH 2/2] editor: remove the_repository usage
  2026-03-01 10:42 ` [GSOC][PATCH 2/2] editor: remove the_repository usage Shreyansh Paliwal
@ 2026-03-09 10:37   ` Karthik Nayak
  0 siblings, 0 replies; 14+ messages in thread
From: Karthik Nayak @ 2026-03-09 10:37 UTC (permalink / raw)
  To: Shreyansh Paliwal, git
  Cc: gitster, christian.couder, jltobler, ayu.chandekar,
	siddharthasthana31, lucasseikioshiro

[-- Attachment #1: Type: text/plain, Size: 878 bytes --]

Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com> writes:

> git_sequence_editor() reads sequence.editor using the_repository. Pass
> struct repository through the callers instead of relying on the global
> state. It is called from,
>
> * builtin/var.c: Mostly the_repository is used in all the functions and
>   there is no proper local access to a repository, so pass the_repository.
>
> * editor.c: The caller is inside launch_sequence_editor() function which is
>   called from rebase-interactive.c:edit_todo_list(), which does have a
>   local repository instance, so pass it down the caller.
>
> With no remaining global states in editor.c remove '#define
> USE_THE_REPOSITORY_VARIABLE'. This removes another dependency on
> the_repository and keeps editor code consistent with the ongoing effort to
> reduce global state.
>

Well explained, the patch looks good to me.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [GSOC][PATCH v2 0/2] Remove global state from editor.c
  2026-03-01 10:42 [GSOC][PATCH 0/2] Remove global state from editor.c Shreyansh Paliwal
                   ` (2 preceding siblings ...)
  2026-03-01 16:39 ` [GSOC][PATCH 0/2] Remove global state from editor.c Tian Yuchen
@ 2026-03-10 17:40 ` Shreyansh Paliwal
  2026-03-10 17:40   ` [GSOC][PATCH v2 1/2] editor: make editor_program local to editor.c Shreyansh Paliwal
                     ` (2 more replies)
  3 siblings, 3 replies; 14+ messages in thread
From: Shreyansh Paliwal @ 2026-03-10 17:40 UTC (permalink / raw)
  To: git
  Cc: gitster, christian.couder, karthik.188, jltobler, ayu.chandekar,
	siddharthasthana31, lucasseikioshiro, Shreyansh Paliwal

This series reduces reliance on global states. Mainly there
are two such global states in editor.c,

* editor_program: defined in environment.c and populated during config
  parsing, but only used by editor.c via git_editor().

* the_repository: used in git_sequence_editor() to read the sequence.editor
  configuration.

In patch 1/2, localize editor_program to editor.c by introducing a helper
that allows git_default_core_config() to continue initializing the value
during initial config parsing.

In patch 2/2, remove the remaining use of the_repository in editor.c by
passing struct repository through git_sequence_editor() and its
callers. With this change, editor.c no longer requires
'USE_THE_REPOSITORY_VARIABLE' and 'environment.h' include.

Shreyansh Paliwal (2):
  editor: make editor_program local to editor.c
  editor: remove the_repository usage

 builtin/var.c        |  2 +-
 editor.c             | 19 ++++++++++++-------
 editor.h             |  6 ++++--
 environment.c        |  5 ++---
 environment.h        |  1 -
 rebase-interactive.c |  2 +-
 6 files changed, 20 insertions(+), 15 deletions(-)

---
Changes in v2:
 - removed 'environment.h' dependency from editor.c as well.

Range-diff against v1:
-:  ---------- > 1:  6f8b82fed5 editor: make editor_program local to editor.c
1:  f9ef18b77a ! 2:  5b858c7e98 editor: remove the_repository usage
    @@ Commit message
           local repository instance, so pass it down the caller.

         With no remaining global states in editor.c remove '#define
    -    USE_THE_REPOSITORY_VARIABLE'. This removes another dependency on
    -    the_repository and keeps editor code consistent with the ongoing effort to
    -    reduce global state.
    +    USE_THE_REPOSITORY_VARIABLE' and drop the dependency on 'environment.h'.
    +    This removes another dependency on the_repository and keeps editor code
    +    consistent with the ongoing effort to reduce global state.

         Signed-off-by: Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com>

    @@ editor.c
      #include "git-compat-util.h"
      #include "abspath.h"
      #include "advice.h"
    + #include "config.h"
    + #include "editor.h"
    +-#include "environment.h"
    + #include "gettext.h"
    + #include "pager.h"
    + #include "path.h"
     @@ editor.c: const char *git_editor(void)
      	return editor;
      }
--
2.53.0


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [GSOC][PATCH v2 1/2] editor: make editor_program local to editor.c
  2026-03-10 17:40 ` [GSOC][PATCH v2 " Shreyansh Paliwal
@ 2026-03-10 17:40   ` Shreyansh Paliwal
  2026-03-10 17:40   ` [GSOC][PATCH v2 2/2] editor: remove the_repository usage Shreyansh Paliwal
  2026-03-17 16:03   ` [GSOC][PATCH v2 0/2] Remove global state from editor.c Shreyansh Paliwal
  2 siblings, 0 replies; 14+ messages in thread
From: Shreyansh Paliwal @ 2026-03-10 17:40 UTC (permalink / raw)
  To: git
  Cc: gitster, christian.couder, karthik.188, jltobler, ayu.chandekar,
	siddharthasthana31, lucasseikioshiro, Shreyansh Paliwal

editor_program is a global variable defined in environment.c, which is set
by git_default_core_config(), but is used only by editor.c only in the
function git_editor().

Remove the global from the environment.c and localize it in editor.c.
Introduce a helper for setting the local editor_program variable by the
help of git_config_string(). Call this helper in the core.editor part of
the git_default_core_config().

This keeps the existing initialization timing and availability of the
variable, so invalid core.editor values are still reported early during
startup, causing no ux regression.

Signed-off-by: Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com>
---
 editor.c      | 8 ++++++++
 editor.h      | 2 ++
 environment.c | 5 ++---
 environment.h | 1 -
 4 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/editor.c b/editor.c
index fd174e6a03..b509d23f3b 100644
--- a/editor.c
+++ b/editor.c
@@ -18,6 +18,14 @@
 #define DEFAULT_EDITOR "vi"
 #endif

+static char *editor_program;
+
+int set_editor_program(const char *var, const char *value)
+{
+	FREE_AND_NULL(editor_program);
+	return git_config_string(&editor_program, var, value);
+}
+
 int is_terminal_dumb(void)
 {
 	const char *terminal = getenv("TERM");
diff --git a/editor.h b/editor.h
index f1c41df378..ced29046f8 100644
--- a/editor.h
+++ b/editor.h
@@ -8,6 +8,8 @@ const char *git_editor(void);
 const char *git_sequence_editor(void);
 int is_terminal_dumb(void);

+int set_editor_program(const char *var, const char *value);
+
 /**
  * Launch the user preferred editor to edit a file and fill the buffer
  * with the file's contents upon the user completing their editing. The
diff --git a/environment.c b/environment.c
index 0026eb2274..9aa9124328 100644
--- a/environment.c
+++ b/environment.c
@@ -37,6 +37,7 @@
 #include "setup.h"
 #include "ws.h"
 #include "write-or-die.h"
+#include "editor.h"

 static int pack_compression_seen;
 static int zlib_compression_seen;
@@ -61,7 +62,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;
 char *excludes_file;
 enum auto_crlf auto_crlf = AUTO_CRLF_FALSE;
@@ -437,8 +437,7 @@ 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);
+		return set_editor_program(var, value);
 	}

 	if (!strcmp(var, "core.commentchar") ||
diff --git a/environment.h b/environment.h
index 27f657af04..053b678786 100644
--- a/environment.h
+++ b/environment.h
@@ -199,7 +199,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;
 extern char *excludes_file;

--
2.53.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [GSOC][PATCH v2 2/2] editor: remove the_repository usage
  2026-03-10 17:40 ` [GSOC][PATCH v2 " Shreyansh Paliwal
  2026-03-10 17:40   ` [GSOC][PATCH v2 1/2] editor: make editor_program local to editor.c Shreyansh Paliwal
@ 2026-03-10 17:40   ` Shreyansh Paliwal
  2026-03-17 16:03   ` [GSOC][PATCH v2 0/2] Remove global state from editor.c Shreyansh Paliwal
  2 siblings, 0 replies; 14+ messages in thread
From: Shreyansh Paliwal @ 2026-03-10 17:40 UTC (permalink / raw)
  To: git
  Cc: gitster, christian.couder, karthik.188, jltobler, ayu.chandekar,
	siddharthasthana31, lucasseikioshiro, Shreyansh Paliwal

git_sequence_editor() reads sequence.editor using the_repository. Pass
struct repository through the callers instead of relying on the global
state. It is called from,

- builtin/var.c: Mostly the_repository is used in all the functions and
  there is no proper local access to a repository, so pass the_repository.

- editor.c: The caller is inside launch_sequence_editor() function which is
  called from rebase-interactive.c:edit_todo_list(), which does have a
  local repository instance, so pass it down the caller.

With no remaining global states in editor.c remove '#define
USE_THE_REPOSITORY_VARIABLE' and drop the dependency on 'environment.h'.
This removes another dependency on the_repository and keeps editor code
consistent with the ongoing effort to reduce global state.

Signed-off-by: Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com>
---
 builtin/var.c        |  2 +-
 editor.c             | 11 ++++-------
 editor.h             |  4 ++--
 rebase-interactive.c |  2 +-
 4 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/builtin/var.c b/builtin/var.c
index cc3a43cde2..7da263b129 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -38,7 +38,7 @@ static char *editor(int ident_flag UNUSED)

 static char *sequence_editor(int ident_flag UNUSED)
 {
-	return xstrdup_or_null(git_sequence_editor());
+	return xstrdup_or_null(git_sequence_editor(the_repository));
 }

 static char *pager(int ident_flag UNUSED)
diff --git a/editor.c b/editor.c
index b509d23f3b..b78c8a687f 100644
--- a/editor.c
+++ b/editor.c
@@ -1,11 +1,8 @@
-#define USE_THE_REPOSITORY_VARIABLE
-
 #include "git-compat-util.h"
 #include "abspath.h"
 #include "advice.h"
 #include "config.h"
 #include "editor.h"
-#include "environment.h"
 #include "gettext.h"
 #include "pager.h"
 #include "path.h"
@@ -53,12 +50,12 @@ const char *git_editor(void)
 	return editor;
 }

-const char *git_sequence_editor(void)
+const char *git_sequence_editor(struct repository *r)
 {
 	const char *editor = getenv("GIT_SEQUENCE_EDITOR");

 	if (!editor)
-		repo_config_get_string_tmp(the_repository, "sequence.editor", &editor);
+		repo_config_get_string_tmp(r, "sequence.editor", &editor);
 	if (!editor)
 		editor = git_editor();

@@ -138,9 +135,9 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
 }

 int launch_sequence_editor(const char *path, struct strbuf *buffer,
-			   const char *const *env)
+			   const char *const *env, struct repository *r)
 {
-	return launch_specified_editor(git_sequence_editor(), path, buffer, env);
+	return launch_specified_editor(git_sequence_editor(r), path, buffer, env);
 }

 int strbuf_edit_interactively(struct repository *r,
diff --git a/editor.h b/editor.h
index ced29046f8..bcd0cebc85 100644
--- a/editor.h
+++ b/editor.h
@@ -5,7 +5,7 @@ struct repository;
 struct strbuf;

 const char *git_editor(void);
-const char *git_sequence_editor(void);
+const char *git_sequence_editor(struct repository *r);
 int is_terminal_dumb(void);

 int set_editor_program(const char *var, const char *value);
@@ -21,7 +21,7 @@ int launch_editor(const char *path, struct strbuf *buffer,
 		  const char *const *env);

 int launch_sequence_editor(const char *path, struct strbuf *buffer,
-			   const char *const *env);
+			   const char *const *env, struct repository *r);

 /*
  * In contrast to `launch_editor()`, this function writes out the contents
diff --git a/rebase-interactive.c b/rebase-interactive.c
index 809f76a87b..405ef353af 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -132,7 +132,7 @@ int edit_todo_list(struct repository *r, struct replay_opts *opts,
 				    (flags | TODO_LIST_APPEND_TODO_HELP) & ~TODO_LIST_SHORTEN_IDS) < 0)
 		return error(_("could not write '%s'."), rebase_path_todo_backup());

-	if (launch_sequence_editor(todo_file, &new_todo->buf, NULL))
+	if (launch_sequence_editor(todo_file, &new_todo->buf, NULL, r))
 		return -2;

 	strbuf_stripspace(&new_todo->buf, comment_line_str);
--
2.53.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [GSOC][PATCH v2 0/2] Remove global state from editor.c
  2026-03-10 17:40 ` [GSOC][PATCH v2 " Shreyansh Paliwal
  2026-03-10 17:40   ` [GSOC][PATCH v2 1/2] editor: make editor_program local to editor.c Shreyansh Paliwal
  2026-03-10 17:40   ` [GSOC][PATCH v2 2/2] editor: remove the_repository usage Shreyansh Paliwal
@ 2026-03-17 16:03   ` Shreyansh Paliwal
  2 siblings, 0 replies; 14+ messages in thread
From: Shreyansh Paliwal @ 2026-03-17 16:03 UTC (permalink / raw)
  To: git
  Cc: gitster, christian.couder, karthik.188, jltobler, ayu.chandekar,
	siddharthasthana31, lucasseikioshiro

> This series reduces reliance on global states. Mainly there
> are two such global states in editor.c,
>
> * editor_program: defined in environment.c and populated during config
>   parsing, but only used by editor.c via git_editor().
>
> * the_repository: used in git_sequence_editor() to read the sequence.editor
>   configuration.
>
> In patch 1/2, localize editor_program to editor.c by introducing a helper
> that allows git_default_core_config() to continue initializing the value
> during initial config parsing.
>
> In patch 2/2, remove the remaining use of the_repository in editor.c by
> passing struct repository through git_sequence_editor() and its
> callers. With this change, editor.c no longer requires
> 'USE_THE_REPOSITORY_VARIABLE' and 'environment.h' include.
>
> Shreyansh Paliwal (2):
>   editor: make editor_program local to editor.c
>   editor: remove the_repository usage
>
>  builtin/var.c        |  2 +-
>  editor.c             | 19 ++++++++++++-------
>  editor.h             |  6 ++++--
>  environment.c        |  5 ++---
>  environment.h        |  1 -
>  rebase-interactive.c |  2 +-
>  6 files changed, 20 insertions(+), 15 deletions(-)
>
> ---
> Changes in v2:
>  - removed 'environment.h' dependency from editor.c as well.
>

Hi,

Sorry for the late follow-up, I was tied up with my college exams.
I just wanted to check if there is any specific feedback or concern
that is holding this series back. Do let me know :)

Best,
Shreyansh

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2026-03-17 16:05 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-03-01 10:42 [GSOC][PATCH 0/2] Remove global state from editor.c Shreyansh Paliwal
2026-03-01 10:42 ` [GSOC][PATCH 1/2] editor: make editor_program local to editor.c Shreyansh Paliwal
2026-03-01 13:19   ` Burak Kaan Karaçay
2026-03-01 15:42     ` Shreyansh Paliwal
2026-03-01 16:22     ` Phillip Wood
2026-03-01 18:30       ` Burak Kaan Karaçay
2026-03-09 10:36         ` Karthik Nayak
2026-03-01 10:42 ` [GSOC][PATCH 2/2] editor: remove the_repository usage Shreyansh Paliwal
2026-03-09 10:37   ` Karthik Nayak
2026-03-01 16:39 ` [GSOC][PATCH 0/2] Remove global state from editor.c Tian Yuchen
2026-03-10 17:40 ` [GSOC][PATCH v2 " Shreyansh Paliwal
2026-03-10 17:40   ` [GSOC][PATCH v2 1/2] editor: make editor_program local to editor.c Shreyansh Paliwal
2026-03-10 17:40   ` [GSOC][PATCH v2 2/2] editor: remove the_repository usage Shreyansh Paliwal
2026-03-17 16:03   ` [GSOC][PATCH v2 0/2] Remove global state from editor.c Shreyansh Paliwal

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox