[BUG] Infinite loop in git send-email if ran non-interactively.

Hi,

I've been hit by c1f2aa45b (send-email: add --confirm option and
configuration setting) running git send-email from a cron job.

The problem is that the cron job is ran non-interactively, and
therefore the confirmation question in git-send-email.perl goes
infinite loop:

		while (1) {
			chomp ($_ = $term->readline(
				"Send this email? ([y]es|[n]o|[q]uit|[a]ll): "
			));
                        print "answer=$_.\n";
			last if /^(?:yes|y|no|n|quit|q|all|a)/i;
			print "\n";
		}

Infinite loop is bad, but it gets even worse since this prints error
messages to stdout, and therefore to a log file in the case of my
script. In short, I woke up this morning with a file filling my disk
with

print() on closed filehandle FOUT at /usr/share/perl/5.8/Term/ReadLine.pm line 193.
readline() on closed filehandle FIN at /usr/share/perl/5.8/Term/ReadLine.pm line 395.
print() on closed filehandle FOUT at /usr/share/perl/5.8/Term/ReadLine.pm line 203.
Use of uninitialized value in scalar chomp at /home/moy/local/usr/libexec/git-core//git-send-email line 856.
Use of uninitialized value in pattern match (m//) at /home/moy/local/usr/libexec/git-core//git-send-email line 859.
print() on closed filehandle FOUT at /usr/share/perl/5.8/Term/ReadLine.pm line 193.
readline() on closed filehandle FIN at /usr/share/perl/5.8/Term/ReadLine.pm line 395.
print() on closed filehandle FOUT at /usr/share/perl/5.8/Term/ReadLine.pm line 203.
Use of uninitialized value in scalar chomp at /home/moy/local/usr/libexec/git-core//git-send-email line 856.
Use of uninitialized value in pattern match (m//) at /home/moy/local/usr/libexec/git-core//git-send-email line 859.
...

I think, non-interactive runs of send-email should assume "yes"
instead of prompting. In any case, it should not do infinite loop (I
guess I don't have to argue for this ;-) )

Can someone more fluent in perl than me add a

if(session-is-interactive) {
...
}

around this confirmation prompt?

(side-note : there is indeed some code to handle the cases where the
terminal doesn't work with readline, added by Junio in 280242d1, but
the FakeTerm part doesn't seem to be executed in my case:

my $term = eval {
	$ENV{"GIT_SEND_EMAIL_NOTTY"}
		? new Term::ReadLine 'git-send-email', \*STDIN, \*STDOUT
		: new Term::ReadLine 'git-send-email';
};
if ($@) {
	$term = new FakeTerm "$@: going non-interactive";
}
)

(in the meantime, I'll add --confirm never to my script)

Thanks,

--
Matthieu

On Wed, Mar 25, 2009 at 5:36 AM, Matthieu Moy [off-list ref] wrote:

I think, non-interactive runs of send-email should assume "yes"

Hmm, probably.

instead of prompting. In any case, it should not do infinite loop (I
guess I don't have to argue for this ;-) )

Right, I'll followup with a fix.

Can someone more fluent in perl than me add a

if(session-is-interactive) {
...
}

around this confirmation prompt?

(side-note : there is indeed some code to handle the cases where the
terminal doesn't work with readline, added by Junio in 280242d1, but
the FakeTerm part doesn't seem to be executed in my case:

my $term = eval {
       $ENV{"GIT_SEND_EMAIL_NOTTY"}
               ? new Term::ReadLine 'git-send-email', \*STDIN, \*STDOUT
               : new Term::ReadLine 'git-send-email';
};
if ($@) {
       $term = new FakeTerm "$@: going non-interactive";
}
)

(in the meantime, I'll add --confirm never to my script)

Actually, FakeTerm is only triggered if Term::ReadLine is not
available. The "going non-interactive" message is misleading in this
case, as it has nothing to do with whether stdin is a tty or not.

j.

On Wed, Mar 25, 2009 at 5:36 AM, Matthieu Moy [off-list ref] wrote:

Hi,

I've been hit by c1f2aa45b (send-email: add --confirm option and
configuration setting) running git send-email from a cron job.

BTW, can you provide the details on how you are invoking send-email?

Have stdin/stdout/stderr been redirected? What arguments are you
giving it? And is this Vixie cron?

Thanks,

j.

On Wed, Mar 25, 2009 at 12:03 PM, Jay Soffian [off-list ref] wrote:

Actually, FakeTerm is only triggered if Term::ReadLine is not
available. The "going non-interactive" message is misleading in this
case, as it has nothing to do with whether stdin is a tty or not.

Sorry, that's not true. Apparently the non-interactive test is not
working properly in this case.

j.