git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH 0/6] git send-email suppress-cc=self fixes
@ 2013-05-26 14:40 Michael S. Tsirkin
  2013-05-26 14:40 ` [PATCH 1/6] t/send-email.sh: add test for suppress self Michael S. Tsirkin
                   ` (5 more replies)
  0 siblings, 6 replies; 12+ messages in thread
From: Michael S. Tsirkin @ 2013-05-26 14:40 UTC (permalink / raw)
  To: git

This includes bugfixes related to handling of --suppress-cc=self
flag. Tests are also included.

-- 
MST

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/6] t/send-email.sh: add test for suppress self
  2013-05-26 14:40 [PATCH 0/6] git send-email suppress-cc=self fixes Michael S. Tsirkin
@ 2013-05-26 14:40 ` Michael S. Tsirkin
  2013-05-29 20:28   ` Junio C Hamano
  2013-05-26 14:40 ` [PATCH 2/6] send-email: fix suppress-cc=self on cccmd Michael S. Tsirkin
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 12+ messages in thread
From: Michael S. Tsirkin @ 2013-05-26 14:40 UTC (permalink / raw)
  To: git

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 t/t9001-send-email.sh | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index ebd5c5d..36ecf73 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -171,6 +171,47 @@ Result: OK
 EOF
 "
 
+test_suppress_self () {
+		test_commit $3 &&
+		test_when_finished "git reset --hard HEAD^" &&
+		{
+			echo "#!$SHELL_PATH"
+			echo sed -n -e s/^cccmd--//p \"\$1\"
+		} > cccmd-sed &&
+		chmod +x cccmd-sed &&
+		git commit --amend --author="$1 <$2>" -F - << EOF && \
+		clean_fake_sendmail && \
+		echo suppress-self-$3.patch > /dev/tty && \
+		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 && \
+		rm -f expected-no-cc-$3 && \
+		touch expected-no-cc-$3 && \
+		grep '^Cc:' msghdr1-$3 > actual-no-cc-$3 && \
+		test_cmp expected-no-cc-$3 actual-no-cc-$3
+test suppress-cc.self $3 with name $1 email $2
+
+$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 'A U Thor' 'author@redhat.com' 'self_name_suppressed'
+"
+
 test_expect_success $PREREQ 'Show all headers' '
 	git send-email \
 		--dry-run \
-- 
MST

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/6] send-email: fix suppress-cc=self on cccmd
  2013-05-26 14:40 [PATCH 0/6] git send-email suppress-cc=self fixes Michael S. Tsirkin
  2013-05-26 14:40 ` [PATCH 1/6] t/send-email.sh: add test for suppress self Michael S. Tsirkin
@ 2013-05-26 14:40 ` Michael S. Tsirkin
  2013-05-26 14:41 ` [PATCH 3/6] t/send-email: test " Michael S. Tsirkin
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 12+ messages in thread
From: Michael S. Tsirkin @ 2013-05-26 14:40 UTC (permalink / raw)
  To: git

When cccmd is used, old-style suppress-from filter
is applied by the newer suppress-cc=self isn't.
Fix this up.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 git-send-email.perl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index bd13cc8..a138615 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1462,7 +1462,7 @@ sub recipients_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;
-- 
MST

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 3/6] t/send-email: test suppress-cc=self on cccmd
  2013-05-26 14:40 [PATCH 0/6] git send-email suppress-cc=self fixes Michael S. Tsirkin
  2013-05-26 14:40 ` [PATCH 1/6] t/send-email.sh: add test for suppress self Michael S. Tsirkin
  2013-05-26 14:40 ` [PATCH 2/6] send-email: fix suppress-cc=self on cccmd Michael S. Tsirkin
@ 2013-05-26 14:41 ` Michael S. Tsirkin
  2013-05-26 14:41 ` [PATCH 4/6] send-email: make --suppress-cc=self sanitize input Michael S. Tsirkin
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 12+ messages in thread
From: Michael S. Tsirkin @ 2013-05-26 14:41 UTC (permalink / raw)
  To: git

