Re: [PATCH 4/5] test: improve rebase -q test

2013-05-29 Thread Felipe Contreras
On Wed, May 29, 2013 at 11:52 AM, Junio C Hamano  wrote:
> Felipe Contreras  writes:
>
>> Junio C Hamano wrote:
>>> Felipe Contreras  writes:
>>>
>>> > Let's show the output so it's clear why it failed.
>>> >
>>> > Signed-off-by: Felipe Contreras 
>>> > ---
>>> >  t/t3400-rebase.sh | 1 +
>>> >  1 file changed, 1 insertion(+)
>>> >
>>> > diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
>>> > index b58fa1a..fb39531 100755
>>> > --- a/t/t3400-rebase.sh
>>> > +++ b/t/t3400-rebase.sh
>>> > @@ -185,6 +185,7 @@ test_expect_success 'default to @{upstream} when 
>>> > upstream arg is missing' '
>>> >  test_expect_success 'rebase -q is quiet' '
>>> >git checkout -b quiet topic &&
>>> >git rebase -q master >output.out 2>&1 &&
>>> > +  cat output.out &&
>>> >test ! -s output.out
>>> >  '
>>>
>>> It is one thing to avoid squelching output that naturally comes out
>>> of command being tested unnecessarily, so that "./t-*.sh -v"
>>> output can be used for debugging.  I however am not sure if adding
>>> "cat" to random places like this is a productive direction for us to
>>> go in.
>>>
>>> A more preferrable alternative may be adding something like this to
>>> test-lib.sh and call it from here and elsewhere (there are about 50
>>> places that do "test ! -s "), perhaps?
>>>
>>> test_must_be_an_empty_file () {
>>> if test -s "$1"
>>> then
>>> cat "$1"
>>> false
>>> fi
>>> }
>>
>> Perhaps, but I'm not interested. I'm tired of obvious fixes getting rejected
>> for hypothetical "ideal" situations that we'll never reach.
>
> That's too bad.  Addition of "cat" where there does not need one is
> clearly not an obvious fix anyway.

If you are an actual real user of this code; a developer that is
running the test; and the test finally achieves it's designed goal of
detecting a failure, you would be left scratching your head wondering
what's the problem if running './test -v' doesn't show anything, even
after you have added debugging code to narrow down the issue.

I had to add that cat line not once, but more than two times in
different lines of development.

So yeah, a cat is needed, and the fact you don't see that amazes me,
specially after you have reprimanded me for using 'grep -q' instead of
'grep' for this very reason.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 4/5] test: improve rebase -q test

2013-05-29 Thread Junio C Hamano
Felipe Contreras  writes:

> Junio C Hamano wrote:
>> Felipe Contreras  writes:
>> 
>> > Let's show the output so it's clear why it failed.
>> >
>> > Signed-off-by: Felipe Contreras 
>> > ---
>> >  t/t3400-rebase.sh | 1 +
>> >  1 file changed, 1 insertion(+)
>> >
>> > diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
>> > index b58fa1a..fb39531 100755
>> > --- a/t/t3400-rebase.sh
>> > +++ b/t/t3400-rebase.sh
>> > @@ -185,6 +185,7 @@ test_expect_success 'default to @{upstream} when 
>> > upstream arg is missing' '
>> >  test_expect_success 'rebase -q is quiet' '
>> >git checkout -b quiet topic &&
>> >git rebase -q master >output.out 2>&1 &&
>> > +  cat output.out &&
>> >test ! -s output.out
>> >  '
>> 
>> It is one thing to avoid squelching output that naturally comes out
>> of command being tested unnecessarily, so that "./t-*.sh -v"
>> output can be used for debugging.  I however am not sure if adding
>> "cat" to random places like this is a productive direction for us to
>> go in.
>> 
>> A more preferrable alternative may be adding something like this to
>> test-lib.sh and call it from here and elsewhere (there are about 50
>> places that do "test ! -s "), perhaps?
>> 
>> test_must_be_an_empty_file () {
>> if test -s "$1"
>> then
>> cat "$1"
>> false
>> fi
>> }
>
> Perhaps, but I'm not interested. I'm tired of obvious fixes getting rejected
> for hypothetical "ideal" situations that we'll never reach.

That's too bad.  Addition of "cat" where there does not need one is
clearly not an obvious fix anyway.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 4/5] test: improve rebase -q test

