[PATCH] send-email: add --confirm option

[PATCH] send-email: add --confirm option

[Jay Soffian]

send-email violates the principle of least surprise by automatically
cc'ing additional recipients without confirming this with the user.

This patch teaches send-email a --confirm option. It takes the
following values:

 --confirm=always   always confirm before sending
 --confirm=never    never confirm before sending
 --confirm=cc       confirm before sending when send-email has
                    automatically added addresses from the patch to
                    the Cc list
 --confirm=compose  confirm before sending the first message when
                    using --compose. (Needed to maintain backwards
                    compatibility with existing behavior.)
 --confirm=auto     'cc' + 'compose'

The option defaults to 'compose' if any suppress Cc related options have
been used, otherwise it defaults to 'auto'.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
Junio,

I based this on top of js/send-email.

j.

 Documentation/git-send-email.txt |   16 ++++++++
 git-send-email.perl              |   68 +++++++++++++++++++++++------------
 t/t9001-send-email.sh            |   72 +++++++++++++++++++++++++++++--------
 3 files changed, 117 insertions(+), 39 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 164d149..bcf7ff1 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -199,6 +199,22 @@ specified, as well as 'body' if --no-signed-off-cc is specified.
 Administering
 ~~~~~~~~~~~~~
 
+--confirm::
+	Confirm just before sending:
++
+--
+- 'always' will always confirm before sending
+- 'never' will never confirm before sending
+- 'cc' will confirm before sending when send-email has automatically
+  added addresses from the patch to the Cc list
+- 'compose' will confirm before sending the first message when using --compose.
+- 'auto' is equivalent to 'cc' + 'compose'
+--
++
+Default is the value of 'sendemail.confirm' configuration value; if that
+is unspecified, default to 'auto' unless any of the suppress options
+have been specified, in which case default to 'compose'.
+
 --dry-run::
 	Do everything except actually send the emails.
 
diff --git a/git-send-email.perl b/git-send-email.perl
index adf7ecb..a777e3f 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -75,6 +75,8 @@ git send-email [options] <file | directory | rev-list options >
     --[no-]thread                  * Use In-Reply-To: field. Default on.
 
   Administering:
+    --confirm               <str>  * Confirm recipients before sending;
+                                     auto, cc, compose, always, or never.
     --quiet                        * Output one line of info per email.
     --dry-run                      * Don't actually send the emails.
     --[no-]validate                * Perform patch sanity checks. Default on.
@@ -181,7 +183,7 @@ sub do_edit {
 my ($thread, $chain_reply_to, $suppress_from, $signed_off_by_cc, $cc_cmd);
 my ($smtp_server, $smtp_server_port, $smtp_authuser, $smtp_encryption);
 my ($identity, $aliasfiletype, @alias_files, @smtp_host_parts);
-my ($validate);
+my ($validate, $confirm);
 my (@suppress_cc);
 
 my %config_bool_settings = (
@@ -207,6 +209,7 @@ my %config_settings = (
     "suppresscc" => \@suppress_cc,
     "envelopesender" => \$envelope_sender,
     "multiedit" => \$multiedit,
+    "confirm"   => \$confirm,
 );
 
 # Handle Uncouth Termination
@@ -258,6 +261,7 @@ my $rc = GetOptions("sender|from=s" => \$sender,
 		    "suppress-from!" => \$suppress_from,
 		    "suppress-cc=s" => \@suppress_cc,
 		    "signed-off-cc|signed-off-by-cc!" => \$signed_off_by_cc,
+		    "confirm=s" => \$confirm,
 		    "dry-run" => \$dry_run,
 		    "envelope-sender=s" => \$envelope_sender,
 		    "thread!" => \$thread,
@@ -346,6 +350,11 @@ if ($suppress_cc{'body'}) {
 	delete $suppress_cc{'body'};
 }
 
+# Set confirm
+if (!defined $confirm) {
+	$confirm = scalar %suppress_cc ? 'compose' : 'auto';
+};
+
 # Debugging, print out the suppressions.
 if (0) {
 	print "suppressions:\n";
@@ -663,25 +672,13 @@ if (!defined $smtp_server) {
 	$smtp_server ||= 'localhost'; # could be 127.0.0.1, too... *shrug*
 }
 
-if ($compose) {
-	while (1) {
-		$_ = $term->readline("Send this email? (y|n) ");
-		last if defined $_;
-		print "\n";
-	}
-
-	if (uc substr($_,0,1) ne 'Y') {
-		cleanup_compose_files();
-		exit(0);
-	}
-
-	if ($compose > 0) {
-		@files = ($compose_filename . ".final", @files);
-	}
+if ($compose && $compose > 0) {
+	@files = ($compose_filename . ".final", @files);
 }
 
 # Variables we set as part of the loop over files
-our ($message_id, %mail, $subject, $reply_to, $references, $message);
+our ($message_id, %mail, $subject, $reply_to, $references, $message,
+	$needs_confirm, $message_num);
 
 sub extract_valid_address {
 	my $address = shift;
@@ -837,6 +834,25 @@ X-Mailer: git-send-email $gitversion
 	unshift (@sendmail_parameters,
 			'-f', $raw_from) if(defined $envelope_sender);
 
+	if ($needs_confirm && !$dry_run) {
+		print "\n$header\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";
+		}
+		if (/^n/i) {
+			return;
+		} elsif (/^q/i) {
+			cleanup_compose_files();
+			exit(0);
+		} elsif (/^a/i) {
+			$confirm = 'never';
+		}
+	}
+
 	if ($dry_run) {
 		# We don't want to send the email.
 	} elsif ($smtp_server =~ m#^/#) {
@@ -935,6 +951,7 @@ X-Mailer: git-send-email $gitversion
 $reply_to = $initial_reply_to;
 $references = $initial_reply_to || '';
 $subject = $initial_subject;
+$message_num = 0;
 
 foreach my $t (@files) {
 	open(F,"<",$t) or die "can't open file $t";
@@ -943,11 +960,12 @@ foreach my $t (@files) {
 	my $author_encoding;
 	my $has_content_type;
 	my $body_encoding;
-	@cc = @initial_cc;
+	@cc = ();
 	@xh = ();
 	my $input_format = undef;
 	my @header = ();
 	$message = "";
+	$message_num++;
 	# First unfold multiline header fields
 	while(<F>) {
 		last if /^\s*$/;
@@ -1080,6 +1098,13 @@ foreach my $t (@files) {
 		}
 	}
 
+	$needs_confirm = (
+		$confirm eq "always" or
+		($confirm =~ /^(?:auto|cc)$/ && @cc) or
+		($confirm =~ /^(?:auto|compose)$/ && $compose && $message_num == 1));
+
+	@cc = (@initial_cc, @cc);
+
 	send_message();
 
 	# set up for the next message
@@ -1094,13 +1119,10 @@ foreach my $t (@files) {
 	$message_id = undef;
 }
 
-if ($compose) {
-	cleanup_compose_files();
-}
+cleanup_compose_files();
 
 sub cleanup_compose_files() {
-	unlink($compose_filename, $compose_filename . ".final");
-
+	unlink($compose_filename, $compose_filename . ".final") if $compose;
 }
 
 $smtp->quit if $smtp;
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 4df4f96..2f4a654 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -31,6 +31,11 @@ clean_fake_sendmail() {
 	rm -f commandline* msgtxt*
 }
 
+# Prevent any tests from hanging
+test_expect_success 'Set sendemail.confirm=never' '
+	git config sendemail.confirm never
+'
+
 test_expect_success 'Extract patches' '
     patches=`git format-patch -s --cc="One <one@example.com>" --cc=two@example.com -n HEAD^1`
 '
@@ -175,15 +180,13 @@ test_set_editor "$(pwd)/fake-editor"
 
 test_expect_success '--compose works' '
 	clean_fake_sendmail &&
-	echo y | \
-		GIT_SEND_EMAIL_NOTTY=1 \
-		git send-email \
-		--compose --subject foo \
-		--from="Example <nobody@example.com>" \
-		--to=nobody@example.com \
-		--smtp-server="$(pwd)/fake.sendmail" \
-		$patches \
-		2>errors
+	git send-email \
+	--compose --subject foo \
+	--from="Example <nobody@example.com>" \
+	--to=nobody@example.com \
+	--smtp-server="$(pwd)/fake.sendmail" \
+	$patches \
+	2>errors
 '
 
 test_expect_success 'first message is compose text' '
@@ -375,15 +378,56 @@ test_expect_success '--suppress-cc=cc' '
 	test_suppression cc
 '
 
+test_confirm () {
+	echo y | \
+		GIT_SEND_EMAIL_NOTTY=1 \
+		git send-email \
+		--from="Example <nobody@example.com>" \
+		--to=nobody@example.com \
+		--smtp-server="$(pwd)/fake.sendmail" \
+		$@ \
+		$patches | grep "Send this email"
+}
+
+test_expect_success '--confirm=always' '
+	test_confirm --confirm=always --suppress-cc=all
+'
+
+test_expect_success '--confirm=auto' '
+	test_confirm --confirm=auto
+'
+
+test_expect_success '--confirm=cc' '
+	test_confirm --confirm=cc
+'
+
+test_expect_success '--confirm=compose' '
+	test_confirm --confirm=compose --compose
+'
+
+test_expect_success 'confirm by default (due to cc)' '
+	CONFIRM=$(git config --get sendemail.confirm) &&
+	git config --unset sendemail.confirm &&
+	test_confirm &&
+	git config sendemail.confirm $CONFIRM
+'
+
+test_expect_success 'confirm by default (due to --compose)' '
+	CONFIRM=$(git config --get sendemail.confirm) &&
+	git config --unset sendemail.confirm &&
+	test_confirm --suppress-cc=all --compose
+	ret="$?"
+	git config sendemail.confirm ${CONFIRM:-never}
+	test $ret = "0"
+'
+
 test_expect_success '--compose adds MIME for utf8 body' '
 	clean_fake_sendmail &&
 	(echo "#!$SHELL_PATH" &&
 	 echo "echo utf8 body: àéìöú >>\"\$1\""
 	) >fake-editor-utf8 &&
 	chmod +x fake-editor-utf8 &&
-	echo y | \
 	  GIT_EDITOR="\"$(pwd)/fake-editor-utf8\"" \
-	  GIT_SEND_EMAIL_NOTTY=1 \
 	  git send-email \
 	  --compose --subject foo \
 	  --from="Example <nobody@example.com>" \
@@ -405,9 +449,7 @@ test_expect_success '--compose respects user mime type' '
 	 echo " echo utf8 body: àéìöú) >\"\$1\""
 	) >fake-editor-utf8-mime &&
 	chmod +x fake-editor-utf8-mime &&
-	echo y | \
 	  GIT_EDITOR="\"$(pwd)/fake-editor-utf8-mime\"" \
-	  GIT_SEND_EMAIL_NOTTY=1 \
 	  git send-email \
 	  --compose --subject foo \
 	  --from="Example <nobody@example.com>" \
@@ -421,9 +463,7 @@ test_expect_success '--compose respects user mime type' '
 
 test_expect_success '--compose adds MIME for utf8 subject' '
 	clean_fake_sendmail &&
-	echo y | \
 	  GIT_EDITOR="\"$(pwd)/fake-editor\"" \
-	  GIT_SEND_EMAIL_NOTTY=1 \
 	  git send-email \
 	  --compose --subject utf8-sübjëct \
 	  --from="Example <nobody@example.com>" \
@@ -445,7 +485,7 @@ test_expect_success 'detects ambiguous reference/file conflict' '
 test_expect_success 'feed two files' '
 	rm -fr outdir &&
 	git format-patch -2 -o outdir &&
-	GIT_SEND_EMAIL_NOTTY=1 git send-email \
+	git send-email \
 	--dry-run \
 	--from="Example <nobody@example.com>" \
 	--to=nobody@example.com \
-- 
1.6.2.rc1.309.g5f417

Re: [PATCH] send-email: add --confirm option

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> send-email violates the principle of least surprise by automatically
> cc'ing additional recipients without confirming this with the user.
>
> This patch teaches send-email a --confirm option. It takes the
> following values:
>
>  --confirm=always   always confirm before sending
>  --confirm=never    never confirm before sending
>  --confirm=cc       confirm before sending when send-email has
>                     automatically added addresses from the patch to
>                     the Cc list
>  --confirm=compose  confirm before sending the first message when
>                     using --compose. (Needed to maintain backwards
>                     compatibility with existing behavior.)
>  --confirm=auto     'cc' + 'compose'
>
> The option defaults to 'compose' if any suppress Cc related options have
> been used, otherwise it defaults to 'auto'.

It is hard to judge if this is a good thing to do _at this point_.  Those
who complained may welcome it but that is hardly the point.  You need to
convince those who stayed silent (because they thought the default won't
change and were not paying attention) that it is a good change.

Especially the changes (not additions) to existing tests worries me; each
change to the existing one is a demonstration of breaking existing users.

You cannot retrofit safety by disallowing things once you used to allow,
without upsetting existing users.

> @@ -837,6 +834,25 @@ X-Mailer: git-send-email $gitversion
>  	unshift (@sendmail_parameters,
>  			'-f', $raw_from) if(defined $envelope_sender);
>  
> +	if ($needs_confirm && !$dry_run) {
> +		print "\n$header\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";
> +		}
> +		if (/^n/i) {
> +			return;
> +		} elsif (/^q/i) {
> +			cleanup_compose_files();
> +			exit(0);
> +		} elsif (/^a/i) {
> +			$confirm = 'never';
> +		}
> +	}

I think "[a]ll" would make it a bit less objectionable to people who hate
unsolicited confirmation dialogs.  Nice touch.

> @@ -1094,13 +1119,10 @@ foreach my $t (@files) {
>  	$message_id = undef;
>  }
>  
> -if ($compose) {
> -	cleanup_compose_files();
> -}
> +cleanup_compose_files();
>  
>  sub cleanup_compose_files() {
> -	unlink($compose_filename, $compose_filename . ".final");
> -
> +	unlink($compose_filename, $compose_filename . ".final") if $compose;
>  }

Does this hunk have anything to do with this topic, or is it just a churn
that does not change any behaviour?

> @@ -175,15 +180,13 @@ test_set_editor "$(pwd)/fake-editor"
>  
>  test_expect_success '--compose works' '
>  	clean_fake_sendmail &&
> -	echo y | \
> -		GIT_SEND_EMAIL_NOTTY=1 \
> -		git send-email \
> -		--compose --subject foo \
> -		--from="Example <nobody@example.com>" \
> -		--to=nobody@example.com \
> -		--smtp-server="$(pwd)/fake.sendmail" \
> -		$patches \
> -		2>errors
> +	git send-email \
> +	--compose --subject foo \
> +	--from="Example <nobody@example.com>" \
> +	--to=nobody@example.com \
> +	--smtp-server="$(pwd)/fake.sendmail" \
> +	$patches \
> +	2>errors
>  '

How would test this break without this hunk?  "echo" dies of sigpipe, or
something?

I am not objecting to this particular change; just asking why this change
is here.  "It does not break, but the command shouldn't ask for
confirmation, and giving 'y' into it is unnecessary" is a perfectly
acceptable answer, but if that is the case you probably would want to
verify that the command indeed does go ahead without asking.

> @@ -375,15 +378,56 @@ test_expect_success '--suppress-cc=cc' '
>  	test_suppression cc
>  '
>  
> +test_confirm () {
> +	echo y | \
> +		GIT_SEND_EMAIL_NOTTY=1 \
> +		git send-email \
> +		--from="Example <nobody@example.com>" \
> +		--to=nobody@example.com \
> +		--smtp-server="$(pwd)/fake.sendmail" \
> +		$@ \
> +		$patches | grep "Send this email"
> +}
> +
> +test_expect_success '--confirm=always' '
> +	test_confirm --confirm=always --suppress-cc=all
> +'
> + ...
> +test_expect_success 'confirm by default (due to --compose)' '
> +	CONFIRM=$(git config --get sendemail.confirm) &&
> +	git config --unset sendemail.confirm &&
> +	test_confirm --suppress-cc=all --compose
> +	ret="$?"
> +	git config sendemail.confirm ${CONFIRM:-never}
> +	test $ret = "0"
> +'

These all test you would get a prompt when you as the author expect the
code to give one.  Do you also need tests that verify you do not ask
needless confirmation when the code shouldn't?

>  test_expect_success '--compose adds MIME for utf8 body' '
>  	clean_fake_sendmail &&
>  	(echo "#!$SHELL_PATH" &&
>  	 echo "echo utf8 body: àéìöú >>\"\$1\""
>  	) >fake-editor-utf8 &&
>  	chmod +x fake-editor-utf8 &&
> -	echo y | \
>  	  GIT_EDITOR="\"$(pwd)/fake-editor-utf8\"" \
> -	  GIT_SEND_EMAIL_NOTTY=1 \
>  	  git send-email \
>  	  --compose --subject foo \
>  	  --from="Example <nobody@example.com>" \

If the change you made to git-send-email.perl is later broken and the
command (incorrectly) starts asking for confirmation with these command
line options, what does this test do?  Does it get stuck, forbidding the
test suite to be run unattended?

Re: [PATCH] send-email: add --confirm option

[Jay Soffian]

On Sun, Mar 1, 2009 at 4:03 AM, Junio C Hamano <gitster@pobox.com> wrote:
> It is hard to judge if this is a good thing to do _at this point_.  Those
> who complained may welcome it but that is hardly the point.  You need to
> convince those who stayed silent (because they thought the default won't
> change and were not paying attention) that it is a good change.

No, I won't. This is not my itch, I don't know who those users are,
and I think I provided any such users several easy ways out:

1) They can type "a" after the first prompt.
2) They can git config --global sendemail.confirm never
3) They won't even notice if they're using any suppresscc option, such
as yourself.

I know you're sensitive to existing users after getting burned by the
dashless-git issue, but I think you're over applying that lesson in
*this* case.

> Especially the changes (not additions) to existing tests worries me; each
> change to the existing one is a demonstration of breaking existing users.

There were two classes of changes:

1) Many of the tests Cc'd w/o warning. Those now prompt. Hence the
"git config sendemail.suppress never" at the top.

2) Since I added (1), the "echo y |" in the other tests was redundant.
I believe had a no-confirm option been available in the past, that the
test authors would not have used "echo y". And I thought it confusing
to leave the "echo y|" as someone might ask "if it's not supposed to
confirm, why is there an echo y here?"

> You cannot retrofit safety by disallowing things once you used to allow,
> without upsetting existing users.

And you hamper your ability to improve the product by not allowing
even minor backward incompatible changes.

But I anticipated this objection and I thought very much about how an
existing user might hypothetically be upset, while accommodating the
very real concern of a new user.

Aside, when I upgrade a product, I expect it to change in some ways.
If I really don't want change, I don't upgrade.

On balance, I think this change will be silently appreciated by many
more users than the few, if any, vocal objectors.

>> @@ -1094,13 +1119,10 @@ foreach my $t (@files) {
>>       $message_id = undef;
>>  }
>>
>> -if ($compose) {
>> -     cleanup_compose_files();
>> -}
>> +cleanup_compose_files();
>>
>>  sub cleanup_compose_files() {
>> -     unlink($compose_filename, $compose_filename . ".final");
>> -
>> +     unlink($compose_filename, $compose_filename . ".final") if $compose;
>>  }
>
> Does this hunk have anything to do with this topic, or is it just a churn
> that does not change any behaviour?

The former. You'll notice there is a "quit" option in the prompt (in
addition to yes/no/all). The quit option needs to call
cleanup_compose_files(), so it now has two callers.

