Re: [PATCH v2 0/2] t/helper/test-tool: implement 'sha1-unsafe' helper

From: Jeff King <peff@peff.net>
Date: Nov 7, 2024, 01:47 UTC
Message-ID: <20241107014737.GB961214@coredump.intra.peff.net>
In-Reply-To: <cover.1730833506.git.me@ttaylorr.com>

On Tue, Nov 05, 2024 at 02:05:10PM -0500, Taylor Blau wrote:

Show 11 quoted lines

> 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