Merge branch 'mt/send-email-cc-match-fix'
authorJunio C Hamano <gitster@pobox.com>
Fri, 14 Jun 2013 15:46:20 +0000 (08:46 -0700)
committerJunio C Hamano <gitster@pobox.com>
Fri, 14 Jun 2013 15:46:20 +0000 (08:46 -0700)
Logic git-send-email used to suppress cc mishandled names like "A
U. Thor" <author@example.xz>, where the human readable part needs
to be quoted (the user input may not have the double quotes around
the name, and comparison was done between quoted and unquoted
strings).

* mt/send-email-cc-match-fix:
test-send-email: test for pre-sanitized self name
t/send-email: test suppress-cc=self with non-ascii
t/send-email: add test with quoted sender
send-email: make --suppress-cc=self sanitize input
t/send-email: test suppress-cc=self on cccmd
send-email: fix suppress-cc=self on cccmd
t/send-email.sh: add test for suppress-cc=self

1  2 
git-send-email.perl
t/t9001-send-email.sh
diff --combined git-send-email.perl
index ec1d6ce4ebfd4810d46c727e99af9a1b54074ec4,7d886958ae04a617ecb9dea7628057638dcbfc9b..671762b93031e66cba2ca837ddf71ed765f7a204
@@@ -54,7 -54,7 +54,7 @@@ git send-email [options] <file | direct
      --[no-]bcc              <str>  * Email Bcc:
      --subject               <str>  * Email "Subject:"
      --in-reply-to           <str>  * Email "In-Reply-To:"
 -    --annotate                     * Review each patch that will be sent in an editor.
 +    --[no-]annotate                * Review each patch that will be sent in an editor.
      --compose                      * Open an editor for introduction.
      --compose-encoding      <str>  * Encoding to assume for introduction.
      --8bit-encoding         <str>  * Encoding to assume 8bit mails if undeclared
