[RFC PATCH 0/1] config: surface editor failure in exit code

[RFC PATCH 0/1] config: surface editor failure in exit code

[Kenneth Lorber]

When the editor invoked by 'git config -e' fails (crashes or calls exit(3)
with a non-zero value), git notices and give an error:
	editor.c:launch_specified_editor()
		return error("there was a problem with the editor '%s'", editor);
which is then lost:
	builtin/config.c:show_editor()
		launch_editor(config_file, NULL, NULL);
which results in git always calling exit(0).  Note that the value is
not explicitly thrown away with "(void)", so this may not have been
intentional.

This patch simply passes the returned error out of show_editor(), which
currently has an unconditional "return 0" even though its callers
both check the return value.

While this didn't trigger anything in 'make test', it's possible that
someone is relying on 'git config -e' always succeeding, even if the
editor failed, so this could be considered a breaking change.

The 2 new tests set GIT_EDITOR to true and false and check the return
from git.

RFC because the community may not want to change this behavior and
I'm not thrilled with my test code.

Kenneth Lorber (1):
  config: surface editor failure in exit code

 builtin/config.c  |  5 +++--
 t/t1300-config.sh | 18 ++++++++++++++++++
 2 files changed, 21 insertions(+), 2 deletions(-)


base-commit: 010afd3166ddc64c9863b1506f12cbcdda0d4ea1
-- 
2.43.0

[RFC PATCH 1/1] config: surface editor failure in exit code

[Kenneth Lorber]

Teach git config --edit to show editor failure to the
parent process.

Add 2 tests to t1300 to check editor exiting successfully
or failing.

Signed-off-by: Kenneth Lorber <keni@his.com>
---
 builtin/config.c  |  5 +++--
 t/t1300-config.sh | 18 ++++++++++++++++++
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 0882899c3f..a166b2131e 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -1291,6 +1291,7 @@ static int cmd_config_remove_section(int argc, const char **argv, const char *pr
 static int show_editor(struct config_location_options *opts)
 {
 	char *config_file;
+	int ret;
 
 	if (!opts->source.file && !startup_info->have_repository)
 		die(_("not in a git directory"));
@@ -1313,10 +1314,10 @@ static int show_editor(struct config_location_options *opts)
 		else if (errno != EEXIST)
 			die_errno(_("cannot create configuration file %s"), config_file);
 	}
-	launch_editor(config_file, NULL, NULL);
+	ret = launch_editor(config_file, NULL, NULL);
 	free(config_file);
 
-	return 0;
+	return ret;
 }
 
 static int cmd_config_edit(int argc, const char **argv, const char *prefix,
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index e3f8064889..9a8f852a86 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1823,6 +1823,24 @@ test_expect_success 'command line overrides environment config' '
 	test_cmp expect actual
 '
 
+test_expect_success 'git config --edit successful exit' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	GIT_EDITOR=true &&
+	export GIT_EDITOR &&
+	git -C repo config -e &&
+	unset GIT_EDITOR
+'
+
+test_expect_success 'git config --edit failure exit' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	GIT_EDITOR=false &&
+	export GIT_EDITOR &&
+	test_must_fail git -C repo config -e &&
+	unset GIT_EDITOR
+'
+
 test_expect_success 'git config --edit works' '
 	git config -f tmp test.value no &&
 	echo test.value=yes >expect &&
-- 
2.43.0

Re: [RFC PATCH 0/1] config: surface editor failure in exit code

[Junio C Hamano]

Kenneth Lorber <keni@his.com> writes:

> When the editor invoked by 'git config -e' fails (crashes or calls exit(3)
> with a non-zero value), git notices and give an error:
> 	editor.c:launch_specified_editor()
> 		return error("there was a problem with the editor '%s'", editor);
> which is then lost:
> 	builtin/config.c:show_editor()
> 		launch_editor(config_file, NULL, NULL);
> which results in git always calling exit(0).  Note that the value is
> not explicitly thrown away with "(void)", so this may not have been
> intentional.

I do not intentionally exit my editor with a non-zero status myself,
but what I hear from others who do is that they do so to affect the
invoking 'git' command, e.g., to stop 'git commit' from creating a
commit.  They somehow realize they botched the edit, and they want
to prevent 'git commit' from committing, signaling that by exiting
their editor.  A cleaner and more modern way to do so, by the way,
is to empty the editor buffer.  In either case, 'git commit' itself
exits with a non-zero status.

It might have been more consistent if 'git config -e' exited with a
non-zero status when it noticed that the editor exited with a
non-zero status, in that sense.  But we have never done so, and that
is probably because we did not care ;-)

