Re: [PATCH 11/14] rust: add functionality to hash an object
On Wed, Oct 29, 2025 at 12:53:20AM +0000, brian m. carlson wrote:
Show 19 quoted lines
> On 2025-10-28 at 09:18:26, Patrick Steinhardt wrote:
> > On Mon, Oct 27, 2025 at 12:44:01AM +0000, brian m. carlson wrote:
> > > In a future commit, we'll want to hash some data when dealing with a
> > > loose object map. Let's make this easy by creating a structure to hash
> > > objects and calling into the C functions as necessary to perform the
> > > hashing. For now, we only implement safe hashing, but in the future we
> > > could add unsafe hashing if we want. Implement Clone and Drop to
> > > appropriately manage our memory. Additionally implement Write to make
> > > it easy to use with other formats that implement this trait.
> >
> > What exactly do you mean with "safe" and "unsafe" hashing? Also, can't
> > we drop this distinction for now until we have a need for it?
>
> It's from the series that Taylor introduced. For SHA-1, safe hashing
> (the default) uses SHA-1-DC, but unsafe hashing, which does not operate
> on untrusted data (say, when we're writing a packfile we've created),
> may use a faster algorithm. See `git_hash_sha1_init_unsafe`.
>
> I can omit the `safe` attribute until we need it, sure.
Ah, I completely forgot about that distinction! Makes sense.
Show 13 quoted lines
> > > +impl Hasher {
> > > + /// Create a new safe hasher.
> > > + pub fn new(algo: HashAlgorithm) -> Hasher {
> > > + let ctx = unsafe { c::git_hash_alloc() };
> > > + unsafe { c::git_hash_init(ctx, algo.hash_algo_ptr()) };
> >
> > I already noticed this in the patch that introduced this, but wouldn't
> > it make sense to expose `git_hash_new()` instead of the combination of
> > `alloc() + init()`?
>
> The benefit to this approach is that it allows us to reset a state in
> the future if we want. If we don't think that's necessary, I can
> certainly switch to `git_hash_new` if we prefer.Hm, fair. I don't mind it much either way.
Patrick