Check that suppress-cc=self works when applied
to output of cccmd.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 t/t9001-send-email.sh | 50 +++++++++++++++++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 17 deletions(-)

diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 36ecf73..8aa55f8 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -179,37 +179,53 @@ test_suppress_self () {
 			echo sed -n -e s/^cccmd--//p \"\$1\"
 		} > cccmd-sed &&
 		chmod +x cccmd-sed &&
-		git commit --amend --author="$1 <$2>" -F - << EOF && \
-		clean_fake_sendmail && \
-		echo suppress-self-$3.patch > /dev/tty && \
-		git format-patch --stdout -1 >suppress-self-$3.patch && \
-		git send-email --from="$1 <$2>" \
+		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 && \
-		rm -f expected-no-cc-$3 && \
-		touch expected-no-cc-$3 && \
-		grep '^Cc:' msghdr1-$3 > actual-no-cc-$3 && \
-		test_cmp expected-no-cc-$3 actual-no-cc-$3
-test suppress-cc.self $3 with name $1 email $2
+		suppress-self-$3.patch && 
+		mv msgtxt1 msgtxt1-$3 && 
+		sed -e '/^$/q' msgtxt1-$3 > msghdr1-$3 && 
+		rm -f expected-no-cc-$3 && 
+		touch 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
 
-$3
+quoted-$3
 
 cccmd--"$1" <$2>
 
-Cc: "$1" <$2>
 Cc: $1 <$2>
-Signed-off-by: "$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 'A U Thor' 'author@redhat.com' 'self_name_suppressed'
+	test_suppress_self_unquoted 'A U Thor' 'author@redhat.com' 'self_name_suppressed'
 "
 
 test_expect_success $PREREQ 'Show all headers' '
-- 
MST

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 4/6] send-email: make --suppress-cc=self sanitize input
  2013-05-26 14:40 [PATCH 0/6] git send-email suppress-cc=self fixes Michael S. Tsirkin
                   ` (2 preceding siblings ...)
  2013-05-26 14:41 ` [PATCH 3/6] t/send-email: test " Michael S. Tsirkin
@ 2013-05-26 14:41 ` Michael S. Tsirkin
  2013-05-26 14:41 ` [PATCH 5/6] t/send-email: add test with quoted sender Michael S. Tsirkin
  2013-05-26 14:41 ` [PATCH 6/6] t/send-email: test suppress-cc=self with non-ascii Michael S. Tsirkin
  5 siblings, 0 replies; 12+ messages in thread
From: Michael S. Tsirkin @ 2013-05-26 14:41 UTC (permalink / raw)
  To: git

--suppress-cc=self fails to filter sender address in many cases where it
needs to be sanitized in some way, for example quoted:
"A U. Thor" <author@example.com>
To fix, make send-email sanitize both sender and the address it is
compared against.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 git-send-email.perl | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index a138615..92df393 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -760,6 +760,8 @@ if (!defined $sender) {
 	$sender = $repoauthor || $repocommitter || '';
 }
 
+$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)? ",
@@ -1113,10 +1115,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
@@ -1133,7 +1134,7 @@ X-Mailer: git-send-email $gitversion
 	}
 
 	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;
 	}
@@ -1308,8 +1309,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;
@@ -1323,7 +1325,9 @@ foreach my $t (@files) {
 			}
 			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'});
@@ -1370,7 +1374,8 @@ foreach my $t (@files) {
 			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;
@@ -1454,7 +1459,6 @@ foreach my $t (@files) {
 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'";
-- 
MST

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 5/6] t/send-email: add test with quoted sender
  2013-05-26 14:40 [PATCH 0/6] git send-email suppress-cc=self fixes Michael S. Tsirkin
                   ` (3 preceding siblings ...)
  2013-05-26 14:41 ` [PATCH 4/6] send-email: make --suppress-cc=self sanitize input Michael S. Tsirkin
@ 2013-05-26 14:41 ` Michael S. Tsirkin
  2013-05-26 14:41 ` [PATCH 6/6] t/send-email: test suppress-cc=self with non-ascii Michael S. Tsirkin
  5 siblings, 0 replies; 12+ messages in thread
From: Michael S. Tsirkin @ 2013-05-26 14:41 UTC (permalink / raw)
  To: git

add test where sender address needs to be quoted.
Make sure --suppress-cc=self works well in this case.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 t/t9001-send-email.sh | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 8aa55f8..0ab4056 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -228,6 +228,10 @@ test_expect_success $PREREQ 'self name is suppressed' "
 	test_suppress_self_unquoted 'A U Thor' 'author@redhat.com' 'self_name_suppressed'
 "
 
+test_expect_success $PREREQ 'quoted self name is suppressed' "
+	test_suppress_self_quoted 'A U. Thor' 'author@redhat.com' 'self_name_suppressed'
+"
+
 test_expect_success $PREREQ 'Show all headers' '
 	git send-email \
 		--dry-run \
-- 
MST

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 6/6] t/send-email: test suppress-cc=self with non-ascii
  2013-05-26 14:40 [PATCH 0/6] git send-email suppress-cc=self fixes Michael S. Tsirkin
                   ` (4 preceding siblings ...)
  2013-05-26 14:41 ` [PATCH 5/6] t/send-email: add test with quoted sender Michael S. Tsirkin
@ 2013-05-26 14:41 ` Michael S. Tsirkin
  5 siblings, 0 replies; 12+ messages in thread
