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

From: Taylor Blau <me@ttaylorr.com>
Date: Nov 7, 2024, 21:36 UTC
Message-ID: <Zy0yxDIgEmKtec/0@nand.local>
In-Reply-To: <20241107014737.GB961214@coredump.intra.peff.net>

On Wed, Nov 06, 2024 at 08:47:37PM -0500, Jeff King wrote:

Show 7 quoted lines

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

Yeah, I was sort of puzzled and thinking the same thing as I wrote the patch.

I wouldn't be opposed to deleting it in the future, and certainly have no strong feelings about keeping it around in the meantime. It just seemed like the path of least resistance to bring it along for the sha1-unsafe ride instead of deleting it outright.

> So I dunno. None of those are the fault of your series, but it is piling
> on yet another test-tool command.

Yeah, I think there was a fair amount of interesting discussion about possible alternatives in this thread, which I am appreciative of.

I think that if nobody has strong feelings or issues with the current implementation to add the sha1-unsafe test-tool, that we should do so and take these patches.

In the future we can consider other things on top, like dropping the test-sha1.sh script, having an unsafe pointer embedded within the_hash_algo, or something else entirely. But those can be done on top, or not at all, and I don't want to hold up this series for them.

Thanks, Taylor