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

>
> 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?