From: Michael S. Tsirkin @ 2013-05-26 14:41 UTC (permalink / raw)
  To: git

test suppress-cc=self when sender is non-acsii

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 t/t9001-send-email.sh | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 0ab4056..66ebb1e 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -232,6 +232,10 @@ test_expect_success $PREREQ 'quoted self name is suppressed' "
 	test_suppress_self_quoted 'A U. Thor' 'author@redhat.com' 'self_name_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 'Show all headers' '
 	git send-email \
 		--dry-run \
-- 
MST

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/6] t/send-email.sh: add test for suppress self
  2013-05-26 14:40 ` [PATCH 1/6] t/send-email.sh: add test for suppress self Michael S. Tsirkin
@ 2013-05-29 20:28   ` Junio C Hamano
  2013-05-29 21:44     ` Michael S. Tsirkin
  0 siblings, 1 reply; 12+ messages in thread
From: Junio C Hamano @ 2013-05-29 20:28 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: git

"Michael S. Tsirkin" <mst@redhat.com> writes:

> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---

Thanks.

>  t/t9001-send-email.sh | 41 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>
> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
> index ebd5c5d..36ecf73 100755
> --- a/t/t9001-send-email.sh
> +++ b/t/t9001-send-email.sh
> @@ -171,6 +171,47 @@ Result: OK
>  EOF
>  "
>  
> +test_suppress_self () {
> +		test_commit $3 &&
> +		test_when_finished "git reset --hard HEAD^" &&
> +		{
> +			echo "#!$SHELL_PATH"
> +			echo sed -n -e s/^cccmd--//p \"\$1\"
> +		} > cccmd-sed &&
> +		chmod +x cccmd-sed &&

We can use write_script for this kind of thing, I think.

> +		git commit --amend --author="$1 <$2>" -F - << EOF && \

Hmm,...  everything below this function is fed as the standard input
to "git commit" as its updated log message (i.e. "--amend -F -")?

Puzzled...

The EOF I can find is at the very bottom of this function, so there
is no "next command" that && at the end of the above line is
cascading the control to.

Doubly puzzled...

In any case, please do not add " \" at the end of line when the line
ends one command and "&&" at the end of line clearly tells the shell
that you haven't stopped talking yet.

> +		clean_fake_sendmail && \
> +		echo suppress-self-$3.patch > /dev/tty && \

Do we always have /dev/tty?  If this is a leftover debugging, please
remove it.  If redirecting it to >&2 does not upset what the test
does, that is good, too (you can run the test with -v option to view
the output).

> +		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 && \

Style.  No SP between redirection operator and redirection target,
e.g. >$filename, <$filename, etc.  Some in this patch is done
correctly (e.g. format-patch above), some others are not.

> +		rm -f expected-no-cc-$3 && \
> +		touch expected-no-cc-$3 && \

Please reserve "touch" for "make sure it has recent timestamp", not
for "make sure it exists and is empty".  The above two should be
more like:

		>"expected-no-cc-$3" &&

Also, even though it is not required by POSIX, please dquote the
redirection target filename if you have variable expansion.  Some
versions of bash (incorrectly) give warning if you don't.

> +		grep '^Cc:' msghdr1-$3 > actual-no-cc-$3 && \
> +		test_cmp expected-no-cc-$3 actual-no-cc-$3
> +test suppress-cc.self $3 with name $1 email $2
> +
> +$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 'A U Thor' 'author@redhat.com' 'self_name_suppressed'
> +"
> +
>  test_expect_success $PREREQ 'Show all headers' '
>  	git send-email \
>  		--dry-run \

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/6] t/send-email.sh: add test for suppress self
  2013-05-29 20:28   ` Junio C Hamano