Instead of both callers having to do this:

  if ($compose) {
       cleanup_compose_files();
  }

I just moved the if compose test into the function so each caller can
simply ask:

  cleanup_compose_files();


>> @@ -175,15 +180,13 @@ test_set_editor "$(pwd)/fake-editor"
>>
>>  test_expect_success '--compose works' '
>>       clean_fake_sendmail &&
>> -     echo y | \
>> -             GIT_SEND_EMAIL_NOTTY=1 \
>> -             git send-email \
>> -             --compose --subject foo \
>> -             --from="Example <nobody@example.com>" \
>> -             --to=nobody@example.com \
>> -             --smtp-server="$(pwd)/fake.sendmail" \
>> -             $patches \
>> -             2>errors
>> +     git send-email \
>> +     --compose --subject foo \
>> +     --from="Example <nobody@example.com>" \
>> +     --to=nobody@example.com \
>> +     --smtp-server="$(pwd)/fake.sendmail" \
>> +     $patches \
>> +     2>errors
>>  '
>
> How would test this break without this hunk?  "echo" dies of sigpipe, or
> something?
>
> I am not objecting to this particular change; just asking why this change
> is here.  "It does not break, but the command shouldn't ask for
> confirmation, and giving 'y' into it is unnecessary" is a perfectly
> acceptable answer, but if that is the case you probably would want to
> verify that the command indeed does go ahead without asking.

The hunk is there because the "echo y" is redundant with confirm=never
which is now set early.

> These all test you would get a prompt when you as the author expect the
> code to give one.  Do you also need tests that verify you do not ask
> needless confirmation when the code shouldn't?

Yes, I couldn't think how to test that last night for some reason, but
it is obvious to me now. I can send a followup.

>>  test_expect_success '--compose adds MIME for utf8 body' '
>>       clean_fake_sendmail &&
>>       (echo "#!$SHELL_PATH" &&
>>        echo "echo utf8 body: àéìöú >>\"\$1\""
>>       ) >fake-editor-utf8 &&
>>       chmod +x fake-editor-utf8 &&
>> -     echo y | \
>>         GIT_EDITOR="\"$(pwd)/fake-editor-utf8\"" \
>> -       GIT_SEND_EMAIL_NOTTY=1 \
>>         git send-email \
>>         --compose --subject foo \
>>         --from="Example <nobody@example.com>" \
>
> If the change you made to git-send-email.perl is later broken and the
> command (incorrectly) starts asking for confirmation with these command
> line options, what does this test do?  Does it get stuck, forbidding the
> test suite to be run unattended?

Many of the tests (because they automatically cc) will hang waiting on
input if git-send-email.perl is later broken in such a way as to start
ignoring --compose.

I could not think of a good way to avoid this. We could possibly "echo
y|" redundantly to every test, but even that does not guarantee that
one of the other prompts cannot cause a hang. So this potential to
hang, I think, already exists.

The best I could think of was something like (sleep 60 && kill $$) &;
sleep_pid=$!; ... kill $sleep_pid; wrapping each test, or even the
entire script. Suggestions here appreciated.

But, I think this can largely be addressed by testing that the change
does *not* prompt when it shouldn't, and moving that test before any
of the existing tests which could prompt.