@@@ -203,15 -203,16 +203,15 @@@ my ($compose_encoding)
  
  my ($debug_net_smtp) = 0;             # Net::SMTP, see send_message()
  
 -my $not_set_by_user = "true but not set by the user";
 -
  my %config_bool_settings = (
      "thread" => [\$thread, 1],
 -    "chainreplyto" => [\$chain_reply_to, $not_set_by_user],
 +    "chainreplyto" => [\$chain_reply_to, 0],
      "suppressfrom" => [\$suppress_from, undef],
      "signedoffbycc" => [\$signed_off_by_cc, undef],
      "signedoffcc" => [\$signed_off_by_cc, undef],      # Deprecated
      "validate" => [\$validate, 1],
 -    "multiedit" => [\$multiedit, undef]
 +    "multiedit" => [\$multiedit, undef],
 +    "annotate" => [\$annotate, undef]
  );
  
  my %config_settings = (
@@@ -239,6 -240,19 +239,6 @@@ my %config_path_settings = 
      "aliasesfile" => \@alias_files,
  );
  
 -# Help users prepare for 1.7.0
 -sub chain_reply_to {
 -      if (defined $chain_reply_to &&
 -          $chain_reply_to eq $not_set_by_user) {
 -              print STDERR
 -                  "In git 1.7.0, the default has changed to --no-chain-reply-to\n" .
 -                  "Set sendemail.chainreplyto configuration variable to true if\n" .
 -                  "you want to keep --chain-reply-to as your default.\n";
 -              $chain_reply_to = 0;
 -      }
 -      return $chain_reply_to;
 -}
 -
  # Handle Uncouth Termination
  sub signal_handler {
  
@@@ -290,7 -304,7 +290,7 @@@ my $rc = GetOptions("h" => \$help
                    "smtp-debug:i" => \$debug_net_smtp,
                    "smtp-domain:s" => \$smtp_domain,
                    "identity=s" => \$identity,
 -                  "annotate" => \$annotate,
 +                  "annotate!" => \$annotate,
                    "compose" => \$compose,
                    "quiet" => \$quiet,
                    "cc-cmd=s" => \$cc_cmd,
@@@ -745,6 -759,11 +745,11 @@@ if (!defined $sender) 
        $sender = $repoauthor || $repocommitter || '';
  }
  
+ # $sender could be an already sanitized address
+ # (e.g. sendemail.from could be manually sanitized by user).
+ # But it's a no-op to run sanitize_address on an already sanitized address.
+ $sender = sanitize_address($sender);
  my $prompting = 0;
  if (!@initial_to && !defined $to_cmd) {
        my $to = ask("Who should the emails be sent to (if any)? ",
@@@ -1033,47 -1052,6 +1038,47 @@@ sub maildomain 
        return maildomain_net() || maildomain_mta() || 'localhost.localdomain';
  }
  
 +sub smtp_host_string {
 +      if (defined $smtp_server_port) {
 +              return "$smtp_server:$smtp_server_port";
 +      } else {
 +              return $smtp_server;
 +      }
 +}
 +
 +# Returns 1 if authentication succeeded or was not necessary
 +# (smtp_user was not specified), and 0 otherwise.
 +
 +sub smtp_auth_maybe {
 +      if (!defined $smtp_authuser || $auth) {
 +              return 1;
 +      }
 +
 +      # Workaround AUTH PLAIN/LOGIN interaction defect
 +      # with Authen::SASL::Cyrus
 +      eval {
 +              require Authen::SASL;
 +              Authen::SASL->import(qw(Perl));
 +      };
 +
 +      # TODO: Authentication may fail not because credentials were
 +      # invalid but due to other reasons, in which we should not
 +      # reject credentials.
 +      $auth = Git::credential({
 +              'protocol' => 'smtp',
 +              'host' => smtp_host_string(),
 +              'username' => $smtp_authuser,
 +              # if there's no password, "git credential fill" will
 +              # give us one, otherwise it'll just pass this one.
 +              'password' => $smtp_authpass
 +      }, sub {
 +              my $cred = shift;
 +              return !!$smtp->auth($cred->{'username'}, $cred->{'password'});
 +      });
 +
 +      return $auth;
 +}
 +
  # Returns 1 if the message was sent, and 0 otherwise.
  # In actuality, the whole program dies when there
  # is an error sending a message.
@@@ -1098,10 -1076,9 +1103,9 @@@ sub send_message 
        if ($cc ne '') {
                $ccline = "\nCc: $cc";
        }
-       my $sanitized_sender = sanitize_address($sender);
        make_message_id() unless defined($message_id);
  
-       my $header = "From: $sanitized_sender
+       my $header = "From: $sender
  To: $to${ccline}
  Subject: $subject
  Date: $date
@@@ -1118,7 -1095,7 +1122,7 @@@ X-Mailer: git-send-email $gitversio
        }
  
        my @sendmail_parameters = ('-i', @recipients);
-       my $raw_from = $sanitized_sender;
+       my $raw_from = $sender;
        if (defined $envelope_sender && $envelope_sender ne "auto") {
                $raw_from = $envelope_sender;
        }
                else {
                        require Net::SMTP;
                        $smtp_domain ||= maildomain();
 -                      $smtp ||= Net::SMTP->new((defined $smtp_server_port)
 -                                               ? "$smtp_server:$smtp_server_port"
 -                                               : $smtp_server,
 +                      $smtp ||= Net::SMTP->new(smtp_host_string(),
                                                 Hello => $smtp_domain,
                                                 Debug => $debug_net_smtp);
                        if ($smtp_encryption eq 'tls' && $smtp) {
                            defined $smtp_server_port ? " port=$smtp_server_port" : "";
                }
  
 -              if (defined $smtp_authuser) {
 -                      # Workaround AUTH PLAIN/LOGIN interaction defect
 -                      # with Authen::SASL::Cyrus
 -                      eval {
 -                              require Authen::SASL;
 -                              Authen::SASL->import(qw(Perl));
 -                      };
 -
 -                      if (!defined $smtp_authpass) {
 -
 -                              system "stty -echo";
 -
 -                              do {
 -                                      print "Password: ";
 -                                      $_ = <STDIN>;
 -                                      print "\n";
 -                              } while (!defined $_);
 -
 -                              chomp($smtp_authpass = $_);
 -
 -                              system "stty echo";
 -                      }
 -
 -                      $auth ||= $smtp->auth( $smtp_authuser, $smtp_authpass ) or die $smtp->message;
 -              }
 +              smtp_auth_maybe or die $smtp->message;
  
                $smtp->mail( $raw_from ) or die $smtp->message;
                $smtp->to( @recipients ) or die $smtp->message;
@@@ -1293,8 -1296,9 +1297,9 @@@ foreach my $t (@files) 
                        }
                        elsif (/^From:\s+(.*)$/i) {
                                ($author, $author_encoding) = unquote_rfc2047($1);
+                               my $sauthor = sanitize_address($author);
                                next if $suppress_cc{'author'};
-                               next if $suppress_cc{'self'} and $author eq $sender;
+                               next if $suppress_cc{'self'} and $sauthor eq $sender;
                                printf("(mbox) Adding cc: %s from line '%s'\n",
                                        $1, $_) unless $quiet;
                                push @cc, $1;
                        }
                        elsif (/^Cc:\s+(.*)$/i) {
                                foreach my $addr (parse_address_line($1)) {
-                                       if (unquote_rfc2047($addr) eq $sender) {
+                                       my $qaddr = unquote_rfc2047($addr);
+                                       my $saddr = sanitize_address($qaddr);
+                                       if ($saddr eq $sender) {
                                                next if ($suppress_cc{'self'});
                                        } else {
                                                next if ($suppress_cc{'cc'});
                        chomp;
                        my ($what, $c) = ($1, $2);
                        chomp $c;
-                       if ($c eq $sender) {
+                       my $sc = sanitize_address($c);
+                       if ($sc eq $sender) {
                                next if ($suppress_cc{'self'});
                        } else {
                                next if $suppress_cc{'sob'} and $what =~ /Signed-off-by/i;
  
        # set up for the next message
        if ($thread && $message_was_sent &&
 -              (chain_reply_to() || !defined $reply_to || length($reply_to) == 0 ||
 +              ($chain_reply_to || !defined $reply_to || length($reply_to) == 0 ||
                $message_num == 1)) {
                $reply_to = $message_id;
                if (length $references > 0) {
  sub recipients_cmd {
        my ($prefix, $what, $cmd, $file) = @_;
  
-       my $sanitized_sender = sanitize_address($sender);
        my @addresses = ();
        open my $fh, "-|", "$cmd \Q$file\E"
            or die "($prefix) Could not execute '$cmd'";
                $address =~ s/^\s*//g;
                $address =~ s/\s*$//g;
                $address = sanitize_address($address);
-               next if ($address eq $sanitized_sender and $suppress_from);
+               next if ($address eq $sender and $suppress_cc{'self'});
                push @addresses, $address;
                printf("($prefix) Adding %s: %s from: '%s'\n",
                       $what, $address, $cmd) unless $quiet;
diff --combined t/t9001-send-email.sh
index 5d2dbe98d5e01e8b18b5d9fe26ce5ed6f2bf171d,ebcb60a050cc68c281bb99d9de4da66452e0efe2..9f46f22ca87be40e217c2ffb199368f0afbf9548
@@@ -171,6 -171,81 +171,81 @@@ Result: O
  EOF
  "
  
+ test_suppress_self () {
+       test_commit $3 &&
+       test_when_finished "git reset --hard HEAD^" &&
+       write_script cccmd-sed <<-EOF &&
+               sed -n -e s/^cccmd--//p "\$1"
+       EOF
+       git commit --amend --author="$1 <$2>" -F - &&
+       clean_fake_sendmail &&
+       git format-patch --stdout -1 >"suppress-self-$3.patch" &&
+       git send-email --from="$1 <$2>" \
+               --to=nobody@example.com \
+               --cc-cmd=./cccmd-sed \
+               --suppress-cc=self \
+               --smtp-server="$(pwd)/fake.sendmail" \
+               suppress-self-$3.patch &&
+       mv msgtxt1 msgtxt1-$3 &&
+       sed -e '/^$/q' msgtxt1-$3 >"msghdr1-$3" &&
+       >"expected-no-cc-$3" &&
+       (grep '^Cc:' msghdr1-$3 >"actual-no-cc-$3";
+        test_cmp expected-no-cc-$3 actual-no-cc-$3)
+ }
+ test_suppress_self_unquoted () {
+       test_suppress_self "$1" "$2" "unquoted-$3" <<-EOF
+               test suppress-cc.self unquoted-$3 with name $1 email $2
+               unquoted-$3
+               cccmd--$1 <$2>
+               Cc: $1 <$2>
+               Signed-off-by: $1 <$2>
+       EOF
+ }
+ test_suppress_self_quoted () {
+       test_suppress_self "$1" "$2" "quoted-$3" <<-EOF
+               test suppress-cc.self quoted-$3 with name $1 email $2
+               quoted-$3
+               cccmd--"$1" <$2>
+               Cc: $1 <$2>
+               Cc: "$1" <$2>
+               Signed-off-by: $1 <$2>
+               Signed-off-by: "$1" <$2>
+       EOF
+ }
+ test_expect_success $PREREQ 'self name is suppressed' "
+       test_suppress_self_unquoted 'A U Thor' 'author@example.com' \
+               'self_name_suppressed'
+ "
+ test_expect_success $PREREQ 'self name with dot is suppressed' "
+       test_suppress_self_quoted 'A U. Thor' 'author@example.com' \
+               'self_name_dot_suppressed'
+ "
+ test_expect_success $PREREQ 'non-ascii self name is suppressed' "
+       test_suppress_self_quoted 'Füñný Nâmé' 'odd_?=mail@example.com' \
+               'non_ascii_self_suppressed'
+ "
+ test_expect_success $PREREQ 'sanitized self name is suppressed' "
+       test_suppress_self_unquoted '\"A U. Thor\"' 'author@example.com' \
+               'self_name_sanitized_suppressed'
+ "
  test_expect_success $PREREQ 'Show all headers' '
        git send-email \
                --dry-run \
@@@ -1003,6 -1078,55 +1078,6 @@@ test_expect_success $PREREQ 'threading 
        grep "In-Reply-To: " stdout
  '
  
 -test_expect_success $PREREQ 'warning with an implicit --chain-reply-to' '
 -      git send-email \
 -      --dry-run \
 -      --from="Example <nobody@example.com>" \
 -      --to=nobody@example.com \
 -      outdir/000?-*.patch 2>errors >out &&
 -      grep "no-chain-reply-to" errors
 -'
 -
 -test_expect_success $PREREQ 'no warning with an explicit --chain-reply-to' '
 -      git send-email \
 -      --dry-run \
 -      --from="Example <nobody@example.com>" \
 -      --to=nobody@example.com \
 -      --chain-reply-to \
 -      outdir/000?-*.patch 2>errors >out &&
 -      ! grep "no-chain-reply-to" errors
 -'
 -
 -test_expect_success $PREREQ 'no warning with an explicit --no-chain-reply-to' '
 -      git send-email \
 -      --dry-run \
 -      --from="Example <nobody@example.com>" \
 -      --to=nobody@example.com \
 -      --nochain-reply-to \
 -      outdir/000?-*.patch 2>errors >out &&
 -      ! grep "no-chain-reply-to" errors
 -'
 -
 -test_expect_success $PREREQ 'no warning with sendemail.chainreplyto = false' '
 -      git config sendemail.chainreplyto false &&
 -      git send-email \
 -      --dry-run \
 -      --from="Example <nobody@example.com>" \
 -      --to=nobody@example.com \
 -      outdir/000?-*.patch 2>errors >out &&
 -      ! grep "no-chain-reply-to" errors
 -'
 -
 -test_expect_success $PREREQ 'no warning with sendemail.chainreplyto = true' '
 -      git config sendemail.chainreplyto true &&
 -      git send-email \
 -      --dry-run \
 -      --from="Example <nobody@example.com>" \
 -      --to=nobody@example.com \
 -      outdir/000?-*.patch 2>errors >out &&
 -      ! grep "no-chain-reply-to" errors
 -'
 -
  test_expect_success $PREREQ 'sendemail.to works' '
        git config --replace-all sendemail.to "Somebody <somebody@ex.com>" &&
        git send-email \