@ 2013-05-29 21:44     ` Michael S. Tsirkin
  2013-05-29 22:46       ` Junio C Hamano
  0 siblings, 1 reply; 12+ messages in thread
From: Michael S. Tsirkin @ 2013-05-29 21:44 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

On Wed, May 29, 2013 at 01:28:52PM -0700, Junio C Hamano wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> 
> Thanks.

Thanks, I'll address your comments and repost.
You asked some questions below, so I tried to answer.

> >  t/t9001-send-email.sh | 41 +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 41 insertions(+)
> >
> > diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
> > index ebd5c5d..36ecf73 100755
> > --- a/t/t9001-send-email.sh
> > +++ b/t/t9001-send-email.sh
> > @@ -171,6 +171,47 @@ Result: OK
> >  EOF
> >  "
> >  
> > +test_suppress_self () {
> > +		test_commit $3 &&
> > +		test_when_finished "git reset --hard HEAD^" &&
> > +		{
> > +			echo "#!$SHELL_PATH"
> > +			echo sed -n -e s/^cccmd--//p \"\$1\"
> > +		} > cccmd-sed &&
> > +		chmod +x cccmd-sed &&
> 
> We can use write_script for this kind of thing, I think.

Important?
It's open-coded elsewhere in this test.

> > +		git commit --amend --author="$1 <$2>" -F - << EOF && \
> 
> Hmm,...  everything below this function is fed as the standard input
> to "git commit" as its updated log message (i.e. "--amend -F -")?
> 
> Puzzled...
> The EOF I can find is at the very bottom of this function, so there
> is no "next command" that && at the end of the above line is
> cascading the control to.
> 
> Doubly puzzled...
> 
> In any case, please do not add " \" at the end of line when the line
> ends one command and "&&" at the end of line clearly tells the shell
> that you haven't stopped talking yet.


Note that \ make following lines count as continuation of this one.
So they are *not* part of standard input.

Look here:

FOO << EOF && BAR && VAR
BLABLA
EOF

this feeds BLABLA as input to FOO, then
runs BAR and then VAR.

Now we don't want to put BAR and VAR on same line
because that line is too long.
So we write the equivalent: \ followed by newline
is same as space for shell, so we can write it as:

FOO << EOF && \
 BAR && \
 VAR
BLABLA
EOF

Clear now?

If we don't want to use \, this can also
be done like this:

FOO << EOF && 
BLABLA
EOF
 BAR && 
 VAR

I think this is what you suggest.


> 
> > +		clean_fake_sendmail && \
> > +		echo suppress-self-$3.patch > /dev/tty && \
> 
> Do we always have /dev/tty?  If this is a leftover debugging, please
> remove it.

Leftover.

>  If redirecting it to >&2 does not upset what the test
> does, that is good, too (you can run the test with -v option to view
> the output).
> 
> > +		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 && \
> 
> Style.  No SP between redirection operator and redirection target,
> e.g. >$filename, <$filename, etc.  Some in this patch is done
> correctly (e.g. format-patch above), some others are not.

will fix.

> > +		rm -f expected-no-cc-$3 && \
> > +		touch expected-no-cc-$3 && \
> 
> Please reserve "touch" for "make sure it has recent timestamp", not
> for "make sure it exists and is empty".  The above two should be
> more like:
> 
> 		>"expected-no-cc-$3" &&

OK

> Also, even though it is not required by POSIX, please dquote the
> redirection target filename if you have variable expansion.  Some
> versions of bash (incorrectly) give warning if you don't.

OK

> > +		grep '^Cc:' msghdr1-$3 > actual-no-cc-$3 && \
> > +		test_cmp expected-no-cc-$3 actual-no-cc-$3
> > +test suppress-cc.self $3 with name $1 email $2
> > +
> > +$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 'A U Thor' 'author@redhat.com' 'self_name_suppressed'
> > +"
> > +
> >  test_expect_success $PREREQ 'Show all headers' '
> >  	git send-email \
> >  		--dry-run \

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/6] t/send-email.sh: add test for suppress self
  2013-05-29 21:44     ` Michael S. Tsirkin
@ 2013-05-29 22:46       ` Junio C Hamano
  2013-05-29 22:59         ` Junio C Hamano
  0 siblings, 1 reply; 12+ messages in thread
From: Junio C Hamano @ 2013-05-29 22:46 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: git

"Michael S. Tsirkin" <mst@redhat.com> writes:

>> > +test_suppress_self () {
>> > +		test_commit $3 &&
>> > +		test_when_finished "git reset --hard HEAD^" &&
>> > +		{
>> > +			echo "#!$SHELL_PATH"
>> > +			echo sed -n -e s/^cccmd--//p \"\$1\"
>> > +		} > cccmd-sed &&
>> > +		chmod +x cccmd-sed &&
>> 
>> We can use write_script for this kind of thing, I think.
>
> Important?
> It's open-coded elsewhere in this test.

Not that important, other than that it is nice not to spread what
the old tests did before write_script was introduced to new ones, as
that is one more thing to update when somebody feels like it.

> If we don't want to use \, this can also be done like this:
>
> FOO << EOF && 
> BLABLA
> EOF
>  BAR && 
>  VAR
>
> I think this is what you suggest.

Yup, that is exactly what I meant (but no leading indentation before
BAR and VAR).

That way, it is a lot more clear where the input is (the BLABLA is
fed to FOO and BAR and VAR do not have anything to do with it).

>> > +		grep '^Cc:' msghdr1-$3 > actual-no-cc-$3 && \
>> > +		test_cmp expected-no-cc-$3 actual-no-cc-$3

OK, so this is where the message begins, with the commit title "test
supress-cc.self...".

>> > +test suppress-cc.self $3 with name $1 email $2
>> > +
>> > +$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 'A U Thor' 'author@redhat.com' 'self_name_suppressed'
>> > +"
>> > +
>> >  test_expect_success $PREREQ 'Show all headers' '
>> >  	git send-email \
>> >  		--dry-run \

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/6] t/send-email.sh: add test for suppress self
  2013-05-29 22:46       ` Junio C Hamano