j.

[PATCH v2] send-email: add --confirm option

[Jay Soffian]

send-email violates the principle of least surprise by automatically
cc'ing additional recipients without confirming this with the user.

This patch teaches send-email a --confirm option. It takes the
following values:

 --confirm=always   always confirm before sending
 --confirm=never    never confirm before sending
 --confirm=cc       confirm before sending when send-email has
                    automatically added addresses from the patch to
                    the Cc list
 --confirm=compose  confirm before sending the first message when
                    using --compose. (Needed to maintain backwards
                    compatibility with existing behavior.)
 --confirm=auto     'cc' + 'compose'

The option defaults to 'compose' if any suppress Cc related options have
been used, otherwise it defaults to 'auto'.

Unfortunately, it is impossible to introduce this patch such that it
helps new users without potentially upsetting some existing users. We
attempt to mitigate the latter by:

 * Allowing the user to "git config sendemail.config never"
 * Allowing the user to say "all" after the first prompt to not be
   prompted on remaining emails during the same invocation.
 * Telling the user about the "sendemail.confirm" setting whenever they
   use "all"
 * Only prompting if no --suppress related options have been passed, as
   using such an option is likely to indicate an experienced send-email
   user.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
Changes from v1:

 - Added no-confirm tests, and put them early to prevent the rest of
   the tests from potentially hanging. Note if one of these tests
   fails then we exit t9001-send-email.sh immediately.

 - Added verification of the --confirm setting. (I had done this
   previously but somehow failed to stage it.)

 - Added additional language to the commit messsage in an attempt to
   provide justification for the change in default behavior.

 - When the user specifies "all" in response to a confirm prompt, we
   hint them about how to use "sendemail.confirm" config setting.

 Documentation/git-send-email.txt |   16 ++++++
 git-send-email.perl              |   72 +++++++++++++++++--------
 t/t9001-send-email.sh            |  108 ++++++++++++++++++++++++++++++++------
 3 files changed, 157 insertions(+), 39 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 164d149..bcf7ff1 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -199,6 +199,22 @@ specified, as well as 'body' if --no-signed-off-cc is specified.
 Administering
 ~~~~~~~~~~~~~
 
+--confirm::
+	Confirm just before sending:
++
+--
+- 'always' will always confirm before sending
+- 'never' will never confirm before sending
+- 'cc' will confirm before sending when send-email has automatically
+  added addresses from the patch to the Cc list
+- 'compose' will confirm before sending the first message when using --compose.
+- 'auto' is equivalent to 'cc' + 'compose'
+--
++
+Default is the value of 'sendemail.confirm' configuration value; if that
+is unspecified, default to 'auto' unless any of the suppress options
+have been specified, in which case default to 'compose'.
+
 --dry-run::
 	Do everything except actually send the emails.
 
diff --git a/git-send-email.perl b/git-send-email.perl
index adf7ecb..439b70b 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -75,6 +75,8 @@ git send-email [options] <file | directory | rev-list options >
     --[no-]thread                  * Use In-Reply-To: field. Default on.
 
   Administering:
+    --confirm               <str>  * Confirm recipients before sending;
+                                     auto, cc, compose, always, or never.
     --quiet                        * Output one line of info per email.
     --dry-run                      * Don't actually send the emails.
     --[no-]validate                * Perform patch sanity checks. Default on.
