From: Junio C Hamano <gitster@pobox.com>
Date: Sun, 20 Dec 2009 18:44:41 GMT
Subject: Re: [PATCH] rebase -i: print the editor name if it fails to launch
Message-ID: <7v4onlbi3q.fsf@alter.siamese.dyndns.org>
In-Reply-To: <4B2DE132.1080700@gmail.com>

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.