Re: [PATCH 2/2] t6500: mark tests as SHA1 reliant

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 28, 2017, 22:14 UTC
Message-ID: <xmqq379gmco6.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170728171817.21458-3-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

Show 10 quoted lines

> The first test marked relies on hard coded sha1:
>
> 	# We need to create two object whose sha1s start with 17
> 	# since this is what git gc counts.  As it happens, these
> 	# two blobs will do so.
> 	test_commit 263 &&
> 	test_commit 410 &&
>
> The next two seem to rely on state from the first one, I did not
> investigate.

I am moderately negative on this approach, if it is meant to suggest the final shape of our test suite patch 1/2 started.

This script may be a good example you can use to demonstrate a much better approach. As the above comment in the test shows, we want to create two objects whose object names begin with "17", and running test_commit with 263 and 410 at this point in the test was a way to achieve that when Git uses SHA-1 as its hash.

When we use a hash different from SHA-1, the exact strings 263 and 410 may change, but we should be able to find two other strings that has the same property (i.e. they results in objects that share the prefix "17"). Perhaps a better way forward for this kind of test is to parameterize these hardcoded constants and make it easier to use different values without having to change the rest of the script when we switch the hash function? So perhaps have something like

	case "$GIT_HASH_FUNCTION" in
	SHA-1)	
		TEST_17_1="263 410" ;;
	CORRUPT-SHA-1)	
		TEST_17_1="something else" ;;
        esac

near the top of the script and update the above two with:

	for token in $TEST_17_1
	do
		test_commit "$token" || return 1
	done &&

would prepare us to switch to SHA-256 or whatever hash function we choose to use in the future?

t1512 is another example with full of such tests that we would want to keep moral equivalents in the world with a different hash, instead of reducing the test coverage.