Re: [RFC PATCH 0/9] Use a structure for object IDs.

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: May 4, 2014, 06:35 UTC
Message-ID: <5365DF94.9060707@alum.mit.edu>
In-Reply-To: <1399147942-165308-1-git-send-email-sandals@crustytoothpaste.net>

On 05/03/2014 10:12 PM, brian m. carlson wrote:

Show 30 quoted lines

> This is a preliminary RFC patch series to move all the relevant uses of
> unsigned char [20] to struct object_id.  It should not be applied to any
> branch yet.
> 
> The goal of this series to improve type-checking in the codebase and to
> make it easier to move to a different hash function if the project
> decides to do that.  This series does not convert all of the codebase,
> but only parts.  I'm looking for feedback to see if there is consensus
> that this is the right direction before investing a large amount of
> time.
> 
> Certain parts of the code have to be converted before others to keep the
> patch sizes small, maintainable, and bisectable, so functions and
> structures that are used across the codebase (e.g. hashcmp and struct
> object) will be converted later.  Conversion has been done in a roughly
> alphabetical order by name of file.
> 
> The constants for raw and hex sizes of SHA-1 values are maintained.
> These constants are used where the quantity is the size of an SHA-1
> value, and sizeof(struct object_id) is used wherever memory is to be
> allocated.  This is done to permit the struct to turn into a union later
> if multiple hashes are supported.  I left the names at GIT_OID_RAWSZ and
> GIT_OID_HEXSZ because that's what libgit2 uses and what Junio seemed to
> prefer, but they can be changed later if there's a desire to do that.
> 
> I called the structure member "oid" because it was easily grepable and
> distinct from the rest of the codebase.  It, too, can be changed if we
> decide on a better name.  I specifically did not choose "sha1" since it
> looks weird to have "sha1->sha1" and I didn't want to rename lots of
> variables.

That means that we will have sha1->oid all over the place, right? That's unfortunate, because it is exactly backwards from what we would want in a hypothetical future where OIDs are not necessarily SHA-1s. In that future we would certainly have to support SHA-1s in parallel with the new hash. So (in that hypothetical future) we will probably want these expressions to look like oid->sha1, to allow, say, a second struct or union field oid->sha256 [1].

If that future would come to pass, then we would also want to have distinct constants like GIT_SHA1_RAWSZ and GIT_SHA256_RAWSZ rather than the generically-named GIT_OID_RAWSZ.

I think that this patch series will improve the code clarity and type safety independent of thoughts about supporting different hash algorithms, so I'm not objecting to your naming decision. But *if* such support is part of your long-term hope, then you might ease the future transition by choosing different names now.

(Maybe renaming local variables "sha1 -> oid" might be a handy way of making clear which code has been converted to the new style.)

Just to be clear, the above are just some random thoughts for your consideration, but feel free to disregard them.

In any case, it sure will be a lot of code churn. If you succeed in this project, then "git blame" will probably consider you the author of about 2/3 of git :-)

Michael

[1] I'm certainly not advocating that we want to support a different hash, let alone that that hash should be SHA-256; these examples are just for illustration.

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/