{"thread":{"id":"39316","subject":"'EDITOR=false git add -e' adds to index","startedAt":"2015-05-13T00:31:26Z","lastAt":"2015-05-13T08:12:31Z","messageCount":3,"participants":["Russ Cox","Jeff King","Johannes Schindelin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"261103","messageId":"CAA8EjDS9HePk6ZFa0kOy7GDa9_c_TsJ7uv7t+N9RL-3z=ZrQFQ@mail.gmail.com","threadId":"39316","inReplyTo":null,"subject":"'EDITOR=false git add -e' adds to index","fromName":"Russ Cox","fromEmail":"rsc@golang.org","sentAt":"2015-05-13T00:31:26Z","receivedAt":"2015-05-13T00:31:26Z","isPatch":false,"sender":{"key":"rsc@golang.org","avatar":null},"body":"In contrast to most other git commands, git add -e does not check\nwhether the editor it invokes exits successfully. This means that if\nyou cause your editor to exit unsuccessfully, hoping to abort the add\n-e, the add -e continues anyway.\n\nThe specific instance I've observed is that you set EDITOR to a\nprogram that starts the editor outside the current terminal and waits\nfor it to exit. Then you realize you didn't want to add anything and\ntype ^C in the terminal where git is waiting for $EDITOR to finish.\nThat makes $EDITOR exit unsuccessfully, and git prints an error about\nnot being able to invoke the editor, but it continues on with the add\n-e, adding everything from the patch to the index.\n\nI am using the acme editor, but I believe that EDITOR='emacs -nw' or\nEDITOR='subl -w' for Sublime Text are more common settings that would\nhave the same problem. In a test, EDITOR=false suffices.\n\nThe root cause seems to be that builtin/add.c's edit_patch does not\ncheck the result of  the launch_editor call. It probably should.\n\nThe call to launch_editor in builtin/config.c should probably also be\nchecked, although there it's not as big a deal, provided the editor\ndid not modify the file; reapplying the same file should be a no-op.\nThis is different from git add -e.\n\nI am seeing this behavior with git 2.2.1, but I checked the main repo\nand commit c518059 from yesterday still has the unchecked\nlaunch_editor call.\n\nThanks.\nRuss\n"},{"id":"261106","messageId":"20150513012158.GA3066@peff.net","threadId":"39316","inReplyTo":"CAA8EjDS9HePk6ZFa0kOy7GDa9_c_TsJ7uv7t+N9RL-3z=ZrQFQ@mail.gmail.com","subject":"[PATCH] add: check return value of launch_editor","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-13T01:21:58Z","receivedAt":"2015-05-13T01:21:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 12, 2015 at 08:31:26PM -0400, Russ Cox wrote:\n\n> The root cause seems to be that builtin/add.c's edit_patch does not\n> check the result of  the launch_editor call. It probably should.\n\nYes, definitely. Patch is below.\n\n> The call to launch_editor in builtin/config.c should probably also be\n> checked, although there it's not as big a deal, provided the editor\n> did not modify the file; reapplying the same file should be a no-op.\n> This is different from git add -e.\n\nThis one is trickier. Certainly we could propagate the return value and\nsay \"hey, editing did not work\". But we otherwise have no operation to\nabort. This is because \"git config --edit\" does not actually take our\nnormal dot-lock, but rather edits the file in place.\n\nIMHO this is questionable, as proper locking seems like half of the\npurpose of \"config --edit\" (the other half being that you do not have to\ntype the location of the config file yourself). But I wonder if\nswitching it would make people unhappy, as it would probably break\neditor rules to do syntax highlighting (e.g., if they are looking for\n\"*/.gitconfig\" in the path).\n\n-- >8 --\nSubject: add: check return value of launch_editor\n\nWhen running \"add -e\", if launching the editor fails, we do\nnot notice and continue as if the output is what the user\nasked for. The likely case is that the editor did not touch\nthe contents at all, and we end up adding everything.\n\nReported-by: Russ Cox <rsc@golang.org>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/add.c       | 3 ++-\n t/t3702-add-edit.sh | 7 +++++++\n 2 files changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 3390933..4bd98b7 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -208,7 +208,8 @@ static int edit_patch(int argc, const char **argv, const char *prefix)\n \tif (run_diff_files(&rev, 0))\n \t\tdie(_(\"Could not write patch\"));\n \n-\tlaunch_editor(file, NULL, NULL);\n+\tif (launch_editor(file, NULL, NULL))\n+\t\tdie(_(\"editing patch failed\"));\n \n \tif (stat(file, &st))\n \t\tdie_errno(_(\"Could not stat '%s'\"), file);\ndiff --git a/t/t3702-add-edit.sh b/t/t3702-add-edit.sh\nindex 4ee47cc..3cb74ca 100755\n--- a/t/t3702-add-edit.sh\n+++ b/t/t3702-add-edit.sh\n@@ -118,4 +118,11 @@ test_expect_success 'add -e' '\n \n '\n \n+test_expect_success 'add -e notices editor failure' '\n+\tgit reset --hard &&\n+\techo change >>file &&\n+\ttest_must_fail env GIT_EDITOR=false git add -e &&\n+\ttest_expect_code 1 git diff --exit-code\n+'\n+\n test_done\n-- \n2.4.0.192.g5f8138b\n"},{"id":"261141","messageId":"4de553a1564152cc951be8675214f7f7@www.dscho.org","threadId":"39316","inReplyTo":"20150513012158.GA3066@peff.net","subject":"Re: [PATCH] add: check return value of launch_editor","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-05-13T08:12:31Z","receivedAt":"2015-05-13T08:12:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn 2015-05-13 03:21, Jeff King wrote:\n> On Tue, May 12, 2015 at 08:31:26PM -0400, Russ Cox wrote:\n> \n>> The root cause seems to be that builtin/add.c's edit_patch does not\n>> check the result of  the launch_editor call. It probably should.\n> \n> Yes, definitely. Patch is below.\n\nThanks for cleaning up my mess!\n\nCiao,\nDscho\n"}]}