no-xmailer tests fail under Mac OS

no-xmailer tests fail under Mac OS

[Michael Blume]

Failures start from

commit d2384abff7a6181fd7b9a51af7e780aa21e5cb8d (refs/bisect/bad)
Author: Luis Henriques <henrix@camandro.org>
Date:   Thu Dec 4 19:11:30 2014 +0000

    test/send-email: --[no-]xmailer tests

    Add tests for the --[no-]xmailer option.

    Signed-off-by: Luis Henriques <henrix@camandro.org>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

but continue with Junio's SQUASH??? commit at b728d078

Verbose output follows

expecting success:
do_xmailer_test 1 "--xmailer" &&
do_xmailer_test 0 "--no-xmailer"

0001-add-master.patch
not ok 109 - --[no-]xmailer without any configuration
#
# do_xmailer_test 1 "--xmailer" &&
# do_xmailer_test 0 "--no-xmailer"
#

expecting success:
test_config sendemail.xmailer true &&
do_xmailer_test 1 "" &&
do_xmailer_test 0 "--no-xmailer" &&
do_xmailer_test 1 "--xmailer"

0001-add-master.patch
not ok 110 - --[no-]xmailer with sendemail.xmailer=true
#
# test_config sendemail.xmailer true &&
# do_xmailer_test 1 "" &&
# do_xmailer_test 0 "--no-xmailer" &&
# do_xmailer_test 1 "--xmailer"
#

expecting success:
test_config sendemail.xmailer false &&
do_xmailer_test 0 "" &&
do_xmailer_test 0 "--no-xmailer" &&
do_xmailer_test 1 "--xmailer"

0001-add-master.patch
not ok 111 - --[no-]xmailer with sendemail.xmailer=false
#
# test_config sendemail.xmailer false &&
# do_xmailer_test 0 "" &&
# do_xmailer_test 0 "--no-xmailer" &&
# do_xmailer_test 1 "--xmailer"
#

# failed 3 among 111 test(s)
1..111

Re: no-xmailer tests fail under Mac OS

[Jeff King]

On Fri, Dec 05, 2014 at 06:05:24PM -0800, Michael Blume wrote:

> Failures start from
> 
> commit d2384abff7a6181fd7b9a51af7e780aa21e5cb8d (refs/bisect/bad)
> Author: Luis Henriques <henrix@camandro.org>
> Date:   Thu Dec 4 19:11:30 2014 +0000
> 
>     test/send-email: --[no-]xmailer tests
> 
>     Add tests for the --[no-]xmailer option.
> 
>     Signed-off-by: Luis Henriques <henrix@camandro.org>
>     Signed-off-by: Junio C Hamano <gitster@pobox.com>
> 
> but continue with Junio's SQUASH??? commit at b728d078

The commit contains:

  +       test "z$(grep ^X-Mailer: out | wc -l)" = "z$expected"

We have had trouble in the past with "wc -l" output not being strictly
portable. I do not recall offhand which systems, but it is a good bet
that this is the culprit. Doing:

  grep ^X-Mailer: out >mailer &&
  test_line_count = $expected mailer

should fix it. It might be even nicer to actually compare the x-mailer
line we find to an expected output, but that may introduce complications
if the value changes with the version or something (you'd have to
sanitize the output, and then I do not know that the test is really
buying much over just seeing whether it exists).

-Peff

Re: no-xmailer tests fail under Mac OS

[Michael Blume]

