Re: svn commit: r1898378 - in /subversion/trunk/subversion: include/ libsvn_client/ libsvn_wc/ svn/ tests/cmdline/getopt_tests_data/

[Daniel Shahaf <d....@daniel.shahaf.name>]

julianfoad@apache.org wrote on Thu, Feb 24, 2022 at 15:58:11 -0000:
> Multi-WC-format: Clarify the supported versions display.
> 
> This patch:
> 
>   - Changes the APIs for querying the default WC format and the supported WC
>     formats;
>   - clarifies the display of supported WC formats in 'svn --version'.
> 
> API changes:
> 
>   - Remove svn_client_supported_wc_version().
>   - Add svn_client_default_wc_version().
>   - Add svn_client_supported_wc_formats() and a type it returns.
> 

Looks good.  Just a few comments:

> CLI changes:
> 
>   Old display in 'svn --version':
> 
>   | Supported working copy (WC) versions: from 1.8 to 1.15
> 
>   New display in 'svn --version':
> 
>   | Supported working copy (WC) versions:
>   |
>   | * compatible with Subversion v1.8 to v1.15 (WC format 31)
>   | * compatible with Subversion v1.15 (WC format 32)

Nice!

> The list style, along with inclusion of the WC format number, helps show
> that each line describes a distinct format. Users sometimes also need to
> know about WC format numbers, and the 'version' command is an appropriate
> place to show these. The presentation style matches that used for the lists
> of RA modules and credential caches.
> 
> * subversion/include/svn_client.h,
>   subversion/libsvn_client/upgrade.c
>   (svn_client_checkout4,
>    svn_client_upgrade2): Update doc string.
>   (svn_client_supported_wc_version): Remove.
>   (svn_client_default_wc_version,
>    svn_client_wc_format_t,
>    svn_client_supported_wc_formats): New.
> 
> * subversion/libsvn_client/checkout.c
>   (svn_client_checkout4): Update caller: use svn_client_default_wc_version().
> 
> * subversion/libsvn_wc/wc.h
>   (SVN_WC__SUPPORTED_VERSION): Update doc string.
> 
> * subversion/svn/help-cmd.c
>   (print_supported_wc_formats): New.
>   (svn_cl__help): Use it to display supported WC formats as a list.
> 
> * subversion/svn/svn.c
>   (parse_compatible_version): Update caller: use
>     svn_client_supported_wc_formats().
> 
> * subversion/tests/cmdline/getopt_tests_data/svn--version_stdout,
>   subversion/tests/cmdline/getopt_tests_data/svn--version--verbose_stdout
>     Update expected output.

> +++ subversion/trunk/subversion/include/svn_client.h Thu Feb 24 15:58:10 2022
> @@ -4428,13 +4428,40 @@ svn_client_upgrade(const char *wcroot_di
>                     apr_pool_t *scratch_pool);
>  
>  /**
> - * Returns the version related to the earliest supported
> + * Returns the version related to the library's default
>   * working copy metadata format.
>   *
>   * @since New in 1.15.
>   */
>  const svn_version_t *
> -svn_client_supported_wc_version(void);
> +svn_client_default_wc_version(apr_pool_t *result_pool);
> +
> +/**
> + * Information about a WC version.
> + *
> + * Only the @c .major and @c .minor version fields are significant: so a
> + * version_max value of 1.15.0 for example means "up to 1.15.x".

Missing @since.

Missing warning not to manually allocate structs of this type since
fields may be added in the future.

> + */
> +typedef struct svn_client_wc_format_t {
> +    /* Oldest version of svn libraries known to support this WC version */
> +    const svn_version_t *version_min;
> +    /* Newest version of svn libraries known to support this WC version. */
> +    const svn_version_t *version_max;
> +    /* The WC format number of this format, as defined by libsvn_wc. */
> +    int wc_format;
> +} svn_client_wc_format_t;
> +
> +/**
> + * Returns a list of the WC formats supported by the client library.
> + *
> + * The list is sorted from oldest to newest, and terminated by an entry
> + * containing all null/zero fields.
> + *
> + * The returned data are allocated in @a result_pool and/or statically.

Missing @since.

> + */
> +const svn_client_wc_format_t *
> +svn_client_supported_wc_formats(apr_pool_t *result_pool,
> +                                apr_pool_t *scratch_pool);

