On Thu, Nov 07, 2024 at 02:05:26AM +0000, brian m. carlson wrote:
> 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
> }
Nice find. I think that it may be worth writing this as:
case "$(test_oid algo)" in
sha1)
test-tool sha1 -b <"$1" >trailer.tmp
;;
sha256)
test-tool sha256 -b <"$1" >trailer.tmp
;;
*)
echo >&2 "unknown algorithm: $(test_oid algo)"
exit 1
;;
esac
To make it more greppable. Obviously the existing implementation is not
wrong, but I do find it remarkably hard to search for ;-).
> 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.
That would be nice too.
Thanks,
Taylor
