# [PATCH] rebase -i: print the editor name if it fails to launch 4 messages from 2009-12-20 to 2009-12-21. Participants: Björn Gustavsson, Junio C Hamano. Thread: https://gitlist.dev/t/21996 ## Björn Gustavsson, 2009-12-20 08:32 Subject: [PATCH] rebase -i: print the editor name if it fails to launch Message-ID: <4B2DE132.1080700@gmail.com> URL: https://gitlist.dev/e/4B2DE132.1080700%40gmail.com ``` The "git rebase -i" commands simply prints "Could not execute editor" if the editor fails to launch. Be a little bit more helpful and be consistent with the built-in commands that use launch_editor() and also print the name of the editor that failed to launch. Signed-off-by: Björn Gustavsson --- Here is a suggestion for how "git rebase -i" can be a little bit more helpful if it fails to launch the editor. This patch can be applied on top of my previous patch. I am not sure whether "git am" also should be modified to call git_editor_error_string. Currently it ignores any errors from the call to git_editor. Should it abort if there is an error or should it print a warning like this echo "Warning: $(git_editor_error_string)" if the editor fails to launch? Grepping all *.sh files, I found no other references to git_editor. git-rebase--interactive.sh | 3 +-- git-sh-setup.sh | 9 +++++++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index d529328..a08a813 100755 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -778,8 +778,7 @@ EOF die_abort "Nothing to do" cp "$TODO" "$TODO".backup - git_editor "$TODO" || - die_abort "Could not execute editor" + git_editor "$TODO" || die_abort "$(git_editor_error_string)" has_action "$TODO" || die_abort "Nothing to do" diff --git a/git-sh-setup.sh b/git-sh-setup.sh index dfcb807..36782ec 100755 --- a/git-sh-setup.sh +++ b/git-sh-setup.sh @@ -107,6 +107,15 @@ git_editor() { eval "$GIT_EDITOR" '"$@"' } +git_editor_error_string() { + if test -z "${GIT_EDITOR:+set}" + then + GIT_EDITOR="$(git var GIT_EDITOR)" || + (echo "No editor configured."; return 0) + fi + echo "There was a problem with the editor '$GIT_EDITOR'." +} + sane_grep () { GREP_OPTIONS= LC_ALL=C grep "$@" } -- 1.6.6.rc3.2.ge3899 ``` ## Björn Gustavsson, 2009-12-20 08:40 Subject: Re: [PATCH] rebase -i: print the editor name if it fails to launch Message-ID: <6672d0160912200040s6fbf5833q252b40bef4ea42b0@mail.gmail.com> URL: https://gitlist.dev/e/6672d0160912200040s6fbf5833q252b40bef4ea42b0%40mail.gmail.com In-Reply-To: <4B2DE132.1080700@gmail.com> ``` Oops, I seem to have hit the Send button too early. Please ignore the first email that stops in mid sentence. -- Björn Gustavsson, Erlang/OTP, Ericsson AB ``` ## Junio C Hamano, 2009-12-20 18:44 Subject: Re: [PATCH] rebase -i: print the editor name if it fails to launch Message-ID: <7v4onlbi3q.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v4onlbi3q.fsf%40alter.siamese.dyndns.org In-Reply-To: <4B2DE132.1080700@gmail.com> ``` Björn Gustavsson writes: > I am not sure whether "git am" also should be modified > to call git_editor_error_string. Currently it ignores > any errors from the call to git_editor. Should it abort > if there is an error or should it print a warning like this > > echo "Warning: $(git_editor_error_string)" > > if the editor fails to launch? It probably is better, even though the lack of check there is not as harmful compared to the one in "rebase -i" that annoyed you (which I also see occasionally after freshly rebooting my environment, as I use emacsclient like you do). You will be taken back to the "what now?" interactive loop and have a chance to \C-c out of it. So such a patch would help not in the sense that it would prevent the command from doing what you didn't intend to (which is your "rebase -i" patch is about), but in the sense that it hints what needs to be fixed once you break out of the command. > - git_editor "$TODO" || > - die_abort "Could not execute editor" > + git_editor "$TODO" || die_abort "$(git_editor_error_string)" Isn't this too elaborate? git_editor() has already run and when it attempted to launch the editor it assigned to GIT_EDITOR in order to use it as an eval string. git_editor "$TODO" || die_abort "Failed to run '${GIT_EDITOR:-your editor}'" would suffice, no? The alternative string is for a case where "git var" gave an empty string, or GIT_EDITOR was an empty string from the beginning. ``` ## Björn Gustavsson, 2009-12-21 07:54 Subject: Re: [PATCH] rebase -i: print the editor name if it fails to launch Message-ID: <6672d0160912202354m124700dbw6e2089446d88b24@mail.gmail.com> URL: https://gitlist.dev/e/6672d0160912202354m124700dbw6e2089446d88b24%40mail.gmail.com In-Reply-To: <7v4onlbi3q.fsf@alter.siamese.dyndns.org> ``` 2009/12/20 Junio C Hamano : > Isn't this too elaborate?  git_editor() has already run and when it > attempted to launch the editor it assigned to GIT_EDITOR in order to use > it as an eval string. I wanted to hide the details about the inner working of git_editor(), but since it is only called in one place, yes, it is too elaborate. > >    git_editor "$TODO" || >    die_abort "Failed to run '${GIT_EDITOR:-your editor}'" > > would suffice, no? Yes. Much better. I will rewrite my patch when I'll get home from work later today. -- Björn Gustavsson, Erlang/OTP, Ericsson AB ```