git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Eric Sunshine <sunshine@sunshineco.com>
To: "brian m. carlson" <sandals@crustytoothpaste.net>,
	Eric Sunshine <sunshine@sunshineco.com>,
	Git List <git@vger.kernel.org>,
	Jonathan Nieder <jrnieder@gmail.com>,
	Stefan Beller <sbeller@google.com>,
	Brandon Williams <bmwill@google.com>
Subject: Re: [PATCH v2 3/4] Integrate hash algorithm support with repo setup
Date: Sun, 29 Oct 2017 15:02:20 -0400	[thread overview]
Message-ID: <CAPig+cRtexDJYMCUR1rGZjJOBRuDbhxP==US2VdNPhSWzeL6LA@mail.gmail.com> (raw)
In-Reply-To: <20171029175712.ijqirnghcgeufqae@genre.crustytoothpaste.net>

On Sun, Oct 29, 2017 at 1:57 PM, brian m. carlson
<sandals@crustytoothpaste.net> wrote:
> 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()?

  reply	other threads:[~2017-10-29 19:02 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-10-28 18:12 [PATCH v2 0/4] Hash Abstraction brian m. carlson
2017-10-28 18:12 ` [PATCH v2 1/4] setup: expose enumerated repo info brian m. carlson
2017-10-30 16:08   ` Stefan Beller
2017-10-28 18:12 ` [PATCH v2 2/4] Add structure representing hash algorithm brian m. carlson
2017-10-29  1:36   ` Eric Sunshine
2017-10-29 17:00     ` brian m. carlson
2017-10-30 16:14   ` Stefan Beller
2017-10-30 23:36   ` Brandon Williams
2017-11-01  1:35     ` brian m. carlson
2017-10-28 18:12 ` [PATCH v2 3/4] Integrate hash algorithm support with repo setup brian m. carlson
2017-10-29  1:44   ` Eric Sunshine
2017-10-29 17:57     ` brian m. carlson
2017-10-29 19:02       ` Eric Sunshine [this message]
2017-10-29 19:33         ` brian m. carlson
2017-10-30  2:13           ` Junio C Hamano
2017-10-30  2:54             ` brian m. carlson
2017-10-30 16:27   ` Stefan Beller
2017-10-28 18:12 ` [PATCH v2 4/4] Switch empty tree and blob lookups to use hash abstraction brian m. carlson
2017-10-30 16:45 ` [PATCH v2 0/4] Hash Abstraction Stefan Beller

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAPig+cRtexDJYMCUR1rGZjJOBRuDbhxP==US2VdNPhSWzeL6LA@mail.gmail.com' \
    --to=sunshine@sunshineco.com \
    --cc=bmwill@google.com \
    --cc=git@vger.kernel.org \
    --cc=jrnieder@gmail.com \
    --cc=sandals@crustytoothpaste.net \
    --cc=sbeller@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).