Re: svn commit: r1807225 - in /subversion/branches/better-pristines/subversion/libsvn_wc: lock.c upgrade.c wc-metadata.sql wc.h wc_db.c wc_db_private.h wc_db_wcroot.c

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1807225 - in /subversion/branches/better-pristines/subversion/libsvn_wc: lock.c upgrade.c wc-metadata.sql wc.h wc_db.c wc_db_private.h wc_db_wcroot.c

Daniel Shahaf-2
[hidden email] wrote on Mon, 04 Sep 2017 13:58 +0000:

> Author: brane
> Date: Mon Sep  4 13:58:26 2017
> New Revision: 1807225
>
> URL: http://svn.apache.org/viewvc?rev=1807225&view=rev
> Log:
> Begin adding support for multiple working copy formats.
>
> Instead of supporting just one format, introduce a current formaat
> (the default for new working copies) and a lowest supported format,
> and change the way new working copies are created: instead of the
> base schema defining the current format, it defines the lowest
> supporting format and a series of format updates are performed
> to bring it to the current shape.

> +++ subversion/branches/better-pristines/subversion/libsvn_wc/lock.c Mon Sep  4 13:58:26 2017
> @@ -566,10 +566,12 @@ open_single(svn_wc_adm_access_t **adm_ac
>      }
>    SVN_ERR(err);
>  
> -  /* The format version must match exactly. Note that wc_db will perform
> -     an auto-upgrade if allowed. If it does *not*, then it has decided a
> -     manual upgrade is required and it should have raised an error.  */
> -  SVN_ERR_ASSERT(wc_format == SVN_WC__VERSION);
> +  /* The format version must be in the supported version range. Note
> +     that wc_db will perform an auto-upgrade if allowed. If it does
> +     *not*, then it has decided a manual upgrade is required and it
> +     should have raised an error.  */

This is a useful comment for people who work on the upgrade logic.  It
might, therefore, be useful to copy/reference it from svn_wc*upgrade*()?

> +  SVN_ERR_ASSERT(SVN_WC__SUPPORTED_VERSION <= wc_format
> +                 && wc_format <= SVN_WC__VERSION);

