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

Message

Taylor Blau Nov. 5, 2024, 7:05 p.m. UTC

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.

Thanks in advance for your review!

Taylor Blau (2):
  t/helper/test-sha1: prepare for an unsafe mode
  t/helper/test-tool: implement sha1-unsafe helper

 t/helper/test-hash.c   | 17 +++++++++++++----
 t/helper/test-sha1.c   |  7 ++++++-
 t/helper/test-sha1.sh  | 38 ++++++++++++++++++++++----------------
 t/helper/test-sha256.c |  2 +-
 t/helper/test-tool.c   |  1 +
 t/helper/test-tool.h   |  3 ++-
 6 files changed, 45 insertions(+), 23 deletions(-)

Range-diff against v1:
1:  3b31db4d2df = 1:  0e2fcee6894 t/helper/test-sha1: prepare for an unsafe mode
2:  d343f5dc9e5 = 2:  d8c1fc78b57 t/helper/test-tool: implement sha1-unsafe helper

base-commit: 8f8d6eee531b3fa1a8ef14f169b0cb5035f7a772

Comments

Jeff King Nov. 7, 2024, 1:47 a.m. UTC | #1

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

brian m. carlson Nov. 7, 2024, 2:05 a.m. UTC | #2

On 2024-11-07 at 01:47:37, Jeff King wrote:
> 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").

I think we do in `pack_trailer` in `t/lib-pack.sh`, but not in a
greppable way:

  # Compute and append pack trailer to "$1"
  pack_trailer () {
  	test-tool $(test_oid algo) -b <"$1" >trailer.tmp &&
  	cat trailer.tmp >>"$1" &&
  	rm -f trailer.tmp
  }

When you posed the question above, I wasn't sure why I added this
functionality: was it just to test my SHA-256 implementation adequately
or did it have some actual utility in the testsuite?  However, I knew if
it didn't appear straightforwardly in `git grep`, any uses would involve
`test_oid`, and boom, I was right.

I think a single helper with `--algo` and `--unsafe` parameters would
also be fine and would probably be a little more tidy, as you mention.