On Fri, Aug 2, 2013 at 5:11 AM, Daniel Shahaf <danie...@elego.de> wrote:
> stef...@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.