@ 2013-05-29 22:59         ` Junio C Hamano
  2013-05-30  4:51           ` Michael S. Tsirkin
  0 siblings, 1 reply; 12+ messages in thread
From: Junio C Hamano @ 2013-05-29 22:59 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: git

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

>> If we don't want to use \, this can also be done like this:
>>
>> FOO << EOF && 
>> BLABLA
>> EOF
>>  BAR && 
>>  VAR
>>
>> I think this is what you suggest.
>
> Yup, that is exactly what I meant (but no leading indentation before
> BAR and VAR).
>
> That way, it is a lot more clear where the input is (the BLABLA is
> fed to FOO and BAR and VAR do not have anything to do with it).
>
>>> > +		grep '^Cc:' msghdr1-$3 > actual-no-cc-$3 && \
>>> > +		test_cmp expected-no-cc-$3 actual-no-cc-$3
>
> OK, so this is where the message begins, with the commit title "test
> supress-cc.self...".

Another thing I forgot to say, if you are rerolling this patch
anyway to follow that style, is that our newer tests typically
write it like this:

test_supress_self () {
	test_commit $3 &&
        test_when_finished "git reset --hard HEAD^" &&
        write_script <<-EOF &&
        sed -n -e s/^cccmd--//p \"\$1\"
	EOF

	git commit --amend --author="$1 <$2>" -F - <<-EOF &&
	test suppress-cc.self $3 with name $1 email $2

	$3

	cccmd--"$1" <$2>

        Cc: "$1" <$2>
	Cc: $1 <$2>
	Signed-off-by: "$1" <$2>
	Signed-off-by: $1 <$2>
        EOF
        
        clean_fake_sendmail &&
        git format-patch --stdout -1 >"suppress-self-$3.patch" &&
        git send-email --from="$1 <$2>" \
        	--to=nobody@example.com \
                ... other args ...
        ... verification steps for the send-email output ...
}

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/6] t/send-email.sh: add test for suppress self
  2013-05-29 22:59         ` Junio C Hamano
@ 2013-05-30  4:51           ` Michael S. Tsirkin
  0 siblings, 0 replies; 12+ messages in thread
