Hi Daniel, Daniel Shahaf writes: > artag...@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
