subversion-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Stefan Fuhrmann <stefan.fuhrm...@wandisco.com>
Subject Re: svn commit: r1509342 - in /subversion/branches/log-addressing/subversion/libsvn_fs_fs: fs.h fs_fs.c util.c util.h
Date Fri, 02 Aug 2013 15:16:50 GMT
On Fri, Aug 2, 2013 at 5:11 AM, Daniel Shahaf <danielsh@elego.de> wrote:

> stefan2@apache.org wrote on Thu, Aug 01, 2013 at 17:36:42 -0000:
> > Author: stefan2
> > Date: Thu Aug  1 17:36:42 2013
> > New Revision: 1509342
> >
> > URL: http://svn.apache.org/r1509342
> > Log:
> > On the log-addressing branch:
>
> High-level question: how does this compare to fsx?  Is this a feature
> fsx already has that is now being backported to fsfs?
>
> > Bump FSFS format number and introduce
> > the new "addressing" option to fsfs format files.  Make that available
> > to our internal code through the new svn_fs_fs__use_log_addressing API.
> > IOW, we support mixed addressing repos from the beginning.
>
> > +++ subversion/branches/log-addressing/subversion/libsvn_fs_fs/fs_fs.c
> Thu Aug  1 17:36:42 2013
> > @@ -323,17 +329,42 @@ read_format(int *pformat, int *max_files
> > +  /* non-shared repositories never use logical addressing */
> > +  if (!*max_files_per_dir)
> > +    *min_log_addressing_rev = SVN_INVALID_REVNUM;
>
> Can we detect
>
>     7
>     layout linear
>     addressing logical 42
>
> and make it an error?
>

Yes. We should catch that kind inconsistency instead of implementing a
fallback.
Done in r1509609,-27.

> @@ -987,14 +1042,59 @@ svn_fs_fs__create(svn_fs_t *fs,
> > +      /* set compatible version according to generic option */
> > +      compatible = svn_hash_gets(fs->config,
> SVN_FS_CONFIG_COMPATIBLE_VERSION);
> ...
> >        else if (svn_hash_gets(fs->config,
> SVN_FS_CONFIG_PRE_1_8_COMPATIBLE))
> > -        format = 4;
> > +        compatible_version->minor = 7;
> > +
> > +      /* select format number */
> > +      switch(compatible_version->minor)
> > +        {
>
> What about case 0?  Right now it'll fall to the "default" case, I think
> we should either make it an error or funnel it into the 1.1 case.
>

I opted for the error. That's also consistent with FSX's behaviour.

> +          case 1:
> > +          case 2:
> > +          case 3: format = 1;
> > +                  break;
> > +
> > +          case 4: format = 2;
> > +                  break;
> > +
> > +          case 5: format = 3;
> > +                  break;
> > +
> > +          case 6:
> > +          case 7: format = 4;
> > +                  break;
> > +
> > +          case 8: format = 5;
> > +                  break;
>
> Format 5 was never released, I think you meant 6.
>

Oops.

Should the definition of SVN_FS_FS__FORMAT_NUMBER point to this switch()
> statement?  eg, "If you increment this, update svn_fs_fs__create()"
>

Yep.


> > +
> > +          default:format = SVN_FS_FS__FORMAT_NUMBER;
> > +        }
> >      }
> >    ffd->format = format;
> >
> > @@ -1002,6 +1102,12 @@ svn_fs_fs__create(svn_fs_t *fs,
> >    if (format >= SVN_FS_FS__MIN_LAYOUT_FORMAT_OPTION_FORMAT)
> >      ffd->max_files_per_dir = SVN_FS_FS_DEFAULT_MAX_FILES_PER_DIR;
> >
> > +  /* Select the addressing mode depending on the format. */
> > +  if (format >= SVN_FS_FS__MIN_LOG_ADDRESSING_FORMAT)
> > +    ffd->min_log_addressing_rev = 0;
> > +  else
> > +    ffd->min_log_addressing_rev = SVN_INVALID_REVNUM;
>
> Shouldn't you set this to SVN_INVALID_REVNUM in initialize_fs_struct()
> as well?
>

As the struct gets filled with the right values in *_create(), *_open()
and friends, it should not be strictly necessary to init anything in
initialize_fs_struct(). Doing it is more robust, though.

Committed all of the suggested changes in r1509595.

Thanks for the review!

-- Stefan^2.

Mime
View raw message