> +++ subversion/branches/better-pristines/subversion/libsvn_wc/upgrade.c Mon Sep  4 13:58:26 2017
> @@ -2202,6 +2155,102 @@ svn_wc__upgrade_sdb(int *result_format,
> +svn_error_t *
> +svn_wc__update_schema(int *result_format,
> +                      const char *wcroot_abspath,
> +                      svn_sqlite__db_t *sdb,
> +                      int start_format,
> +                      int target_format,
> +                      apr_pool_t *scratch_pool)
> +{
> +  struct bump_baton bb;
> +  bb.wcroot_abspath = wcroot_abspath;
> +
> +  /* Repeatedly upgrade until the target format version is reached. */
> +  for (*result_format = start_format;
> +       *result_format < target_format;)
> +    {
> +      switch (*result_format)
> +        {
> +          case 19:
> +            SVN_ERR(svn_sqlite__with_lock(sdb, bump_to_20, &bb, scratch_pool));
> +            *result_format = 20;
> +            break;
> +
> +          case 20:
> +            SVN_ERR(svn_sqlite__with_lock(sdb, bump_to_21, &bb, scratch_pool));
> +            *result_format = 21;
> +            break;
> +
> +          case 21:
> +            SVN_ERR(svn_sqlite__with_lock(sdb, bump_to_22, &bb, scratch_pool));
> +            *result_format = 22;
> +            break;

This block is very repetitive.  I wonder if we should use:

    #define FOO(twenty_two) \
      case (twenty_two)-1: \
        SVN_ERR(svn_sqlite__with_lock(sdb, bump_to_##twenty_two, &bb, scratch_pool)); \
        *result_format = twenty_two; \
        break;

    switch (*result_format)
      {
        FOO(20);
        FOO(21);
        FOO(22);
        ⋮
      }

>

Cheers,

Daniel
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1807225 - in /subversion/branches/better-pristines/subversion/libsvn_wc: lock.c upgrade.c wc-metadata.sql wc.h wc_db.c wc_db_private.h wc_db_wcroot.c

Branko Čibej
On 05.09.2017 15:58, Daniel Shahaf wrote:

> [hidden email] wrote on Mon, 04 Sep 2017 13:58 +0000:
>> Author: brane
>> Date: Mon Sep  4 13:58:26 2017
>> New Revision: 1807225
>>
>> URL: http://svn.apache.org/viewvc?rev=1807225&view=rev
>> Log:
>> Begin adding support for multiple working copy formats.
>>
>> Instead of supporting just one format, introduce a current formaat
>> (the default for new working copies) and a lowest supported format,
>> and change the way new working copies are created: instead of the
>> base schema defining the current format, it defines the lowest
>> supporting format and a series of format updates are performed
>> to bring it to the current shape.
>> +++ subversion/branches/better-pristines/subversion/libsvn_wc/lock.c Mon Sep  4 13:58:26 2017
>> @@ -566,10 +566,12 @@ open_single(svn_wc_adm_access_t **adm_ac
>>      }
>>    SVN_ERR(err);
>>  
>> -  /* The format version must match exactly. Note that wc_db will perform
>> -     an auto-upgrade if allowed. If it does *not*, then it has decided a
>> -     manual upgrade is required and it should have raised an error.  */
>> -  SVN_ERR_ASSERT(wc_format == SVN_WC__VERSION);
>> +  /* The format version must be in the supported version range. Note
>> +     that wc_db will perform an auto-upgrade if allowed. If it does
>> +     *not*, then it has decided a manual upgrade is required and it
>> +     should have raised an error.  */
> This is a useful comment for people who work on the upgrade logic.  It
> might, therefore, be useful to copy/reference it from svn_wc*upgrade*()?

Ack, thanks.

>> +  SVN_ERR_ASSERT(SVN_WC__SUPPORTED_VERSION <= wc_format
>> +                 && wc_format <= SVN_WC__VERSION);
>> +++ subversion/branches/better-pristines/subversion/libsvn_wc/upgrade.c Mon Sep  4 13:58:26 2017
>> @@ -2202,6 +2155,102 @@ svn_wc__upgrade_sdb(int *result_format,
>> +svn_error_t *
>> +svn_wc__update_schema(int *result_format,
>> +                      const char *wcroot_abspath,
>> +                      svn_sqlite__db_t *sdb,
>> +                      int start_format,
>> +                      int target_format,
>> +                      apr_pool_t *scratch_pool)
>> +{
>> +  struct bump_baton bb;
>> +  bb.wcroot_abspath = wcroot_abspath;
>> +
>> +  /* Repeatedly upgrade until the target format version is reached. */
>> +  for (*result_format = start_format;
>> +       *result_format < target_format;)
>> +    {
>> +      switch (*result_format)
>> +        {
>> +          case 19:
>> +            SVN_ERR(svn_sqlite__with_lock(sdb, bump_to_20, &bb, scratch_pool));
>> +            *result_format = 20;
>> +            break;
>> +
>> +          case 20:
>> +            SVN_ERR(svn_sqlite__with_lock(sdb, bump_to_21, &bb, scratch_pool));
>> +            *result_format = 21;
>> +            break;
>> +
>> +          case 21:
>> +            SVN_ERR(svn_sqlite__with_lock(sdb, bump_to_22, &bb, scratch_pool));
>> +            *result_format = 22;
>> +            break;
> This block is very repetitive.  I wonder if we should use:
>
>     #define FOO(twenty_two) \
>       case (twenty_two)-1: \
>         SVN_ERR(svn_sqlite__with_lock(sdb, bump_to_##twenty_two, &bb, scratch_pool)); \
>         *result_format = twenty_two; \
>         break;
>
>     switch (*result_format)
>       {
>         FOO(20);
>         FOO(21);
>         FOO(22);
>         ⋮
>       }

It wasn't originally; before my changes, these cases were all
fall-through and the stats1 table insertion was within the switch block
... which was just a bit too much of a good thing. :)

I think I might take your advice and add the boilerplate macro, if only
to avoid future typos.

-- Brane