Re: [PATCH] send-email: validate & reconfirm interactive responses
On Thu, Sep 6, 2012 at 1:03 PM, Junio C Hamano wrote: > > If you let $to to go empty with the first hunk of your patch, where > does the mail eventually go? Does anybody later in the code decide > to add some recipient? If there is a reason why an empty input is a > valid here, I think there is a stronger need (that is, stronger than > the above ase for $initial_reply_to) to explain when the user wants > to leave this empty. > I almost never type anything and just use the To header in the patch I want to send. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] send-email: validate & reconfirm interactive responses
Stephen Boyd writes: > It works fine for "Who should the emails appear to be from?" but > beyond that we have "Who should the emails be sent to?" and > "Message-ID to be used as In-Reply-To for the first email?" which I > typically just hit enter to. It seems that they have no "default" > argument so that second if fails. I suppose we can add a default => "" > to these two asks? For $initial_reply_to, I think "empty" means "I do not want to make this message reply to anything", so I think it is OK to either give a default "", or extendign valid_re to also catch an empty string. In either case, the prompt message may want to clarify what happens when you give an empty input (e.g. "leave this empty to start a new thread", or something). If you let $to to go empty with the first hunk of your patch, where does the mail eventually go? Does anybody later in the code decide to add some recipient? If there is a reason why an empty input is a valid here, I think there is a stronger need (that is, stronger than the above ase for $initial_reply_to) to explain when the user wants to leave this empty. > 8<- > diff --git a/git-send-email.perl b/git-send-email.perl > index 607137b..13d813e 100755 > --- a/git-send-email.perl > +++ b/git-send-email.perl > @@ -760,6 +760,7 @@ if (!defined $sender) { > > if (!@initial_to && !defined $to_cmd) { > my $to = ask("Who should the emails be sent to? ", > +default => "", > valid_re => qr/\@.*\./, confirm_only => 1); > push @initial_to, parse_address_line($to) if defined $to; # > sanitized/validated later > $prompting++; > @@ -787,6 +788,7 @@ sub expand_one_alias { > if ($thread && !defined $initial_reply_to && $prompting) { > $initial_reply_to = ask( > "Message-ID to be used as In-Reply-To for the first email? ", > + default => "", > valid_re => qr/\@.*\./, confirm_only => 1); > } > if (defined $initial_reply_to) { -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] send-email: validate & reconfirm interactive responses
(Sorry sending this from web interface) On Wed, Sep 5, 2012 at 8:29 PM, Junio C Hamano wrote: > Stephen Boyd writes: >> This is now bugging me if I just hit enter and don't want to specify >> anything for >> these headers (I want the defaults or what's in the files already). >> Can we allow >> the empty string to be valid as well so I don't have to go through >> these prompts? > > That indeed was the intention, and if it is not behaving, you found > a bug. > > The relevant code in "sub ask" does this: > > ... > $resp = $term->readline($prompt); > if (!defined $resp) { # EOF > print "\n"; > return defined $default ? $default : undef; > } > if ($resp eq '' and defined $default) { > return $default; > } > if (!defined $valid_re or $resp =~ /$valid_re/) { > return $resp; > } > > I am scratching my head wondering why your "just hit enter" does not > trigger the "if response is empty and we have default, just return it" > codepath we can see above. It shouldn't even trigger the regexp > based validation codepath in the first place. > It works fine for "Who should the emails appear to be from?" but beyond that we have "Who should the emails be sent to?" and "Message-ID to be used as In-Reply-To for the first email?" which I typically just hit enter to. It seems that they have no "default" argument so that second if fails. I suppose we can add a default => "" to these two asks? 8<- diff --git a/git-send-email.perl b/git-send-email.perl index 607137b..13d813e 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -760,6 +760,7 @@ if (!defined $sender) { if (!@initial_to && !defined $to_cmd) { my $to = ask("Who should the emails be sent to? ", +default => "", valid_re => qr/\@.*\./, confirm_only => 1); push @initial_to, parse_address_line($to) if defined $to; # sanitized/validated later $prompting++; @@ -787,6 +788,7 @@ sub expand_one_alias { if ($thread && !defined $initial_reply_to && $prompting) { $initial_reply_to = ask( "Message-ID to be used as In-Reply-To for the first email? ", + default => "", valid_re => qr/\@.*\./, confirm_only => 1); } if (defined $initial_reply_to) { -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] send-email: validate & reconfirm interactive responses
Stephen Boyd writes: > On Tue, Aug 14, 2012 at 3:25 PM, Junio C Hamano wrote: >> @@ -745,13 +752,15 @@ sub file_declares_8bit_cte { >> if (!defined $sender) { >> $sender = $repoauthor || $repocommitter || ''; >> $sender = ask("Who should the emails appear to be from? [$sender] ", >> - default => $sender); >> + default => $sender, >> + valid_re => qr/\@.*\./, confirm_only => 1); > > This is now bugging me if I just hit enter and don't want to specify > anything for > these headers (I want the defaults or what's in the files already). > Can we allow > the empty string to be valid as well so I don't have to go through > these prompts? That indeed was the intention, and if it is not behaving, you found a bug. The relevant code in "sub ask" does this: ... $resp = $term->readline($prompt); if (!defined $resp) { # EOF print "\n"; return defined $default ? $default : undef; } if ($resp eq '' and defined $default) { return $default; } if (!defined $valid_re or $resp =~ /$valid_re/) { return $resp; } I am scratching my head wondering why your "just hit enter" does not trigger the "if response is empty and we have default, just return it" codepath we can see above. It shouldn't even trigger the regexp based validation codepath in the first place. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] send-email: validate & reconfirm interactive responses
On Tue, Aug 14, 2012 at 3:25 PM, Junio C Hamano wrote: > @@ -745,13 +752,15 @@ sub file_declares_8bit_cte { > if (!defined $sender) { > $sender = $repoauthor || $repocommitter || ''; > $sender = ask("Who should the emails appear to be from? [$sender] ", > - default => $sender); > + default => $sender, > + valid_re => qr/\@.*\./, confirm_only => 1); This is now bugging me if I just hit enter and don't want to specify anything for these headers (I want the defaults or what's in the files already). Can we allow the empty string to be valid as well so I don't have to go through these prompts? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] send-email: validate & reconfirm interactive responses
Martin von Zweigbergk writes: > On Tue, Aug 14, 2012 at 3:25 PM, Junio C Hamano wrote: >> People answer 'y' to "Who should the emails appear to be from?" and >> 'n' to "Message-ID to be used as In-Reply-To for the first email?" >> for some unknown reason. > > Yeah, I know :-(. I did feel stupid already. Thanks for improving. Actually, it is a very understandable mistake and I do not think it is a user stupidity. It is a UI bug in the prompter that gives: Who should the emails appear to be from? [Junio C Hamano ] and does *not* tell the user that the way to accept the default is to just press RETURN. It makes it look as if it is asking "Is it OK to use this?", and it is a natural response to say "Yes" to the prompt. We would want to do something like the following pseudo-patch, I think, but I do not know what is the best way to show both $prompt and the "press return" suggestion to the user, so I am not going to do this myself. A tested patch to improve this is very much welcomed. Thanks. diff --git a/git-send-email.perl b/git-send-email.perl index 607137b..2ec0ce8 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -688,6 +688,9 @@ sub ask { unless defined $term->IN and defined fileno($term->IN) and defined $term->OUT and defined fileno($term->OUT); while ($i++ < 10) { + if (defined $default) { + SAY "(press RETURN to accept the default)"; + } $resp = $term->readline($prompt); if (!defined $resp) { # EOF print "\n"; -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] send-email: validate & reconfirm interactive responses
On Tue, Aug 14, 2012 at 3:25 PM, Junio C Hamano wrote: > People answer 'y' to "Who should the emails appear to be from?" and > 'n' to "Message-ID to be used as In-Reply-To for the first email?" > for some unknown reason. Yeah, I know :-(. I did feel stupid already. Thanks for improving. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] send-email: validate & reconfirm interactive responses
People answer 'y' to "Who should the emails appear to be from?" and 'n' to "Message-ID to be used as In-Reply-To for the first email?" for some unknown reason. While it is possible that really have "y" as your local username and sending the mail to your local colleagues, it is plausible that it could be an error. Fortunately, our interactive prompter already has input validation mechanism built-in. Enhance it so that we can optionally reconfirm and allow the user to pass an input that does not validate, and "softly" require input to the sender, in-reply-to, and recipient to contain "@" and "." in this order, which would catch most cases of mistakes. Signed-off-by: Junio C Hamano --- git-send-email.perl | 16 +--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/git-send-email.perl b/git-send-email.perl index ef30c55..e89729b 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -681,6 +681,7 @@ sub ask { my ($prompt, %arg) = @_; my $valid_re = $arg{valid_re}; my $default = $arg{default}; + my $confirm_only = $arg{confirm_only}; my $resp; my $i = 0; return defined $default ? $default : undef @@ -698,6 +699,12 @@ sub ask { if (!defined $valid_re or $resp =~ /$valid_re/) { return $resp; } + if ($confirm_only) { + my $yesno = $term->readline("Are you sure you want to use <$resp> [y/N]? "); + if (defined $yesno && $yesno =~ /y/i) { + return $resp; + } + } } return undef; } @@ -745,13 +752,15 @@ sub file_declares_8bit_cte { if (!defined $sender) { $sender = $repoauthor || $repocommitter || ''; $sender = ask("Who should the emails appear to be from? [$sender] ", - default => $sender); + default => $sender, + valid_re => qr/\@.*\./, confirm_only => 1); print "Emails will be sent from: ", $sender, "\n"; $prompting++; } if (!@initial_to && !defined $to_cmd) { - my $to = ask("Who should the emails be sent to? "); + my $to = ask("Who should the emails be sent to? ", +valid_re => qr/\@.*\./, confirm_only => 1); push @initial_to, parse_address_line($to) if defined $to; # sanitized/validated later $prompting++; } @@ -777,7 +786,8 @@ sub expand_one_alias { if ($thread && !defined $initial_reply_to && $prompting) { $initial_reply_to = ask( - "Message-ID to be used as In-Reply-To for the first email? "); + "Message-ID to be used as In-Reply-To for the first email? ", + valid_re => qr/\@.*\./, confirm_only => 1); } if (defined $initial_reply_to) { $initial_reply_to =~ s/^\s*http://vger.kernel.org/majordomo-info.html