[REGRESSION] git var GIT_EDITOR fails without tty

> b4479f0 (add -i, send-email, svn, p4, etc: use "git var GIT_EDITOR",
> 2009-10-30) introduced the use of git var GIT_EDITOR which may lead to
> problems when send-mail is used without a tty.
> 
> Therefore, use git var GIT_EDITOR only when we actually edit something.
> 
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  git-send-email.perl |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/git-send-email.perl b/git-send-email.perl
> index d612ae8..bb09c0d 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -162,9 +162,12 @@ my $compose_filename;
>  
>  # Handle interactive edition of files.
>  my $multiedit;
> -my $editor = Git::command_oneline('var', 'GIT_EDITOR');
> +my $editor;
>  
>  sub do_edit {
> +	if (!defined($editor)) {
> +		$editor = Git::command_oneline('var', 'GIT_EDITOR');
> +	}
>  	if (defined($multiedit) && !$multiedit) {
>  		map {
>  			system('sh', '-c', $editor.' "$@"', $editor, $_);

> b4479f0 (add -i, send-email, svn, p4, etc: use "git var GIT_EDITOR",
> 2009-10-30) introduced the use of git var GIT_EDITOR which may lead to
> problems when send-mail is used without a tty.
> 
> Therefore, use git var GIT_EDITOR only when we actually edit something.
> 
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

> Michael J Gruber wrote:
> 
>> b4479f0 (add -i, send-email, svn, p4, etc: use "git var GIT_EDITOR",
>> 2009-10-30) introduced the use of git var GIT_EDITOR which may lead to
>> problems when send-mail is used without a tty.
>>
>> Therefore, use git var GIT_EDITOR only when we actually edit something.
>>
>> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
>> Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> FWIW:
> 
>   Acked-by: Jonathan Nieder <jrnieder@gmail.com>
> 
> Next time, please CC me.

> We would want to describe what kind of problems they are better than "may
> lead to problems", though.  Something like this?
> 
>     b4479f0 (add -i, send-email, svn, p4, etc: use "git var GIT_EDITOR",
>     2009-10-30) introduced the use of "git var GIT_EDITOR" to obtain the
>     preferred editor program, instead of reading environment variables
>     themselves.
> 
>     However, "git var GIT_EDITOR" run without a tty (think "cron job") would
>     give a fatal error "Terminal is dumb, but EDITOR unset".  This is not a
>     problem for add-i, svn, p4 and callers of git_editor() defined in
>     git-sh-setup, as all of these call it just before launching the editor.
>     At that point, we know the caller wants to edit, and they cannot without a
>     tty.
> 
>     But send-email ran this near the beginning of the program, even if it is
>     not going to use any editor (e.g. run without --compose).  Fix this by
>     calling the command only when we edit a file.

> Yes, please.  It would be more precise to
>
>   s/without a tty/with TERM=dumb/
>
> in the second paragraph but regardless this is a good description of the
> problem.

>> Yes, please.  It would be more precise to
>>
>>   s/without a tty/with TERM=dumb/
>>
>> in the second paragraph but regardless this is a good description of the
>> problem.
>
> Strictly speaking, with TERM=dumb the true "vi" fell back to the ex mode
> and should have been usable.  A purist might say it is a bug that we fatal
> out in this case (I know that it is deliberately done to help newbies from
> common confusion by not running any editor with TERM=dumb, but to a purist
> in me it feels somewhat wrong).

> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> b4479f0 (add -i, send-email, svn, p4, etc: use "git var GIT_EDITOR",
>> 2009-10-30) introduced the use of git var GIT_EDITOR which may lead to
>> problems when send-mail is used without a tty.
> 
> We would want to describe what kind of problems they are better than "may
> lead to problems", though.  Something like this?
> 
>     b4479f0 (add -i, send-email, svn, p4, etc: use "git var GIT_EDITOR",
>     2009-10-30) introduced the use of "git var GIT_EDITOR" to obtain the
>     preferred editor program, instead of reading environment variables
>     themselves.
> 
>     However, "git var GIT_EDITOR" run without a tty (think "cron job") would

>     give a fatal error "Terminal is dumb, but EDITOR unset".  This is not a
>     problem for add-i, svn, p4 and callers of git_editor() defined in
>     git-sh-setup, as all of these call it just before launching the editor.
>     At that point, we know the caller wants to edit, and they cannot without a
>     tty.
> 
>     But send-email ran this near the beginning of the program, even if it is
>     not going to use any editor (e.g. run without --compose).  Fix this by
>     calling the command only when we edit a file.
> 
> 

> -----Original Message-----
> From: git-owner@vger.kernel.org [mailto:git-owner@vger.kernel.org] On
> Behalf Of Jonathan Nieder
> Sent: den 23 mars 2010 00:26
> To: Uwe Kleine-König
> Cc: git@vger.kernel.org; Junio C Hamano; Marc Kleine-Budde; Jay Soffian
> Subject: [PATCH] send-email: do not check for editor until needed
> 
> Since b4479f074 (add -i, send-email, svn, p4, etc: use "git var
> GIT_EDITOR", 2009-10-30), when called with TERM=dumb and without
> GIT_EDITOR set, git send-email has been failing whether it needs
> an editor or not:
> 
>  $ ssh localhost git send-email --to=me --suppress-cc=all HEAD^..HEAD
>  fatal: Terminal is dumb, but EDITOR unset
>  var GIT_EDITOR: command returned error: 128
> 
> This breaks use of git send-email in existing hook scripts.
> 
> So do not check for an editor unless it is needed.
> 
> Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> Uwe Kleine-König wrote:
> 
> > since b4479f074760a788dd4e353b8c86a7d735afc53e git send-email (and
> > others) use git var GIT_EDITOR.  This is OK as such but it breaks the
> > post-receive hooks that I use on several repositories.
> [...]
> > IMHO git send-email should only call $(git var GIT_EDITOR) when it
> > actually needs it.
> 
> Thanks for reporting.  Does this patch work for you?
> 
> Now if I try without a tty, I get a different error:
> 
>  $ ssh localhost cd $(pwd) '&&' \
> 	git send-email --to=me --suppress-cc=all HEAD^..HEAD
>  Can't locate object method "IN" via package "FakeTerm" at
>  /home/jrn/tmp-git/libexec/git-core/git-send-email line 645.
>  /tmp/olTiwjzrjx/0001-Git-1.7.0.3.patch
> 
> I assume I am not using it correctly, since the relevant code has
> been around for a while.
> 
>  git-send-email.perl |    9 ++++++++-
>  1 files changed, 8 insertions(+), 1 deletions(-)
> 
> diff --git a/git-send-email.perl b/git-send-email.perl
> index e05455f..9406cdd 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -162,9 +162,16 @@ my $compose_filename;
> 
>  # Handle interactive edition of files.
>  my $multiedit;
> -my $editor = Git::command_oneline('var', 'GIT_EDITOR');
> 
>  sub do_edit {
> +	my $editor;
> +
> +	if ($#_ == 0) {
> +		return;
> +	}

> +	git_cmd_try {
> +		$editor = Git::command_oneline('var', 'GIT_EDITOR');
> +	} "no suitable text editor configured\n";
>  	if (defined($multiedit) && !$multiedit) {
>  		map {
>  			system('sh', '-c', $editor.' "$@"', $editor, $_);
> --
> 1.7.0.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

> Shouldn't that be:
> 
> 	if ($#_ == -1) {
> 		return;
> 	}
> 
> or more readable:
> 
> 	return if (!@_);