From: Paul Gortmaker <paul.gortmaker@windriver.com>
Date: Sun, 01 Mar 2009 17:09:17 GMT
Subject: Re: [PATCH v2] send-email: add --confirm option
Message-ID: <7d1d9c250903010909h7d92f165oc703a05e819671a4@mail.gmail.com>
In-Reply-To: <1235924234-16923-1-git-send-email-jaysoffian@gmail.com>

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
>