On Fri, May 03, 2024 at 10:24:00AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@xxxxxx> 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