On Fri, Dec 5, 2014 at 9:34 PM, Jeff King <peff@peff.net> wrote:
> On Fri, Dec 05, 2014 at 06:05:24PM -0800, Michael Blume wrote:
>
>> Failures start from
>>
>> commit d2384abff7a6181fd7b9a51af7e780aa21e5cb8d (refs/bisect/bad)
>> Author: Luis Henriques <henrix@camandro.org>
>> Date:   Thu Dec 4 19:11:30 2014 +0000
>>
>>     test/send-email: --[no-]xmailer tests
>>
>>     Add tests for the --[no-]xmailer option.
>>
>>     Signed-off-by: Luis Henriques <henrix@camandro.org>
>>     Signed-off-by: Junio C Hamano <gitster@pobox.com>
>>
>> but continue with Junio's SQUASH??? commit at b728d078
>
> The commit contains:
>
>   +       test "z$(grep ^X-Mailer: out | wc -l)" = "z$expected"
>
> We have had trouble in the past with "wc -l" output not being strictly
> portable. I do not recall offhand which systems, but it is a good bet
> that this is the culprit. Doing:
>
>   grep ^X-Mailer: out >mailer &&
>   test_line_count = $expected mailer
>
> should fix it. It might be even nicer to actually compare the x-mailer
> line we find to an expected output, but that may introduce complications
> if the value changes with the version or something (you'd have to
> sanitize the output, and then I do not know that the test is really
> buying much over just seeing whether it exists).
>
> -Peff

Actually need to drop the '&&', but yes, that works perfectly, thanks =)

Re: no-xmailer tests fail under Mac OS

[Jeff King]

On Fri, Dec 05, 2014 at 10:27:40PM -0800, Michael Blume wrote:

> > We have had trouble in the past with "wc -l" output not being strictly
> > portable. I do not recall offhand which systems, but it is a good bet
> > that this is the culprit. Doing:
> >
> >   grep ^X-Mailer: out >mailer &&
> >   test_line_count = $expected mailer
> >
> > should fix it. It might be even nicer to actually compare the x-mailer
> > line we find to an expected output, but that may introduce complications
> > if the value changes with the version or something (you'd have to
> > sanitize the output, and then I do not know that the test is really
> > buying much over just seeing whether it exists).
> >
> > -Peff
> 
> Actually need to drop the '&&', but yes, that works perfectly, thanks =)

Ah, right, we might be looking for 0 sometimes. The right way to do it
without destroying the &&-chaining is:

  { grep ^X-Mailer: out || true } &&
  test_line_count = $expected mailer

-Peff

Re: no-xmailer tests fail under Mac OS

[Jeff King]

On Fri, Dec 05, 2014 at 11:07:37PM -0800, Michael Blume wrote:

> > Ah, right, we might be looking for 0 sometimes. The right way to do it
> > without destroying the &&-chaining is:
> >
> >   { grep ^X-Mailer: out || true } &&
> >   test_line_count = $expected mailer
> 
> Hmm, it doesn't look like that helper is &&-chained though? So it
> seems like we could just do without the &&

