Re: [PATCH 3/4] rev-parse test: use test_cmp instead of "test" builtin

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 3, 2013, 20:01 UTC
Message-ID: <xmqqk3ixr7xy.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20130903170715.GD29921@google.com>

Jonathan Nieder <jrnieder@gmail.com> writes:

Show 8 quoted lines

> Use test_cmp instead of passing two command substitutions to the
> "test" builtin.  This way:
>
>  - when tests fail, they can print a helpful diff if run with
>    "--verbose"
>
>  - the argument order "test_cmp expect actual" feels natural,
>    unlike test <known> = <unknown> that seems a little backwards

I do not mind to drop s/a little // here, by the way.

Show 26 quoted lines

>  - the exit status from invoking git is checked, so if rev-parse
>    starts segfaulting then the test will notice and fail
>
> Use a custom function for this instead of test_cmp_rev to emphasize
> that we are testing the output from "git rev-parse" with certain
> arguments, not checking that the revisions are equal in abstract.
>
> Reported-by: Felipe Contreras <felipe.contreras@gmail.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
>  t/t6101-rev-parse-parents.sh | 33 +++++++++++++++++++--------------
>  1 file changed, 19 insertions(+), 14 deletions(-)
>
> diff --git a/t/t6101-rev-parse-parents.sh b/t/t6101-rev-parse-parents.sh
> index 416067c..8a6ff66 100755
> --- a/t/t6101-rev-parse-parents.sh
> +++ b/t/t6101-rev-parse-parents.sh
> @@ -8,6 +8,12 @@ test_description='Test git rev-parse with different parent options'
>  . ./test-lib.sh
>  . "$TEST_DIRECTORY"/lib-t6000.sh # t6xxx specific functions
>  
> +test_cmp_rev_output () {
> +	git rev-parse --verify "$1" >expect &&
> +	eval "$2" >actual &&
> +	test_cmp expect actual
> +}

After applying this patch and running "git show | grep test_cmp_rev_output", I notice that the second is always "git rev-parse <something>". Do we still need to eval these, or would it be sufficient to do

        test_cmp_rev_output () {
                git rev-parse --verify "$1" >expect &&
                git rev-parse --verify "$2" >actual &&
                test_cmp expect actual
        }

here, and make users like so:

	-	test_cmp_rev_output tags/start "git rev-parse start^0"
	+	test_cmp_rev_output tags/start start^0

Am I missing something?