Re: [PATCH 0/5] refs: remove functions without ref store

From: Patrick Steinhardt <ps@pks.im>
Date: May 6, 2024, 06:44 UTC
Message-ID: <Zjh8XWwJKp_I1dwE@tanuki>
In-Reply-To: <xmqq7cga7nzo.fsf@gitster.g>

On Fri, May 03, 2024 at 11:24:11AM -0700, Junio C Hamano wrote:

Show 8 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > Though maybe an even more radical proposal: now that read_ref_full(),
> > etc, are gone, and we have only refs_read_ref_full(), could/should we
> > shorten the latter to drop the "refs_" prefix?
> 
> I view it as a good longer-term goal.  But I also view it as an
> orthogonal issue to the transition.

Personally, I'd prefer to keep the `refs_` prefix. This may be personal preference, but I find it way easier to reason about code when there are prefixes for our functions that clearly indicate the subsystem they belong to.

It's also in line with how other subsystems behave. Everything relating to strbufs has a `strbuf_` prefix, attr-related code has `attr_` or `git_attr_`, mem-pool has `mem_pool_`. So ref-related code having a `ref_` prefix just feels natural to me.

> We need a smooth migration path for remaining callers of these older
> functions.  We could do the USE_THE_INDEX_MACROS like compatibility
> layer during transition period.

Wouldn't this be overengineered? We already have all the required functions. So my take would be to drop the last patch for now, wait a release cycle, and then remove it in the next one.

The only problem is that this allows in-flight patch series to introduce _new_ callers. So we're basically working against a moving target in that case. That is something that would be addressed by having something like your proposed `USE_THE_INDEX_MACROS` macro. But honestly, I doubt that it would be faster for any author of a patch series to figure out that they now need to a define something compared to just adding the `refs_` prefix to their functions.

Patrick