On Thu, Nov 07, 2024 at 02:05:26AM +0000, brian m. carlson wrote:
> > 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.
Ah, good catch. Or good recall, I suppose. ;)
It feels a tiny bit hacky that we have to specify the algo here. If
there were a plumbing tool to work with the trailing hash of a
csum-file, then naturally it would use the repo's algorithm instead of
having to specify it. And I have run into situations once or twice where
that might have been a useful debugging tool, rather than hacking
together shell tools like dd. But since we have already done that
hacking together in the test suite, and this is the only spot that has
needed it so far, it's probably worth letting sleeping dogs lie.
> 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.
Yup.
-Peff
