On Sun, Mar 1, 2009 at 11:17 AM, Jay Soffian <jaysoffian@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxx>" --cc=two@xxxxxxxxxxx -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@xxxxxxxxxxx>" \
> + --to=nobody@xxxxxxxxxxx \
> + --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@xxxxxxxxxxx>" --to=nobody@xxxxxxxxxxx --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@xxxxxxxxxxx>" \
> - --to=nobody@xxxxxxxxxxx \
> - --smtp-server="$(pwd)/fake.sendmail" \
> - $patches \
> - 2>errors
> + git send-email \
> + --compose --subject foo \
> + --from="Example <nobody@xxxxxxxxxxx>" \
> + --to=nobody@xxxxxxxxxxx \
> + --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@xxxxxxxxxxx>" \
> + --to=nobody@xxxxxxxxxxx \
> + --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@xxxxxxxxxxx>" \
> @@ -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@xxxxxxxxxxx>" \
> @@ -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@xxxxxxxxxxx>" \
> @@ -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@xxxxxxxxxxx>" \
> --to=nobody@xxxxxxxxxxx \
> --
> 1.6.2.rc1.309.g5f417
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
