# [REGRESSION] git var GIT_EDITOR fails without tty 14 messages from 2010-03-22 to 2010-03-26. Participants: Uwe Kleine-König, Michael J Gruber, Jonathan Nieder, Peter Kjellerstedt, Junio C Hamano. Thread: https://gitlist.dev/t/23140 ## Uwe Kleine-König, 2010-03-22 14:59 Subject: [REGRESSION] git var GIT_EDITOR fails without tty Message-ID: <20100322145947.GA1709@pengutronix.de> URL: https://gitlist.dev/e/20100322145947.GA1709%40pengutronix.de ``` 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/ | ``` ## Michael J Gruber, 2010-03-22 16:12 Subject: [PATCH] send-email: lazily assign editor variable Message-ID: URL: https://gitlist.dev/e/eb0d92cf28820eb9783e789a8c4c62c3af4dd0d0.1269274203.git.git%40drmicha.warpmail.net In-Reply-To: <20100322145947.GA1709@pengutronix.de> ``` 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 Reported-by: Uwe Kleine-König --- 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 ``` ## Uwe Kleine-König, 2010-03-22 16:41 Subject: Re: [PATCH] send-email: lazily assign editor variable Message-ID: <20100322164152.GA30332@pengutronix.de> URL: https://gitlist.dev/e/20100322164152.GA30332%40pengutronix.de In-Reply-To: ``` 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 > Reported-by: Uwe Kleine-König > --- > 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 Best regards and thanks for the quick response, Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | ``` ## Jonathan Nieder, 2010-03-22 23:25 Subject: [PATCH] send-email: do not check for editor until needed Message-ID: <20100322232531.GA2168@progeny.tock> URL: https://gitlist.dev/e/20100322232531.GA2168%40progeny.tock In-Reply-To: <20100322145947.GA1709@pengutronix.de> ``` 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 Signed-off-by: Jonathan Nieder --- 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 ``` ## Jonathan Nieder, 2010-03-23 00:58 Subject: Re: [PATCH] send-email: lazily assign editor variable Message-ID: <20100323005806.GA9335@progeny.tock> URL: https://gitlist.dev/e/20100323005806.GA9335%40progeny.tock In-Reply-To: ``` 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 > Reported-by: Uwe Kleine-König FWIW: Acked-by: Jonathan Nieder Next time, please CC me. Thanks, Jonathan ``` ## Peter Kjellerstedt, 2010-03-23 09:15 Subject: RE: [PATCH] send-email: do not check for editor until needed Message-ID: URL: https://gitlist.dev/e/A612847CFE53224C91B23E3A5B48BAC73EC0B01EE8%40xmail3.se.axis.com In-Reply-To: <20100322232531.GA2168@progeny.tock> ``` > -----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 > Signed-off-by: Jonathan Nieder > --- > 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 ``` ## Michael J Gruber, 2010-03-23 10:56 Subject: Re: [PATCH] send-email: lazily assign editor variable Message-ID: <4BA89E53.3000201@drmicha.warpmail.net> URL: https://gitlist.dev/e/4BA89E53.3000201%40drmicha.warpmail.net In-Reply-To: <20100323005806.GA9335@progeny.tock> ``` 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 >> Reported-by: Uwe Kleine-König > > FWIW: > > Acked-by: Jonathan Nieder > > Next time, please CC me. Yep, sorry for the omission. Michael ``` ## Jonathan Nieder, 2010-03-23 19:25 Subject: Re: [PATCH] send-email: do not check for editor until needed Message-ID: <20100323192557.GA1511@progeny.tock> URL: https://gitlist.dev/e/20100323192557.GA1511%40progeny.tock In-Reply-To: ``` 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 ``` ## Junio C Hamano, 2010-03-24 17:52 Subject: Re: [PATCH] send-email: lazily assign editor variable Message-ID: <7vaatxobef.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vaatxobef.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Michael J Gruber 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. ``` ## Jonathan Nieder, 2010-03-25 05:17 Subject: Re: [PATCH] send-email: lazily assign editor variable Message-ID: <20100325051743.GB5784@progeny.tock> URL: https://gitlist.dev/e/20100325051743.GB5784%40progeny.tock In-Reply-To: <7vaatxobef.fsf@alter.siamese.dyndns.org> ``` 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. ``` ## Michael J Gruber, 2010-03-25 08:03 Subject: Re: [PATCH] send-email: lazily assign editor variable Message-ID: <4BAB18C6.6010202@drmicha.warpmail.net> URL: https://gitlist.dev/e/4BAB18C6.6010202%40drmicha.warpmail.net In-Reply-To: <7vaatxobef.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano venit, vidit, dixit 24.03.2010 18:52: > Michael J Gruber 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 ``` ## Junio C Hamano, 2010-03-26 19:32 Subject: Re: [PATCH] send-email: lazily assign editor variable Message-ID: <7v8w9elvz7.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v8w9elvz7.fsf%40alter.siamese.dyndns.org In-Reply-To: <4BAB18C6.6010202@drmicha.warpmail.net> ``` Michael J Gruber 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. ``` ## Junio C Hamano, 2010-03-26 19:32 Subject: Re: [PATCH] send-email: lazily assign editor variable Message-ID: <7v39zmlvz1.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v39zmlvz1.fsf%40alter.siamese.dyndns.org In-Reply-To: <20100325051743.GB5784@progeny.tock> ``` Jonathan Nieder 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). ``` ## Jonathan Nieder, 2010-03-26 21:45 Subject: Re: [PATCH] send-email: lazily assign editor variable Message-ID: <20100326214540.GA17503@progeny.tock> URL: https://gitlist.dev/e/20100326214540.GA17503%40progeny.tock In-Reply-To: <7v39zmlvz1.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano wrote: > Jonathan Nieder 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 ```