Re: [PATCH v2 0/4] Hash Abstraction

From: Stefan Beller <sbeller@google.com>
Date: Oct 30, 2017, 16:45 UTC
Message-ID: <CAGZ79kbX_6ONL1RK3xPopE8Rg15AinvzVZv3rAwLCXgX15NQ_Q@mail.gmail.com>
In-Reply-To: <20171028181239.59458-1-sandals@crustytoothpaste.net>

On Sat, Oct 28, 2017 at 11:12 AM, brian m. carlson <sandals@crustytoothpaste.net> wrote:

Show 6 quoted lines

> This is a series proposing a basic abstraction for hash functions.
>
> As we get closer to converting the remainder of the codebase to use
> struct object_id, we should think about the design we want our hash
> function abstraction to take.  This series is a proposal for one idea.
> Input on any aspect of this proposal is welcome.

I like the series/idea.

Show 8 quoted lines

> This series exposes a struct git_hash_algo that contains basic
> information about a given hash algorithm that distinguishes it from
> other algorithms: name, identifiers, lengths, implementing functions,
> and empty tree and blob constants.  It also exposes an array of hash
> algorithms, and a constant for indexing them.
>
> The series also demonstrates a simple conversion using the abstraction
> over empty blob and tree values.

That looks good, though I wonder if we'll have to give a way a little performance during the transition period (or even indefinitely, as the hash abstraction won't go away; we'd rather want to keep it in place long term). I guess we can measure after all is said and done, no big deal.

>
> In order to avoid conflicting with the struct repository work and with
> the goal of avoiding global variables as much as possible, I've pushed
> the hash algorithm into struct repository and exposed it via a #define.

If you are referring to that long series[1] that Jonathan and I were working on, then no worries. Unfortunately that series got some delays, but I rebased its prepared successors (of over 100 patches) on Friday and it was surprisingly easy to do so; just tedious.

[1] https://public-inbox.org/git/20170830064634.GA153983@aiede.mtv.corp.google.com/

>
> I propose this series now as it will inform the way we go about
> converting other parts of the codebase, especially some of the pack
> algorithms.

Thanks for doing this now.

> Because we share some hash computation code between pack
> checksums and object hashing, we need to decide whether to expose pack
> checksums as struct object_id, even though they are technically not
> object IDs.

I think we used sha1 as checksums, because it was a hash function easily accessible, not because its other properties were the best to do its job. So I am undecided if we want to just "keep the same hash function for checksum as well as object hashing" or pickup another checksum, that actually *only* does checksumming (we don't need the cryptographic properties, such that an easy hash [such as crc, djb, fnv or murmur] would do; raw speed, barely protecting the pack file against bit flips).

For the sake of symmetry I tend to go with the former, i.e use the object hash function for pack files, too.

Show 11 quoted lines

>  Furthermore, if we end up needing to stuff an algorithm
> value into struct object_id, we'll no longer be able to directly
> reference object IDs in a pack without a copy.
>
> I've updated this series in some significant ways to reflect and better
> implement the transition plan as it's developed.  If there are ways
> in which this series (or future series) can converge better on the
> transition plan, that input would be valuable.
>
> This series is available from the usual places as branch hash-struct,
> based against master as of 2.15-rc2.

Thanks, Stefan