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

[Taylor Blau <me@ttaylorr.com>]

On Thu, Nov 07, 2024 at 04:30:37PM -0500, Taylor Blau wrote:
> > > All that said, I do not think it buys us anything for "real" code. There
> > > the decision between safe/unsafe is in the context of how we are using
> > > it, and not based on some conditional. So while the code in this test
> > > helper is ugly, I don't think we'd ever write anything like that for
> > > real. So it may not be worth the effort to refactor into a more virtual
> > > object-oriented way.
> >
> > 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).
>
> I think it does buy you something for real code, which is that you don't
> have to remember to consistently call the unsafe_ variants of all of the
> various function pointers.
>
> For instance, if you do
>
>     the_hash_algo->unsafe_init_fn(...);
>
> early on, and then later on by mistake write:
>
>     the_hash_algo->update_fn(...);
>
> Then your code is broken and will (as brian said) either in the best
> case produce wrong results, or likely segfault.
>
> So in that sense it is worth doing as it avoids the caller from having
> to keep track of what "mode" they are using the_hash_algo in. But let's
> take it up later, I think.

The idea seemed too fun to play with, so I tried my hand at implementing
it and was quite pleased with the result.

The WIP-quality patches are available in my tree under the
wip/tb/unsafe-hash-algop[1] branch. The result, at least as it applies
to the test-tool modified here, is quite pleasing IMHO:

--- 8< ---
Subject: [PATCH] t/helper/test-hash.c: use unsafe_hash_algo()

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/helper/test-hash.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/t/helper/test-hash.c b/t/helper/test-hash.c
index d0ee668df95..aa82638c621 100644
--- a/t/helper/test-hash.c
+++ b/t/helper/test-hash.c
@@ -9,6 +9,8 @@ int cmd_hash_impl(int ac, const char **av, int algo, int unsafe)
 	int binary = 0;
 	char *buffer;
 	const struct git_hash_algo *algop = &hash_algos[algo];
+	if (unsafe)
+		algop = unsafe_hash_algo(algop);

 	if (ac == 2) {
 		if (!strcmp(av[1], "-b"))
@@ -27,10 +29,7 @@ int cmd_hash_impl(int ac, const char **av, int algo, int unsafe)
 			die("OOPS");
 	}

-	if (unsafe)
-		algop->unsafe_init_fn(&ctx);
-	else
-		algop->init_fn(&ctx);
+	algop->init_fn(&ctx);

 	while (1) {
 		ssize_t sz, this_sz;
@@ -49,15 +48,9 @@ int cmd_hash_impl(int ac, const char **av, int algo, int unsafe)
 		}
 		if (this_sz == 0)
 			break;
-		if (unsafe)
-			algop->unsafe_update_fn(&ctx, buffer, this_sz);
-		else
-			algop->update_fn(&ctx, buffer, this_sz);
+		algop->update_fn(&ctx, buffer, this_sz);
 	}
-	if (unsafe)
-		algop->unsafe_final_fn(hash, &ctx);
-	else
-		algop->final_fn(hash, &ctx);
+	algop->final_fn(hash, &ctx);

 	if (binary)
 		fwrite(hash, 1, algop->rawsz, stdout);

base-commit: d8c1fc78b57e02a140b5c363caaa14c2dc2bb274
prerequisite-patch-id: 5df87a6ba8a596bc7834ce8b5a656a3c4f1a869f
prerequisite-patch-id: c3f346e57f98b067eef8adf1e20c70ac23f41c36
prerequisite-patch-id: c5d1ec4f5e9796c5c9232442a3fc3be79379b8c4
prerequisite-patch-id: bdc2d6cbc23c467b24f184a889a5242e5c9fe774
--
2.47.0.238.g87615aecf12
--- >8 ---

To be clear: I think that we should still take this series as-is, and
I'll send the additional half dozen or so patches on top as a new series
dependent on this one.

Thanks,
Taylor

[1]: https://github.com/ttaylorr/git/compare/tb/sha1-unsafe-helper...ttaylorr:git:wip/tb/unsafe-hash-algop