Re: [PATCH v2 0/4] Hash Abstraction

On Sat, Oct 28, 2017 at 11:12 AM, brian m. carlson
[off-list ref] wrote:

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.

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.

 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