[PATCH] send-email: confirm on empty mail subjects

[PATCH] send-email: confirm on empty mail subjects

[Jan Engelhardt]

When the user forgot to enter a subject in a compose session,
send-email will now inquire whether this is really intended, similar
to what the Alpine MUA does when a subject is absent.

Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
---
 git-send-email.perl |   25 ++++++++++++++++++++-----
 1 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index d508f83..7d56fba 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -553,11 +553,26 @@ EOT
 	}
 	close(C);
 
-	if ($annotate) {
-		do_edit($compose_filename, @files);
-	} else {
-		do_edit($compose_filename);
-	}
+	my $re_edit = 0;
+	do {
+		if ($annotate) {
+			do_edit($compose_filename, @files);
+		} else {
+			do_edit($compose_filename);
+		}
+
+		open(C, "<", $compose_filename) ||
+			die "Failed to open $compose_filename: $!";
+		if (grep(/^Subject:\s*$/i, <C>)) {
+			my $r = ask("No Subject, send anyway? ".
+			            "([y]es|[n]o|[e]dit again): ",
+			            valid_re => qr/^[yne]/i,
+			            default => "n");
+			$re_edit = lc(substr($r, 0, 1)) eq "e";
+			exit(0) if lc(substr($r, 0, 1)) eq "n";
+		}
+		close C;
+	} while ($re_edit);
 
 	open(C2,">",$compose_filename . ".final")
 		or die "Failed to open $compose_filename.final : " . $!;
-- 
1.6.4

Re: [PATCH] send-email: confirm on empty mail subjects

[Junio C Hamano]

Jan Engelhardt <jengelh@medozas.de> writes:

> When the user forgot to enter a subject in a compose session,
> send-email will now inquire whether this is really intended, similar
> to what the Alpine MUA does when a subject is absent.

This seems to break t9001...

Re: [PATCH] send-email: confirm on empty mail subjects

[Jan Engelhardt]

On Thursday 2009-08-06 08:25, Junio C Hamano wrote:

>Jan Engelhardt <jengelh@medozas.de> writes:
>
>> When the user forgot to enter a subject in a compose session,
>> send-email will now inquire whether this is really intended, similar
>> to what the Alpine MUA does when a subject is absent.
>
>This seems to break t9001...
>

Did I miss something in building?

19:26 sovereign:../git/git-1.6.4.1 > quilt pu
Applying patch patches/send-email-empty-subject.diff
patching file git-send-email.perl

Now at patch patches/send-email-empty-subject.diff
19:26 sovereign:../git/git-1.6.4.1 > cd t
19:26 sovereign:../git-1.6.4.1/t > ./t9001-send-email.sh 
*   ok 1: prepare reference tree
*   ok 2: Setup helper tool
*   ok 3: Extract patches
*   ok 4: No confirm with --suppress-cc
*   ok 5: No confirm with --confirm=never
*   ok 6: No confirm with sendemail.confirm=never
*   ok 7: Send patches
*   ok 8: Verify commandline
*   ok 9: Show all headers
*   ok 10: Prompting works
*   ok 11: cccmd works
*   ok 12: reject long lines
*   ok 13: no patch was sent
*   ok 14: Author From: in message body
*   ok 15: Author From: not in message body
*   ok 16: allow long lines with --no-validate
*   ok 17: Invalid In-Reply-To
*   ok 18: Valid In-Reply-To when prompting
*   ok 19: setup fake editor
*   ok 20: --compose works
*   ok 21: first message is compose text
*   ok 22: second message is patch
*   ok 23: sendemail.cc set
*   ok 24: sendemail.cc unset
*   ok 25: sendemail.cccmd
*   ok 26: --suppress-cc=all
*   ok 27: --suppress-cc=body
*   ok 28: --suppress-cc=body --suppress-cc=cccmd
*   ok 29: --suppress-cc=sob
*   ok 30: --suppress-cc=bodycc
*   ok 31: --suppress-cc=cc
*   ok 32: --confirm=always
*   ok 33: --confirm=auto
*   ok 34: --confirm=cc
*   ok 35: --confirm=compose
*   ok 36: confirm by default (due to cc)
*   ok 37: confirm by default (due to --compose)
*   ok 38: confirm detects EOF (inform assumes y)
*   ok 39: confirm detects EOF (auto causes failure)
*   ok 40: confirm doesnt loop forever
*   ok 41: utf8 Cc is rfc2047 encoded
*   ok 42: --compose adds MIME for utf8 body
*   ok 43: --compose respects user mime type
*   ok 44: --compose adds MIME for utf8 subject
*   ok 45: detects ambiguous reference/file conflict
*   ok 46: feed two files
*   ok 47: in-reply-to but no threading
*   ok 48: no in-reply-to and no threading
*   ok 49: threading but no chain-reply-to
* passed all 49 test(s)

Re: [PATCH] send-email: confirm on empty mail subjects

[Junio C Hamano]

