Re: [PATCH v2 3/4] Integrate hash algorithm support with repo setup

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Oct 29, 2017, 19:02 UTC
Message-ID: <CAPig+cRtexDJYMCUR1rGZjJOBRuDbhxP==US2VdNPhSWzeL6LA@mail.gmail.com>
In-Reply-To: <20171029175712.ijqirnghcgeufqae@genre.crustytoothpaste.net>

On Sun, Oct 29, 2017 at 1:57 PM, brian m. carlson <sandals@crustytoothpaste.net> wrote:

Show 13 quoted lines

> On Sat, Oct 28, 2017 at 09:44:07PM -0400, Eric Sunshine wrote:
>> > +#define current_hash the_repository->hash_algo
>>
>> The all-lowercase name "current_hash" seems likely to conflict with a
>> variable name some day; the fact that it is also a #define makes such
>> a collision even more worrisome. Although it is retrieving the "hash
>> algorithm", when reading the terse name "current_hash", one may
>> instead intuitively think it is referring to a hash _value_ (not an
>> algorithm).
>
> I can do CURRENT_HASH_ALGO or CURRENT_HASH instead if you think that's
> an improvement.  I originally omitted the "algo" portion to keep it
> short.

I don't have strong feelings about it aside from worrying about a "current_hash" name clash or a reader misunderstanding what it represents.

Does "current" need to be in the name? What about HASH_ALGO or REPO_HASH_ALGO?

> Alternatively, we could have a current_hash() (or current_hash_algo())
> inline function if people like that better.

hash_algo() or repo_hash_algo()?