Re: [PATCH 1/6] t/send-email.sh: add test for suppress self

From: Junio C Hamano <gitster@pobox.com>
Date: May 29, 2013, 20:28 UTC
Message-ID: <7vy5axr0mz.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1369579187-27573-2-git-send-email-mst@redhat.com>

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

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

Thanks.

Show 19 quoted lines

>  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).

Show 9 quoted lines

> +		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.

Show 22 quoted lines

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