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

[Christian Couder <christian.couder@gmail.com>]

On Fri, May 24, 2024 at 3:30 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> 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.

It could perhaps work if we used git-hash-object(1) instead of
`test-tool sha1` in t0013-sha1dc to check that SHA1DC does its thing,
but we could do that in a separate patch or patch series.

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

As far as I remember it is needed as check() is expecting an 'int'
while 'data' is a 'void *'.

> > +             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".

Actually I think something like "BUG: Null data pointer provided"
would be even better.

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

No, it uses 'sizeof(literal)' which works only for string literals.

> Is there a
> partiuclar reason why we don't unify them?

The comments above them try to explain that the first one doesn't work
when the data contains a NUL char as it uses strlen() while the second
one works only for string literals including those which contain NUL
characters.

Thanks for your review.