You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2014/08/06 22:15:44 UTC
svn commit: r1616338 - /subversion/trunk/subversion/libsvn_fs_fs/fs.c
Author: stefan2
Date: Wed Aug 6 20:15:43 2014
New Revision: 1616338
URL: http://svn.apache.org/r1616338
Log:
Beautify bits of the hotcopying code.
* subversion/libsvn_fs_fs/fs.c
(uninitialize_fs_struct): New utility function.
(fs_hotcopy): Use that new function to make the init/uninit symmetry
more obvious and add more commentary.
Modified:
subversion/trunk/subversion/libsvn_fs_fs/fs.c
Modified: subversion/trunk/subversion/libsvn_fs_fs/fs.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs.c?rev=1616338&r1=1616337&r2=1616338&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/fs.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/fs.c Wed Aug 6 20:15:43 2014
@@ -245,6 +245,16 @@ initialize_fs_struct(svn_fs_t *fs)
return SVN_NO_ERROR;
}
+/* Reset vtable and fsap_data fields in FS such that the FS is basically
+ * closed now. Note that FS must not hold locks when you call this. */
+static svn_error_t *
+uninitialize_fs_struct(svn_fs_t *fs)
+{
+ fs->vtable = NULL;
+ fs->fsap_data = NULL;
+ return SVN_NO_ERROR;
+}
+
/* This implements the fs_library_vtable_t.create() API. Create a new
fsfs-backed Subversion filesystem at path PATH and link it into
*FS. Perform temporary allocations in POOL, and fs-global allocations
@@ -413,11 +423,13 @@ fs_hotcopy(svn_fs_t *src_fs,
if (cancel_func)
SVN_ERR(cancel_func(cancel_baton));
- /* Provide FFD for DST_FS, test / initialize target repo, remove FFD. */
+ /* Test target repo when in INCREMENTAL mode, initialize it when not.
+ * For this, we need our FS internal data structures to be temporarily
+ * available. */
SVN_ERR(initialize_fs_struct(dst_fs));
SVN_ERR(svn_fs_fs__hotcopy_prepare_target(src_fs, dst_fs, dst_path,
incremental, pool));
- dst_fs->fsap_data = NULL;
+ SVN_ERR(uninitialize_fs_struct(dst_fs));
/* Now, the destination repo should open just fine. */
SVN_ERR(fs_open(dst_fs, dst_path, common_pool_lock, pool, common_pool));
Re: svn commit: r1616338 - /subversion/trunk/subversion/libsvn_fs_fs/fs.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Mon, Aug 11, 2014 at 6:41 PM, Stefan Sperling <st...@elego.de> wrote:
> On Mon, Aug 11, 2014 at 06:35:09PM +0200, Stefan Fuhrmann wrote:
> > On Wed, Aug 6, 2014 at 10:59 PM, Stefan Sperling <st...@elego.de> wrote:
> > > > @@ -245,6 +245,16 @@ initialize_fs_struct(svn_fs_t *fs)
> > > > return SVN_NO_ERROR;
> > > > }
> > > >
> > > > +/* Reset vtable and fsap_data fields in FS such that the FS is
> basically
> > > > + * closed now. Note that FS must not hold locks when you call
> this. */
> > > > +static svn_error_t *
> > > > +uninitialize_fs_struct(svn_fs_t *fs)
> > >
> > > I'd suggest to declare this as:
> > >
> > > static void
> > > uninitialize_fs_struct(svn_fs_t *fs)
> > >
> >
> > Thanks for the review.
> >
> > You are right, I might change that to plain void but I'll keep
> > the error return type for now, just for symmetry with the init
> > code. This is also a function that might have actual error
> > conditions in later releases and a missed caller update would
> > than create an error leak (and probably worse).
> >
> > -- Stefan^2.
>
> By that logic we shouldn't have any void functions ;-)
>
> I'm still in favour of changing it because it makes the code look
> untidy from my point of view. But whatever...
>
Alright, alright :), changed in r1617688.
-- Stefan^2.
Re: svn commit: r1616338 -
/subversion/trunk/subversion/libsvn_fs_fs/fs.c
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Aug 11, 2014 at 06:35:09PM +0200, Stefan Fuhrmann wrote:
> On Wed, Aug 6, 2014 at 10:59 PM, Stefan Sperling <st...@elego.de> wrote:
> > > @@ -245,6 +245,16 @@ initialize_fs_struct(svn_fs_t *fs)
> > > return SVN_NO_ERROR;
> > > }
> > >
> > > +/* Reset vtable and fsap_data fields in FS such that the FS is basically
> > > + * closed now. Note that FS must not hold locks when you call this. */
> > > +static svn_error_t *
> > > +uninitialize_fs_struct(svn_fs_t *fs)
> >
> > I'd suggest to declare this as:
> >
> > static void
> > uninitialize_fs_struct(svn_fs_t *fs)
> >
>
> Thanks for the review.
>
> You are right, I might change that to plain void but I'll keep
> the error return type for now, just for symmetry with the init
> code. This is also a function that might have actual error
> conditions in later releases and a missed caller update would
> than create an error leak (and probably worse).
>
> -- Stefan^2.
By that logic we shouldn't have any void functions ;-)
I'm still in favour of changing it because it makes the code look
untidy from my point of view. But whatever...
Re: svn commit: r1616338 - /subversion/trunk/subversion/libsvn_fs_fs/fs.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Wed, Aug 6, 2014 at 10:59 PM, Stefan Sperling <st...@elego.de> wrote:
> On Wed, Aug 06, 2014 at 08:15:44PM -0000, stefan2@apache.org wrote:
> > Author: stefan2
> > Date: Wed Aug 6 20:15:43 2014
> > New Revision: 1616338
> >
> > URL: http://svn.apache.org/r1616338
> > Log:
> > Beautify bits of the hotcopying code.
> >
> > * subversion/libsvn_fs_fs/fs.c
> > (uninitialize_fs_struct): New utility function.
> > (fs_hotcopy): Use that new function to make the init/uninit symmetry
> > more obvious and add more commentary.
> >
> > Modified:
> > subversion/trunk/subversion/libsvn_fs_fs/fs.c
> >
> > Modified: subversion/trunk/subversion/libsvn_fs_fs/fs.c
> > URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs.c?rev=1616338&r1=1616337&r2=1616338&view=diff
> >
> ==============================================================================
> > --- subversion/trunk/subversion/libsvn_fs_fs/fs.c (original)
> > +++ subversion/trunk/subversion/libsvn_fs_fs/fs.c Wed Aug 6 20:15:43
> 2014
> > @@ -245,6 +245,16 @@ initialize_fs_struct(svn_fs_t *fs)
> > return SVN_NO_ERROR;
> > }
> >
> > +/* Reset vtable and fsap_data fields in FS such that the FS is basically
> > + * closed now. Note that FS must not hold locks when you call this. */
> > +static svn_error_t *
> > +uninitialize_fs_struct(svn_fs_t *fs)
>
> I'd suggest to declare this as:
>
> static void
> uninitialize_fs_struct(svn_fs_t *fs)
>
Thanks for the review.
You are right, I might change that to plain void but I'll keep
the error return type for now, just for symmetry with the init
code. This is also a function that might have actual error
conditions in later releases and a missed caller update would
than create an error leak (and probably worse).
-- Stefan^2.
Re: svn commit: r1616338 -
/subversion/trunk/subversion/libsvn_fs_fs/fs.c
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Aug 06, 2014 at 08:15:44PM -0000, stefan2@apache.org wrote:
> Author: stefan2
> Date: Wed Aug 6 20:15:43 2014
> New Revision: 1616338
>
> URL: http://svn.apache.org/r1616338
> Log:
> Beautify bits of the hotcopying code.
>
> * subversion/libsvn_fs_fs/fs.c
> (uninitialize_fs_struct): New utility function.
> (fs_hotcopy): Use that new function to make the init/uninit symmetry
> more obvious and add more commentary.
>
> Modified:
> subversion/trunk/subversion/libsvn_fs_fs/fs.c
>
> Modified: subversion/trunk/subversion/libsvn_fs_fs/fs.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs.c?rev=1616338&r1=1616337&r2=1616338&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_fs_fs/fs.c (original)
> +++ subversion/trunk/subversion/libsvn_fs_fs/fs.c Wed Aug 6 20:15:43 2014
> @@ -245,6 +245,16 @@ initialize_fs_struct(svn_fs_t *fs)
> return SVN_NO_ERROR;
> }
>
> +/* Reset vtable and fsap_data fields in FS such that the FS is basically
> + * closed now. Note that FS must not hold locks when you call this. */
> +static svn_error_t *
> +uninitialize_fs_struct(svn_fs_t *fs)
I'd suggest to declare this as:
static void
uninitialize_fs_struct(svn_fs_t *fs)
> +{
> + fs->vtable = NULL;
> + fs->fsap_data = NULL;
> + return SVN_NO_ERROR;
> +}
> +
> /* This implements the fs_library_vtable_t.create() API. Create a new
> fsfs-backed Subversion filesystem at path PATH and link it into
> *FS. Perform temporary allocations in POOL, and fs-global allocations
> @@ -413,11 +423,13 @@ fs_hotcopy(svn_fs_t *src_fs,
> if (cancel_func)
> SVN_ERR(cancel_func(cancel_baton));
>
> - /* Provide FFD for DST_FS, test / initialize target repo, remove FFD. */
> + /* Test target repo when in INCREMENTAL mode, initialize it when not.
> + * For this, we need our FS internal data structures to be temporarily
> + * available. */
> SVN_ERR(initialize_fs_struct(dst_fs));
> SVN_ERR(svn_fs_fs__hotcopy_prepare_target(src_fs, dst_fs, dst_path,
> incremental, pool));
> - dst_fs->fsap_data = NULL;
> + SVN_ERR(uninitialize_fs_struct(dst_fs));
>
> /* Now, the destination repo should open just fine. */
> SVN_ERR(fs_open(dst_fs, dst_path, common_pool_lock, pool, common_pool));
>