Re: [PATCH v3 2/3] t/: port helper/test-sha1.c to unit-tests/t-hash.c

From: Patrick Steinhardt <ps@pks.im>
Date: May 24, 2024, 13:30 UTC
Message-ID: <ZlCWcpcUkgUMWJYz@tanuki>
In-Reply-To: <20240523235945.26833-3-shyamthakkar001@gmail.com>

On Fri, May 24, 2024 at 05:29:44AM +0530, Ghanshyam Thakkar wrote:

Show 8 quoted lines

> t/helper/test-sha1 and t/t0015-hash.sh test the hash implementation of
> SHA-1 in Git with basic SHA-1 hash values. Migrate them to the new unit
> testing framework for better debugging and runtime performance.
> 
> The sha1 subcommand from test-tool is still not removed because it is
> relied upon by t0013-sha1dc (which requires 'test-tool sha1' dying
> when it is used on a file created to contain the known sha1 attack)
> and pack_trailer():lib-pack.sh.

Can we refactor this test to stop doing that? E.g., would it work if we used git-hash-object(1) to check that SHA1DC does its thing? Then we could get rid of the helper altogether, as far as I understand.

Show 19 quoted lines

> diff --git a/t/unit-tests/t-hash.c b/t/unit-tests/t-hash.c
> new file mode 100644
> index 0000000000..89dfea9cc1
> --- /dev/null
> +++ b/t/unit-tests/t-hash.c
> @@ -0,0 +1,54 @@
> +#include "test-lib.h"
> +#include "hash-ll.h"
> +#include "hex.h"
> +#include "strbuf.h"
> +
> +static void check_hash_data(const void *data, size_t data_length,
> +			    const char *expected, int algo)
> +{
> +	git_hash_ctx ctx;
> +	unsigned char hash[GIT_MAX_HEXSZ];
> +	const struct git_hash_algo *algop = &hash_algos[algo];
> +
> +	if (!check(!!data)) {

Is this double negation needed? Can't we just `if (!check(data))`?

> +		test_msg("Error: No data provided when expecting: %s", expected);

This error message is a bit atypical compared to the other callers of this function. We could say something like "BUG: test has no data", which would match something we have in "t/unit-tests/test-lib.c".

Show 20 quoted lines

> +		return;
> +	}
> +
> +	algop->init_fn(&ctx);
> +	algop->update_fn(&ctx, data, data_length);
> +	algop->final_fn(hash, &ctx);
> +
> +	check_str(hash_to_hex_algop(hash, algop), expected);
> +}
> +
> +/* Works with a NUL terminated string. Doesn't work if it should contain a NUL character. */
> +#define TEST_SHA1_STR(data, expected) \
> +	TEST(check_hash_data(data, strlen(data), expected, GIT_HASH_SHA1), \
> +	     "SHA1 (%s) works", #data)
> +
> +/* Only works with a literal string, useful when it contains a NUL character. */
> +#define TEST_SHA1_LITERAL(literal, expected) \
> +	TEST(check_hash_data(literal, (sizeof(literal) - 1), expected, GIT_HASH_SHA1), \
> +	     "SHA1 (%s) works", #literal)
>

This macro also works for `TEST_SHA1_STR()`, right? Is there a partiuclar reason why we don't unify them?

Patrick