Volume XXII, number 279Tuesday, October 6, 2026Latest message 11 minutes ago

The Git List

News and archive of git@vger.kernel.org, since April 2005

RFC patchconfig: surface editor failure in exit code

16 messages between Aug 17, 2026 and Aug 19, 2026, from Kenneth Lorber, Junio C Hamano, Karthik Nayak, brian m. carlson.

Plain Markdown or JSON for tools and agents. Diffs are folded; open one to read it.

Kenneth LorberAug 17, 2026, 21:19 UTC in reply to Kenneth Lorber on lore

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

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(-)
Show changes to 2 files +21 −2

builtin/config.c, t/t1300-config.sh

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
Kenneth LorberAug 17, 2026, 21:19 UTC on lore
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
Junio C HamanoAug 17, 2026, 22:39 UTC in reply to Kenneth Lorber on lore

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

Kenneth Lorber <keni@his.com> writes:
Show 10 quoted lines
> 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.
Karthik NayakAug 18, 2026, 08:26 UTC in reply to Junio C Hamano on lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 35 quoted lines
> 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.
Karthik NayakAug 18, 2026, 08:42 UTC in reply to Kenneth Lorber on lore

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

Kenneth Lorber <keni@his.com> writes:
Show 53 quoted lines
> 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?
Show 9 quoted lines
> +
> +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..
Show 6 quoted lines
> +
>  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 :)
Junio C HamanoAug 18, 2026, 14:31 UTC in reply to Karthik Nayak on lore

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

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.
brian m. carlsonAug 18, 2026, 22:12 UTC in reply to Junio C Hamano on lore

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

On 2026-08-18 at 14:31:05, Junio C Hamano wrote:
Show 17 quoted lines
> 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
Kenneth LorberAug 19, 2026, 11:17 UTC in reply to Karthik Nayak on lore

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

Show 60 quoted lines
> On Aug 18, 2026, at 4:42 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> 
> 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?
Thank you, this is exactly the cleanup I was looking for.
Show 21 quoted lines
> 
>> +
>> +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 :)
Thank you.
Kenneth LorberAug 19, 2026, 15:09 UTC in reply to Kenneth Lorber on lore

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

(Apologies to anyone who gets this twice.)

Simplified the tests and changed the test names from "--edit" to "-e" since that's what the test is actually running. Did not change the tests to use "--edit" as nothing else is checking "-e".

1:  d54d260aa0 ! 1:  05d02b80dc config: surface editor failure in exit code
     @@ t/t1300-config.sh: test_expect_success 'command line overrides environment config' '
      	test_cmp expect actual
      '
      
    -+test_expect_success 'git config --edit successful exit' '
    ++test_expect_success 'git config -e 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_env GIT_EDITOR=true git -C repo config -e
     +'
     +
    -+test_expect_success 'git config --edit failure exit' '
    ++test_expect_success 'git config -e 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_env GIT_EDITOR=false test_must_fail git -C repo config -e
     +'
     +
      test_expect_success 'git config --edit works' '
--
2.43.0
Kenneth LorberAug 19, 2026, 15:09 UTC in reply to Kenneth Lorber on lore

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

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 | 12 ++++++++++++
 2 files changed, 15 insertions(+), 2 deletions(-)
Show changes to 2 files +15 −2

builtin/config.c, t/t1300-config.sh

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..3e218079ee 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1823,6 +1823,18 @@ test_expect_success 'command line overrides environment config' '
 	test_cmp expect actual
 '
 
+test_expect_success 'git config -e successful exit' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	test_env GIT_EDITOR=true git -C repo config -e
+'
+
+test_expect_success 'git config -e failure exit' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	test_env GIT_EDITOR=false test_must_fail git -C repo config -e
+'
+
 test_expect_success 'git config --edit works' '
 	git config -f tmp test.value no &&
 	echo test.value=yes >expect &&

-- 
2.43.0
Kenneth LorberAug 19, 2026, 15:09 UTC in reply to Kenneth Lorber on lore

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

(Apologies to anyone who gets this twice.)

Simplified the tests and changed the test names from "--edit" to "-e" since that's what the test is actually running. Did not change the tests to use "--edit" as nothing else is checking "-e".

