Re: [PATCH v2 03/13] hex: introduce functions to print arbitrary hashes

From: brian m. carlson <sandals@crustytoothpaste.net>
Date: Oct 17, 2018, 23:49 UTC
Message-ID: <20181017234904.GL432229@genre.crustytoothpaste.net>
In-Reply-To: <xmqqo9bu4pww.fsf@gitster-ct.c.googlers.com>

On Tue, Oct 16, 2018 at 10:54:23AM +0900, Junio C Hamano wrote:

Show 6 quoted lines

> Even though in hex.c I see mixture of *_algo and *_algop helper
> functions, I see only "algo" variants above.  Is it our official
> stance to use primarily the integer index into the algo array when
> specifying the hash, and when a caller into 'multi-hash' API happens
> to have other things, it should use functions in 2/13 to convert it
> to the canonical "int algo" beforehand?

That was my intention, yes.

Show 5 quoted lines

> I am not saying it is bad or good to choose the index into the algo
> array as the primary way to specify the algorithm.  I think it is a
> good idea to pick one and stick to it, and I wanted to make sure
> that the choice we made here is clearly communicated to any future
> developer who read this code.

Yeah, that was my feeling as well. I wanted to pick something fixed and stick to it.

Show 12 quoted lines

> Having said the above, seeing the use of hash_algo_by_ptr() here
> makes me suspect if it makes more sense to use the algop as the
> primary way to specify which algorithm the caller wants to use.
> IOW, making the set of helpers in 02/13 to allow quering by name,
> format-id, or the integer index and have them all return a pointer
> to "const struct git_hash_algo".  Two immediate downsides I can see
> is that it exposes the actual structure to the callers (but is it
> really a problem?  Outside callers learn hash sizes etc. by accessing
> its fields anyway without treating the algo struct as opaque.), and
> passing an 8-byte pointer may be more costly than passing a small
> integer index that ranges between 0 and 1 at most (assuming that
> we'd only use SHA-1 and "the current NewHash" in the code).

I thought about this. The one downside to this is that we can't use those values anywhere we need an integer constant expression, like a switch. I suppose that just means we need hash_algo_by_ptr in those cases, and not everywhere else, which would make the code cleaner.

Let me reroll with that change, and we'll see if we like it better. If we don't, I can pull the old version out of history.

-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204