[REGRESSION] git var GIT_EDITOR fails without tty

[REGRESSION] git var GIT_EDITOR fails without tty

[Uwe Kleine-König]

Hello,

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.

When called they don't have a tty, and this makes git var fail:

	user@host:~$ ssh localhost "git var GIT_EDITOR"
	fatal: Terminal is dumb, but EDITOR unset

IMHO git send-email should only call $(git var GIT_EDITOR) when it
actually needs it.

(Note I'm using the Debian packaged version of git.  I don't think this
problem is Debian specific, I didn't check though.)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH] send-email: lazily assign editor variable

[Michael J Gruber]

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, $_);
-- 
1.7.0.3.435.g097f4

Re: [PATCH] send-email: lazily assign editor variable

[Uwe Kleine-König]

Hello Michael,

On Mon, Mar 22, 2010 at 05:12:53PM +0100, 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>
> ---
>  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, $_);
I havn't tested yet, but (maybe apart from a comment) this looks exactly
like the patch I would have done if I knew perl :-)

So, Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Best regards and thanks for the quick response,
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH] send-email: do not check for editor until needed

[Jonathan Nieder]

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

Re: [PATCH] send-email: lazily assign editor variable

[Jonathan Nieder]

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.

Thanks,
Jonathan

RE: [PATCH] send-email: do not check for editor until needed

[Peter Kjellerstedt]

> -----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;
> +	}

Shouldn't that be:

	if ($#_ == -1) {
		return;
	}

or more readable:

	return if (!@_);

as I assume you are trying to protect do_edit() from being called 
without arguments?

> +	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

//Peter

Re: [PATCH] send-email: lazily assign editor variable

[Michael J Gruber]

Jonathan Nieder venit, vidit, dixit 23.03.2010 01:58:
> 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.

Yep, sorry for the omission.

Michael

Re: [PATCH] send-email: do not check for editor until needed

[Jonathan Nieder]

Peter Kjellerstedt wrote:

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

Probably. :)

> as I assume you are trying to protect do_edit() from being called 
> without arguments?

I tested out Michael J Gruber’s simpler patch and it both makes sense
and works fine, so I’d suggest going with it instead.

Still, thanks for the comments.  It is good to know what I need to
learn.

Regards,
Jonathan

Re: [PATCH] send-email: lazily assign editor variable

[Junio C Hamano]

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.

Re: [PATCH] send-email: lazily assign editor variable

[Jonathan Nieder]

Junio C Hamano wrote:

> 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.

Re: [PATCH] send-email: lazily assign editor variable

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 24.03.2010 18:52:
> 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

Also think "hook" ;)

>     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.
> 
> 

Yep, I'm fine with this.

Cheers,
Michael

Re: [PATCH] send-email: lazily assign editor variable

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

>>     However, "git var GIT_EDITOR" run without a tty (think "cron job") would
>
> Also think "hook" ;)

Yes, remote hooks invoked over ssh transport won't usually have a tty, but
the local hooks (e.g. post-commit) usually do, I think.

> Yep, I'm fine with this.

Thanks.

Re: [PATCH] send-email: lazily assign editor variable

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> 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).

Re: [PATCH] send-email: lazily assign editor variable

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> 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).

With DISPLAY set, running “TERM=dumb vi | cat” with an appropriate
symlink in $PATH:

vim
	Prints “Vim: Warning: Output is not to a terminal” to stderr,
	waits about a second, then
	runs without clearing the screen, using ANSI escapes to move around.

elvis 1.4
	Prints “This termcap entry lacks the :up=: capability” to stderr,
	fails with status 1.

elvis 2 (more precisely, the elvisnox script from Debian)
	Prints “termcap needs up” to stderr,
	fails with status 1.

nvi
	Prints “ex/vi: Vi's standard input and output must be a terminal” to stderr,
	fails with status 1.

With TERM=dumb but standard output going straight to the terminal, the
situation is not very different for vim and elvis, while nvi gets
utterly confused (it only writes output to the bottom line for some
reason).

So I think it still makes sense to work around this bug in the vi clones.

HTH,
Jonathan