[PATCH 1/2] send-email: refactor and ensure prompting doesn't loop forever

* [PATCH 1/2] send-email: refactor and ensure prompting doesn't loop forever
@ 2009-03-29  1:39 Jay Soffian
  2009-03-29  1:39 ` [PATCH 2/2] send-email: add tests for refactored prompting Jay Soffian
                   ` (2 more replies)
  0 siblings, 3 replies; 17+ messages in thread
From: Jay Soffian @ 2009-03-29  1:39 UTC (permalink / raw)
  To: git; +Cc: Jay Soffian, Matthieu Moy, Junio C Hamano

Several places in send-email prompt for input, and will do so forever
when the input is EOF. This is poor behavior when send-email is run
unattended (say from cron).

This patch refactors the prompting to an ask() function which takes a
prompt, an optional default, and an optional regex to validate the
input. The function returns on EOF, or if a default is provided and the
user simply types return, or if the input passes the validating regex
(which accepts all input by default). The ask() function gives up after
10 tries in case of invalid input.

There are three callers of the function:

1) "Who should the emails appear to be from?" which provides a default
sender. Previously the user would have to type ctrl-d to accept the
default. Now the user can just hit return, or type ctrl-d.

2) "Who should the emails be sent to?". Previously this prompt passed a
second argument ("") to $term->readline() which was ignored. I believe
the intent was to allow the user to just hit return. Now the user
can do so, or type ctrl-d.

3) "Send this email?". Previously this prompt would loop forever until
it got a valid reply. Now it stops prompting on EOF or a valid reply. In
the case where confirm = "inform", it now defaults to "y" on EOF or the
user hitting return, otherwise an invalid reply causes send-email to
terminate.

A followup patch adds tests for the new functionality.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
 git-send-email.perl |   66 ++++++++++++++++++++++++++------------------------
 1 files changed, 34 insertions(+), 32 deletions(-)

This and the next patch address the bug reported by Matthieu in
http://article.gmane.org/gmane.comp.version-control.git/114577

Of course, I've been wanting to refactor the prompting for a while, so I used
the bug fix as an excuse to do so.

j.

diff --git a/git-send-email.perl b/git-send-email.perl
index 546d2eb..f0405c8 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -606,32 +606,40 @@ EOT
 	do_edit(@files);
 }
 
+sub ask {
+	my ($prompt, %arg) = @_;
+	my $valid_re =$arg{valid_re} || ""; # "" matches anything
+	my $default = $arg{default};
+	my $resp;
+	my $i = 0;
+	while ($i++ < 10) {
+		$resp = $term->readline($prompt);
+		if (!defined $resp) { # EOF
+			print "\n";
+			return defined $default ? $default : undef;
+		}
+		if ($resp eq '' and defined $default) {
+			return $default;
+		}
+		if ($resp =~ /$valid_re/) {
+			return $resp;
+		}
+	}
+	return undef;
+}
+
 my $prompting = 0;
 if (!defined $sender) {
 	$sender = $repoauthor || $repocommitter || '';
-
-	while (1) {
-		$_ = $term->readline("Who should the emails appear to be from? [$sender] ");
-		last if defined $_;
-		print "\n";
-	}
-
-	$sender = $_ if ($_);
+	$sender = ask("Who should the emails appear to be from? [$sender] ",
+	              default => $sender);
 	print "Emails will be sent from: ", $sender, "\n";
 	$prompting++;
 }
 
 if (!@to) {
-
-
-	while (1) {
-		$_ = $term->readline("Who should the emails be sent to? ", "");
-		last if defined $_;
-		print "\n";
-	}
-
-	my $to = $_;
-	push @to, parse_address_line($to);
+	my $to = ask("Who should the emails be sent to? ");
+	push @to, parse_address_line($to) if defined $to; # sanitized/validated later
 	$prompting++;
 }
 
@@ -651,13 +659,8 @@ sub expand_aliases {
 @bcclist = expand_aliases(@bcclist);
 
 if ($thread && !defined $initial_reply_to && $prompting) {
-	while (1) {
-		$_= $term->readline("Message-ID to be used as In-Reply-To for the first email? ", $initial_reply_to);
-		last if defined $_;
-		print "\n";
-	}
-
-	$initial_reply_to = $_;
+	$initial_reply_to = ask(
+		"Message-ID to be used as In-Reply-To for the first email? ");
 }
 if (defined $initial_reply_to) {
 	$initial_reply_to =~ s/^\s*<?//;
@@ -839,8 +842,10 @@ X-Mailer: git-send-email $gitversion
 
 	if ($needs_confirm && !$dry_run) {
 		print "\n$header\n";
+		my $ask_default;
 		if ($needs_confirm eq "inform") {
 			$confirm_unconfigured = 0; # squelch this message for the rest of this run
+			$ask_default = "y"; # assume yes on EOF since user hasn't explicitly asked for confirmation
 			print "    The Cc list above has been expanded by additional\n";
 			print "    addresses found in the patch commit message. By default\n";
 			print "    send-email prompts before sending whenever this occurs.\n";
@@ -851,13 +856,10 @@ X-Mailer: git-send-email $gitversion
 			print "    To retain the current behavior, but squelch this message,\n";
 			print "    run 'git config --global sendemail.confirm auto'.\n\n";
 		}
-		while (1) {
-			chomp ($_ = $term->readline(
-				"Send this email? ([y]es|[n]o|[q]uit|[a]ll): "
-			));
-			last if /^(?:yes|y|no|n|quit|q|all|a)/i;
-			print "\n";
-		}
+		$_ = ask("Send this email? ([y]es|[n]o|[q]uit|[a]ll): ",
+		         valid_re => qr/^(?:yes|y|no|n|quit|q|all|a)/i,
+		         default => $ask_default);
+		die "Send this email reply required" unless defined $_;
 		if (/^n/i) {
 			return;
 		} elsif (/^q/i) {
-- 
1.6.2.313.g33352

^ permalink raw reply related	[flat|nested] 17+ messages in thread