1:  d54d260aa0 ! 1:  05d02b80dc config: surface editor failure in exit code
     @@ t/t1300-config.sh: test_expect_success 'command line overrides environment config' '
      	test_cmp expect actual
      '
      
    -+test_expect_success 'git config --edit successful exit' '
    ++test_expect_success 'git config -e 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_env GIT_EDITOR=true git -C repo config -e
     +'
     +
    -+test_expect_success 'git config --edit failure exit' '
    ++test_expect_success 'git config -e 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_env GIT_EDITOR=false test_must_fail git -C repo config -e
     +'
     +
      test_expect_success 'git config --edit works' '
--
2.43.0
Kenneth LorberAug 19, 2026, 15:09 UTC in reply to Kenneth Lorber on lore

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

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 | 12 ++++++++++++
 2 files changed, 15 insertions(+), 2 deletions(-)
Show changes to 2 files +15 −2

builtin/config.c, t/t1300-config.sh

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..3e218079ee 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1823,6 +1823,18 @@ test_expect_success 'command line overrides environment config' '
 	test_cmp expect actual
 '
 
+test_expect_success 'git config -e successful exit' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	test_env GIT_EDITOR=true git -C repo config -e
+'
+
+test_expect_success 'git config -e failure exit' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	test_env GIT_EDITOR=false test_must_fail git -C repo config -e
+'
+
 test_expect_success 'git config --edit works' '
 	git config -f tmp test.value no &&
 	echo test.value=yes >expect &&

-- 
2.43.0
Junio C HamanoAug 19, 2026, 17:58 UTC in reply to Kenneth Lorber on lore

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

Kenneth Lorber <keni@his.com> writes:
> (Apologies to anyone who gets this twice.)

You should not apologize; instead make sure you do not send out the same thing twice ;-).

We actually have 633ac346ee (config: propagate launch_editor() failure in show_editor(), 2026-08-12) in flight, so we do not need this patch.

Please build from 'next' and use the resulting "git" binary to try it out.

Thanks.
Kenneth LorberAug 19, 2026, 18:58 UTC in reply to Junio C Hamano on lore

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

Show 18 quoted lines
> On Aug 19, 2026, at 1:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Kenneth Lorber <keni@his.com> writes:
> 
>> (Apologies to anyone who gets this twice.)
> 
> You should not apologize; instead make sure you do not send out the
> same thing twice ;-).
> 
> 
> We actually have 633ac346ee (config: propagate launch_editor()
> failure in show_editor(), 2026-08-12) in flight, so we do not need
> this patch.
> 
> Please build from 'next' and use the resulting "git" binary to try
> it out.
> 
> Thanks.
Works fine, but includes no tests.
Thanks.
Junio C HamanoAug 19, 2026, 19:24 UTC in reply to Kenneth Lorber on lore

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

Kenneth Lorber <keni@his.com> writes:
Show 22 quoted lines
>> On Aug 19, 2026, at 1:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> 
>> Kenneth Lorber <keni@his.com> writes:
>> 
>>> (Apologies to anyone who gets this twice.)
>> 
>> You should not apologize; instead make sure you do not send out the
>> same thing twice ;-).
>> 
>> 
>> We actually have 633ac346ee (config: propagate launch_editor()
>> failure in show_editor(), 2026-08-12) in flight, so we do not need
>> this patch.
>> 
>> Please build from 'next' and use the resulting "git" binary to try
>> it out.
>> 
>> Thanks.
>
> Works fine, but includes no tests.
>
> Thanks.

Complaints to the author of the other patch are very much welcome ;-)

Junio C HamanoAug 19, 2026, 20:11 UTC in reply to Karthik Nayak on lore

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

Karthik Nayak <karthik.188@gmail.com> writes:
Show 11 quoted lines
>> +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?
No, it should just be a single liner:
	GIT_EDITOR=true git -C repo config -e

I would recommend against use of test_env in most cases, because it introduces a subshell without making it obvious.

Show 10 quoted lines
>> +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..

Even when you truly a need subshell, it is better to spell the subshell invocation out explicitly, i.e.,

    ...
    git init repo &&
    (
	GIT_EDITOR=false &&
	export GIT_EDITOR &&
	test_must_fail git -C repo config -e
    )
rather than using test_env.

But in a case like this where you do not even need a subshell to help you shield your actions from later steps, you can just use "env", like everybody else:

	test_must_fail env GIT_EDITOR=false git -C repo config -e
There are many uses of this pattern.
Thanks.

Back to recent threads