send-email and in-reply-to = n

send-email and in-reply-to = n

[Stephen Boyd]

Can we throw up a big warning or just outright fail if someone types
'n' or 'y' and hits enter for the in-reply-to question in
git-send-email? I saw a git-send-email sent patch with an In-Reply-To
header containing n on lkml today and it makes threading in my mail
client get confused.

https://lkml.org/lkml/headers/2012/8/13/503

Re: send-email and in-reply-to = n

[Junio C Hamano]

Stephen Boyd <bebarino@gmail.com> writes:

> Can we throw up a big warning or just outright fail if someone types
> 'n' or 'y' and hits enter for the in-reply-to question in
> git-send-email? I saw a git-send-email sent patch with an In-Reply-To
> header containing n on lkml today and it makes threading in my mail
> client get confused.

Yeah, I think it is a good idea to minimally sanity check the answer
to in-reply-to (and possibly other fields); perhaps "does it have @
and dot" would be a good enough heuristics.

Please make it so ;-)

Re: send-email and in-reply-to = n

[Martin von Zweigbergk]

On Mon, Aug 13, 2012 at 4:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stephen Boyd <bebarino@gmail.com> writes:
>
>> Can we throw up a big warning or just outright fail if someone types
>> 'n' or 'y' and hits enter for the in-reply-to question in
>> git-send-email? I saw a git-send-email sent patch with an In-Reply-To
>> header containing n on lkml today and it makes threading in my mail
>> client get confused.
>
> Yeah, I think it is a good idea to minimally sanity check the answer
> to in-reply-to (and possibly other fields); perhaps "does it have @
> and dot" would be a good enough heuristics.
>
> Please make it so ;-)

And if you do, please include the check for the value for the From:
header in the "and possibly other fields". I made the same mistake
when asked about that value just a few days ago.

[PATCH] send-email: validate & reconfirm interactive responses

[Junio C Hamano]

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 <gitster@pobox.com>
---
 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*<?//;
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH] send-email: validate & reconfirm interactive responses

[Martin von Zweigbergk]

On Tue, Aug 14, 2012 at 3:25 PM, Junio C Hamano <gitster@pobox.com> 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.

Re: [PATCH] send-email: validate & reconfirm interactive responses

[Junio C Hamano]

Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:

> On Tue, Aug 14, 2012 at 3:25 PM, Junio C Hamano <gitster@pobox.com> 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 <gitster@pobox.com>]

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

Re: [PATCH] send-email: validate & reconfirm interactive responses

[Stephen Boyd]

On Tue, Aug 14, 2012 at 3:25 PM, Junio C Hamano <gitster@pobox.com> 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?

Re: [PATCH] send-email: validate & reconfirm interactive responses

[Junio C Hamano]

Stephen Boyd <bebarino@gmail.com> writes:

> On Tue, Aug 14, 2012 at 3:25 PM, Junio C Hamano <gitster@pobox.com> 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.

Re: [PATCH] send-email: validate & reconfirm interactive responses

[Stephen Boyd]

(Sorry sending this from web interface)

On Wed, Sep 5, 2012 at 8:29 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stephen Boyd <bebarino@gmail.com> 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) {

Re: [PATCH] send-email: validate & reconfirm interactive responses

[Junio C Hamano]

Stephen Boyd <bebarino@gmail.com> 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) {

[PATCH] send-email: initial_to and initial_reply_to are both optional

[Junio C Hamano]

We may pick up additional recipients from the format-patch output
files we are sending, in which case it is perfectly valid to leave
the @initial_to empty when the prompt asks.  We may want to start
a new discussion thread without replying to anything, and it is
valid to leave $initial_reply_to empty.

An earlier update to avoid y@example.com stuffed in address fields
did not take these two cases into account.

Noticed and fix suggested by Stephen Boyd.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * I am tempted to queue this, after asking you to eyeball it, and
   then update the author to pass the blame to you before merging it
   to 'next'.

 git-send-email.perl | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index e89729b..b1fb7e6 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -759,7 +759,8 @@ sub file_declares_8bit_cte {
 }
 
 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 (if any)? ",
+		     default => "",
 		     valid_re => qr/\@.*\./, confirm_only => 1);
 	push @initial_to, parse_address_line($to) if defined $to; # sanitized/validated later
 	$prompting++;
@@ -786,7 +787,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 (if any)? ",
+		default => "",
 		valid_re => qr/\@.*\./, confirm_only => 1);
 }
 if (defined $initial_reply_to) {
-- 
1.7.12.321.g60f00e5

Re: [PATCH] send-email: initial_to and initial_reply_to are both optional

[Stephen Boyd]

On Thu, Sep 6, 2012 at 2:21 PM, Junio C Hamano <gitster@pobox.com> wrote:
> We may pick up additional recipients from the format-patch output
> files we are sending, in which case it is perfectly valid to leave
> the @initial_to empty when the prompt asks.  We may want to start
> a new discussion thread without replying to anything, and it is
> valid to leave $initial_reply_to empty.
>
> An earlier update to avoid y@example.com stuffed in address fields
> did not take these two cases into account.
>
> Noticed and fix suggested by Stephen Boyd.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
>  * I am tempted to queue this, after asking you to eyeball it, and
>    then update the author to pass the blame to you before merging it
>    to 'next'.
>

Looks good, thanks.

Re: [PATCH] send-email: validate & reconfirm interactive responses

[Stephen Boyd]

On Thu, Sep 6, 2012 at 1:03 PM, Junio C Hamano <gitster@pobox.com> 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.