On Mon, Sep 02, 2024 at 03:41:24PM +0200, Patrick Steinhardt wrote:
> > This commit does not actually introduce any new compile-time knobs to
> > control which implementation is used as the fast SHA-1 variant, but does
> > add scaffolding so that the "git_hash_algo" structure has five new
> > function pointers which are "fast" variants of the five existing
> > hashing-related function pointers:
> >
> > - git_hash_init_fn fast_init_fn
> > - git_hash_clone_fn fast_clone_fn
> > - git_hash_update_fn fast_update_fn
> > - git_hash_final_fn fast_final_fn
> > - git_hash_final_oid_fn fast_final_oid_fn
> >
> > The following commit will introduce compile-time knobs to specify which
> > SHA-1 implementation is used for non-cryptographic uses.
>
> While the property we care about in the context of this patch series
> indeed is that the second hash is faster, I think the more important
> property is that it's insecure. If I were seeing two APIs, one labelled
> fast and one labelled slow, I would of course pick the fast one. So I
> wonder whether we should rename things accordingly so that developers
> aren't intrigued to pick the fast one without thinking, and also to have
> a more useful signal that stands out to reviewers.
I tried to come up with a different name myself when writing this
series, and wasn't happy with any of the others that I came up with. I
thought of "insecure_init_fn()", or "non_cryptographic_init_fn()". The
first one appears scarier than the second, but both are mouthfuls.
As a middle-ground, I updated the comments to say "fast /
non-cryptographic" in places where they just said "fast" previously. Let
me know if you think that's sufficient, otherwise I can try and come up
with some more names.
> We may want to apply our new coding guidelines around nested
> preprocessor directives, which should also use indenting.
Gotcha.
> > @@ -222,6 +249,21 @@ struct git_hash_algo {
> > /* The hash finalization function for object IDs. */
> > git_hash_final_oid_fn final_oid_fn;
> >
> > + /* The fast hash initialization function. */
>
> Providing some context here why there are two sets of functions would
> help future readers.
Very fair, will adjust (as above).
Thanks,
Taylor
