Re: [PATCH v2 1/2] t/helper/test-sha1: prepare for an unsafe mode

From: Jeff King <peff@peff.net>
Date: Nov 7, 2024, 02:08 UTC
Message-ID: <20241107020810.GD961214@coredump.intra.peff.net>
In-Reply-To: <Zywcr2lMM_Ij8suu@tapette.crustytoothpaste.net>

On Thu, Nov 07, 2024 at 01:49:35AM +0000, brian m. carlson wrote:

Show 7 quoted lines

> Ah, yes, I think that approach would be simpler and nicer to work with
> than a separate entry in the `hash_algos` array.  We do, however, have
> some places that assume that a `struct git_hash_algo *` points into the
> `hash_algos` array (notably `hash_algo_by_ptr`), so we'd have to adjust
> for that, move the function pointers out into their own struct which
> we'd use for `unsafe_hash_algo`, or be careful never to call the
> relevant functions on our special `git_hash_algo` struct.

Yeah, I wondered if some code might be surprised by having a separate hash algo. Another weird thing is that the sub-implementation algo struct will have its own rawsz, hexsz, etc, even though those _must_ be the same its parent.

If all of the virtual implementation functions were in a git_hash_impl struct, then each git_hash_algo struct could have one embedded for the main implementation (which in sha1's case would be a collision detecting one), and an optional pointer to another unsafe _impl struct.

And then you get more type-safety, because you can never confuse the _impl struct for a parent git_hash_algo.

The downside is that every single caller, even if it doesn't care about the unsafe variant, needs to refer to the_hash_algo->impl.init_fn(), etc, due to the extra layer of indirection. Probably not worth it.

Show 5 quoted lines

> Yeah, I don't have a strong opinion one way or the other.  I think, with
> the limitation I mentioned above, it would probably require a decent
> amount of refactoring if we took a different approach, and I'm fine with
> going with Taylor's current approach unless he wants to do that
> refactoring (in which case, great).

Me too. If we were fixing something ugly or error-prone that we expected to come up in real code, it might be worth it. But it's probably not for trying to accommodate a one-off test helper.

-Peff