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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Sep 4, 2013, 04:28 UTC
Message-ID: <20130904042825.GA2661@elie.Belkin>
In-Reply-To: <xmqqk3ixr7xy.fsf@gitster.dls.corp.google.com>

Junio C Hamano wrote:

Show 16 quoted lines

> 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?

I was tempted to use test_cmp_rev, which would have the same effect. The original test checked the output of "git rev-parse start^0", while the test as modified above checks the output of "git rev-parse --verify start^0".

I slightly prefer the version without --verify because "git rev-parse --verify" is well exercised elsewhere in the testsuite (but then, so is rev-parse without --verify, so it's not a big deal).

Abbreviating as follows

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

would also be fine with me. All it would take is a good replacement for the placeholder "foo". The added flexibility of "compare a rev to the output of an arbitrary command" doesn't get used.

Jonathan