In any case, I am not sure whether there is much value in making
'git config -e' start behaving that way.  Even if it can notice a
failed editor, the damage to the file is already done, and there is
not enough information to undo the damage even if you wanted to when
detecting such an error.  This is quite different from when an editor
edits the 'COMMIT_EDITMSG' file and fails.

So, I dunno.

Re: [RFC PATCH 0/1] config: surface editor failure in exit code

[Karthik Nayak]

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

Junio C Hamano <gitster@pobox.com> writes:

> Kenneth Lorber <keni@his.com> writes:
>
>> When the editor invoked by 'git config -e' fails (crashes or calls exit(3)
>> with a non-zero value), git notices and give an error:
>> 	editor.c:launch_specified_editor()
>> 		return error("there was a problem with the editor '%s'", editor);
>> which is then lost:
>> 	builtin/config.c:show_editor()
>> 		launch_editor(config_file, NULL, NULL);
>> which results in git always calling exit(0).  Note that the value is
>> not explicitly thrown away with "(void)", so this may not have been
>> intentional.
>
> I do not intentionally exit my editor with a non-zero status myself,
> but what I hear from others who do is that they do so to affect the
> invoking 'git' command, e.g., to stop 'git commit' from creating a
> commit.  They somehow realize they botched the edit, and they want
> to prevent 'git commit' from committing, signaling that by exiting
> their editor.  A cleaner and more modern way to do so, by the way,
> is to empty the editor buffer.  In either case, 'git commit' itself
> exits with a non-zero status.
>
> It might have been more consistent if 'git config -e' exited with a
> non-zero status when it noticed that the editor exited with a
> non-zero status, in that sense.  But we have never done so, and that
> is probably because we did not care ;-)
>
> In any case, I am not sure whether there is much value in making
> 'git config -e' start behaving that way.  Even if it can notice a
> failed editor, the damage to the file is already done, and there is
> not enough information to undo the damage even if you wanted to when
> detecting such an error.  This is quite different from when an editor
> edits the 'COMMIT_EDITMSG' file and fails.
>
> So, I dunno.

Wouldn't it be better to notify the user that something went wrong
rather than simply brush it off?

I would be in support of the patch:

  $ GIT_EDITOR=false git config --edit
  error: there was a problem with the editor 'false'
  $ echo $status
  0

As a user the expectation here would be a non-zero exit status.

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

Re: [RFC PATCH 1/1] config: surface editor failure in exit code

[Karthik Nayak]

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

Kenneth Lorber <keni@his.com> writes:

