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

From: Jeff King <peff@peff.net>
Date: May 3, 2024, 17:35 UTC
Message-ID: <20240503173553.GC3631237@coredump.intra.peff.net>
In-Reply-To: <xmqqikzu95cf.fsf@gitster.g>

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

Show 20 quoted lines

> Patrick Steinhardt <ps@pks.im> writes:
> 
> > It is quite likely that this patch series will impact in-flight patch
> > series. I'd be quite happy to drop the last patch that removes the old
> > interfaces to make this a bit less painful.
> 
> The last step could replace these deprecated-to-be-removed functions
> with a stub that BUG()s out [*], with a comment to instruct how a
> caller can be rewritten to use the corresponding refs_ variant with
> a call to get_main_ref_store(the_repository) as the first parameter,
> which would help out of tree and in-flight series to migrate.
> 
> [Footnote]
> 
>  * The exact mechanism to cause an attempted use of an old function
>    fail is immaterial.  We can remove the definition of these
>    functions while retaining the old implementation as comments, or
>    wrap them in an #ifdef USE_REF_STORE_LESS_FUNCTIONS .. #endif
>    pair _without_ defining USE_REF_STORE_LESS_FUNCTIONS, purely for
>    the documentation value to help us migration.

I prefer strict removal, as then the problem is caught by the compiler, rather than runtime/tests. The error message does not point the user in the right direction, but IMHO that is trumped by finding it sooner in the edit-compile-test cycle.

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? Its original purpose of
distinguishing the "takes a ref store vs using the_repository" variants
is now done, and shorter names are less annoying. But:
 
  - maybe there is value in having ref-related functions namespaced? We
    certainly don't cover all ref functions here, though, and aside from
    tight OO-ish APIs (e.g. strbuf) we don't usually do so at all.

  - the error message for in-flight callers of the "old" names will be
    even more confusing (it will not be "foo() does not exist" but
    rather "you did not pass enough arguments to foo()").

-Peff