Re: [PATCH v4 4/9] t3701: don't hard code sha1 hash values

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 2, 2018, 16:09 UTC
Message-ID: <xmqqa7vqe9xn.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180302155528.12106-1-szeder.dev@gmail.com>

SZEDER Gábor <szeder.dev@gmail.com> writes:

Show 24 quoted lines

>> +diff_cmp () {
>> +	for x
>> +	do
>> +		sed  -e '/^index/s/[0-9a-f]*[1-9a-f][0-9a-f]*\.\./1234567../' \
>> +		     -e '/^index/s/\.\.[0-9a-f]*[1-9a-f][0-9a-f]*/..9abcdef/' \
>> +		     -e '/^index/s/ 00*\.\./ 0000000../' \
>> +		     -e '/^index/s/\.\.00*$/..0000000/' \
>> +		     -e '/^index/s/\.\.00* /..0000000 /' \
>> +		     "$x" >"$x.filtered"
>> +	done
>> +	test_cmp "$1.filtered" "$2.filtered"
>> +}
>
> t3701 is not the only test script comparing diffs, many other
> tests do so:
>
>   $ git grep 'diff --git' t/ |wc -l
>   835
>
> It's totally inaccurate, but a good ballpark figure.
>
> I think this function should be a widely available test helper
> function in 'test-lib-functions.sh', perhaps renamed to
> 'test_cmp_diff', so those other tests could use it as well.

I am on the fence.

The above does not redact enough (e.g. rename/copy score should be redacted while comparing) to serve as a generic filter.

We could certainly extend it when we make code in other tests use the helper, but then we can do the moving to test-lib-functions when that happens, too.

Those who will be writing new tests that need to compare two diff outputs are either (1) people who are careful and try to find existing tests that do the same, to locate the helper we already have to be reused in theirs, or (2) people who don't and roll their own. The latter camp cannot be helped either way, but the former will run "git grep 'diff --git' t/" and find the helper whether it is in this script or in test-lib-functions, I would think.

So, I certainly do not mind a reroll to move it to a more generic place, but I do not think I would terribly mind if we leave it in its current place, later to be moved by the first new caller that wants to use it from outside this script.