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

[ message continues ]

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

--

[ message continues ]

Hello Michael,

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

[ message continues ]

FWIW:

  Acked-by: Jonathan Nieder <jrnieder@gmail.com>

Next time, please CC me.

Thanks,
Jonathan
--

[ message continues ]

Yep, sorry for the omission.

Michael
--

[ message continues ]

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.


--

[ message continues ]

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

[ message continues ]

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

[ message continues ]

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

[ message continues ]

Yep, I'm fine with this.

Cheers,
Michael
--

[ message continues ]

Yes, remote hooks invoked over ssh transport won't usually have a tty, but

Thanks.
--

[ message continues ]

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

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

--

[ message continues ]

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 

//Peter

--

[ message continues ]

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

[ message continues ]