[PATCH] t1503: test rev-parse --verify --quiet with deleted reflogs

[PATCH] t1503: test rev-parse --verify --quiet with deleted reflogs

[David Aguilar]

Ensure that rev-parse --verify --quiet is silent when asked
about deleted reflog entries.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
This verifies and depends on "refs: make rev-parse --quiet actually quiet".

 t/t1503-rev-parse-verify.sh | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/t/t1503-rev-parse-verify.sh b/t/t1503-rev-parse-verify.sh
index 813cc1b..731c21c 100755
--- a/t/t1503-rev-parse-verify.sh
+++ b/t/t1503-rev-parse-verify.sh
@@ -83,6 +83,15 @@ test_expect_success 'fails silently when using -q' '
 	test -z "$(cat error)"
 '
 
+test_expect_success 'fails silently when using -q with deleted reflogs' '
+	ref=$(git rev-parse HEAD) &&
+	: >.git/logs/refs/test &&
+	git update-ref -m test refs/test "$ref" &&
+	git reflog delete --updateref --rewrite refs/test@{0} &&
+	test_must_fail git rev-parse --verify --quiet refs/test@{0} 2>error &&
+	test -z "$(cat error)"
+'
+
 test_expect_success 'no stdout output on error' '
 	test -z "$(git rev-parse --verify)" &&
 	test -z "$(git rev-parse --verify foo)" &&
-- 
2.1.0.29.gf6d9003.dirty

Re: [PATCH] t1503: test rev-parse --verify --quiet with deleted reflogs

[Fabian Ruch]

Hi David,

On 09/14/2014 10:30 AM, David Aguilar wrote:
> Ensure that rev-parse --verify --quiet is silent when asked
> about deleted reflog entries.
> 
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
> This verifies and depends on "refs: make rev-parse --quiet actually quiet".
> 
>  t/t1503-rev-parse-verify.sh | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/t/t1503-rev-parse-verify.sh b/t/t1503-rev-parse-verify.sh
> index 813cc1b..731c21c 100755
> --- a/t/t1503-rev-parse-verify.sh
> +++ b/t/t1503-rev-parse-verify.sh
> @@ -83,6 +83,15 @@ test_expect_success 'fails silently when using -q' '
>  	test -z "$(cat error)"
>  '
>  
> +test_expect_success 'fails silently when using -q with deleted reflogs' '
> +	ref=$(git rev-parse HEAD) &&
> +	: >.git/logs/refs/test &&
> +	git update-ref -m test refs/test "$ref" &&

I'm just curious, why not simply

   git branch test

?

> +	git reflog delete --updateref --rewrite refs/test@{0} &&
> +	test_must_fail git rev-parse --verify --quiet refs/test@{0} 2>error &&

Is it a shortcoming of the specification that it doesn't consider
whatever might be written to stdout? Is it acceptable that if the
git-rev-parse command succeeds, the error message from test_must_fail
will be written to the file "error" and, therefore, somewhat hidden from
the user running the tests?

> +	test -z "$(cat error)"

test(1) comes with an option (-s) to perform such tests and test-lib.sh
defines test_must_be_empty which additionally outputs the given file's
contents if its not empty.

> +'
> +
>  test_expect_success 'no stdout output on error' '
>  	test -z "$(git rev-parse --verify)" &&
>  	test -z "$(git rev-parse --verify foo)" &&
> 

Kind regards,
   Fabian

Re: [PATCH] t1503: test rev-parse --verify --quiet with deleted reflogs

[David Aguilar]

On Sun, Sep 14, 2014 at 06:20:57PM +0200, Fabian Ruch wrote:
> Hi David,
> 
> On 09/14/2014 10:30 AM, David Aguilar wrote:
> > Ensure that rev-parse --verify --quiet is silent when asked
> > about deleted reflog entries.
> > 
> > Signed-off-by: David Aguilar <davvid@gmail.com>
> > ---
> > This verifies and depends on "refs: make rev-parse --quiet actually quiet".
> > 
> >  t/t1503-rev-parse-verify.sh | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/t/t1503-rev-parse-verify.sh b/t/t1503-rev-parse-verify.sh
> > index 813cc1b..731c21c 100755
> > --- a/t/t1503-rev-parse-verify.sh
> > +++ b/t/t1503-rev-parse-verify.sh
> > @@ -83,6 +83,15 @@ test_expect_success 'fails silently when using -q' '
> >  	test -z "$(cat error)"
> >  '
> >  
> > +test_expect_success 'fails silently when using -q with deleted reflogs' '
> > +	ref=$(git rev-parse HEAD) &&
> > +	: >.git/logs/refs/test &&
> > +	git update-ref -m test refs/test "$ref" &&
> 
> I'm just curious, why not simply
> 
>    git branch test
> ?

Maybe it's a bad reason, but I wanted to replicate the behavior
that git stash expects -- it writes to a ref outside of
refs/heads/.  I thought it'd be good to exercise that same
machinery since it will involve different code paths.

> > +	git reflog delete --updateref --rewrite refs/test@{0} &&
> > +	test_must_fail git rev-parse --verify --quiet refs/test@{0} 2>error &&
> 
> Is it a shortcoming of the specification that it doesn't consider
> whatever might be written to stdout? Is it acceptable that if the
> git-rev-parse command succeeds, the error message from test_must_fail
> will be written to the file "error" and, therefore, somewhat hidden from
> the user running the tests?

Good point. The --quiet spec doesn't say anything about stdout,
but for this test it probably wouldn't hurt to capture both
stdout and stderr and assert emptiness.

I can reroll this patch so that 2>error becomes >error 2>&1.

> > +	test -z "$(cat error)"
> 
> test(1) comes with an option (-s) to perform such tests and test-lib.sh
> defines test_must_be_empty which additionally outputs the given file's
> contents if its not empty.

test_must_be_empty would be a good fit here.  That said, none of
the other tests in this file use test_must_be_empty.

It might be worth doing a follow-up patch that converts all of the
tests in this file to use test_must_be_empty instead of
test -z "$(cat error)".  I'll reroll.

Thanks,
-- 
David

Re: [PATCH] t1503: test rev-parse --verify --quiet with deleted reflogs

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> Good point. The --quiet spec doesn't say anything about stdout,

Please correct it while at it in the doc ;-)

I think I had to look it up in the documentation and then in code if

    git rev-parse --verify --quiet "$object"

the right way to check if the object is a good name without output
when it is, and get diagnosis in an appropriate error message when
it isn't.

Re: [PATCH] t1503: test rev-parse --verify --quiet with deleted reflogs

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

>> > +test_expect_success 'fails silently when using -q with deleted reflogs' '
>> > +	ref=$(git rev-parse HEAD) &&
>> > +	: >.git/logs/refs/test &&
>> > +	git update-ref -m test refs/test "$ref" &&
>> 
>> I'm just curious, why not simply
>> 
>>    git branch test
>> ?
>
> Maybe it's a bad reason, but I wanted to replicate the behavior
> that git stash expects -- it writes to a ref outside of
> refs/heads/.  I thought it'd be good to exercise that same
> machinery since it will involve different code paths.

I think that is a very sensible thing to do.  Another reason to
avoid using "branch" when you care about what "update-ref" does is
that "branch" does more than what "update-ref" does.