You're right, but that is IMHO a bug. We would not notice if send-email
or format-patch barfed, and we are expecting to find no X-Mailer (we
wouldn't, but for the wrong reason).

It should also be using test_config in the last two tests.

-Peff

Re: no-xmailer tests fail under Mac OS

[Junio C Hamano]

Michael Blume <blume.mike@gmail.com> writes:

> Failures start from
>
> commit d2384abff7a6181fd7b9a51af7e780aa21e5cb8d (refs/bisect/bad)
> Author: Luis Henriques <henrix@camandro.org>
> Date:   Thu Dec 4 19:11:30 2014 +0000
>
>     test/send-email: --[no-]xmailer tests
>
>     Add tests for the --[no-]xmailer option.
>
>     Signed-off-by: Luis Henriques <henrix@camandro.org>
>     Signed-off-by: Junio C Hamano <gitster@pobox.com>
>
> but continue with Junio's SQUASH??? commit at b728d078

I missed this bit in the patch you pointed out:

+       test "z$(grep ^X-Mailer: out | wc -l)" = "z$expected"

which depends on "wc -l" not to add any extra whitespace around its
count.  We know that some implementations do, and we should be using

	test $(grep xxx | wc -l) -eq $expected

or something, or test_line_count.

Thanks for noticing.

Re: no-xmailer tests fail under Mac OS

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Dec 05, 2014 at 11:07:37PM -0800, Michael Blume wrote:
>
>> > Ah, right, we might be looking for 0 sometimes. The right way to do it
>> > without destroying the &&-chaining is:
>> >
>> >   { grep ^X-Mailer: out || true } &&
>> >   test_line_count = $expected mailer
>> 
>> Hmm, it doesn't look like that helper is &&-chained though? So it
>> seems like we could just do without the &&
>
> You're right, but that is IMHO a bug. We would not notice if send-email
> or format-patch barfed, and we are expecting to find no X-Mailer (we
> wouldn't, but for the wrong reason).

Let me patch this up further by amending the SQUASH??? at the tip.

 t/t9001-send-email.sh | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index bb573ef..7826aa8 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -1408,18 +1408,17 @@ test_expect_success $PREREQ 'sendemail.aliasfile=~/.mailrc' '
 '
 
 do_xmailer_test() {
-	expected=$1
-	params=$2
-	git format-patch -1
+	expected=$1 params=$2 &&
+	git format-patch -1 &&
 	git send-email \
 		--from="Example <nobody@example.com>" \
 		--to=someone@example.com \
 		--smtp-server="$(pwd)/fake.sendmail" \
 		$params \
 		0001-*.patch \
-		2>errors >out
-	test "z$(grep ^X-Mailer: out | wc -l)" = "z$expected"
-	return $?
+		2>errors >out &&
+	{ grep '^X-Mailer:' out || :; } >mailer &&
+	test_line_count = $expected mailer
 }
 
 test_expect_success $PREREQ '--[no-]xmailer without any configuration' '

Re: no-xmailer tests fail under Mac OS

[Jeff King]

On Thu, Dec 11, 2014 at 02:11:04PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Fri, Dec 05, 2014 at 11:07:37PM -0800, Michael Blume wrote:
> >
> >> > Ah, right, we might be looking for 0 sometimes. The right way to do it
> >> > without destroying the &&-chaining is:
> >> >
> >> >   { grep ^X-Mailer: out || true } &&
> >> >   test_line_count = $expected mailer
> >> 
> >> Hmm, it doesn't look like that helper is &&-chained though? So it
> >> seems like we could just do without the &&
> >
> > You're right, but that is IMHO a bug. We would not notice if send-email
> > or format-patch barfed, and we are expecting to find no X-Mailer (we
> > wouldn't, but for the wrong reason).
> 
> Let me patch this up further by amending the SQUASH??? at the tip.
> 
>  t/t9001-send-email.sh | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> [...]

Yeah, looks good to me.

-Peff

Re: no-xmailer tests fail under Mac OS

[Luis Henriques]

Jeff King <peff@peff.net> writes:

> On Thu, Dec 11, 2014 at 02:11:04PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > On Fri, Dec 05, 2014 at 11:07:37PM -0800, Michael Blume wrote:
>> >
>> >> > Ah, right, we might be looking for 0 sometimes. The right way to do it
>> >> > without destroying the &&-chaining is:
>> >> >
>> >> >   { grep ^X-Mailer: out || true } &&
>> >> >   test_line_count = $expected mailer
>> >> 
>> >> Hmm, it doesn't look like that helper is &&-chained though? So it
>> >> seems like we could just do without the &&
>> >
>> > You're right, but that is IMHO a bug. We would not notice if send-email
>> > or format-patch barfed, and we are expecting to find no X-Mailer (we
>> > wouldn't, but for the wrong reason).
>> 
>> Let me patch this up further by amending the SQUASH??? at the tip.
>> 
>>  t/t9001-send-email.sh | 11 +++++------
>>  1 file changed, 5 insertions(+), 6 deletions(-)
>> [...]
>
> Yeah, looks good to me.
>
> -Peff

Same here.  Thanks a lot for fixing this.

Cheers,
--
Luís