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

[Jeff King <peff@peff.net>]

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

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

> 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