[PATCH] rebase -i: print the editor name if it fails to launch

[PATCH] rebase -i: print the editor name if it fails to launch

[Björn Gustavsson]

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 <bgustavsson@gmail.com>
---
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

Re: [PATCH] rebase -i: print the editor name if it fails to launch

[Björn Gustavsson]

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

Re: [PATCH] rebase -i: print the editor name if it fails to launch

[Junio C Hamano]

Björn Gustavsson <bgustavsson@gmail.com> 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.

Re: [PATCH] rebase -i: print the editor name if it fails to launch

[Björn Gustavsson]

2009/12/20 Junio C Hamano <gitster@pobox.com>:

> 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