Julian Foad wrote on Thu, Mar 03, 2022 at 10:53:13 +0000: > I can think of a number of further API clean-ups possible, related to > multi-WC-format support. > > > Commentary: > > At first we were trying to keep knowledge of WC format numbers internal > to libsvn_wc. However it seems clear to me now that we need to expose > them. The rest of the system does need to know about different formats. > Compatible-version numbers are not such good identifiers because a range > of version numbers maps to each single format (even though there is one > version number where the format was first introduced). To be more > specific, our UI currently accepts > '--wc-compatible-version=1.14.0-alpha' to specify the format introduced > in 1.8; that mapping is not one-to-one, leading to slightly awkward > input and output semantics and conversion APIs, quite possibly already > having inconsistencies.
Would you elaborate? This is the one sentence in this long paragraph that actually explains _why_ you think the API should be written in terms of format numbers rather than version numbers (as opposed to stating that that is your opinion), and it's not very specific. What are the awkward semnatics? What are the inconsistencies? What questions would API users be able to answer for themselves if we hand them format numbers, that they can't easily answer with trunk@HEAD? > If we're going to have version numbers as the 'compatible version' UI > option, perhaps we should eliminate these issues by requiring to > specify the exact first-introduced version, MAJOR.MINOR format (with > no .PATCH, no -TAG). _Prima facie_ I would -0 this, because it should be possible to do «/opt/foo/bin/svn upgrade --compatible-version=$(/opt/bar/bin/svn --version --quiet)» even when bar is v1.9. What issues would be eliminated? > In exposing WC format numbers in the APIs, certainly we should treat > them as opaque identifiers with no intrinsic meaning to their numeric > value. We could of course also introduce non-numeric identifiers for > them, for avoidance of 'magic constants' in source code. > I'm not sure I understand what you have in mind here. Suppose we have a non-numeric identifier SVN_WC_F31, what now? The cmdline client would need either to have a --format-31 [sic] flag or to accept a string and map it to SVN_WC_F31. The former is a pattern we switched away from in «svnadmin upgrade». The latter adds a layer of indirection, but what is gained by that? How is having svn(1) pass SVN_WC_F31 an improvement over having it pass the int «31»? > So it now seems to me the right path is to expose WC format numbers, and > WC-format-related APIs, deliberately. > > I am not sure how best to distribute all these APIs between libsvn_wc > and libsvn_client, and how public to make each of them. Most of them > probably ought to be in libsvn_wc because they are meaningful there and > libsvn_wc itself and small apps built on it (tests at least) may need > them without wanting to pull in libsvn_client. Of course typical clients > mainly want to deal with libsvn_client, and to know about what that > client library supports, so would initially expect to find the public > APIs they need there, but it is no hardship for them to use (public) > libsvn_wc APIs for their WC-specific needs. > > > Some possible API clean-ups: > > * Move SVN_WC__VERSION and friends from wc.h, at least to the > inter-library scope of 'include/private/svn_wc_private.h', so they are > properly accessible to libsvn_client and the other libraries. (Avoiding > the need for "#include ../libsvn_wc/wc.h".) The format-related > definitions there are: > > - version history of all the WC formats > - SVN_WC__VERSION, SVN_WC__SUPPORTED_VERSION, SVN_WC__DEFAULT_VERSION > - SVN_WC__*: minimum format for certain features > - svn_wc__version_string_from_format() > - SVN_WC__ERR_IS_NOT_CURRENT_WC() > > Of these, 'SVN_WC__SUPPORTED_VERSION' and 'SVN_WC__DEFAULT_VERSION' are new. > > (Also, rename 'SVN_WC__VERSION' to include the words 'maximum/latest > supported'; and rename 'SUPPORTED_VERSION' to include the word > 'minimum/oldest'; and group those "SVN_WC__*: minimum format ..." > definitions with their own common prefix.) Why should we move any of that to include/private/? The rule of thumb is that everything should have the _smallest_ scope possible, and that's as valid for function-local variables as it is for intra- v. inter-library header files. SVN_WC__SUPPORTED_VERSION and SVN_WC__VERSION are used by libsvn_client, so for those two there's an obvious argument for moving them. However, I don't know what the argument is for moving the others. > * The definitions of oldest/default/newest supported WC format could be > more consistently named and less duplicated. Currently there are: > > SVN_WC__VERSION > == svn_wc__max_supported_format() > ~= svn_client_latest_wc_version() > > SVN_WC__SUPPORTED_VERSION > == svn_wc__min_supported_format() > ~= svn_client_oldest_wc_version() > > SVN_WC__DEFAULT_VERSION > ~= svn_client_default_wc_version() > > svn_wc__is_supported_format() > overlaps with svn_client_get_wc_versions_supported() > > Of these, all are new except 'SVN_WC__VERSION'. > > > I don't want to do any unnecessary churn, but maybe we should do some of > this, especially any that is in the public API name space. I'm not particularly worried about the private ones, but yes, the ones in the public namespace should be named well. Let's file an issue for this. Cheers, Daniel
