On Fri, May 24, 2024 at 05:29:44AM +0530, Ghanshyam Thakkar wrote:
> 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.
> 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".
> + 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
Attachment:
signature.asc
Description: PGP signature
