[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.