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

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 1755 bytes --]

On Fri, May 03, 2024 at 11:24:11AM -0700, Junio C Hamano wrote:
> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]