Jan Engelhardt <jengelh@medozas.de> writes:

> On Thursday 2009-08-06 08:25, Junio C Hamano wrote:
>
>>Jan Engelhardt <jengelh@medozas.de> writes:
>>
>>> When the user forgot to enter a subject in a compose session,
>>> send-email will now inquire whether this is really intended, similar
>>> to what the Alpine MUA does when a subject is absent.
>>
>>This seems to break t9001...
>>
>
> Did I miss something in building?
>
> 19:26 sovereign:../git/git-1.6.4.1 > quilt pu
> Applying patch patches/send-email-empty-subject.diff
> patching file git-send-email.perl

Is this using 'pu' with your patch?  Near the tip of the 'pu' branch I
have a iffy workaround to "unbreak" the issue, but it is a rather
sledgehammer approach I do not feel comfortable enough to squash into your
patch yet.

Re: [PATCH] send-email: confirm on empty mail subjects

[Jan Engelhardt]

On Montag 2009-08-24 20:19, Junio C Hamano wrote:
>>>
>>>> When the user forgot to enter a subject in a compose session,
>>>> send-email will now inquire whether this is really intended, similar
>>>> to what the Alpine MUA does when a subject is absent.
>>>
>>>This seems to break t9001...
>>
>> Did I miss something in building?
>>
>> 19:26 sovereign:../git/git-1.6.4.1 > quilt pu
>> Applying patch patches/send-email-empty-subject.diff
>> patching file git-send-email.perl
>
>Is this using 'pu' with your patch?

Ah no, `quilt pu` is an autoalias for `quilt push`.

>Near the tip of the 'pu' branch I
>have a iffy workaround to "unbreak" the issue, but it is a rather
>sledgehammer approach I do not feel comfortable enough to squash into your
>patch yet.

I see. Perhaps

	echo -en 'y\ny\n' | ...

would be more gentle? (Noting that, how else should it be,
many a shell do not have -e/-n again.)

Re: [PATCH] send-email: confirm on empty mail subjects

[Junio C Hamano]

Jan Engelhardt <jengelh@medozas.de> writes:

>>Near the tip of the 'pu' branch I
>>have a iffy workaround to "unbreak" the issue, but it is a rather
>>sledgehammer approach I do not feel comfortable enough to squash into your
>>patch yet.
>
> I see. Perhaps
>
> 	echo -en 'y\ny\n' | ...
>
> would be more gentle? (Noting that, how else should it be,
> many a shell do not have -e/-n again.)

You can solve it with printf "y\ny\n", but the reason I said it feels
wrong was because your added tests are the _only_ ones that expect more
than one "yes".

If some _other_ tests that currently need only one "yes" are broken in the
future and starts asking for more than one, we would like to know about
the breakage, but we won't notice it if we unconditionally fed "yes | ..."
or your "two y's | ..." to them.  That is what I am unhappy about the
"iffy workaround".

Re: [PATCH] send-email: confirm on empty mail subjects

[Junio C Hamano]

Jan Engelhardt <jengelh@medozas.de> writes:

> When the user forgot to enter a subject in a compose session,
> send-email will now inquire whether this is really intended, similar
> to what the Alpine MUA does when a subject is absent.
>
> Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
> ---
>  git-send-email.perl |   25 ++++++++++++++++++++-----
>  1 files changed, 20 insertions(+), 5 deletions(-)

We have had this for quite a long time but I have two niggling worries,
one minor, another one showstopper from maintainability point of view.

 - With --confirm=never the program still seems to talk with the terminal.
   I think with --confirm=neber we should not ask but just fail.

 - This does not hook into the confirmation framework the program already
   has, and does not have any way to turn it off.

> diff --git a/git-send-email.perl b/git-send-email.perl
> index d508f83..7d56fba 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -553,11 +553,26 @@ EOT
>  	}
>  	close(C);
>  
> -	if ($annotate) {
> -		do_edit($compose_filename, @files);
> -	} else {
> -		do_edit($compose_filename);
> -	}
> +	my $re_edit = 0;
> +	do {
> +		if ($annotate) {
> +			do_edit($compose_filename, @files);
> +		} else {
> +			do_edit($compose_filename);
> +		}
> +
> +		open(C, "<", $compose_filename) ||
> +			die "Failed to open $compose_filename: $!";
> +		if (grep(/^Subject:\s*$/i, <C>)) {
> +			my $r = ask("No Subject, send anyway? ".
> +			            "([y]es|[n]o|[e]dit again): ",
> +			            valid_re => qr/^[yne]/i,
> +			            default => "n");
> +			$re_edit = lc(substr($r, 0, 1)) eq "e";
> +			exit(0) if lc(substr($r, 0, 1)) eq "n";
> +		}
> +		close C;
> +	} while ($re_edit);
>  
>  	open(C2,">",$compose_filename . ".final")
>  		or die "Failed to open $compose_filename.final : " . $!;
> -- 
> 1.6.4