Stefan Sperling wrote on Tue, May 08, 2012 at 12:56:46 +0200:
> On Thu, Apr 26, 2012 at 10:04:35PM -0000, danielsh_at_apache.org wrote:
> > Author: danielsh
> > Date: Thu Apr 26 22:04:34 2012
> > New Revision: 1331125
> >
> > URL: http://svn.apache.org/viewvc?rev=1331125&view=rev
> > Log:
> > Follow-up to r1331050: try to unbreak the build.
> >
> > * subversion/libsvn_fs_fs/fs.c
> > (fs_hotcopy): Properly open SRC_FS. Call svn_fs__check_fs(dst_fs).
>
> The log message lacks some substance.
> What exactly is the problem being solved by opening the source FS
> here instead of letting fs_fs.c open it?
>
> I understand the rush involved in fixing the build, but it would
> be nice to revise this log message to make the semantic change
> that actually happened more clear.
>
Edied the log message in light of these comments.
> One more remark below.
>
> >
> > 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=1331125&r1=1331124&r2=1331125&view=diff
> > ==============================================================================
> > --- subversion/trunk/subversion/libsvn_fs_fs/fs.c (original)
> > +++ subversion/trunk/subversion/libsvn_fs_fs/fs.c Thu Apr 26 22:04:34 2012
> > @@ -293,10 +293,34 @@ fs_hotcopy(svn_fs_t *src_fs,
> > void *cancel_baton,
> > apr_pool_t *pool)
> > {
> > - SVN_ERR(initialize_fs_struct(src_fs));
> > - SVN_ERR(fs_serialized_init(src_fs, pool, pool));
> > - SVN_ERR(initialize_fs_struct(dst_fs));
> > - SVN_ERR(fs_serialized_init(dst_fs, pool, pool));
> > + {
> > + svn_fs_t *fs = src_fs;
> > + const char *path = src_path;
> > +
> > + SVN_ERR(svn_fs__check_fs(fs, FALSE));
> > + SVN_ERR(initialize_fs_struct(fs));
> > + SVN_ERR(svn_fs_fs__open(fs, path, pool));
> > + SVN_ERR(svn_fs_fs__initialize_caches(fs, pool));
> > + SVN_ERR(fs_serialized_init(fs, pool, pool));
> > + }
> > +
> > + {
> > + svn_fs_t *fs = dst_fs;
> > + const char *path = dst_path;
> > +
> > + SVN_ERR(svn_fs__check_fs(fs, FALSE));
> > + SVN_ERR(initialize_fs_struct(fs));
> > +#if 0
>
> Did you intend to clean up this #if 0 marker or leave it in here forever?
>
> I find it is somewhat confusing. It makes it look as if we were undecided
> about what to do here.
>
The code is #if 0'd because I wanted it when incremental=TRUE but it
breaks the incremental=FALSE case. We should remove that temp code and
ensure we open DST_FS and initialize its caches properly in all case.s
> > + /* In INCREMENTAL mode, svn_fs_fs__hotcopy() will open DST_FS.
> > + Otherwise, it's not an FS yet --- possibly just an empty dir --- so
> > + can't be opened.
> > + */
> > + SVN_ERR(svn_fs_fs__open(fs, path, pool));
> > + SVN_ERR(svn_fs_fs__initialize_caches(fs, pool));
> > +#endif
> > + SVN_ERR(fs_serialized_init(fs, pool, pool));
> > + }
> > +
> > return svn_fs_fs__hotcopy(src_fs, dst_fs, src_path, dst_path,
> > incremental, cancel_func, cancel_baton, pool);
> > }
> >
Received on 2012-05-08 13:35:11 CEST