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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 20, 2009, 18:44 UTC
Message-ID: <7v4onlbi3q.fsf@alter.siamese.dyndns.org>
In-Reply-To: <4B2DE132.1080700@gmail.com>

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.