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

From: brian m. carlson <sandals@crustytoothpaste.net>
Date: Nov 7, 2024, 02:05 UTC
Message-ID: <ZywgZt419SfPUVkQ@tapette.crustytoothpaste.net>
In-Reply-To: <20241107014737.GB961214@coredump.intra.peff.net>

On 2024-11-07 at 01:47:37, Jeff King wrote:

Show 11 quoted lines

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

-- 
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA