{"thread":{"id":"21996","subject":"[PATCH] rebase -i: print the editor name if it fails to launch","startedAt":"2009-12-20T08:32:50Z","lastAt":"2009-12-21T07:54:05Z","messageCount":4,"participants":["Björn Gustavsson","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"130183","messageId":"4B2DE132.1080700@gmail.com","threadId":"21996","inReplyTo":null,"subject":"[PATCH] rebase -i: print the editor name if it fails to launch","fromName":"Björn Gustavsson","fromEmail":"bgustavsson@gmail.com","sentAt":"2009-12-20T08:32:50Z","receivedAt":"2009-12-20T08:32:50Z","isPatch":true,"sender":{"key":"bgustavsson@gmail.com","avatar":"https://avatars.githubusercontent.com/u/74840?v=4"},"body":"The \"git rebase -i\" commands simply prints \"Could not execute\neditor\" if the editor fails to launch.\n\nBe a little bit more helpful and be consistent with the\nbuilt-in commands that use launch_editor() and also print\nthe name of the editor that failed to launch.\n\nSigned-off-by: Björn Gustavsson <bgustavsson@gmail.com>\n---\nHere is a suggestion for how \"git rebase -i\" can be a\nlittle bit more helpful if it fails to launch the editor.\nThis patch can be applied on top of my previous patch.\n\nI am not sure whether \"git am\" also should be modified\nto call git_editor_error_string. Currently it ignores\nany errors from the call to git_editor. Should it abort\nif there is an error or should it print a warning like this\n\necho \"Warning: $(git_editor_error_string)\"\n\nif the editor fails to launch?\n\nGrepping all *.sh files, I found no other references\nto git_editor.\n\n git-rebase--interactive.sh |    3 +--\n git-sh-setup.sh            |    9 +++++++++\n 2 files changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex d529328..a08a813 100755\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -778,8 +778,7 @@ EOF\n \t\t\tdie_abort \"Nothing to do\"\n \n \t\tcp \"$TODO\" \"$TODO\".backup\n-\t\tgit_editor \"$TODO\" ||\n-\t\t\tdie_abort \"Could not execute editor\"\n+\t\tgit_editor \"$TODO\" || die_abort \"$(git_editor_error_string)\"\n \n \t\thas_action \"$TODO\" ||\n \t\t\tdie_abort \"Nothing to do\"\ndiff --git a/git-sh-setup.sh b/git-sh-setup.sh\nindex dfcb807..36782ec 100755\n--- a/git-sh-setup.sh\n+++ b/git-sh-setup.sh\n@@ -107,6 +107,15 @@ git_editor() {\n \teval \"$GIT_EDITOR\" '\"$@\"'\n }\n \n+git_editor_error_string() {\n+\tif test -z \"${GIT_EDITOR:+set}\"\n+\tthen\n+\t\tGIT_EDITOR=\"$(git var GIT_EDITOR)\" ||\n+\t\t(echo \"No editor configured.\"; return 0)\n+\tfi\n+\techo \"There was a problem with the editor '$GIT_EDITOR'.\"\n+}\n+\n sane_grep () {\n \tGREP_OPTIONS= LC_ALL=C grep \"$@\"\n }\n-- \n1.6.6.rc3.2.ge3899\n"},{"id":"130185","messageId":"6672d0160912200040s6fbf5833q252b40bef4ea42b0@mail.gmail.com","threadId":"21996","inReplyTo":"4B2DE132.1080700@gmail.com","subject":"Re: [PATCH] rebase -i: print the editor name if it fails to launch","fromName":"Björn Gustavsson","fromEmail":"bgustavsson@gmail.com","sentAt":"2009-12-20T08:40:14Z","receivedAt":"2009-12-20T08:40:14Z","isPatch":true,"sender":{"key":"bgustavsson@gmail.com","avatar":"https://avatars.githubusercontent.com/u/74840?v=4"},"body":"Oops, I seem to have hit the Send button too early. Please ignore the\nfirst email that stops in mid sentence.\n-- \nBjörn Gustavsson, Erlang/OTP, Ericsson AB\n"},{"id":"130190","messageId":"7v4onlbi3q.fsf@alter.siamese.dyndns.org","threadId":"21996","inReplyTo":"4B2DE132.1080700@gmail.com","subject":"Re: [PATCH] rebase -i: print the editor name if it fails to launch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-20T18:44:41Z","receivedAt":"2009-12-20T18:44:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Björn Gustavsson <bgustavsson@gmail.com> writes:\n\n> I am not sure whether \"git am\" also should be modified\n> to call git_editor_error_string. Currently it ignores\n> any errors from the call to git_editor. Should it abort\n> if there is an error or should it print a warning like this\n>\n> echo \"Warning: $(git_editor_error_string)\"\n>\n> if the editor fails to launch?\n\nIt probably is better, even though the lack of check there is not as\nharmful compared to the one in \"rebase -i\" that annoyed you (which I also\nsee occasionally after freshly rebooting my environment, as I use\nemacsclient like you do).  You will be taken back to the \"what now?\"\ninteractive loop and have a chance to \\C-c out of it.  So such a patch\nwould help not in the sense that it would prevent the command from doing\nwhat you didn't intend to (which is your \"rebase -i\" patch is about), but\nin the sense that it hints what needs to be fixed once you break out of\nthe command.\n\n> -\t\tgit_editor \"$TODO\" ||\n> -\t\t\tdie_abort \"Could not execute editor\"\n> +\t\tgit_editor \"$TODO\" || die_abort \"$(git_editor_error_string)\"\n\nIsn't this too elaborate?  git_editor() has already run and when it\nattempted to launch the editor it assigned to GIT_EDITOR in order to use\nit as an eval string.\n\n    git_editor \"$TODO\" ||\n    die_abort \"Failed to run '${GIT_EDITOR:-your editor}'\"\n\nwould suffice, no?\n\nThe alternative string is for a case where \"git var\" gave an empty string,\nor GIT_EDITOR was an empty string from the beginning.\n"},{"id":"130205","messageId":"6672d0160912202354m124700dbw6e2089446d88b24@mail.gmail.com","threadId":"21996","inReplyTo":"7v4onlbi3q.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] rebase -i: print the editor name if it fails to launch","fromName":"Björn Gustavsson","fromEmail":"bgustavsson@gmail.com","sentAt":"2009-12-21T07:54:05Z","receivedAt":"2009-12-21T07:54:05Z","isPatch":true,"sender":{"key":"bgustavsson@gmail.com","avatar":"https://avatars.githubusercontent.com/u/74840?v=4"},"body":"2009/12/20 Junio C Hamano <gitster@pobox.com>:\n\n> Isn't this too elaborate?  git_editor() has already run and when it\n> attempted to launch the editor it assigned to GIT_EDITOR in order to use\n> it as an eval string.\n\nI wanted to hide the details about the inner working of git_editor(),\nbut since it is only called in one place, yes, it is too elaborate.\n\n>\n>    git_editor \"$TODO\" ||\n>    die_abort \"Failed to run '${GIT_EDITOR:-your editor}'\"\n>\n> would suffice, no?\n\nYes. Much better. I will rewrite my patch when I'll get home from\nwork later today.\n\n-- \nBjörn Gustavsson, Erlang/OTP, Ericsson AB\n"}]}