* 'EDITOR=false git add -e' adds to index @ 2015-05-13 0:31 Russ Cox 2015-05-13 1:21 ` [PATCH] add: check return value of launch_editor Jeff King 0 siblings, 1 reply; 3+ messages in thread From: Russ Cox @ 2015-05-13 0:31 UTC (permalink / raw) To: git In contrast to most other git commands, git add -e does not check whether the editor it invokes exits successfully. This means that if you cause your editor to exit unsuccessfully, hoping to abort the add -e, the add -e continues anyway. The specific instance I've observed is that you set EDITOR to a program that starts the editor outside the current terminal and waits for it to exit. Then you realize you didn't want to add anything and type ^C in the terminal where git is waiting for $EDITOR to finish. That makes $EDITOR exit unsuccessfully, and git prints an error about not being able to invoke the editor, but it continues on with the add -e, adding everything from the patch to the index. I am using the acme editor, but I believe that EDITOR='emacs -nw' or EDITOR='subl -w' for Sublime Text are more common settings that would have the same problem. In a test, EDITOR=false suffices. The root cause seems to be that builtin/add.c's edit_patch does not check the result of the launch_editor call. It probably should. The call to launch_editor in builtin/config.c should probably also be checked, although there it's not as big a deal, provided the editor did not modify the file; reapplying the same file should be a no-op. This is different from git add -e. I am seeing this behavior with git 2.2.1, but I checked the main repo and commit c518059 from yesterday still has the unchecked launch_editor call. Thanks. Russ ^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH] add: check return value of launch_editor 2015-05-13 0:31 'EDITOR=false git add -e' adds to index Russ Cox @ 2015-05-13 1:21 ` Jeff King 2015-05-13 8:12 ` Johannes Schindelin 0 siblings, 1 reply; 3+ messages in thread From: Jeff King @ 2015-05-13 1:21 UTC (permalink / raw) To: Russ Cox; +Cc: git On Tue, May 12, 2015 at 08:31:26PM -0400, Russ Cox wrote: > The root cause seems to be that builtin/add.c's edit_patch does not > check the result of the launch_editor call. It probably should. Yes, definitely. Patch is below. > The call to launch_editor in builtin/config.c should probably also be > checked, although there it's not as big a deal, provided the editor > did not modify the file; reapplying the same file should be a no-op. > This is different from git add -e. This one is trickier. Certainly we could propagate the return value and say "hey, editing did not work". But we otherwise have no operation to abort. This is because "git config --edit" does not actually take our normal dot-lock, but rather edits the file in place. IMHO this is questionable, as proper locking seems like half of the purpose of "config --edit" (the other half being that you do not have to type the location of the config file yourself). But I wonder if switching it would make people unhappy, as it would probably break editor rules to do syntax highlighting (e.g., if they are looking for "*/.gitconfig" in the path). -- >8 -- Subject: add: check return value of launch_editor When running "add -e", if launching the editor fails, we do not notice and continue as if the output is what the user asked for. The likely case is that the editor did not touch the contents at all, and we end up adding everything. Reported-by: Russ Cox <rsc@golang.org> Signed-off-by: Jeff King <peff@peff.net> --- builtin/add.c | 3 ++- t/t3702-add-edit.sh | 7 +++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/builtin/add.c b/builtin/add.c index 3390933..4bd98b7 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -208,7 +208,8 @@ static int edit_patch(int argc, const char **argv, const char *prefix) if (run_diff_files(&rev, 0)) die(_("Could not write patch")); - launch_editor(file, NULL, NULL); + if (launch_editor(file, NULL, NULL)) + die(_("editing patch failed")); if (stat(file, &st)) die_errno(_("Could not stat '%s'"), file); diff --git a/t/t3702-add-edit.sh b/t/t3702-add-edit.sh index 4ee47cc..3cb74ca 100755 --- a/t/t3702-add-edit.sh +++ b/t/t3702-add-edit.sh @@ -118,4 +118,11 @@ test_expect_success 'add -e' ' ' +test_expect_success 'add -e notices editor failure' ' + git reset --hard && + echo change >>file && + test_must_fail env GIT_EDITOR=false git add -e && + test_expect_code 1 git diff --exit-code +' + test_done -- 2.4.0.192.g5f8138b ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] add: check return value of launch_editor 2015-05-13 1:21 ` [PATCH] add: check return value of launch_editor Jeff King @ 2015-05-13 8:12 ` Johannes Schindelin 0 siblings, 0 replies; 3+ messages in thread From: Johannes Schindelin @ 2015-05-13 8:12 UTC (permalink / raw) To: Jeff King; +Cc: Russ Cox, git Hi Peff, On 2015-05-13 03:21, Jeff King wrote: > On Tue, May 12, 2015 at 08:31:26PM -0400, Russ Cox wrote: > >> The root cause seems to be that builtin/add.c's edit_patch does not >> check the result of the launch_editor call. It probably should. > > Yes, definitely. Patch is below. Thanks for cleaning up my mess! Ciao, Dscho ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-05-13 8:12 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-05-13 0:31 'EDITOR=false git add -e' adds to index Russ Cox 2015-05-13 1:21 ` [PATCH] add: check return value of launch_editor Jeff King 2015-05-13 8:12 ` Johannes Schindelin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).