On Tue, Nov 05, 2024 at 02:05:10PM -0500, Taylor Blau wrote:
> This series implements a new 'sha1-unsafe' test helper, similar to
> 't/helper/test-tool sha1'.
>
> I have found such a helper to be really handy when debugging the new
> SHA1_UNSAFE build knobs, e.g., to easily compare the performance of the
> safe versus unsafe routines, different unsafe variants, etc.
>
> The first patch prepares us by setting up the existing cmd_hash_impl()
> function to be able to conditionally use the unsafe variant. The final
> patch introduces a new 'sha1-unsafe' test helper which calls the new
> variant.
I think this is a useful thing to have, and I didn't see anything wrong
in the implementation. I did notice some oddities that existed before
your series:
1. Why do we have "test-tool sha256" at all? Nobody in the test suite
calls it. It feels like the whole test-sha1/sha256/hash split is
overly complicated. A single "test-tool hash" seems like it would
be simpler, and it could take an "--algo" parameter (and an
"--unsafe" one). I guess in the end we end up with the same options
,but the proliferation of top-level test-tool commands seems ugly
to me (likewise "sha1_is_sha1dc").
2. You modified test-sha1.sh, but I've wondered if we should just
delete that script. It is not ever invoked in the test suite AFAIK.
If we want correctness tests, they should go into a real t[0-9]*.sh
script (though one imagines we exercise the sha1 code quite a lot
in the rest of the tests). And it starts with a weird ad-hoc
performance test that doesn't really tell us much. A t/perf test
would be better (if it is even worth measuring at all).
So I dunno. None of those are the fault of your series, but it is piling
on yet another test-tool command.
-Peff
