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@xxxxxxxxxxxx>
---
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
