[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r998502 - /subversion/trunk/subversion/svnrdump/load_editor.c

From: Ramkumar Ramachandra <artagnon_at_gmail.com>
Date: Mon, 20 Sep 2010 09:29:25 +0530

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

This is an archived mail posted to the Subversion Dev mailing list.