Re: [PATCH v2 03/21] t/test-lib-functions.sh: generalize test_cmp_rev

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 15, 2016, 20:00 UTC
Message-ID: <xmqqd1pqbq2a.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1460294354-7031-4-git-send-email-s-beyer@gmx.net>

Stephan Beyer <s-beyer@gmx.net> writes:

Show 6 quoted lines

> test_cmp_rev() took exactly two parameters, the expected revision
> and the revision to test. This commit generalizes this function
> such that it takes any number of at least two revisions: the
> expected one and a list of actual ones. The function returns true
> if and only if at least one actual revision coincides with the
> expected revision.

There may be cases where you want to find the expected one among various things you actually have (which is what the above talks about; it is like "list-what-I-actually-got | grep what-i-want"), but an equally useful use case would be "I would get only one outcome from test, I anticipate one of these things, all of which is OK, but I cannot dictate which one of them should come out" (it is like "list-what-I-can-accept | grep what-I-actually-got").

I am not enthused by the new test that implements the "match one against multi" check only in one way among these possible two to squat on a very generic name, test_cmp_rev.

The above _may_ appear a non-issue until you realize one thing that is there to help those who debug the tests, which is ...

> While at it, the side effect of generating two (temporary) files
> is removed.

That is not strictly a side effect. test_cmp allows you to see what was expected and what you actually had when the test failed (we always compare expect with actual and not the other way around, so that "diff -u expect actual" would show how the actual behaviour diverted from our expectation in a natural way).

Something with the semantics of these two:

	test_revs_have_expected () {
        	expect=$1
		shift
		git rev-parse "$@" | grep -e "$expect" >/dev/null && return
		echo >&2 "The expected '$1' is not found in:"
                printf >&2 " '%s'\n", "$@"
                return 1
	}

	test_rev_among_expected () {
		actual=$1
                shift
		git rev-parse "$@" | grep -e "$actual" >/dev/null && return
		echo >&2 "'$1' is not among expected ones:"
                printf >&2 " '%s'\n", "$@"
                return 1
	}

might be more appropriate.

Show 32 quoted lines

>
> Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
> ---
>  t/test-lib-functions.sh | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 8d99eb3..8caf59c 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -711,11 +711,17 @@ test_must_be_empty () {
>  	fi
>  }
>  
> -# Tests that its two parameters refer to the same revision
> +# Tests that the first parameter refers to the same revision
> +# of at least one other parameter
>  test_cmp_rev () {
> -	git rev-parse --verify "$1" >expect.rev &&
> -	git rev-parse --verify "$2" >actual.rev &&
> -	test_cmp expect.rev actual.rev
> +	hash1="$(git rev-parse --verify "$1")" || return
> +	shift
> +	for rev
> +	do
> +		hash2="$(git rev-parse --verify "$rev")" || return
> +		test "$hash1" = "$hash2" && return 0
> +	done
> +	return 1
>  }
>  
>  # Print a sequence of numbers or letters in increasing order.  This is