> +++ subversion/trunk/subversion/libsvn_client/upgrade.c Thu Feb 24 15:58:10 2022
> @@ -41,6 +41,7 @@
>  
>  #include "svn_private_config.h"
>  #include "private/svn_wc_private.h"
> +#include "../libsvn_wc/wc.h"

I don't think libsvn_client is allowed to use this header.

libsvn_foo/bar.h headers are for internal use by libsvn_foo, not for
interlibrary use; that would be include/private/.  We generally follow
this, too:

% grep -R 'include.*[.][.]/libsvn' subversion/lib* | grep -Eo '[<"].*[">]' | sort | uniq -c 
     14 "../../libsvn_fs/fs-loader.h"
      2 "../libsvn_delta/delta.h"
     53 "../libsvn_fs/fs-loader.h"
      1 "../libsvn_fs_base/fs_init.h"
      1 "../libsvn_fs_fs/fs_init.h"
      1 "../libsvn_fs_x/fs_init.h"
     24 "../libsvn_ra/ra_loader.h"
      3 "../libsvn_ra/wrapper_template.h"
% 

The RA/FS ones are presumably related to the pluggable design of those
layers.  (Sorry, I haven't got time to confirm this in detail.)

The delta.h one is in FSFS/FSX, which, as the comment there claims, use
SVN_DELTA_WINDOW_SIZE and nothing else declared or #define'd in that
header.

> +++ subversion/trunk/subversion/tests/cmdline/getopt_tests_data/svn--version_stdout Thu Feb 24 15:58:10 2022
> @@ -6,7 +6,10 @@ This software consists of contributions
>  see the NOTICE file for more information.
>  Subversion is open source software, see http://subversion.apache.org/
>  
> -Supported working copy (WC) versions: from 1.8 to 1.15
> +Supported working copy (WC) formats:
> +
> +* compatible with Subversion v1.8 to v1.15 (WC format 31)
> +* compatible with Subversion v1.15 (WC format 32)
>  

Bikeshed, but the bullet point parses as a sentence fragment.  Suggest
instead:

* format 31, compatible with Subversion v1.8 to v1.15

>  The following repository access (RA) modules are available:
>  

Cheers,

Daniel

Re: svn commit: r1898378 - in /subversion/trunk/subversion: include/ libsvn_client/ libsvn_wc/ svn/ tests/cmdline/getopt_tests_data/

[Julian Foad <ju...@apache.org>]

> Output will be, I suggest, like this:
> 
> * WC format 31, compatible with Subversion v1.8 and newer
> * WC format 32, compatible with Subversion v1.15 and newer

r1898524.

Re: svn commit: r1898378 - in /subversion/trunk/subversion: include/ libsvn_client/ libsvn_wc/ svn/ tests/cmdline/getopt_tests_data/

[Julian Foad <ju...@apache.org>]

Thanks for the reviews.

Daniel Shahaf wrote:
> Missing @since. [...]

r1898461.

>> +#include "../libsvn_wc/wc.h"
> 
> I don't think libsvn_client is allowed to use this header.

I'll fix that later.


>> +* compatible with Subversion v1.8 to v1.15 (WC format 31)
>> +* compatible with Subversion v1.15 (WC format 32)
> 
> Bikeshed, but the bullet point parses as a sentence fragment.  Suggest
> instead:
> 
> * format 31, compatible with Subversion v1.8 to v1.15

I had wanted to present the (minimum) compatible-version as the primary
info and the WC format number as secondary. However, it does indeed work
better that way. Also the "to 1.15" is implicit in being supported by
1.15 (unless we were to have disjoint support ranges, but the reader
isn't likely to assume that).

Continuing that way, I can then simplify that "get supported versions
list" API, first removing the "max supported version" from that struct,
and then by using another function to convert from wc-format number to
min-supported version, we can get rid of that struct entirely. Much
cleaner. Return just a list of wc-format ints. I'll do that soon.

Output will be, I suggest, like this:

* WC format 31, compatible with Subversion v1.8 and newer
* WC format 32, compatible with Subversion v1.15 and newer

- Julian