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

From: Taylor Blau <me@ttaylorr.com>
Date: Nov 7, 2024, 21:33 UTC
Message-ID: <Zy0yRLt5dM554yq1@nand.local>
In-Reply-To: <ZywgZt419SfPUVkQ@tapette.crustytoothpaste.net>

On Thu, Nov 07, 2024 at 02:05:26AM +0000, brian m. carlson wrote:

Show 22 quoted lines

> 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