Hi Daniel,
Daniel Shahaf writes:
> artagnon_at_apache.org wrote on Sat, Sep 18, 2010 at 17:54:50 -0000:
> > +#define SVNRDUMP_PROP_LOCK "rdump-lock"
>
> Need SVN_PROP_PREFIX.
Fixed in r998772.
> 1. Please don't duplicate code.
>
> 2. If you do duplicate code, then add big comments (in all instances of
> the duplication) pointing to the other instances.
>
> 3. Incidentally, I have modified the svnsync instance of this function
> on the atomic-revprops branch. So the desire to avoid duplication isn't
> just theoretical in this case...
Discussed in another email.
> > - /* ### TODO: Figure out if we're allowed to set revprops before
> > - ### we're too late and mess up the repository. svnsync uses some
> > - ### sort of locking mechanism. */
> > -
> > + SVN_ERR(get_lock(session, pool));
> > SVN_ERR(svn_ra_get_repos_root2(session, &(pb->root_url), pool));
> > SVN_ERR(svn_repos_parse_dumpstream2(stream, parser, parse_baton,
> > NULL, NULL, pool));
> > + SVN_ERR(svn_ra_change_rev_prop(session, 0, SVNRDUMP_PROP_LOCK, NULL, pool));
>
> 1. You can't change a property which isn't in the svn:* namespace.
Fixed in r998772.
> 2. Isn't svn_repos_parse_dumpstream2() an expensive function? You could
> do the revprop test before that, in order to (if you have to fail) fail
> sooner.
Oh, the last svn_ra_change_rev_prop is only meant to release the
lock. I do the revprop test in the first line: see get_lock.
Thanks for the review.
-- Ram
Received on 2010-09-20 06:01:31 CEST