> Teach git config --edit to show editor failure to the
> parent process.
>
> Add 2 tests to t1300 to check editor exiting successfully
> or failing.
>
> Signed-off-by: Kenneth Lorber <keni@his.com>
> ---
>  builtin/config.c  |  5 +++--
>  t/t1300-config.sh | 18 ++++++++++++++++++
>  2 files changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/config.c b/builtin/config.c
> index 0882899c3f..a166b2131e 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -1291,6 +1291,7 @@ static int cmd_config_remove_section(int argc, const char **argv, const char *pr
>  static int show_editor(struct config_location_options *opts)
>  {
>  	char *config_file;
> +	int ret;
>
>  	if (!opts->source.file && !startup_info->have_repository)
>  		die(_("not in a git directory"));
> @@ -1313,10 +1314,10 @@ static int show_editor(struct config_location_options *opts)
>  		else if (errno != EEXIST)
>  			die_errno(_("cannot create configuration file %s"), config_file);
>  	}
> -	launch_editor(config_file, NULL, NULL);
> +	ret = launch_editor(config_file, NULL, NULL);
>  	free(config_file);
>
> -	return 0;
> +	return ret;
>  }
>
>  static int cmd_config_edit(int argc, const char **argv, const char *prefix,
> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> index e3f8064889..9a8f852a86 100755
> --- a/t/t1300-config.sh
> +++ b/t/t1300-config.sh
> @@ -1823,6 +1823,24 @@ test_expect_success 'command line overrides environment config' '
>  	test_cmp expect actual
>  '
>
> +test_expect_success 'git config --edit successful exit' '
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	GIT_EDITOR=true &&
> +	export GIT_EDITOR &&
> +	git -C repo config -e &&
> +	unset GIT_EDITOR
> +'

Nit: couldn't this be simply `test_env GIT_EDITOR=true git -C repo
config -e` and avoid the set, export and unset?

> +
> +test_expect_success 'git config --edit failure exit' '
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	GIT_EDITOR=false &&
> +	export GIT_EDITOR &&
> +	test_must_fail git -C repo config -e &&
> +	unset GIT_EDITOR
> +'

Same here..

> +
>  test_expect_success 'git config --edit works' '
>  	git config -f tmp test.value no &&
>  	echo test.value=yes >expect &&
> --
> 2.43.0

The patch looks good to me otherwise :)

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

Re: [RFC PATCH 0/1] config: surface editor failure in exit code

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> Wouldn't it be better to notify the user that something went wrong
> rather than simply brush it off?

If we were adding 'git config -e' today, absolutely.  The issue is
not the comparison between signaling with an exit code and not
doing so.  The question is whether the benefit or conceptual
correctness outweighs any possible downside of changing the
behavior existing users have grown accustomed to.

Having said that, 'git config -e' is relatively new, introduced in
commit 3cbace5ee0 (builtin/config: introduce "edit" subcommand,
2024-05-06).  The folks who may be affected are those who used
'git config -e' in their scripts and carefully checked the exit
status (or rather, lazily used 'set -e'), and did so in the past
two years.  So the fallout might not be so great.

So, I dunno.

Re: [RFC PATCH 0/1] config: surface editor failure in exit code

[brian m. carlson]

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

On 2026-08-18 at 14:31:05, Junio C Hamano wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
> 
> > Wouldn't it be better to notify the user that something went wrong
> > rather than simply brush it off?
> 
> If we were adding 'git config -e' today, absolutely.  The issue is
> not the comparison between signaling with an exit code and not
> doing so.  The question is whether the benefit or conceptual
> correctness outweighs any possible downside of changing the
> behavior existing users have grown accustomed to.
> 
> Having said that, 'git config -e' is relatively new, introduced in
> commit 3cbace5ee0 (builtin/config: introduce "edit" subcommand,
> 2024-05-06).  The folks who may be affected are those who used
> 'git config -e' in their scripts and carefully checked the exit
> status (or rather, lazily used 'set -e'), and did so in the past
> two years.  So the fallout might not be so great.

I think we should propagate the error code.  Other than ed(1) and POSIX
vi(1) implementations, editors only exit nonzero when there's an error.
If someone's scripting, then most of the major programming languages
should not exit nonzero unless something seriously went wrong or the
user requested a nonzero exit code, in which case they wanted the
process to abort.

I would actually argue that people might be ignoring errors with `set
-e` that they intended to catch just because they're not getting a
nonzero status code.
-- 
brian m. carlson (they/them)
Toronto, Ontario, CA

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