Re: [PATCH 08/16] t5304: use helper to report failure of "test foo = bar"

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 7, 2014, 17:29 UTC
Message-ID: <xmqqsiiziy94.fsf@gitster.dls.corp.google.com>
In-Reply-To: <5433E8CB.1050005@alum.mit.edu>

Michael Haggerty <mhagger@alum.mit.edu> writes:

> I don't like the three-argument version of test_eq. Wouldn't using a
> comparison operator other than "=" would be very confusing, given that
> "eq" is in the name of the function? It also doesn't look like you use
> this feature.

Show 14 quoted lines

> An alternative direction to go would be to specialize the function for
> equality testing and delegate to test_cmp to get better output for
> failures, but optimized to avoid excess process creation in the happy path:
>
> test_eq () {
> 	if test "$1" != "$2"
> 	then
> 		printf "%s" "$1" >expect &&
> 		printf "%s" "$2" >actual &&
> 		test_cmp expect actual
> 	fi
> }
>
> (but using properly-created temporary file names).

I agree that it would be a good idea to use a randomly generated temporary filename that does not collide, as long as we make sure not to leave cruft in the working tree of the test and we leave the file there when the test script is run under "-i" or "-d" option.

The above superficially looks nice; "! test_eq a b" would give useless output under "-v", and "test_ne a b" needs to be added if you go that route, though.

Anyway, with the version posted, you cannot do "! test_eq a b", either but with "test_eq a b !=", you do not have to.

	Side note. Yes, now I looked at it again, I agree that the
	three-arg form is awkwards in at least two ways.  The
	operator, if we are to take one, should be infix, and the
	name "eq" no longer matches what it does when it is given an
	operator.

The function is similar to test_cmp which takes two files but takes two strings, so "test_cmp_str" or something perhaps (we already have test_cmp_rev to compare two revisions, and the suggested name follows that pattern)?