You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Stefan Sperling <st...@elego.de> on 2012/10/16 12:40:39 UTC
Re: svn commit: r1398598 -
/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
On Tue, Oct 16, 2012 at 01:12:10AM -0000, stefan2@apache.org wrote:
> Author: stefan2
> Date: Tue Oct 16 01:12:09 2012
> New Revision: 1398598
>
> URL: http://svn.apache.org/viewvc?rev=1398598&view=rev
> Log:
> Fix svnadmin hotcopy for packed format 6 repos. Packed revprops were
> not copied properly. Fixes issue #4246.
>
> * subversion/libsvn_fs_fs/fs_fs.c
> (hotcopy_copy_packed_shard): use "packed" copying for packed revprops
>
> Modified:
> subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
>
> Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c?rev=1398598&r1=1398597&r2=1398598&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
> +++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Tue Oct 16 01:12:09 2012
> @@ -10316,6 +10316,7 @@ hotcopy_copy_packed_shard(svn_revnum_t *
> const char *src_subdir_packed_shard;
> svn_revnum_t revprop_rev;
> apr_pool_t *iterpool;
> + fs_fs_data_t *src_ffd = src_fs->fsap_data;
>
> /* Copy the packed shard. */
> src_subdir = svn_dirent_join(src_fs->path, PATH_REVS_DIR, scratch_pool);
> @@ -10333,18 +10334,43 @@ hotcopy_copy_packed_shard(svn_revnum_t *
> /* Copy revprops belonging to revisions in this pack. */
> src_subdir = svn_dirent_join(src_fs->path, PATH_REVPROPS_DIR, scratch_pool);
> dst_subdir = svn_dirent_join(dst_fs->path, PATH_REVPROPS_DIR, scratch_pool);
> - iterpool = svn_pool_create(scratch_pool);
> - for (revprop_rev = rev;
> - revprop_rev < rev + max_files_per_dir;
> - revprop_rev++)
> +
> + if ( src_ffd->format < SVN_FS_FS__MIN_PACKED_REVPROP_FORMAT
> + || src_ffd->min_unpacked_rev < rev + max_files_per_dir)
The spacing here is a bit... unconventional.
I'd expect this code to be formatted like this:
if (src_ffd->format < SVN_FS_FS__MIN_PACKED_REVPROP_FORMAT
|| src_ffd->min_unpacked_rev < rev + max_files_per_dir)
or like this:
if (src_ffd->format < SVN_FS_FS__MIN_PACKED_REVPROP_FORMAT ||
src_ffd->min_unpacked_rev < rev + max_files_per_dir)
Other than that, this commit is looking good to me.
> {
> - svn_pool_clear(iterpool);
> + /* copy unpacked revprops rev by rev */
> + iterpool = svn_pool_create(scratch_pool);
> + for (revprop_rev = rev;
> + revprop_rev < rev + max_files_per_dir;
> + revprop_rev++)
> + {
> + svn_pool_clear(iterpool);
>
> - SVN_ERR(hotcopy_copy_shard_file(src_subdir, dst_subdir,
> - revprop_rev, max_files_per_dir,
> - iterpool));
> + SVN_ERR(hotcopy_copy_shard_file(src_subdir, dst_subdir,
> + revprop_rev, max_files_per_dir,
> + iterpool));
> + }
> + svn_pool_destroy(iterpool);
> + }
> + else
> + {
> + /* revprop for revision 0 will never be packed */
> + if (rev == 0)
> + SVN_ERR(hotcopy_copy_shard_file(src_subdir, dst_subdir,
> + 0, max_files_per_dir,
> + scratch_pool));
> +
> + /* packed revprops folder */
> + packed_shard = apr_psprintf(scratch_pool, "%ld" PATH_EXT_PACKED_SHARD,
> + rev / max_files_per_dir);
> + src_subdir_packed_shard = svn_dirent_join(src_subdir, packed_shard,
> + scratch_pool);
> + SVN_ERR(hotcopy_io_copy_dir_recursively(src_subdir_packed_shard,
> + dst_subdir, packed_shard,
> + TRUE /* copy_perms */,
> + NULL /* cancel_func */, NULL,
> + scratch_pool));
> }
> - svn_pool_destroy(iterpool);
>
> /* If necessary, update the min-unpacked rev file in the hotcopy. */
> if (*dst_min_unpacked_rev < rev + max_files_per_dir)
>
Re: svn commit: r1398598 - /subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Tue, Oct 16, 2012 at 12:40 PM, Stefan Sperling <st...@elego.de> wrote:
> On Tue, Oct 16, 2012 at 01:12:10AM -0000, stefan2@apache.org wrote:
> > Author: stefan2
> > Date: Tue Oct 16 01:12:09 2012
> > New Revision: 1398598
> >
> > URL: http://svn.apache.org/viewvc?rev=1398598&view=rev
> > Log:
> > Fix svnadmin hotcopy for packed format 6 repos. Packed revprops were
> > not copied properly. Fixes issue #4246.
> >
> > * subversion/libsvn_fs_fs/fs_fs.c
> > (hotcopy_copy_packed_shard): use "packed" copying for packed revprops
> >
> > Modified:
> > subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
> >
> > Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
> > URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c?rev=1398598&r1=1398597&r2=1398598&view=diff
> >
> ==============================================================================
> > --- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
> > +++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Tue Oct 16 01:12:09
> 2012
> > @@ -10316,6 +10316,7 @@ hotcopy_copy_packed_shard(svn_revnum_t *
> > const char *src_subdir_packed_shard;
> > svn_revnum_t revprop_rev;
> > apr_pool_t *iterpool;
> > + fs_fs_data_t *src_ffd = src_fs->fsap_data;
> >
> > /* Copy the packed shard. */
> > src_subdir = svn_dirent_join(src_fs->path, PATH_REVS_DIR,
> scratch_pool);
> > @@ -10333,18 +10334,43 @@ hotcopy_copy_packed_shard(svn_revnum_t *
> > /* Copy revprops belonging to revisions in this pack. */
> > src_subdir = svn_dirent_join(src_fs->path, PATH_REVPROPS_DIR,
> scratch_pool);
> > dst_subdir = svn_dirent_join(dst_fs->path, PATH_REVPROPS_DIR,
> scratch_pool);
> > - iterpool = svn_pool_create(scratch_pool);
> > - for (revprop_rev = rev;
> > - revprop_rev < rev + max_files_per_dir;
> > - revprop_rev++)
> > +
> > + if ( src_ffd->format < SVN_FS_FS__MIN_PACKED_REVPROP_FORMAT
> > + || src_ffd->min_unpacked_rev < rev + max_files_per_dir)
>
> The spacing here is a bit... unconventional.
>
I completely agree but I've grown to like it
for the following reasons:
> I'd expect this code to be formatted like this:
>
> if (src_ffd->format < SVN_FS_FS__MIN_PACKED_REVPROP_FORMAT
> || src_ffd->min_unpacked_rev < rev + max_files_per_dir)
>
Operand "nesting" level is obscured (just slightly).
Remember, how you learned in school to do a tabular /
place-aligned addition or subtraction.
> or like this:
>
> if (src_ffd->format < SVN_FS_FS__MIN_PACKED_REVPROP_FORMAT ||
> src_ffd->min_unpacked_rev < rev + max_files_per_dir)
>
That moves the root of the operation tree, i.e. the ||
operation, out of sight.
> Other than that, this commit is looking good to me.
>
I don't cling to my tabular formatting but I use it for
the above reasons rather than at a whim.
-- Stefan^2.
--
*
Join us this October at Subversion Live
2012<http://www.wandisco.com/svn-live-2012>
for two days of best practice SVN training, networking, live demos,
committer meet and greet, and more! Space is limited, so get signed up
today<http://www.wandisco.com/svn-live-2012>
!
*