@@ -181,7 +183,7 @@ sub do_edit {
 my ($thread, $chain_reply_to, $suppress_from, $signed_off_by_cc, $cc_cmd);
 my ($smtp_server, $smtp_server_port, $smtp_authuser, $smtp_encryption);
 my ($identity, $aliasfiletype, @alias_files, @smtp_host_parts);
-my ($validate);
+my ($validate, $confirm);
 my (@suppress_cc);
 
 my %config_bool_settings = (
@@ -207,6 +209,7 @@ my %config_settings = (
     "suppresscc" => \@suppress_cc,
     "envelopesender" => \$envelope_sender,
     "multiedit" => \$multiedit,
+    "confirm"   => \$confirm,
 );
 
 # Handle Uncouth Termination
@@ -258,6 +261,7 @@ my $rc = GetOptions("sender|from=s" => \$sender,
 		    "suppress-from!" => \$suppress_from,
 		    "suppress-cc=s" => \@suppress_cc,
 		    "signed-off-cc|signed-off-by-cc!" => \$signed_off_by_cc,
+		    "confirm=s" => \$confirm,
 		    "dry-run" => \$dry_run,
 		    "envelope-sender=s" => \$envelope_sender,
 		    "thread!" => \$thread,
@@ -346,6 +350,13 @@ if ($suppress_cc{'body'}) {
 	delete $suppress_cc{'body'};
 }
 
+# Set confirm
+if (!defined $confirm) {
+	$confirm = scalar %suppress_cc ? 'compose' : 'auto';
+};
+die "Unknown --confirm setting: '$confirm'\n"
+	unless $confirm =~ /^(?:auto|cc|compose|always|never)/;
+
 # Debugging, print out the suppressions.
 if (0) {
 	print "suppressions:\n";
@@ -663,25 +674,13 @@ if (!defined $smtp_server) {
 	$smtp_server ||= 'localhost'; # could be 127.0.0.1, too... *shrug*
 }
 
-if ($compose) {
-	while (1) {
-		$_ = $term->readline("Send this email? (y|n) ");
-		last if defined $_;
-		print "\n";
-	}
-
-	if (uc substr($_,0,1) ne 'Y') {
-		cleanup_compose_files();
-		exit(0);
-	}
-
-	if ($compose > 0) {
-		@files = ($compose_filename . ".final", @files);
-	}
+if ($compose && $compose > 0) {
+	@files = ($compose_filename . ".final", @files);
 }
 
 # Variables we set as part of the loop over files
-our ($message_id, %mail, $subject, $reply_to, $references, $message);
+our ($message_id, %mail, $subject, $reply_to, $references, $message,
+	$needs_confirm, $message_num);
 
 sub extract_valid_address {
 	my $address = shift;
@@ -837,6 +836,27 @@ X-Mailer: git-send-email $gitversion
 	unshift (@sendmail_parameters,
 			'-f', $raw_from) if(defined $envelope_sender);
 
+	if ($needs_confirm && !$dry_run) {
+		print "\n$header\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";
+		}
+		if (/^n/i) {
+			return;
+		} elsif (/^q/i) {
+			cleanup_compose_files();
+			exit(0);
+		} elsif (/^a/i) {
+			$confirm = 'never';
+			print "You may disable all future prompting via sendemail.confirm;\n";
+			print "Run 'git send-email --help' for details.\n";
+		}
+	}
+
 	if ($dry_run) {
 		# We don't want to send the email.
 	} elsif ($smtp_server =~ m#^/#) {
@@ -935,6 +955,7 @@ X-Mailer: git-send-email $gitversion
 $reply_to = $initial_reply_to;
 $references = $initial_reply_to || '';
 $subject = $initial_subject;
+$message_num = 0;
 
 foreach my $t (@files) {
 	open(F,"<",$t) or die "can't open file $t";
@@ -943,11 +964,12 @@ foreach my $t (@files) {
 	my $author_encoding;
 	my $has_content_type;
 	my $body_encoding;
-	@cc = @initial_cc;
+	@cc = ();
 	@xh = ();
 	my $input_format = undef;
 	my @header = ();
 	$message = "";
+	$message_num++;
 	# First unfold multiline header fields
 	while(<F>) {
 		last if /^\s*$/;
@@ -1080,6 +1102,13 @@ foreach my $t (@files) {
 		}
 	}
 
+	$needs_confirm = (
+		$confirm eq "always" or
+		($confirm =~ /^(?:auto|cc)$/ && @cc) or
+		($confirm =~ /^(?:auto|compose)$/ && $compose && $message_num == 1));
+
+	@cc = (@initial_cc, @cc);
+
 	send_message();
 
 	# set up for the next message
@@ -1094,13 +1123,10 @@ foreach my $t (@files) {
 	$message_id = undef;
 }
 
-if ($compose) {
-	cleanup_compose_files();
-}
+cleanup_compose_files();
 
 sub cleanup_compose_files() {
-	unlink($compose_filename, $compose_filename . ".final");
-
+	unlink($compose_filename, $compose_filename . ".final") if $compose;
 }
 
 $smtp->quit if $smtp;
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 4df4f96..08d5b91 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -35,6 +35,47 @@ test_expect_success 'Extract patches' '
     patches=`git format-patch -s --cc="One <one@example.com>" --cc=two@example.com -n HEAD^1`
 '
 
+# Test no confirm early to ensure remaining tests will not hang
+test_no_confirm () {
+	rm -f no_confirm_okay
+	echo n | \
+		GIT_SEND_EMAIL_NOTTY=1 \
+		git send-email \
+		--from="Example <from@example.com>" \
+		--to=nobody@example.com \
+		--smtp-server="$(pwd)/fake.sendmail" \
+		$@ \
+		$patches > stdout &&
+		test_must_fail grep "Send this email" stdout &&
+		> no_confirm_okay
+}
+
+# Exit immediately to prevent hang if a no-confirm test fails
+check_no_confirm () {
+	test -f no_confirm_okay || {
+		say 'No confirm test failed; skipping remaining tests to prevent hanging'
+		test_done
+	}
+}
+
+test_expect_success 'No confirm with --suppress-cc' '
+	test_no_confirm --suppress-cc=sob
+'
+check_no_confirm
+
+test_expect_success 'No confirm with --confirm=never' '
+	test_no_confirm --confirm=never
+'
+check_no_confirm
+
+# leave sendemail.confirm set to never after this so that none of the
+# remaining tests prompt unintentionally.
+test_expect_success 'No confirm with sendemail.confirm=never' '
+	git config sendemail.confirm never &&
+	test_no_confirm --compose --subject=foo
+'
+check_no_confirm
+
 test_expect_success 'Send patches' '
      git send-email --suppress-cc=sob --from="Example <nobody@example.com>" --to=nobody@example.com --smtp-server="$(pwd)/fake.sendmail" $patches 2>errors
 '
@@ -175,15 +216,13 @@ test_set_editor "$(pwd)/fake-editor"
 
 test_expect_success '--compose works' '
 	clean_fake_sendmail &&
-	echo y | \
-		GIT_SEND_EMAIL_NOTTY=1 \
-		git send-email \
-		--compose --subject foo \
-		--from="Example <nobody@example.com>" \
-		--to=nobody@example.com \
-		--smtp-server="$(pwd)/fake.sendmail" \
-		$patches \
-		2>errors
+	git send-email \
+	--compose --subject foo \
+	--from="Example <nobody@example.com>" \
+	--to=nobody@example.com \
+	--smtp-server="$(pwd)/fake.sendmail" \
+	$patches \
+	2>errors
 '
 
 test_expect_success 'first message is compose text' '
@@ -375,15 +414,56 @@ test_expect_success '--suppress-cc=cc' '
 	test_suppression cc
 '
 
+test_confirm () {
+	echo y | \
+		GIT_SEND_EMAIL_NOTTY=1 \
+		git send-email \
+		--from="Example <nobody@example.com>" \
+		--to=nobody@example.com \
+		--smtp-server="$(pwd)/fake.sendmail" \
+		$@ \
+		$patches | grep "Send this email"
+}
+
+test_expect_success '--confirm=always' '
+	test_confirm --confirm=always --suppress-cc=all
+'
+
+test_expect_success '--confirm=auto' '
+	test_confirm --confirm=auto
+'
+
+test_expect_success '--confirm=cc' '
+	test_confirm --confirm=cc
+'
+
+test_expect_success '--confirm=compose' '
+	test_confirm --confirm=compose --compose
+'
+
+test_expect_success 'confirm by default (due to cc)' '
+	CONFIRM=$(git config --get sendemail.confirm) &&
+	git config --unset sendemail.confirm &&
+	test_confirm &&
+	git config sendemail.confirm $CONFIRM
+'
+
+test_expect_success 'confirm by default (due to --compose)' '
+	CONFIRM=$(git config --get sendemail.confirm) &&
+	git config --unset sendemail.confirm &&
+	test_confirm --suppress-cc=all --compose
+	ret="$?"
+	git config sendemail.confirm ${CONFIRM:-never}
+	test $ret = "0"
+'
+
 test_expect_success '--compose adds MIME for utf8 body' '
 	clean_fake_sendmail &&
 	(echo "#!$SHELL_PATH" &&
 	 echo "echo utf8 body: àéìöú >>\"\$1\""
 	) >fake-editor-utf8 &&
 	chmod +x fake-editor-utf8 &&
-	echo y | \
 	  GIT_EDITOR="\"$(pwd)/fake-editor-utf8\"" \
-	  GIT_SEND_EMAIL_NOTTY=1 \
 	  git send-email \
 	  --compose --subject foo \
 	  --from="Example <nobody@example.com>" \
@@ -405,9 +485,7 @@ test_expect_success '--compose respects user mime type' '
 	 echo " echo utf8 body: àéìöú) >\"\$1\""
 	) >fake-editor-utf8-mime &&
 	chmod +x fake-editor-utf8-mime &&
-	echo y | \
 	  GIT_EDITOR="\"$(pwd)/fake-editor-utf8-mime\"" \
-	  GIT_SEND_EMAIL_NOTTY=1 \
 	  git send-email \
 	  --compose --subject foo \
 	  --from="Example <nobody@example.com>" \
@@ -421,9 +499,7 @@ test_expect_success '--compose respects user mime type' '
 
 test_expect_success '--compose adds MIME for utf8 subject' '
 	clean_fake_sendmail &&
-	echo y | \
 	  GIT_EDITOR="\"$(pwd)/fake-editor\"" \
-	  GIT_SEND_EMAIL_NOTTY=1 \
 	  git send-email \
 	  --compose --subject utf8-sübjëct \
 	  --from="Example <nobody@example.com>" \
@@ -445,7 +521,7 @@ test_expect_success 'detects ambiguous reference/file conflict' '
 test_expect_success 'feed two files' '
 	rm -fr outdir &&
 	git format-patch -2 -o outdir &&
-	GIT_SEND_EMAIL_NOTTY=1 git send-email \
+	git send-email \
 	--dry-run \
 	--from="Example <nobody@example.com>" \
 	--to=nobody@example.com \
-- 
1.6.2.rc1.309.g5f417

Re: [PATCH v2] send-email: add --confirm option

[Paul Gortmaker]

On Sun, Mar 1, 2009 at 11:17 AM, Jay Soffian <jaysoffian@gmail.com> wrote:
> send-email violates the principle of least surprise by automatically
> cc'ing additional recipients without confirming this with the user.
>
> This patch teaches send-email a --confirm option. It takes the
> following values:
>
>  --confirm=always   always confirm before sending
>  --confirm=never    never confirm before sending
>  --confirm=cc       confirm before sending when send-email has
>                    automatically added addresses from the patch to
>                    the Cc list
>  --confirm=compose  confirm before sending the first message when
>                    using --compose. (Needed to maintain backwards
>                    compatibility with existing behavior.)
>  --confirm=auto     'cc' + 'compose'
>
> The option defaults to 'compose' if any suppress Cc related options have
> been used, otherwise it defaults to 'auto'.
>
> Unfortunately, it is impossible to introduce this patch such that it
> helps new users without potentially upsetting some existing users. We
> attempt to mitigate the latter by:
>
>  * Allowing the user to "git config sendemail.config never"

I think it should be sendemail.confirm in the above.   Thanks for
taking this seriously -- I think lots of new git users (who probably
will never make it to this list) will benefit from it without ever
even knowing.

Paul.

>  * Allowing the user to say "all" after the first prompt to not be
>   prompted on remaining emails during the same invocation.
>  * Telling the user about the "sendemail.confirm" setting whenever they
>   use "all"
>  * Only prompting if no --suppress related options have been passed, as
>   using such an option is likely to indicate an experienced send-email
>   user.
>
> Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
> ---
> Changes from v1:
>
>  - Added no-confirm tests, and put them early to prevent the rest of
>   the tests from potentially hanging. Note if one of these tests
>   fails then we exit t9001-send-email.sh immediately.
>
>  - Added verification of the --confirm setting. (I had done this
>   previously but somehow failed to stage it.)
>
>  - Added additional language to the commit messsage in an attempt to
>   provide justification for the change in default behavior.
>
>  - When the user specifies "all" in response to a confirm prompt, we
>   hint them about how to use "sendemail.confirm" config setting.
>
>  Documentation/git-send-email.txt |   16 ++++++
>  git-send-email.perl              |   72 +++++++++++++++++--------
>  t/t9001-send-email.sh            |  108 ++++++++++++++++++++++++++++++++------
>  3 files changed, 157 insertions(+), 39 deletions(-)
>
> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
> index 164d149..bcf7ff1 100644
> --- a/Documentation/git-send-email.txt
> +++ b/Documentation/git-send-email.txt
> @@ -199,6 +199,22 @@ specified, as well as 'body' if --no-signed-off-cc is specified.
>  Administering
>  ~~~~~~~~~~~~~
>
> +--confirm::
> +       Confirm just before sending:
> ++
> +--
> +- 'always' will always confirm before sending
> +- 'never' will never confirm before sending
> +- 'cc' will confirm before sending when send-email has automatically
> +  added addresses from the patch to the Cc list
> +- 'compose' will confirm before sending the first message when using --compose.
> +- 'auto' is equivalent to 'cc' + 'compose'
> +--
> ++
> +Default is the value of 'sendemail.confirm' configuration value; if that
> +is unspecified, default to 'auto' unless any of the suppress options
> +have been specified, in which case default to 'compose'.
> +
>  --dry-run::
>        Do everything except actually send the emails.
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index adf7ecb..439b70b 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -75,6 +75,8 @@ git send-email [options] <file | directory | rev-list options >
>     --[no-]thread                  * Use In-Reply-To: field. Default on.
>
>   Administering:
> +    --confirm               <str>  * Confirm recipients before sending;
> +                                     auto, cc, compose, always, or never.
>     --quiet                        * Output one line of info per email.
>     --dry-run                      * Don't actually send the emails.
>     --[no-]validate                * Perform patch sanity checks. Default on.
> @@ -181,7 +183,7 @@ sub do_edit {
>  my ($thread, $chain_reply_to, $suppress_from, $signed_off_by_cc, $cc_cmd);
>  my ($smtp_server, $smtp_server_port, $smtp_authuser, $smtp_encryption);
>  my ($identity, $aliasfiletype, @alias_files, @smtp_host_parts);
> -my ($validate);
> +my ($validate, $confirm);
>  my (@suppress_cc);
>
>  my %config_bool_settings = (
> @@ -207,6 +209,7 @@ my %config_settings = (
>     "suppresscc" => \@suppress_cc,
>     "envelopesender" => \$envelope_sender,
>     "multiedit" => \$multiedit,
> +    "confirm"   => \$confirm,
>  );
>
>  # Handle Uncouth Termination
> @@ -258,6 +261,7 @@ my $rc = GetOptions("sender|from=s" => \$sender,
>                    "suppress-from!" => \$suppress_from,
>                    "suppress-cc=s" => \@suppress_cc,
>                    "signed-off-cc|signed-off-by-cc!" => \$signed_off_by_cc,
> +                   "confirm=s" => \$confirm,
>                    "dry-run" => \$dry_run,
>                    "envelope-sender=s" => \$envelope_sender,
>                    "thread!" => \$thread,
> @@ -346,6 +350,13 @@ if ($suppress_cc{'body'}) {
>        delete $suppress_cc{'body'};
>  }
>
> +# Set confirm
> +if (!defined $confirm) {
> +       $confirm = scalar %suppress_cc ? 'compose' : 'auto';
> +};
> +die "Unknown --confirm setting: '$confirm'\n"
> +       unless $confirm =~ /^(?:auto|cc|compose|always|never)/;
> +
>  # Debugging, print out the suppressions.
>  if (0) {
>        print "suppressions:\n";
> @@ -663,25 +674,13 @@ if (!defined $smtp_server) {
>        $smtp_server ||= 'localhost'; # could be 127.0.0.1, too... *shrug*
>  }
>
> -if ($compose) {
> -       while (1) {
> -               $_ = $term->readline("Send this email? (y|n) ");
> -               last if defined $_;
> -               print "\n";
> -       }
> -
> -       if (uc substr($_,0,1) ne 'Y') {
> -               cleanup_compose_files();
> -               exit(0);
> -       }
> -
> -       if ($compose > 0) {
> -               @files = ($compose_filename . ".final", @files);
> -       }
> +if ($compose && $compose > 0) {
> +       @files = ($compose_filename . ".final", @files);
>  }
>
>  # Variables we set as part of the loop over files
> -our ($message_id, %mail, $subject, $reply_to, $references, $message);
> +our ($message_id, %mail, $subject, $reply_to, $references, $message,
> +       $needs_confirm, $message_num);
>
>  sub extract_valid_address {
>        my $address = shift;
> @@ -837,6 +836,27 @@ X-Mailer: git-send-email $gitversion
>        unshift (@sendmail_parameters,
>                        '-f', $raw_from) if(defined $envelope_sender);
>
> +       if ($needs_confirm && !$dry_run) {
> +               print "\n$header\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";
> +               }
> +               if (/^n/i) {
> +                       return;
> +               } elsif (/^q/i) {
> +                       cleanup_compose_files();
> +                       exit(0);
> +               } elsif (/^a/i) {
> +                       $confirm = 'never';
> +                       print "You may disable all future prompting via sendemail.confirm;\n";
> +                       print "Run 'git send-email --help' for details.\n";
> +               }
> +       }
> +
>        if ($dry_run) {
>                # We don't want to send the email.
>        } elsif ($smtp_server =~ m#^/#) {
> @@ -935,6 +955,7 @@ X-Mailer: git-send-email $gitversion
>  $reply_to = $initial_reply_to;
>  $references = $initial_reply_to || '';
>  $subject = $initial_subject;
> +$message_num = 0;
>
>  foreach my $t (@files) {
>        open(F,"<",$t) or die "can't open file $t";
> @@ -943,11 +964,12 @@ foreach my $t (@files) {
>        my $author_encoding;
>        my $has_content_type;
>        my $body_encoding;
> -       @cc = @initial_cc;
> +       @cc = ();
>        @xh = ();
>        my $input_format = undef;
>        my @header = ();
>        $message = "";
> +       $message_num++;
>        # First unfold multiline header fields
>        while(<F>) {
>                last if /^\s*$/;
> @@ -1080,6 +1102,13 @@ foreach my $t (@files) {
>                }
>        }
>
> +       $needs_confirm = (
> +               $confirm eq "always" or
> +               ($confirm =~ /^(?:auto|cc)$/ && @cc) or
> +               ($confirm =~ /^(?:auto|compose)$/ && $compose && $message_num == 1));
> +
> +       @cc = (@initial_cc, @cc);
> +
>        send_message();
>
>        # set up for the next message
> @@ -1094,13 +1123,10 @@ foreach my $t (@files) {
>        $message_id = undef;
>  }
>
> -if ($compose) {
> -       cleanup_compose_files();
> -}
> +cleanup_compose_files();
>
>  sub cleanup_compose_files() {
> -       unlink($compose_filename, $compose_filename . ".final");
> -
> +       unlink($compose_filename, $compose_filename . ".final") if $compose;
>  }
>
>  $smtp->quit if $smtp;
> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
> index 4df4f96..08d5b91 100755
> --- a/t/t9001-send-email.sh
> +++ b/t/t9001-send-email.sh
> @@ -35,6 +35,47 @@ test_expect_success 'Extract patches' '
>     patches=`git format-patch -s --cc="One <one@example.com>" --cc=two@example.com -n HEAD^1`
>  '
>
> +# Test no confirm early to ensure remaining tests will not hang
> +test_no_confirm () {
> +       rm -f no_confirm_okay
> +       echo n | \
> +               GIT_SEND_EMAIL_NOTTY=1 \
> +               git send-email \
> +               --from="Example <from@example.com>" \
> +               --to=nobody@example.com \
> +               --smtp-server="$(pwd)/fake.sendmail" \
> +               $@ \
> +               $patches > stdout &&
> +               test_must_fail grep "Send this email" stdout &&
> +               > no_confirm_okay
> +}
> +
> +# Exit immediately to prevent hang if a no-confirm test fails
> +check_no_confirm () {
> +       test -f no_confirm_okay || {
> +               say 'No confirm test failed; skipping remaining tests to prevent hanging'
> +               test_done
> +       }
> +}
> +
> +test_expect_success 'No confirm with --suppress-cc' '
> +       test_no_confirm --suppress-cc=sob
> +'
> +check_no_confirm
> +
> +test_expect_success 'No confirm with --confirm=never' '
> +       test_no_confirm --confirm=never
> +'
> +check_no_confirm
> +
> +# leave sendemail.confirm set to never after this so that none of the
> +# remaining tests prompt unintentionally.
> +test_expect_success 'No confirm with sendemail.confirm=never' '
> +       git config sendemail.confirm never &&
> +       test_no_confirm --compose --subject=foo
> +'
> +check_no_confirm
> +
>  test_expect_success 'Send patches' '
>      git send-email --suppress-cc=sob --from="Example <nobody@example.com>" --to=nobody@example.com --smtp-server="$(pwd)/fake.sendmail" $patches 2>errors
>  '
> @@ -175,15 +216,13 @@ test_set_editor "$(pwd)/fake-editor"
>
>  test_expect_success '--compose works' '
>        clean_fake_sendmail &&
> -       echo y | \
> -               GIT_SEND_EMAIL_NOTTY=1 \
> -               git send-email \
> -               --compose --subject foo \
> -               --from="Example <nobody@example.com>" \
> -               --to=nobody@example.com \
> -               --smtp-server="$(pwd)/fake.sendmail" \
> -               $patches \
> -               2>errors
> +       git send-email \
> +       --compose --subject foo \
> +       --from="Example <nobody@example.com>" \
> +       --to=nobody@example.com \
> +       --smtp-server="$(pwd)/fake.sendmail" \
> +       $patches \
> +       2>errors
>  '
>
>  test_expect_success 'first message is compose text' '
> @@ -375,15 +414,56 @@ test_expect_success '--suppress-cc=cc' '
>        test_suppression cc
>  '
>
> +test_confirm () {
> +       echo y | \
> +               GIT_SEND_EMAIL_NOTTY=1 \
> +               git send-email \
> +               --from="Example <nobody@example.com>" \
> +               --to=nobody@example.com \
> +               --smtp-server="$(pwd)/fake.sendmail" \
> +               $@ \
> +               $patches | grep "Send this email"
> +}
> +
> +test_expect_success '--confirm=always' '
> +       test_confirm --confirm=always --suppress-cc=all
> +'
> +
> +test_expect_success '--confirm=auto' '
> +       test_confirm --confirm=auto
> +'
> +
> +test_expect_success '--confirm=cc' '
> +       test_confirm --confirm=cc
> +'
> +
> +test_expect_success '--confirm=compose' '
> +       test_confirm --confirm=compose --compose
> +'
> +
> +test_expect_success 'confirm by default (due to cc)' '
> +       CONFIRM=$(git config --get sendemail.confirm) &&
> +       git config --unset sendemail.confirm &&
> +       test_confirm &&
> +       git config sendemail.confirm $CONFIRM
> +'
> +
> +test_expect_success 'confirm by default (due to --compose)' '
> +       CONFIRM=$(git config --get sendemail.confirm) &&
> +       git config --unset sendemail.confirm &&
> +       test_confirm --suppress-cc=all --compose
> +       ret="$?"
> +       git config sendemail.confirm ${CONFIRM:-never}
> +       test $ret = "0"
> +'
> +
>  test_expect_success '--compose adds MIME for utf8 body' '
>        clean_fake_sendmail &&
>        (echo "#!$SHELL_PATH" &&
>         echo "echo utf8 body: àéìöú >>\"\$1\""
>        ) >fake-editor-utf8 &&
>        chmod +x fake-editor-utf8 &&
> -       echo y | \
>          GIT_EDITOR="\"$(pwd)/fake-editor-utf8\"" \
> -         GIT_SEND_EMAIL_NOTTY=1 \
>          git send-email \
>          --compose --subject foo \
>          --from="Example <nobody@example.com>" \
> @@ -405,9 +485,7 @@ test_expect_success '--compose respects user mime type' '
>         echo " echo utf8 body: àéìöú) >\"\$1\""
>        ) >fake-editor-utf8-mime &&
>        chmod +x fake-editor-utf8-mime &&
> -       echo y | \
>          GIT_EDITOR="\"$(pwd)/fake-editor-utf8-mime\"" \
> -         GIT_SEND_EMAIL_NOTTY=1 \
>          git send-email \
>          --compose --subject foo \
>          --from="Example <nobody@example.com>" \
> @@ -421,9 +499,7 @@ test_expect_success '--compose respects user mime type' '
>
>  test_expect_success '--compose adds MIME for utf8 subject' '
>        clean_fake_sendmail &&
> -       echo y | \
>          GIT_EDITOR="\"$(pwd)/fake-editor\"" \
> -         GIT_SEND_EMAIL_NOTTY=1 \
>          git send-email \
>          --compose --subject utf8-sübjëct \
>          --from="Example <nobody@example.com>" \
> @@ -445,7 +521,7 @@ test_expect_success 'detects ambiguous reference/file conflict' '
>  test_expect_success 'feed two files' '
>        rm -fr outdir &&
>        git format-patch -2 -o outdir &&
> -       GIT_SEND_EMAIL_NOTTY=1 git send-email \
> +       git send-email \
>        --dry-run \
>        --from="Example <nobody@example.com>" \
>        --to=nobody@example.com \
> --
> 1.6.2.rc1.309.g5f417
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH v2] send-email: add --confirm option

[Jay Soffian]

On Sun, Mar 1, 2009 at 12:09 PM, Paul Gortmaker
<paul.gortmaker@windriver.com> wrote:
>
> On Sun, Mar 1, 2009 at 11:17 AM, Jay Soffian <jaysoffian@gmail.com> wrote:
>>  * Allowing the user to "git config sendemail.config never"
>
> I think it should be sendemail.confirm in the above.

Yep, thanks. Junio, please amend my commit message if v2 is acceptable as is.

> Thanks for
> taking this seriously -- I think lots of new git users (who probably
> will never make it to this list) will benefit from it without ever
> even knowing.

Well it's all for naught unless Junio can be convinced that it's an
acceptable trade-off. On this point I want to elaborate a little more,
so pardon me while I step up on the soap box.

Once upon a time, all git commands were git-something and they were
installed in PATH. A smattering of users complained about this.
Eventually git learned "git something" in addition to "git-something".
Then some folks decided why not just get rid of "git-something"
entirely. And so it was done. And many other users who were happy with
the way it was complained.

And rightly so. The change was made w/no escape hatch for those users.
And to plumbing no less. The users who wanted "git-something" out of
PATH (which wasn't really hurting anything) got what they wanted, but
nothing was done to accommodate the existing users. Eventually a
compromise was reached. The git-something commands moved out of PATH,
but were still installed, and existing users could relatively easily get
to them by adding "git --exec-path" to their PATH.

(Please correct me if my summary is wrong, but that's how I recall it.)

If the compromise is how it was done in the first place, perhaps Junio
would not be as hyper-sensitive to any change which affects existing
users expectations.

But this patch is not like the git-something situation. This patch
benefits new (all?) users, while bending over backwards to accommodate
existing users. And it is a porcelain change.

I sympathize with Junio's aversion to accepting patches which affect
existing users expectations. But I also think there are times when the
greater good is served by such patches, and it would be nice to have
some guidelines for how and when such patches can be made. For example:

- No non-backwards compatible changes to plumbing.
- Non-backwards compatible changes to porcelain IFF:
  - The change provides a notable (non-trivial) benefit to new users.
    Some litmus tests for such a change might be:
    - "in hindsight, this is how it clearly should've been done."
    - "this is really confusing for new users; existing users users have
      forgotten how confusing it was when they first encountered it."
    - "the default behavior is potentially dangerous/embarrassing."
  - Existing users can easily get the prior behavior. i.e. via a config
    setting
  - The change, where possible, maintains previous expectations if it
    appears the command is being used by an experienced user.

(In fairness, there appears to be a framework for non-backwards
compatible changes; you introduce a warning about the change in the next
minor release that the behavior of X will change and inform the user how
they can keep the existing behavior of X; wait one patch release, then
make the actual change. But my reading of Junio's message is that he
doesn't think _this_ patch is even worthy of that step until I can
demonstrate that there is not a silent-majority who really likes the
existing behavior of send-email. But as I said, this is not my itch, and
while I'm happy to offer up this patch, that's as far as I go.)

Stepping off soapbox.

j.

Re: [PATCH v2] send-email: add --confirm option

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> send-email violates the principle of least surprise by automatically
> cc'ing additional recipients without confirming this with the user.
>
> This patch teaches send-email a --confirm option. It takes the
> following values:
>
>  --confirm=always   always confirm before sending
>  --confirm=never    never confirm before sending
>  --confirm=cc       confirm before sending when send-email has
>                     automatically added addresses from the patch to
>                     the Cc list
>  --confirm=compose  confirm before sending the first message when
>                     using --compose. (Needed to maintain backwards
>                     compatibility with existing behavior.)
>  --confirm=auto     'cc' + 'compose'
>
> The option defaults to 'compose' if any suppress Cc related options have
> been used, otherwise it defaults to 'auto'.
>
> Unfortunately, it is impossible to introduce this patch such that it
> helps new users without potentially upsetting some existing users. We
> attempt to mitigate the latter by:
>
>  * Allowing the user to "git config sendemail.config never"
>  * Allowing the user to say "all" after the first prompt to not be
>    prompted on remaining emails during the same invocation.
>  * Telling the user about the "sendemail.confirm" setting whenever they
>    use "all"
>  * Only prompting if no --suppress related options have been passed, as
>    using such an option is likely to indicate an experienced send-email
>    user.
>
> Signed-off-by: Jay Soffian <jaysoffian@gmail.com>

Other than that the "sendemail.config never" is probably a typo, I think
this is the best you (or anybody) could do at this moment, unless we take
the route to introduce an "improved and different command", which I
actually am slightly in favor.

In any case, with the lesson I learned from the post v1.6.0 fiasco, it
might make sense to make the command louder when it needs to look at the
setting of $confirm option and when the user does not have anything in the
config nor command line.

What I mean is this.

In this codepath,

> +	$needs_confirm = (
> +		$confirm eq "always" or
> +		($confirm =~ /^(?:auto|cc)$/ && @cc) or
> +		($confirm =~ /^(?:auto|compose)$/ && $compose && $message_num == 1));
> +

if @cc is not empty, or $compose is true, you _need to know_ what the user
wants to happen, and you need to look at $confirm to decide (otherwise the
value of $confirm does not matter).  In such a case, if the repository is
unconfigured with a sendemail.confirm configuration and the there was no
explicit --confirm from the command line, you do not know what the user
wants.  Instead of silently assuming 'auto' to confirm and potentially
annoying the users with this new extra step, the command could also say
"by the way, if you do not want to see this message, you can squelch it by
'git config sendemail.confirm never'" when it defaults to 'auto'.

The command could also refuse to work in such a case and instead explain
what the user's choices are, and instruct "say 'git config
sendemail.confirm auto' if you are not sure" or something like that, but I
think for this particular new option it is a bit overkill and I wouldn't
suggest going that far.

Re: [PATCH v2] send-email: add --confirm option

[Nanako Shiraishi]

Quoting Jay Soffian <jaysoffian@gmail.com>:

> (Please correct me if my summary is wrong, but that's how I recall it.)

I think your summary has some things right.

 - Many people complained git-foo being on their paths in the past;
 - Version 1.6.0 removed git-foo from users' path; and
 - Many people didn't like the change after the fact.

However, you are completely wrong about the escape hatch.

If your judgement on how seriously you need to treat the backward compatibility is based on your understanding of the issue you summarized in your message, you need to reconsider what you are proposing, after learning from the true history.

The transition plan was first announced officially as part of the
release notes for version 1.5.4. It said three things:

 - Using dashed forms of git commands (e.g. "git-commit") from the
   command line has been informally deprecated since early 2006, but
   now it officially is, and will be removed in the future.  Use
   dash-less forms (e.g. "git commit") instead.

 - Using dashed forms from your scripts, without first prepending the
   return value from "git --exec-path" to the scripts' PATH, has been
   informally deprecated since early 2006, but now it officially is.

 - Use of dashed forms with "PATH=$(git --exec-path):$PATH; export
   PATH" early in your script is not deprecated with this change.

Notice that:

 - "now" is December 1st, 2007.

 - "will be removed in the future" happened on August 17th, 2008 at version 1.6.0.

 - The notice EXPLICITLY promises to keep supporting the use of git-foo if you prepend output of 'git --exec-path'.

In other words, there was an escape hatch that had been very carefully in preparation for nine months. The same escape hach, and the promise to keep supporting it, was repeated in the release notes for version 1.6.0.

You can refresh your memory (or read it for the first time, if you weren't around back then) with a message from Junio on August 24th, 2008 after version 1.6.0 was released:

http://thread.gmane.org/gmane.comp.version-control.git/93511

It summarized "the original plan" (read the message and find the phrase in the middle) and outlined how it should be implemented if the git community still wanted to go through with the plan.

With the discussion that followed the message, the community decided not to do anything (also known as the "alternative A").

The escape hatch was there from the beginning, is still there, and it will remain there. I should also add that it was Junio's veto of Linus'es proposal to stop installing git-foo commands for builtins that enabled this escape hatch.

http://thread.gmane.org/gmane.comp.version-control.git/51245/focus=51272

By the way, I don't think the lesson you should take home is the need for an escape hatch. Read the message by Junio on August 24th, 2008. Being nice and not too loud during the deprecation period kept users complacent about upcoming changes and upset them when the change finally came. Being un-nice and too loud during the deprecation period would have upset them early instead. You cannot avoid upsetting users either way whenever you change the behavior. That's the lesson you should learn.

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

Re: [PATCH v2] send-email: add --confirm option

[Junio C Hamano]

Nanako Shiraishi <nanako3@lavabit.com> writes:

> The escape hatch was there from the beginning, is still there, and it
> will remain there. I should also add that it was Junio's veto of
> Linus'es proposal to stop installing git-foo commands for builtins
> that enabled this escape hatch.

I think veto is too strong a word to describe what really happened, but in
retrospect, if we went ahead and removed built-ins from the filesystem as
Linus and other people advocated, the escape hatch wouldn't have worked at
all, so in that sense you are correct.  But I do not think I deserve the
credit for that---I do not see myself making an argument based on that
"possible escape-hatch" value in that old thread.

By the way, how are you researching these old discussions?  Do you have
a huge list of bookmarks?

> By the way, I don't think the lesson you should take home is the need
> for an escape hatch. Read the message by Junio on August 24th,
> 2008. Being nice and not too loud during the deprecation period kept
> users complacent about upcoming changes and upset them when the change
> finally came. Being un-nice and too loud during the deprecation period
> would have upset them early instead. You cannot avoid upsetting users
> either way whenever you change the behavior.

Yup.

And the most scary part of all is that you cannot try both.  We now know
that for 1.6.0 transition people _claimed_ that they would have liked
louder deprecation period than the way 1.6.0 transition was handled, but
that is not (and cannot be) backed by real world experience. Nobody tried
versions of git that warned loudly about the upcoming change every time he
typed "git-commit" to see if the louder deprecation period was really
preferrable.

We are taking that route for 1.7.0 to warn very loudly about pushing into
the currently checked-out branch in 1.6.2 and onwards.  We may now find
out that people hate a loud deprecation period.  Then what?

Re: [PATCH v2] send-email: add --confirm option

[Nanako Shiraishi]

Quoting Junio C Hamano <gitster@pobox.com>:

> By the way, how are you researching these old discussions?  Do you have
> a huge list of bookmarks?

There weren't that many threads that became the turning points for the project; the list need not to be huge even if somebody were to keep one.

But I don't keep such a list; I just ask the gmane archive or google.

> And the most scary part of all is that you cannot try both.  We now know
> that for 1.6.0 transition people _claimed_ that they would have liked
> louder deprecation period than the way 1.6.0 transition was handled, but
> that is not (and cannot be) backed by real world experience. Nobody tried
> versions of git that warned loudly about the upcoming change every time he
> typed "git-commit" to see if the louder deprecation period was really
> preferrable.
>
> We are taking that route for 1.7.0 to warn very loudly about pushing into
> the currently checked-out branch in 1.6.2 and onwards.  We may now find
> out that people hate a loud deprecation period.  Then what?

Then you get flamed.

But isn't that what the maintainer is there for? To take blame on behalf of others, so that contributors can propose what they genuinely believe improvements without fearing what the end users would say?

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

Re: [PATCH v2] send-email: add --confirm option

[Felipe Contreras]

On Mon, Mar 2, 2009 at 11:01 AM, Junio C Hamano <gitster@pobox.com> wrote:
> We are taking that route for 1.7.0 to warn very loudly about pushing into
> the currently checked-out branch in 1.6.2 and onwards.  We may now find
> out that people hate a loud deprecation period.  Then what?

The problem is not the 'loud deprecation period' it's the deprecation
itself. You cannot avoid deprecation, so you cannot avoid users
complaining, but you can avoid surprises, and that's what the 'loud
deprecation period' is for.

The 'loud deprecation period' allows users to find out *earlier* so
that they can comment on the issue. If a huge amount of users
complain, maybe the deprecation should not proceed, or maybe someone
comes up with a plan B. Sill, some people would not be happy, but at
least their voice would have been heard.

Sure, it doesn't matter how it's handled, some people will still not be happy...

-- 
Felipe Contreras

Re: [PATCH v2] send-email: add --confirm option

[Jay Soffian]

On Mon, Mar 2, 2009 at 3:24 AM, Nanako Shiraishi <nanako3@lavabit.com> wrote:
> By the way, I don't think the lesson you should take home is the need for an escape hatch. Read the message by Junio on August 24th, 2008. Being nice and not too loud during the deprecation period kept users complacent about upcoming changes and upset them when the change finally came. Being un-nice and too loud during the deprecation period would have upset them early instead. You cannot avoid upsetting users either way whenever you change the behavior. That's the lesson you should learn.

Thank you for correcting me, and I apologize for the misinformation.

It is very difficult to balance between the users who are upset enough
by the change to be vocal about it, and the users who benefit from the
change but that this list never hears from, but I appreciate that
Junio is trying to accommodate both.

j.

Re: [PATCH v2] send-email: add --confirm option

[Jay Soffian]

On Mon, Mar 2, 2009 at 2:34 AM, Junio C Hamano <gitster@pobox.com> wrote:
> In any case, with the lesson I learned from the post v1.6.0 fiasco, it
> might make sense to make the command louder when it needs to look at the
> setting of $confirm option and when the user does not have anything in the
> config nor command line.
>
> What I mean is this.

Okay, I'll re-send. Thanks for the feedback.

j.

Re: [PATCH v2] send-email: add --confirm option

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

>  - When the user specifies "all" in response to a confirm prompt, we
>    hint them about how to use "sendemail.confirm" config setting.

I think the logic is wrong.  Even when you have configured to usually ask,
you may want to say "I know Cc settings for this particular series is all
good, but I still want the safety in my future invocations.".

How about doing it this way instead?

> @@ -346,6 +350,13 @@ if ($suppress_cc{'body'}) {
>  	delete $suppress_cc{'body'};
>  }
>  
> +# Set confirm
> +if (!defined $confirm) {
> +	$confirm = scalar %suppress_cc ? 'compose' : 'auto';

This is not "Set confirm" but "set the default value to $confirm".  Also
set $confirm_unconfigured = "true" here.

> @@ -837,6 +836,27 @@ X-Mailer: git-send-email $gitversion
>  	unshift (@sendmail_parameters,
>  			'-f', $raw_from) if(defined $envelope_sender);
> +	if ($needs_confirm && !$dry_run) {

And use it here; give some help for people who haven't seen this new
message and haven't decided:

	    if (!$confirm_unconfigured) {
		print "By default the command asks if you are sure about\n";
		print "the CC list computed from various places.  If you\n";
                print "do not want this confirmation step, you can say\n";
                print "    $ git config --global sendemail.confirm never\n";
                print "to squelch it.\n";
		print "A good setting might be:\n";
                print "    $ git config --global sendemail.confirm auto\n";
		print "to squelch this help text that is given until you\n";
		print "configure the variable to some value.\n";
	    }

> +		print "\n$header\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";
> +		}
> +		if (/^n/i) {
> +			return;
> +		} elsif (/^q/i) {
> +			cleanup_compose_files();
> +			exit(0);
> +		} elsif (/^a/i) {
> +			$confirm = 'never';

And drop the following two:

> +			print "You may disable all future prompting via sendemail.confirm;\n";
> +			print "Run 'git send-email --help' for details.\n";
> +		}
> +	}

[PATCH v3] send-email: add --confirm option and configuration setting

[Jay Soffian]

send-email violates the principle of least surprise by automatically
cc'ing additional recipients without confirming this with the user.

This patch teaches send-email a --confirm option. It takes the
following values:

 --confirm=always   always confirm before sending
 --confirm=never    never confirm before sending
 --confirm=cc       confirm before sending when send-email has
                    automatically added addresses from the patch to
                    the Cc list
 --confirm=compose  confirm before sending the first message when
                    using --compose. (Needed to maintain backwards
                    compatibility with existing behavior.)
 --confirm=auto     'cc' + 'compose'

If sendemail.confirm is unconfigured, the option defaults to 'compose'
if any suppress-Cc related options have been used, otherwise it defaults
to 'auto'.

Unfortunately, it is impossible to introduce this patch such that it
helps new users without potentially annoying some existing users. We
attempt to mitigate the latter by:

 * Allowing the user to set 'git config sendemail.confirm never'
 * Allowing the user to say 'all' after the first prompt to not be
   prompted on remaining emails during the same invocation.
 * Telling the user about the 'sendemail.confirm' setting if it is
   unconfigured whenever we prompt due to Cc before sending.
 * Only prompting if no --suppress related options have been passed, as
   using such an option is likely to indicate an experienced send-email
   user.

There is a slight fib in message informing the user of the
sendemail.confirm setting and this is intentional. Setting 'auto'
differs from leaving sendemail.confirm unset in two ways: 1) 'auto'
obviously squelches the informational message; 2) 'auto' prompts when
the Cc list has been expanded even in the presence of a --suppress
related option, where leaving sendemail.confirm unset does not. This is
intentional to keep the message simple, and to avoid adding another
sendemail.confirm value ('auto-except-suppress'?).

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
Differences from v2:

 * Inform user of sendemail.confirm if it is unconfigured and we are
   in a prompting situation where the Cc list has been expanded, per
   Junio's suggestion. This is instead of informing everytime "all"
   is used.

 * Fixed typo in commit message

 * Documented sendemail.confirm configuration setting.

 Documentation/git-send-email.txt |   21 +++++++
 git-send-email.perl              |   84 +++++++++++++++++++++--------
 t/t9001-send-email.sh            |  108 ++++++++++++++++++++++++++++++++------
 3 files changed, 174 insertions(+), 39 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 164d149..0335727 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -199,6 +199,22 @@ specified, as well as 'body' if --no-signed-off-cc is specified.
 Administering
 ~~~~~~~~~~~~~
 
+--confirm::
+	Confirm just before sending:
++
+--
+- 'always' will always confirm before sending
+- 'never' will never confirm before sending
+- 'cc' will confirm before sending when send-email has automatically
+  added addresses from the patch to the Cc list
+- 'compose' will confirm before sending the first message when using --compose.
+- 'auto' is equivalent to 'cc' + 'compose'
+--
++
+Default is the value of 'sendemail.confirm' configuration value; if that
+is unspecified, default to 'auto' unless any of the suppress options
+have been specified, in which case default to 'compose'.
+
 --dry-run::
 	Do everything except actually send the emails.
 
@@ -242,6 +258,11 @@ sendemail.multiedit::
 	summary when '--compose' is used). If false, files will be edited one
 	after the other, spawning a new editor each time.
 
+sendemail.confirm::
+	Sets the default for whether to confirm before sending. Must be
+	one of 'always', 'never', 'cc', 'compose', or 'auto'. See '--confirm'
+	in the previous section for the meaning of these values.
+
 
 Author
 ------
diff --git a/git-send-email.perl b/git-send-email.perl
index adf7ecb..57127aa 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -75,6 +75,8 @@ git send-email [options] <file | directory | rev-list options >
     --[no-]thread                  * Use In-Reply-To: field. Default on.
 
   Administering:
+    --confirm               <str>  * Confirm recipients before sending;
+                                     auto, cc, compose, always, or never.
     --quiet                        * Output one line of info per email.
     --dry-run                      * Don't actually send the emails.
     --[no-]validate                * Perform patch sanity checks. Default on.
@@ -181,7 +183,7 @@ sub do_edit {
 my ($thread, $chain_reply_to, $suppress_from, $signed_off_by_cc, $cc_cmd);
 my ($smtp_server, $smtp_server_port, $smtp_authuser, $smtp_encryption);
 my ($identity, $aliasfiletype, @alias_files, @smtp_host_parts);
-my ($validate);
+my ($validate, $confirm);
 my (@suppress_cc);
 
 my %config_bool_settings = (
@@ -207,6 +209,7 @@ my %config_settings = (
     "suppresscc" => \@suppress_cc,
     "envelopesender" => \$envelope_sender,
     "multiedit" => \$multiedit,
+    "confirm"   => \$confirm,
 );
 
 # Handle Uncouth Termination
@@ -258,6 +261,7 @@ my $rc = GetOptions("sender|from=s" => \$sender,
 		    "suppress-from!" => \$suppress_from,
 		    "suppress-cc=s" => \@suppress_cc,
 		    "signed-off-cc|signed-off-by-cc!" => \$signed_off_by_cc,
+		    "confirm=s" => \$confirm,
 		    "dry-run" => \$dry_run,
 		    "envelope-sender=s" => \$envelope_sender,
 		    "thread!" => \$thread,
@@ -346,6 +350,14 @@ if ($suppress_cc{'body'}) {
 	delete $suppress_cc{'body'};
 }
 
+# Set confirm's default value
+my $confirm_unconfigured = !defined $confirm;
+if ($confirm_unconfigured) {
+	$confirm = scalar %suppress_cc ? 'compose' : 'auto';
+};
+die "Unknown --confirm setting: '$confirm'\n"
+	unless $confirm =~ /^(?:auto|cc|compose|always|never)/;
+
 # Debugging, print out the suppressions.
 if (0) {
 	print "suppressions:\n";
@@ -663,25 +675,13 @@ if (!defined $smtp_server) {
 	$smtp_server ||= 'localhost'; # could be 127.0.0.1, too... *shrug*
 }
 
-if ($compose) {
-	while (1) {
-		$_ = $term->readline("Send this email? (y|n) ");
-		last if defined $_;
-		print "\n";
-	}
-
-	if (uc substr($_,0,1) ne 'Y') {
-		cleanup_compose_files();
-		exit(0);
-	}
-
-	if ($compose > 0) {
-		@files = ($compose_filename . ".final", @files);
-	}
+if ($compose && $compose > 0) {
+	@files = ($compose_filename . ".final", @files);
 }
 
 # Variables we set as part of the loop over files
-our ($message_id, %mail, $subject, $reply_to, $references, $message);
+our ($message_id, %mail, $subject, $reply_to, $references, $message,
+	$needs_confirm, $message_num);
 
 sub extract_valid_address {
 	my $address = shift;
@@ -837,6 +837,37 @@ X-Mailer: git-send-email $gitversion
 	unshift (@sendmail_parameters,
 			'-f', $raw_from) if(defined $envelope_sender);
 
+	if ($needs_confirm && !$dry_run) {
+		print "\n$header\n";
+		if ($needs_confirm eq "inform") {
+			$confirm_unconfigured = 0; # squelch this message for the rest of this run
+			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";
+			print "    This behavior is controlled by the sendemail.confirm\n";
+			print "    configuration setting.\n";
+			print "\n";
+			print "    For additional information, run 'git send-email --help'.\n";
+			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";
+		}
+		if (/^n/i) {
+			return;
+		} elsif (/^q/i) {
+			cleanup_compose_files();
+			exit(0);
+		} elsif (/^a/i) {
+			$confirm = 'never';
+		}
+	}
+
 	if ($dry_run) {
 		# We don't want to send the email.
 	} elsif ($smtp_server =~ m#^/#) {
@@ -935,6 +966,7 @@ X-Mailer: git-send-email $gitversion
 $reply_to = $initial_reply_to;
 $references = $initial_reply_to || '';
 $subject = $initial_subject;
+$message_num = 0;
 
 foreach my $t (@files) {
 	open(F,"<",$t) or die "can't open file $t";
@@ -943,11 +975,12 @@ foreach my $t (@files) {
 	my $author_encoding;
 	my $has_content_type;
 	my $body_encoding;
-	@cc = @initial_cc;
+	@cc = ();
 	@xh = ();
 	my $input_format = undef;
 	my @header = ();
 	$message = "";
+	$message_num++;
 	# First unfold multiline header fields
 	while(<F>) {
 		last if /^\s*$/;
@@ -1080,6 +1113,14 @@ foreach my $t (@files) {
 		}
 	}
 
+	$needs_confirm = (
+		$confirm eq "always" or
+		($confirm =~ /^(?:auto|cc)$/ && @cc) or
+		($confirm =~ /^(?:auto|compose)$/ && $compose && $message_num == 1));
+	$needs_confirm = "inform" if ($needs_confirm && $confirm_unconfigured && @cc);
+
+	@cc = (@initial_cc, @cc);
+
 	send_message();
 
 	# set up for the next message
@@ -1094,13 +1135,10 @@ foreach my $t (@files) {
 	$message_id = undef;
 }
 
-if ($compose) {
-	cleanup_compose_files();
-}
+cleanup_compose_files();
 
 sub cleanup_compose_files() {
-	unlink($compose_filename, $compose_filename . ".final");
-
+	unlink($compose_filename, $compose_filename . ".final") if $compose;
 }
 
 $smtp->quit if $smtp;
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 4df4f96..08d5b91 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -35,6 +35,47 @@ test_expect_success 'Extract patches' '
     patches=`git format-patch -s --cc="One <one@example.com>" --cc=two@example.com -n HEAD^1`
 '
 
+# Test no confirm early to ensure remaining tests will not hang
+test_no_confirm () {
+	rm -f no_confirm_okay
+	echo n | \
+		GIT_SEND_EMAIL_NOTTY=1 \
+		git send-email \
+		--from="Example <from@example.com>" \
+		--to=nobody@example.com \
+		--smtp-server="$(pwd)/fake.sendmail" \
+		$@ \
+		$patches > stdout &&
+		test_must_fail grep "Send this email" stdout &&
+		> no_confirm_okay
+}
+
+# Exit immediately to prevent hang if a no-confirm test fails
+check_no_confirm () {
+	test -f no_confirm_okay || {
+		say 'No confirm test failed; skipping remaining tests to prevent hanging'
+		test_done
+	}
+}
+
+test_expect_success 'No confirm with --suppress-cc' '
+	test_no_confirm --suppress-cc=sob
+'
+check_no_confirm
+
+test_expect_success 'No confirm with --confirm=never' '
+	test_no_confirm --confirm=never
+'
+check_no_confirm
+
+# leave sendemail.confirm set to never after this so that none of the
+# remaining tests prompt unintentionally.
+test_expect_success 'No confirm with sendemail.confirm=never' '
+	git config sendemail.confirm never &&
+	test_no_confirm --compose --subject=foo
+'
+check_no_confirm
+
 test_expect_success 'Send patches' '
      git send-email --suppress-cc=sob --from="Example <nobody@example.com>" --to=nobody@example.com --smtp-server="$(pwd)/fake.sendmail" $patches 2>errors
 '
@@ -175,15 +216,13 @@ test_set_editor "$(pwd)/fake-editor"
 
 test_expect_success '--compose works' '
 	clean_fake_sendmail &&
-	echo y | \
-		GIT_SEND_EMAIL_NOTTY=1 \
-		git send-email \
-		--compose --subject foo \
-		--from="Example <nobody@example.com>" \
-		--to=nobody@example.com \
-		--smtp-server="$(pwd)/fake.sendmail" \
-		$patches \
-		2>errors
+	git send-email \
+	--compose --subject foo \
+	--from="Example <nobody@example.com>" \
+	--to=nobody@example.com \
+	--smtp-server="$(pwd)/fake.sendmail" \
+	$patches \
+	2>errors
 '
 
 test_expect_success 'first message is compose text' '
@@ -375,15 +414,56 @@ test_expect_success '--suppress-cc=cc' '
 	test_suppression cc
 '
 
+test_confirm () {
+	echo y | \
+		GIT_SEND_EMAIL_NOTTY=1 \
+		git send-email \
+		--from="Example <nobody@example.com>" \
+		--to=nobody@example.com \
+		--smtp-server="$(pwd)/fake.sendmail" \
+		$@ \
+		$patches | grep "Send this email"
+}
+
+test_expect_success '--confirm=always' '
+	test_confirm --confirm=always --suppress-cc=all
+'
+
+test_expect_success '--confirm=auto' '
+	test_confirm --confirm=auto
+'
+
+test_expect_success '--confirm=cc' '
+	test_confirm --confirm=cc
+'
+
+test_expect_success '--confirm=compose' '
+	test_confirm --confirm=compose --compose
+'
+
+test_expect_success 'confirm by default (due to cc)' '
+	CONFIRM=$(git config --get sendemail.confirm) &&
+	git config --unset sendemail.confirm &&
+	test_confirm &&
+	git config sendemail.confirm $CONFIRM
+'
+
+test_expect_success 'confirm by default (due to --compose)' '
+	CONFIRM=$(git config --get sendemail.confirm) &&
+	git config --unset sendemail.confirm &&
+	test_confirm --suppress-cc=all --compose
+	ret="$?"
+	git config sendemail.confirm ${CONFIRM:-never}
+	test $ret = "0"
+'
+
 test_expect_success '--compose adds MIME for utf8 body' '
 	clean_fake_sendmail &&
 	(echo "#!$SHELL_PATH" &&
 	 echo "echo utf8 body: àéìöú >>\"\$1\""
 	) >fake-editor-utf8 &&
 	chmod +x fake-editor-utf8 &&
-	echo y | \
 	  GIT_EDITOR="\"$(pwd)/fake-editor-utf8\"" \
-	  GIT_SEND_EMAIL_NOTTY=1 \
 	  git send-email \
 	  --compose --subject foo \
 	  --from="Example <nobody@example.com>" \
@@ -405,9 +485,7 @@ test_expect_success '--compose respects user mime type' '
 	 echo " echo utf8 body: àéìöú) >\"\$1\""
 	) >fake-editor-utf8-mime &&
 	chmod +x fake-editor-utf8-mime &&
-	echo y | \
 	  GIT_EDITOR="\"$(pwd)/fake-editor-utf8-mime\"" \
-	  GIT_SEND_EMAIL_NOTTY=1 \
 	  git send-email \
 	  --compose --subject foo \
 	  --from="Example <nobody@example.com>" \
@@ -421,9 +499,7 @@ test_expect_success '--compose respects user mime type' '
 
 test_expect_success '--compose adds MIME for utf8 subject' '
 	clean_fake_sendmail &&
-	echo y | \
 	  GIT_EDITOR="\"$(pwd)/fake-editor\"" \
-	  GIT_SEND_EMAIL_NOTTY=1 \
 	  git send-email \
 	  --compose --subject utf8-sübjëct \
 	  --from="Example <nobody@example.com>" \
@@ -445,7 +521,7 @@ test_expect_success 'detects ambiguous reference/file conflict' '
 test_expect_success 'feed two files' '
 	rm -fr outdir &&
 	git format-patch -2 -o outdir &&
-	GIT_SEND_EMAIL_NOTTY=1 git send-email \
+	git send-email \
 	--dry-run \
 	--from="Example <nobody@example.com>" \
 	--to=nobody@example.com \
-- 
1.6.2.rc1.309.g5f417

Re: [PATCH v3] send-email: add --confirm option and configuration setting

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> There is a slight fib in message informing the user of the
> sendemail.confirm setting and this is intentional. Setting 'auto'
> differs from leaving sendemail.confirm unset in two ways: 1) 'auto'
> obviously squelches the informational message; 2) 'auto' prompts when
> the Cc list has been expanded even in the presence of a --suppress
> related option, where leaving sendemail.confirm unset does not. This is
> intentional to keep the message simple, and to avoid adding another
> sendemail.confirm value ('auto-except-suppress'?).

I suspect you may get bug reports and refinement patches on top of this to
fix the last issue up, but I think that better should happen once the base
patch is in-tree.

Let's queue this to 'pu' and see how people react. 

Thanks.

Re: [PATCH v3] send-email: add --confirm option and configuration setting

[Nanako Shiraishi]

Quoting Jay Soffian <jaysoffian@gmail.com>:

> Unfortunately, it is impossible to introduce this patch such that it
> helps new users without potentially annoying some existing users. We
> attempt to mitigate the latter by:
>
>  * Allowing the user to set 'git config sendemail.confirm never'
>  * Allowing the user to say 'all' after the first prompt to not be
>    prompted on remaining emails during the same invocation.
>  * Telling the user about the 'sendemail.confirm' setting if it is
>    unconfigured whenever we prompt due to Cc before sending.
>  * Only prompting if no --suppress related options have been passed, as
>    using such an option is likely to indicate an experienced send-email
>    user.
>
> There is a slight fib in message informing the user of the
> sendemail.confirm setting and this is intentional. Setting 'auto'
> differs from leaving sendemail.confirm unset in two ways: 1) 'auto'
> obviously squelches the informational message; 2) 'auto' prompts when
> the Cc list has been expanded even in the presence of a --suppress
> related option, where leaving sendemail.confirm unset does not. This is
> intentional to keep the message simple, and to avoid adding another
> sendemail.confirm value ('auto-except-suppress'?).

For what it's worth, I think this much more carefully addresses the issues to migrate existing users by giving adequate help where it matters than the previous patches.

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

Re: [PATCH v3] send-email: add --confirm option and configuration setting

[Bert Wesarg]

On Tue, Mar 3, 2009 at 05:52, Jay Soffian <jaysoffian@gmail.com> wrote:
> diff --git a/git-send-email.perl b/git-send-email.perl
> index adf7ecb..57127aa 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -837,6 +837,37 @@ X-Mailer: git-send-email $gitversion
>        unshift (@sendmail_parameters,
>                        '-f', $raw_from) if(defined $envelope_sender);
>
> +       if ($needs_confirm && !$dry_run) {
So, the output is now differnt with and without --dry-run?

Bert

Re: [PATCH v3] send-email: add --confirm option and configuration setting

[Jay Soffian]

On Tue, Mar 3, 2009 at 6:54 AM, Bert Wesarg <bert.wesarg@googlemail.com> wrote:
> On Tue, Mar 3, 2009 at 05:52, Jay Soffian <jaysoffian@gmail.com> wrote:
>> diff --git a/git-send-email.perl b/git-send-email.perl
>> index adf7ecb..57127aa 100755
>> --- a/git-send-email.perl
>> +++ b/git-send-email.perl
>> @@ -837,6 +837,37 @@ X-Mailer: git-send-email $gitversion
>>        unshift (@sendmail_parameters,
>>                        '-f', $raw_from) if(defined $envelope_sender);
>>
>> +       if ($needs_confirm && !$dry_run) {
> So, the output is now differnt with and without --dry-run?

There doesn't seem to be any point in having the user confirm before
sending the message if the message is not actually going to be sent.
Am I missing something?

j.

Re: [PATCH v3] send-email: add --confirm option and configuration setting

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> On Tue, Mar 3, 2009 at 6:54 AM, Bert Wesarg <bert.wesarg@googlemail.com> wrote:
>> On Tue, Mar 3, 2009 at 05:52, Jay Soffian <jaysoffian@gmail.com> wrote:
>>> diff --git a/git-send-email.perl b/git-send-email.perl
>>> index adf7ecb..57127aa 100755
>>> --- a/git-send-email.perl
>>> +++ b/git-send-email.perl
>>> @@ -837,6 +837,37 @@ X-Mailer: git-send-email $gitversion
>>>        unshift (@sendmail_parameters,
>>>                        '-f', $raw_from) if(defined $envelope_sender);
>>>
>>> +       if ($needs_confirm && !$dry_run) {
>> So, the output is now differnt with and without --dry-run?
>
> There doesn't seem to be any point in having the user confirm before
> sending the message if the message is not actually going to be sent.
> Am I missing something?

I do not think you are missing anything.

IIRC, the --dry-run mode shows more clearly to whom you would be CC'ing
the messages; in other words, the behaviour would be different, but it
gives an uninteractive way to confirm, and not pausing for confirmation is
a good thing.

Re: [PATCH v3] send-email: add --confirm option and configuration setting

[Bert Wesarg]

On Tue, Mar 3, 2009 at 17:48, Junio C Hamano <gitster@pobox.com> wrote:
> Jay Soffian <jaysoffian@gmail.com> writes:
>
>> On Tue, Mar 3, 2009 at 6:54 AM, Bert Wesarg <bert.wesarg@googlemail.com> wrote:
>>> On Tue, Mar 3, 2009 at 05:52, Jay Soffian <jaysoffian@gmail.com> wrote:
>>>> diff --git a/git-send-email.perl b/git-send-email.perl
>>>> index adf7ecb..57127aa 100755
>>>> --- a/git-send-email.perl
>>>> +++ b/git-send-email.perl
>>>> @@ -837,6 +837,37 @@ X-Mailer: git-send-email $gitversion
>>>>        unshift (@sendmail_parameters,
>>>>                        '-f', $raw_from) if(defined $envelope_sender);
>>>>
>>>> +       if ($needs_confirm && !$dry_run) {
>>> So, the output is now differnt with and without --dry-run?
>>
>> There doesn't seem to be any point in having the user confirm before
>> sending the message if the message is not actually going to be sent.
>> Am I missing something?
>
> I do not think you are missing anything.
>
> IIRC, the --dry-run mode shows more clearly to whom you would be CC'ing
> the messages; in other words, the behaviour would be different, but it
> gives an uninteractive way to confirm, and not pausing for confirmation is
> a good thing.
>
Just to clarify: A user who runs a --dry-run before every sending
(like me) would check the Cc list anyway (like me), so he either would
have sendmail.confirm=never in the config, so that he will not
bothered by send-email while sending or he sees some Cc's that he
don't want and can remove them in the sending process.

Ok, than I'm fine with this.

Regards,
Bert

Re: [PATCH v3] send-email: add --confirm option and configuration setting

[Jay Soffian]

On Tue, Mar 3, 2009 at 1:05 PM, Bert Wesarg <bert.wesarg@googlemail.com> wrote:
>
> Just to clarify: A user who runs a --dry-run before every sending
> (like me) would check the Cc list anyway (like me), so he either would
> have sendmail.confirm=never in the config, so that he will not
> bothered by send-email while sending or he sees some Cc's that he
> don't want and can remove them in the sending process.

Correct. This confirm addition was motivated by new users who do not know about
--dry-run and are surprised when their email is sent to unintended recipients.

If you always use --dry-run, and don't want to be prompted when you actually
send, then setting sendemail.confirm=never is what you want.

> Ok, than I'm fine with this.

Phew. :-)

j.