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

From: Jeff King <peff@peff.net>
Date: Oct 7, 2014, 20:18 UTC
Message-ID: <20141007201805.GA22703@peff.net>
In-Reply-To: <xmqqsiiziy94.fsf@gitster.dls.corp.google.com>

On Tue, Oct 07, 2014 at 10:29:59AM -0700, Junio C Hamano wrote:

Show 13 quoted lines

> > test_eq () {
> > 	if test "$1" != "$2"
> > 	then
> > 		printf "%s" "$1" >expect &&
> > 		printf "%s" "$2" >actual &&
> > 		test_cmp expect actual
> > 	fi
> > }
> [...]
> 
> 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.

Yeah, that is why I ended up with the operator as a parameter. I modeled after test_line_count, which faces the same problem (negation must happen in the operator, not the full command).

Show 8 quoted lines

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

I made it postfix because it's optional, and my inclination is to handle arguments left-to-right, since that extends to multiple optional arguments. But of course we have just one optional argument and can simply treat 2-arg and 3-arg forms differently. However, Michael noted that this is really just 'test "$@"', and I think we should allow any "test" parameters.

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

Based on your responses, I'm leaning towards:

  test_cmp_str() {
	test "$@" && return 0
	echo >&2 "command failed: test $*"
	return 1
  }

since the point is really just to print _something_ when the test fails (any quoting or whitespace may be wrong, of course, but that's OK; it's for human consumption, and is just a hint).

Maybe "str" is not right here. Michael suggested "test_test" which is semantically what we want, but just looks silly[1]. Maybe "test_pred" or something? "test_cond"? Or I guess going the other way, "sane_test" or "verbose_test" or something.

I think test_cond is my favorite of those.

-Peff

[1] Of course, we could always do "test_[". :)