2013-05-28 Thread Felipe Contreras
Junio C Hamano wrote:
> Felipe Contreras  writes:
> 
> > Let's show the output so it's clear why it failed.
> >
> > Signed-off-by: Felipe Contreras 
> > ---
> >  t/t3400-rebase.sh | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
> > index b58fa1a..fb39531 100755
> > --- a/t/t3400-rebase.sh
> > +++ b/t/t3400-rebase.sh
> > @@ -185,6 +185,7 @@ test_expect_success 'default to @{upstream} when 
> > upstream arg is missing' '
> >  test_expect_success 'rebase -q is quiet' '
> > git checkout -b quiet topic &&
> > git rebase -q master >output.out 2>&1 &&
> > +   cat output.out &&
> > test ! -s output.out
> >  '
> 
> It is one thing to avoid squelching output that naturally comes out
> of command being tested unnecessarily, so that "./t-*.sh -v"
> output can be used for debugging.  I however am not sure if adding
> "cat" to random places like this is a productive direction for us to
> go in.
> 
> A more preferrable alternative may be adding something like this to
> test-lib.sh and call it from here and elsewhere (there are about 50
> places that do "test ! -s "), perhaps?
> 
> test_must_be_an_empty_file () {
> if test -s "$1"
> then
> cat "$1"
> false
> fi
> }

Perhaps, but I'm not interested. I'm tired of obvious fixes getting rejected
for hypothetical "ideal" situations that we'll never reach.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 4/5] test: improve rebase -q test

2013-05-28 Thread Junio C Hamano
Jonathan Nieder  writes:

> Junio C Hamano wrote:
>
>> A more preferrable alternative may be adding something like this to
>> test-lib.sh and call it from here and elsewhere (there are about 50
>> places that do "test ! -s "), perhaps?
>>
>> test_must_be_an_empty_file () {
>> if test -s "$1"
>> then
>> cat "$1"
>> false
>> fi
>> }
>
> I generally just use the two-liner
>
>   >empty &&
>   test_cmp empty output
>
> directly in cases like this.

That would work, too.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 4/5] test: improve rebase -q test

2013-05-28 Thread Jonathan Nieder
Junio C Hamano wrote:

> A more preferrable alternative may be adding something like this to
> test-lib.sh and call it from here and elsewhere (there are about 50
> places that do "test ! -s "), perhaps?
>
> test_must_be_an_empty_file () {
> if test -s "$1"
> then
> cat "$1"
> false
> fi
> }

I generally just use the two-liner

>empty &&
test_cmp empty output

directly in cases like this.

Thanks,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 4/5] test: improve rebase -q test

2013-05-28 Thread Junio C Hamano
Felipe Contreras  writes:

> Let's show the output so it's clear why it failed.
>
> Signed-off-by: Felipe Contreras 
> ---
>  t/t3400-rebase.sh | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
> index b58fa1a..fb39531 100755
> --- a/t/t3400-rebase.sh
> +++ b/t/t3400-rebase.sh
> @@ -185,6 +185,7 @@ test_expect_success 'default to @{upstream} when upstream 
> arg is missing' '
>  test_expect_success 'rebase -q is quiet' '
>   git checkout -b quiet topic &&
>   git rebase -q master >output.out 2>&1 &&
> + cat output.out &&
>   test ! -s output.out
>  '

It is one thing to avoid squelching output that naturally comes out
of command being tested unnecessarily, so that "./t-*.sh -v"
output can be used for debugging.  I however am not sure if adding
"cat" to random places like this is a productive direction for us to
go in.

A more preferrable alternative may be adding something like this to
test-lib.sh and call it from here and elsewhere (there are about 50
places that do "test ! -s "), perhaps?

test_must_be_an_empty_file () {
if test -s "$1"
then
cat "$1"
false
fi
}

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 4/5] test: improve rebase -q test

2013-05-28 Thread Felipe Contreras
Let's show the output so it's clear why it failed.

Signed-off-by: Felipe Contreras 
---
 t/t3400-rebase.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index b58fa1a..fb39531 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -185,6 +185,7 @@ test_expect_success 'default to @{upstream} when upstream 
arg is missing' '
 test_expect_success 'rebase -q is quiet' '
git checkout -b quiet topic &&
git rebase -q master >output.out 2>&1 &&
+   cat output.out &&
test ! -s output.out
 '
 
-- 
1.8.3.rc3.312.g47657de

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html