{"thread":{"id":"66184","subject":"[RFC PATCH 0/1] config: surface editor failure in exit code","startedAt":"2026-08-17T21:25:09Z","lastAt":"2026-08-19T20:11:11Z","messageCount":16,"participants":["Kenneth Lorber","Junio C Hamano","Karthik Nayak","brian m. carlson"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"550725","messageId":"20260817211936.2943278-2-keni@his.com","threadId":"66184","inReplyTo":"20260817211936.2943278-1-keni@his.com","subject":"[RFC PATCH 1/1] config: surface editor failure in exit code","fromName":"Kenneth Lorber","fromEmail":"keni@his.com","sentAt":"2026-08-17T21:19:33Z","receivedAt":"2026-08-17T21:25:08Z","isPatch":true,"body":"Teach git config --edit to show editor failure to the\nparent process.\n\nAdd 2 tests to t1300 to check editor exiting successfully\nor failing.\n\nSigned-off-by: Kenneth Lorber <keni@his.com>\n---\n builtin/config.c  |  5 +++--\n t/t1300-config.sh | 18 ++++++++++++++++++\n 2 files changed, 21 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 0882899c3f..a166b2131e 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -1291,6 +1291,7 @@ static int cmd_config_remove_section(int argc, const char **argv, const char *pr\n static int show_editor(struct config_location_options *opts)\n {\n \tchar *config_file;\n+\tint ret;\n \n \tif (!opts->source.file && !startup_info->have_repository)\n \t\tdie(_(\"not in a git directory\"));\n@@ -1313,10 +1314,10 @@ static int show_editor(struct config_location_options *opts)\n \t\telse if (errno != EEXIST)\n \t\t\tdie_errno(_(\"cannot create configuration file %s\"), config_file);\n \t}\n-\tlaunch_editor(config_file, NULL, NULL);\n+\tret = launch_editor(config_file, NULL, NULL);\n \tfree(config_file);\n \n-\treturn 0;\n+\treturn ret;\n }\n \n static int cmd_config_edit(int argc, const char **argv, const char *prefix,\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex e3f8064889..9a8f852a86 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -1823,6 +1823,24 @@ test_expect_success 'command line overrides environment config' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'git config --edit successful exit' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\tGIT_EDITOR=true &&\n+\texport GIT_EDITOR &&\n+\tgit -C repo config -e &&\n+\tunset GIT_EDITOR\n+'\n+\n+test_expect_success 'git config --edit failure exit' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\tGIT_EDITOR=false &&\n+\texport GIT_EDITOR &&\n+\ttest_must_fail git -C repo config -e &&\n+\tunset GIT_EDITOR\n+'\n+\n test_expect_success 'git config --edit works' '\n \tgit config -f tmp test.value no &&\n \techo test.value=yes >expect &&\n-- \n2.43.0\n\n\n"},{"id":"550724","messageId":"20260817211936.2943278-1-keni@his.com","threadId":"66184","inReplyTo":null,"subject":"[RFC PATCH 0/1] config: surface editor failure in exit code","fromName":"Kenneth Lorber","fromEmail":"keni@his.com","sentAt":"2026-08-17T21:19:32Z","receivedAt":"2026-08-17T21:25:09Z","isPatch":true,"body":"When the editor invoked by 'git config -e' fails (crashes or calls exit(3)\nwith a non-zero value), git notices and give an error:\n\teditor.c:launch_specified_editor()\n\t\treturn error(\"there was a problem with the editor '%s'\", editor);\nwhich is then lost:\n\tbuiltin/config.c:show_editor()\n\t\tlaunch_editor(config_file, NULL, NULL);\nwhich results in git always calling exit(0).  Note that the value is\nnot explicitly thrown away with \"(void)\", so this may not have been\nintentional.\n\nThis patch simply passes the returned error out of show_editor(), which\ncurrently has an unconditional \"return 0\" even though its callers\nboth check the return value.\n\nWhile this didn't trigger anything in 'make test', it's possible that\nsomeone is relying on 'git config -e' always succeeding, even if the\neditor failed, so this could be considered a breaking change.\n\nThe 2 new tests set GIT_EDITOR to true and false and check the return\nfrom git.\n\nRFC because the community may not want to change this behavior and\nI'm not thrilled with my test code.\n\nKenneth Lorber (1):\n  config: surface editor failure in exit code\n\n builtin/config.c  |  5 +++--\n t/t1300-config.sh | 18 ++++++++++++++++++\n 2 files changed, 21 insertions(+), 2 deletions(-)\n\n\nbase-commit: 010afd3166ddc64c9863b1506f12cbcdda0d4ea1\n-- \n2.43.0\n\n\n"},{"id":"550728","messageId":"xmqqse4c2wyu.fsf@gitster.g","threadId":"66184","inReplyTo":"20260817211936.2943278-1-keni@his.com","subject":"Re: [RFC PATCH 0/1] config: surface editor failure in exit code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-08-17T22:39:21Z","receivedAt":"2026-08-17T22:39:24Z","isPatch":true,"body":"Kenneth Lorber <keni@his.com> writes:\n\n> When the editor invoked by 'git config -e' fails (crashes or calls exit(3)\n> with a non-zero value), git notices and give an error:\n> \teditor.c:launch_specified_editor()\n> \t\treturn error(\"there was a problem with the editor '%s'\", editor);\n> which is then lost:\n> \tbuiltin/config.c:show_editor()\n> \t\tlaunch_editor(config_file, NULL, NULL);\n> which results in git always calling exit(0).  Note that the value is\n> not explicitly thrown away with \"(void)\", so this may not have been\n> intentional.\n\nI do not intentionally exit my editor with a non-zero status myself,\nbut what I hear from others who do is that they do so to affect the\ninvoking 'git' command, e.g., to stop 'git commit' from creating a\ncommit.  They somehow realize they botched the edit, and they want\nto prevent 'git commit' from committing, signaling that by exiting\ntheir editor.  A cleaner and more modern way to do so, by the way,\nis to empty the editor buffer.  In either case, 'git commit' itself\nexits with a non-zero status.\n\nIt might have been more consistent if 'git config -e' exited with a\nnon-zero status when it noticed that the editor exited with a\nnon-zero status, in that sense.  But we have never done so, and that\nis probably because we did not care ;-)\n\nIn any case, I am not sure whether there is much value in making\n'git config -e' start behaving that way.  Even if it can notice a\nfailed editor, the damage to the file is already done, and there is\nnot enough information to undo the damage even if you wanted to when\ndetecting such an error.  This is quite different from when an editor\nedits the 'COMMIT_EDITMSG' file and fails.\n\nSo, I dunno.\n"},{"id":"550735","messageId":"CAOLa=ZTykwSDcFaEmEJJ1PTnX5L9=2t+tkCWhF+hV4J9EPBwWg@mail.gmail.com","threadId":"66184","inReplyTo":"xmqqse4c2wyu.fsf@gitster.g","subject":"Re: [RFC PATCH 0/1] config: surface editor failure in exit code","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-08-18T08:26:36Z","receivedAt":"2026-08-18T08:26:38Z","isPatch":true,"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Kenneth Lorber <keni@his.com> writes:\n>\n>> When the editor invoked by 'git config -e' fails (crashes or calls exit(3)\n>> with a non-zero value), git notices and give an error:\n>> \teditor.c:launch_specified_editor()\n>> \t\treturn error(\"there was a problem with the editor '%s'\", editor);\n>> which is then lost:\n>> \tbuiltin/config.c:show_editor()\n>> \t\tlaunch_editor(config_file, NULL, NULL);\n>> which results in git always calling exit(0).  Note that the value is\n>> not explicitly thrown away with \"(void)\", so this may not have been\n>> intentional.\n>\n> I do not intentionally exit my editor with a non-zero status myself,\n> but what I hear from others who do is that they do so to affect the\n> invoking 'git' command, e.g., to stop 'git commit' from creating a\n> commit.  They somehow realize they botched the edit, and they want\n> to prevent 'git commit' from committing, signaling that by exiting\n> their editor.  A cleaner and more modern way to do so, by the way,\n> is to empty the editor buffer.  In either case, 'git commit' itself\n> exits with a non-zero status.\n>\n> It might have been more consistent if 'git config -e' exited with a\n> non-zero status when it noticed that the editor exited with a\n> non-zero status, in that sense.  But we have never done so, and that\n> is probably because we did not care ;-)\n>\n> In any case, I am not sure whether there is much value in making\n> 'git config -e' start behaving that way.  Even if it can notice a\n> failed editor, the damage to the file is already done, and there is\n> not enough information to undo the damage even if you wanted to when\n> detecting such an error.  This is quite different from when an editor\n> edits the 'COMMIT_EDITMSG' file and fails.\n>\n> So, I dunno.\n\nWouldn't it be better to notify the user that something went wrong\nrather than simply brush it off?\n\nI would be in support of the patch:\n\n  $ GIT_EDITOR=false git config --edit\n  error: there was a problem with the editor 'false'\n  $ echo $status\n  0\n\nAs a user the expectation here would be a non-zero exit status.\n"},{"id":"550736","messageId":"CAOLa=ZQLgxhq2TVS1AYpRoAc_8AkWVtv_VhEm2HovgEX_cFvWg@mail.gmail.com","threadId":"66184","inReplyTo":"20260817211936.2943278-2-keni@his.com","subject":"Re: [RFC PATCH 1/1] config: surface editor failure in exit code","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-08-18T08:42:59Z","receivedAt":"2026-08-18T08:43:01Z","isPatch":true,"body":"Kenneth Lorber <keni@his.com> writes:\n\n> Teach git config --edit to show editor failure to the\n> parent process.\n>\n> Add 2 tests to t1300 to check editor exiting successfully\n> or failing.\n>\n> Signed-off-by: Kenneth Lorber <keni@his.com>\n> ---\n>  builtin/config.c  |  5 +++--\n>  t/t1300-config.sh | 18 ++++++++++++++++++\n>  2 files changed, 21 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/config.c b/builtin/config.c\n> index 0882899c3f..a166b2131e 100644\n> --- a/builtin/config.c\n> +++ b/builtin/config.c\n> @@ -1291,6 +1291,7 @@ static int cmd_config_remove_section(int argc, const char **argv, const char *pr\n>  static int show_editor(struct config_location_options *opts)\n>  {\n>  \tchar *config_file;\n> +\tint ret;\n>\n>  \tif (!opts->source.file && !startup_info->have_repository)\n>  \t\tdie(_(\"not in a git directory\"));\n> @@ -1313,10 +1314,10 @@ static int show_editor(struct config_location_options *opts)\n>  \t\telse if (errno != EEXIST)\n>  \t\t\tdie_errno(_(\"cannot create configuration file %s\"), config_file);\n>  \t}\n> -\tlaunch_editor(config_file, NULL, NULL);\n> +\tret = launch_editor(config_file, NULL, NULL);\n>  \tfree(config_file);\n>\n> -\treturn 0;\n> +\treturn ret;\n>  }\n>\n>  static int cmd_config_edit(int argc, const char **argv, const char *prefix,\n> diff --git a/t/t1300-config.sh b/t/t1300-config.sh\n> index e3f8064889..9a8f852a86 100755\n> --- a/t/t1300-config.sh\n> +++ b/t/t1300-config.sh\n> @@ -1823,6 +1823,24 @@ test_expect_success 'command line overrides environment config' '\n>  \ttest_cmp expect actual\n>  '\n>\n> +test_expect_success 'git config --edit successful exit' '\n> +\ttest_when_finished \"rm -rf repo\" &&\n> +\tgit init repo &&\n> +\tGIT_EDITOR=true &&\n> +\texport GIT_EDITOR &&\n> +\tgit -C repo config -e &&\n> +\tunset GIT_EDITOR\n> +'\n\nNit: couldn't this be simply `test_env GIT_EDITOR=true git -C repo\nconfig -e` and avoid the set, export and unset?\n\n> +\n> +test_expect_success 'git config --edit failure exit' '\n> +\ttest_when_finished \"rm -rf repo\" &&\n> +\tgit init repo &&\n> +\tGIT_EDITOR=false &&\n> +\texport GIT_EDITOR &&\n> +\ttest_must_fail git -C repo config -e &&\n> +\tunset GIT_EDITOR\n> +'\n\nSame here..\n\n> +\n>  test_expect_success 'git config --edit works' '\n>  \tgit config -f tmp test.value no &&\n>  \techo test.value=yes >expect &&\n> --\n> 2.43.0\n\nThe patch looks good to me otherwise :)\n"},{"id":"550756","messageId":"xmqqecfv33h2.fsf@gitster.g","threadId":"66184","inReplyTo":"CAOLa=ZTykwSDcFaEmEJJ1PTnX5L9=2t+tkCWhF+hV4J9EPBwWg@mail.gmail.com","subject":"Re: [RFC PATCH 0/1] config: surface editor failure in exit code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-08-18T14:31:05Z","receivedAt":"2026-08-18T14:31:07Z","isPatch":true,"body":"Karthik Nayak <karthik.188@gmail.com> writes:\n\n> Wouldn't it be better to notify the user that something went wrong\n> rather than simply brush it off?\n\nIf we were adding 'git config -e' today, absolutely.  The issue is\nnot the comparison between signaling with an exit code and not\ndoing so.  The question is whether the benefit or conceptual\ncorrectness outweighs any possible downside of changing the\nbehavior existing users have grown accustomed to.\n\nHaving said that, 'git config -e' is relatively new, introduced in\ncommit 3cbace5ee0 (builtin/config: introduce \"edit\" subcommand,\n2024-05-06).  The folks who may be affected are those who used\n'git config -e' in their scripts and carefully checked the exit\nstatus (or rather, lazily used 'set -e'), and did so in the past\ntwo years.  So the fallout might not be so great.\n\nSo, I dunno.\n"},{"id":"550780","messageId":"aoTY6_wfcroOwrob@fruit.crustytoothpaste.net","threadId":"66184","inReplyTo":"xmqqecfv33h2.fsf@gitster.g","subject":"Re: [RFC PATCH 0/1] config: surface editor failure in exit code","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2026-08-18T22:12:59Z","receivedAt":"2026-08-18T22:13:08Z","isPatch":true,"body":"On 2026-08-18 at 14:31:05, Junio C Hamano wrote:\n> Karthik Nayak <karthik.188@gmail.com> writes:\n> \n> > Wouldn't it be better to notify the user that something went wrong\n> > rather than simply brush it off?\n> \n> If we were adding 'git config -e' today, absolutely.  The issue is\n> not the comparison between signaling with an exit code and not\n> doing so.  The question is whether the benefit or conceptual\n> correctness outweighs any possible downside of changing the\n> behavior existing users have grown accustomed to.\n> \n> Having said that, 'git config -e' is relatively new, introduced in\n> commit 3cbace5ee0 (builtin/config: introduce \"edit\" subcommand,\n> 2024-05-06).  The folks who may be affected are those who used\n> 'git config -e' in their scripts and carefully checked the exit\n> status (or rather, lazily used 'set -e'), and did so in the past\n> two years.  So the fallout might not be so great.\n\nI think we should propagate the error code.  Other than ed(1) and POSIX\nvi(1) implementations, editors only exit nonzero when there's an error.\nIf someone's scripting, then most of the major programming languages\nshould not exit nonzero unless something seriously went wrong or the\nuser requested a nonzero exit code, in which case they wanted the\nprocess to abort.\n\nI would actually argue that people might be ignoring errors with `set\n-e` that they intended to catch just because they're not getting a\nnonzero status code.\n-- \nbrian m. carlson (they/them)\nToronto, Ontario, CA\n"},{"id":"550799","messageId":"30A43EB3-6B97-4476-BF48-4820AAE39AFA@his.com","threadId":"66184","inReplyTo":"CAOLa=ZQLgxhq2TVS1AYpRoAc_8AkWVtv_VhEm2HovgEX_cFvWg@mail.gmail.com","subject":"Re: [RFC PATCH 1/1] config: surface editor failure in exit code","fromName":"Kenneth Lorber","fromEmail":"keni@his.com","sentAt":"2026-08-19T11:17:56Z","receivedAt":"2026-08-19T11:18:10Z","isPatch":true,"body":"\n> On Aug 18, 2026, at 4:42 AM, Karthik Nayak <karthik.188@gmail.com> wrote:\n> \n> Kenneth Lorber <keni@his.com> writes:\n> \n>> Teach git config --edit to show editor failure to the\n>> parent process.\n>> \n>> Add 2 tests to t1300 to check editor exiting successfully\n>> or failing.\n>> \n>> Signed-off-by: Kenneth Lorber <keni@his.com>\n>> ---\n>> builtin/config.c  |  5 +++--\n>> t/t1300-config.sh | 18 ++++++++++++++++++\n>> 2 files changed, 21 insertions(+), 2 deletions(-)\n>> \n>> diff --git a/builtin/config.c b/builtin/config.c\n>> index 0882899c3f..a166b2131e 100644\n>> --- a/builtin/config.c\n>> +++ b/builtin/config.c\n>> @@ -1291,6 +1291,7 @@ static int cmd_config_remove_section(int argc, const char **argv, const char *pr\n>> static int show_editor(struct config_location_options *opts)\n>> {\n>> \tchar *config_file;\n>> +\tint ret;\n>> \n>> \tif (!opts->source.file && !startup_info->have_repository)\n>> \t\tdie(_(\"not in a git directory\"));\n>> @@ -1313,10 +1314,10 @@ static int show_editor(struct config_location_options *opts)\n>> \t\telse if (errno != EEXIST)\n>> \t\t\tdie_errno(_(\"cannot create configuration file %s\"), config_file);\n>> \t}\n>> -\tlaunch_editor(config_file, NULL, NULL);\n>> +\tret = launch_editor(config_file, NULL, NULL);\n>> \tfree(config_file);\n>> \n>> -\treturn 0;\n>> +\treturn ret;\n>> }\n>> \n>> static int cmd_config_edit(int argc, const char **argv, const char *prefix,\n>> diff --git a/t/t1300-config.sh b/t/t1300-config.sh\n>> index e3f8064889..9a8f852a86 100755\n>> --- a/t/t1300-config.sh\n>> +++ b/t/t1300-config.sh\n>> @@ -1823,6 +1823,24 @@ test_expect_success 'command line overrides environment config' '\n>> \ttest_cmp expect actual\n>> '\n>> \n>> +test_expect_success 'git config --edit successful exit' '\n>> +\ttest_when_finished \"rm -rf repo\" &&\n>> +\tgit init repo &&\n>> +\tGIT_EDITOR=true &&\n>> +\texport GIT_EDITOR &&\n>> +\tgit -C repo config -e &&\n>> +\tunset GIT_EDITOR\n>> +'\n> \n> Nit: couldn't this be simply `test_env GIT_EDITOR=true git -C repo\n> config -e` and avoid the set, export and unset?\n\nThank you, this is exactly the cleanup I was looking for.\n\n> \n>> +\n>> +test_expect_success 'git config --edit failure exit' '\n>> +\ttest_when_finished \"rm -rf repo\" &&\n>> +\tgit init repo &&\n>> +\tGIT_EDITOR=false &&\n>> +\texport GIT_EDITOR &&\n>> +\ttest_must_fail git -C repo config -e &&\n>> +\tunset GIT_EDITOR\n>> +'\n> \n> Same here..\n> \n>> +\n>> test_expect_success 'git config --edit works' '\n>> \tgit config -f tmp test.value no &&\n>> \techo test.value=yes >expect &&\n>> --\n>> 2.43.0\n> \n> The patch looks good to me otherwise :)\n\nThank you.\n\n\n"},{"id":"550823","messageId":"20260819150922.2984850-1-keni@his.com","threadId":"66184","inReplyTo":"20260817211936.2943278-1-keni@his.com","subject":"[PATCH v2 0/1] config: surface editor failure in exit code","fromName":"Kenneth Lorber","fromEmail":"keni@his.com","sentAt":"2026-08-19T15:09:17Z","receivedAt":"2026-08-19T15:09:35Z","isPatch":true,"body":"(Apologies to anyone who gets this twice.)\n\nSimplified the tests and changed the test names from \"--edit\" to \"-e\"\nsince that's what the test is actually running.  Did not change the\ntests to use \"--edit\" as nothing else is checking \"-e\".\n\n\n1:  d54d260aa0 ! 1:  05d02b80dc config: surface editor failure in exit code\n     @@ t/t1300-config.sh: test_expect_success 'command line overrides environment config' '\n      \ttest_cmp expect actual\n      '\n      \n    -+test_expect_success 'git config --edit successful exit' '\n    ++test_expect_success 'git config -e successful exit' '\n     +\ttest_when_finished \"rm -rf repo\" &&\n     +\tgit init repo &&\n    -+\tGIT_EDITOR=true &&\n    -+\texport GIT_EDITOR &&\n    -+\tgit -C repo config -e &&\n    -+\tunset GIT_EDITOR\n    ++\ttest_env GIT_EDITOR=true git -C repo config -e\n     +'\n     +\n    -+test_expect_success 'git config --edit failure exit' '\n    ++test_expect_success 'git config -e failure exit' '\n     +\ttest_when_finished \"rm -rf repo\" &&\n     +\tgit init repo &&\n    -+\tGIT_EDITOR=false &&\n    -+\texport GIT_EDITOR &&\n    -+\ttest_must_fail git -C repo config -e &&\n    -+\tunset GIT_EDITOR\n    ++\ttest_env GIT_EDITOR=false test_must_fail git -C repo config -e\n     +'\n     +\n      test_expect_success 'git config --edit works' '\n--\n2.43.0\n\n\n"},{"id":"550824","messageId":"20260819150922.2984850-2-keni@his.com","threadId":"66184","inReplyTo":"20260819150922.2984850-1-keni@his.com","subject":"[PATCH v2 1/1] config: surface editor failure in exit code","fromName":"Kenneth Lorber","fromEmail":"keni@his.com","sentAt":"2026-08-19T15:09:18Z","receivedAt":"2026-08-19T15:09:40Z","isPatch":true,"body":"Teach git config --edit to show editor failure to the\nparent process.\n\nAdd 2 tests to t1300 to check editor exiting successfully\nor failing.\n\nSigned-off-by: Kenneth Lorber <keni@his.com>\n---\n builtin/config.c  |  5 +++--\n t/t1300-config.sh | 12 ++++++++++++\n 2 files changed, 15 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 0882899c3f..a166b2131e 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -1291,6 +1291,7 @@ static int cmd_config_remove_section(int argc, const char **argv, const char *pr\n static int show_editor(struct config_location_options *opts)\n {\n \tchar *config_file;\n+\tint ret;\n \n \tif (!opts->source.file && !startup_info->have_repository)\n \t\tdie(_(\"not in a git directory\"));\n@@ -1313,10 +1314,10 @@ static int show_editor(struct config_location_options *opts)\n \t\telse if (errno != EEXIST)\n \t\t\tdie_errno(_(\"cannot create configuration file %s\"), config_file);\n \t}\n-\tlaunch_editor(config_file, NULL, NULL);\n+\tret = launch_editor(config_file, NULL, NULL);\n \tfree(config_file);\n \n-\treturn 0;\n+\treturn ret;\n }\n \n static int cmd_config_edit(int argc, const char **argv, const char *prefix,\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex e3f8064889..3e218079ee 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -1823,6 +1823,18 @@ test_expect_success 'command line overrides environment config' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'git config -e successful exit' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\ttest_env GIT_EDITOR=true git -C repo config -e\n+'\n+\n+test_expect_success 'git config -e failure exit' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\ttest_env GIT_EDITOR=false test_must_fail git -C repo config -e\n+'\n+\n test_expect_success 'git config --edit works' '\n \tgit config -f tmp test.value no &&\n \techo test.value=yes >expect &&\n-- \n2.43.0\n\n\n"},{"id":"550825","messageId":"20260819150922.2984850-3-keni@his.com","threadId":"66184","inReplyTo":"20260819150922.2984850-1-keni@his.com","subject":"[PATCH v2 0/1] config: surface editor failure in exit code","fromName":"Kenneth Lorber","fromEmail":"keni@his.com","sentAt":"2026-08-19T15:09:19Z","receivedAt":"2026-08-19T15:10:26Z","isPatch":true,"body":"(Apologies to anyone who gets this twice.)\n\nSimplified the tests and changed the test names from \"--edit\" to \"-e\"\nsince that's what the test is actually running.  Did not change the\ntests to use \"--edit\" as nothing else is checking \"-e\".\n\n\n1:  d54d260aa0 ! 1:  05d02b80dc config: surface editor failure in exit code\n     @@ t/t1300-config.sh: test_expect_success 'command line overrides environment config' '\n      \ttest_cmp expect actual\n      '\n      \n    -+test_expect_success 'git config --edit successful exit' '\n    ++test_expect_success 'git config -e successful exit' '\n     +\ttest_when_finished \"rm -rf repo\" &&\n     +\tgit init repo &&\n    -+\tGIT_EDITOR=true &&\n    -+\texport GIT_EDITOR &&\n    -+\tgit -C repo config -e &&\n    -+\tunset GIT_EDITOR\n    ++\ttest_env GIT_EDITOR=true git -C repo config -e\n     +'\n     +\n    -+test_expect_success 'git config --edit failure exit' '\n    ++test_expect_success 'git config -e failure exit' '\n     +\ttest_when_finished \"rm -rf repo\" &&\n     +\tgit init repo &&\n    -+\tGIT_EDITOR=false &&\n    -+\texport GIT_EDITOR &&\n    -+\ttest_must_fail git -C repo config -e &&\n    -+\tunset GIT_EDITOR\n    ++\ttest_env GIT_EDITOR=false test_must_fail git -C repo config -e\n     +'\n     +\n      test_expect_success 'git config --edit works' '\n--\n2.43.0\n\n\n"},{"id":"550826","messageId":"20260819150922.2984850-4-keni@his.com","threadId":"66184","inReplyTo":"20260819150922.2984850-1-keni@his.com","subject":"[PATCH v2 1/1] config: surface editor failure in exit code","fromName":"Kenneth Lorber","fromEmail":"keni@his.com","sentAt":"2026-08-19T15:09:20Z","receivedAt":"2026-08-19T15:11:59Z","isPatch":true,"body":"Teach git config --edit to show editor failure to the\nparent process.\n\nAdd 2 tests to t1300 to check editor exiting successfully\nor failing.\n\nSigned-off-by: Kenneth Lorber <keni@his.com>\n---\n builtin/config.c  |  5 +++--\n t/t1300-config.sh | 12 ++++++++++++\n 2 files changed, 15 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 0882899c3f..a166b2131e 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -1291,6 +1291,7 @@ static int cmd_config_remove_section(int argc, const char **argv, const char *pr\n static int show_editor(struct config_location_options *opts)\n {\n \tchar *config_file;\n+\tint ret;\n \n \tif (!opts->source.file && !startup_info->have_repository)\n \t\tdie(_(\"not in a git directory\"));\n@@ -1313,10 +1314,10 @@ static int show_editor(struct config_location_options *opts)\n \t\telse if (errno != EEXIST)\n \t\t\tdie_errno(_(\"cannot create configuration file %s\"), config_file);\n \t}\n-\tlaunch_editor(config_file, NULL, NULL);\n+\tret = launch_editor(config_file, NULL, NULL);\n \tfree(config_file);\n \n-\treturn 0;\n+\treturn ret;\n }\n \n static int cmd_config_edit(int argc, const char **argv, const char *prefix,\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex e3f8064889..3e218079ee 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -1823,6 +1823,18 @@ test_expect_success 'command line overrides environment config' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'git config -e successful exit' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\ttest_env GIT_EDITOR=true git -C repo config -e\n+'\n+\n+test_expect_success 'git config -e failure exit' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\ttest_env GIT_EDITOR=false test_must_fail git -C repo config -e\n+'\n+\n test_expect_success 'git config --edit works' '\n \tgit config -f tmp test.value no &&\n \techo test.value=yes >expect &&\n-- \n2.43.0\n\n\n"},{"id":"550834","messageId":"xmqqjypmuh3z.fsf@gitster.g","threadId":"66184","inReplyTo":"20260819150922.2984850-1-keni@his.com","subject":"Re: [PATCH v2 0/1] config: surface editor failure in exit code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-08-19T17:58:56Z","receivedAt":"2026-08-19T17:58:58Z","isPatch":true,"body":"Kenneth Lorber <keni@his.com> writes:\n\n> (Apologies to anyone who gets this twice.)\n\nYou should not apologize; instead make sure you do not send out the\nsame thing twice ;-).\n\n\nWe actually have 633ac346ee (config: propagate launch_editor()\nfailure in show_editor(), 2026-08-12) in flight, so we do not need\nthis patch.\n\nPlease build from 'next' and use the resulting \"git\" binary to try\nit out.\n\nThanks.\n"},{"id":"550837","messageId":"D0A00C40-47CC-40A7-BE71-F59C02AF3CCB@his.com","threadId":"66184","inReplyTo":"xmqqjypmuh3z.fsf@gitster.g","subject":"Re: [PATCH v2 0/1] config: surface editor failure in exit code","fromName":"Kenneth Lorber","fromEmail":"keni@his.com","sentAt":"2026-08-19T18:58:38Z","receivedAt":"2026-08-19T18:58:42Z","isPatch":true,"body":"\n\n> On Aug 19, 2026, at 1:58 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> \n> Kenneth Lorber <keni@his.com> writes:\n> \n>> (Apologies to anyone who gets this twice.)\n> \n> You should not apologize; instead make sure you do not send out the\n> same thing twice ;-).\n> \n> \n> We actually have 633ac346ee (config: propagate launch_editor()\n> failure in show_editor(), 2026-08-12) in flight, so we do not need\n> this patch.\n> \n> Please build from 'next' and use the resulting \"git\" binary to try\n> it out.\n> \n> Thanks.\n\nWorks fine, but includes no tests.\n\nThanks.\n"},{"id":"550838","messageId":"xmqq7bllvrq0.fsf@gitster.g","threadId":"66184","inReplyTo":"D0A00C40-47CC-40A7-BE71-F59C02AF3CCB@his.com","subject":"Re: [PATCH v2 0/1] config: surface editor failure in exit code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-08-19T19:24:23Z","receivedAt":"2026-08-19T19:24:26Z","isPatch":true,"body":"Kenneth Lorber <keni@his.com> writes:\n\n>> On Aug 19, 2026, at 1:58 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> \n>> Kenneth Lorber <keni@his.com> writes:\n>> \n>>> (Apologies to anyone who gets this twice.)\n>> \n>> You should not apologize; instead make sure you do not send out the\n>> same thing twice ;-).\n>> \n>> \n>> We actually have 633ac346ee (config: propagate launch_editor()\n>> failure in show_editor(), 2026-08-12) in flight, so we do not need\n>> this patch.\n>> \n>> Please build from 'next' and use the resulting \"git\" binary to try\n>> it out.\n>> \n>> Thanks.\n>\n> Works fine, but includes no tests.\n>\n> Thanks.\n\nComplaints to the author of the other patch are very much welcome\n;-)\n"},{"id":"550839","messageId":"xmqqy0e1uazm.fsf@gitster.g","threadId":"66184","inReplyTo":"CAOLa=ZQLgxhq2TVS1AYpRoAc_8AkWVtv_VhEm2HovgEX_cFvWg@mail.gmail.com","subject":"Re: [RFC PATCH 1/1] config: surface editor failure in exit code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-08-19T20:11:09Z","receivedAt":"2026-08-19T20:11:11Z","isPatch":true,"body":"Karthik Nayak <karthik.188@gmail.com> writes:\n\n>> +test_expect_success 'git config --edit successful exit' '\n>> +\ttest_when_finished \"rm -rf repo\" &&\n>> +\tgit init repo &&\n>> +\tGIT_EDITOR=true &&\n>> +\texport GIT_EDITOR &&\n>> +\tgit -C repo config -e &&\n>> +\tunset GIT_EDITOR\n>> +'\n>\n> Nit: couldn't this be simply `test_env GIT_EDITOR=true git -C repo\n> config -e` and avoid the set, export and unset?\n\nNo, it should just be a single liner:\n\n\tGIT_EDITOR=true git -C repo config -e\n\nI would recommend against use of test_env in most cases, because it\nintroduces a subshell without making it obvious.\n\n>> +test_expect_success 'git config --edit failure exit' '\n>> +\ttest_when_finished \"rm -rf repo\" &&\n>> +\tgit init repo &&\n>> +\tGIT_EDITOR=false &&\n>> +\texport GIT_EDITOR &&\n>> +\ttest_must_fail git -C repo config -e &&\n>> +\tunset GIT_EDITOR\n>> +'\n>\n> Same here..\n\nEven when you truly a need subshell, it is better to spell the\nsubshell invocation out explicitly, i.e.,\n\n    ...\n    git init repo &&\n    (\n\tGIT_EDITOR=false &&\n\texport GIT_EDITOR &&\n\ttest_must_fail git -C repo config -e\n    )\n\nrather than using test_env.\n\nBut in a case like this where you do not even need a subshell to\nhelp you shield your actions from later steps, you can just use\n\"env\", like everybody else:\n\n\ttest_must_fail env GIT_EDITOR=false git -C repo config -e\n\nThere are many uses of this pattern.\n\nThanks.\n"}]}