From: Michael S. Tsirkin @ 2013-05-30  4:51 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

On Wed, May 29, 2013 at 03:59:51PM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> >> If we don't want to use \, this can also be done like this:
> >>
> >> FOO << EOF && 
> >> BLABLA
> >> EOF
> >>  BAR && 
> >>  VAR
> >>
> >> I think this is what you suggest.
> >
> > Yup, that is exactly what I meant (but no leading indentation before
> > BAR and VAR).
> >
> > That way, it is a lot more clear where the input is (the BLABLA is
> > fed to FOO and BAR and VAR do not have anything to do with it).
> >
> >>> > +		grep '^Cc:' msghdr1-$3 > actual-no-cc-$3 && \
> >>> > +		test_cmp expected-no-cc-$3 actual-no-cc-$3
> >
> > OK, so this is where the message begins, with the commit title "test
> > supress-cc.self...".
> 
> Another thing I forgot to say, if you are rerolling this patch
> anyway to follow that style, is that our newer tests typically
> write it like this:

What exactly should I notice here?

> test_supress_self () {
> 	test_commit $3 &&
>         test_when_finished "git reset --hard HEAD^" &&
>         write_script <<-EOF &&
>         sed -n -e s/^cccmd--//p \"\$1\"
> 	EOF
> 
> 	git commit --amend --author="$1 <$2>" -F - <<-EOF &&
> 	test suppress-cc.self $3 with name $1 email $2
> 
> 	$3
> 
> 	cccmd--"$1" <$2>
> 
>         Cc: "$1" <$2>
> 	Cc: $1 <$2>
> 	Signed-off-by: "$1" <$2>
> 	Signed-off-by: $1 <$2>
>         EOF
>         
>         clean_fake_sendmail &&
>         git format-patch --stdout -1 >"suppress-self-$3.patch" &&
>         git send-email --from="$1 <$2>" \
>         	--to=nobody@example.com \
>                 ... other args ...
>         ... verification steps for the send-email output ...
> }

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2013-05-30  4:51 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-05-26 14:40 [PATCH 0/6] git send-email suppress-cc=self fixes Michael S. Tsirkin
2013-05-26 14:40 ` [PATCH 1/6] t/send-email.sh: add test for suppress self Michael S. Tsirkin
2013-05-29 20:28   ` Junio C Hamano
2013-05-29 21:44     ` Michael S. Tsirkin
2013-05-29 22:46       ` Junio C Hamano
2013-05-29 22:59         ` Junio C Hamano
2013-05-30  4:51           ` Michael S. Tsirkin
2013-05-26 14:40 ` [PATCH 2/6] send-email: fix suppress-cc=self on cccmd Michael S. Tsirkin
2013-05-26 14:41 ` [PATCH 3/6] t/send-email: test " Michael S. Tsirkin
2013-05-26 14:41 ` [PATCH 4/6] send-email: make --suppress-cc=self sanitize input Michael S. Tsirkin
2013-05-26 14:41 ` [PATCH 5/6] t/send-email: add test with quoted sender Michael S. Tsirkin
2013-05-26 14:41 ` [PATCH 6/6] t/send-email: test suppress-cc=self with non-ascii Michael S. Tsirkin

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).