threads / patch / 21996

patchrebase -i: print the editor name if it fails to launch

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

## tl;dr

4 messages between Dec 20, 2009 and Dec 21, 2009. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Björn Gustavsson· Dec 20, 2009, 08:32 UTC · lore

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(-)
Show changes to 2 files +10 −2

git-rebase--interactive.sh, git-sh-setup.sh

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
Junio C Hamano· Dec 20, 2009, 18:44 UTC · re: Björn Gustavsson · lore

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

Björn Gustavsson <bgustavsson@gmail.com> writes:
Show 8 quoted lines
> 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· Dec 21, 2009, 07:54 UTC · re: Junio C Hamano · lore

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

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.

Show 5 quoted lines
>
>    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

← back to recent threads