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

Hi,

Daniel Shahaf writes:
> Stefan Sperling wrote on Sun, Sep 19, 2010 at 11:40:49 +0200:
> > On Sun, Sep 19, 2010 at 10:29:58AM +0100, Daniel Shahaf wrote:
> > > 1. Please don't duplicate code.
> >
> > I think it's fine for svnrdump to have its own copy of this for now.
> > We could at some point merge them into libsvn_subr/ of course, but I
> > don't see a huge problem with just 2 instances.

Ok, let's have this copy for now.

> > > 2. If you do duplicate code, then add big comments (in all instances of
> > > the duplication) pointing to the other instances.
> >
> > +1

I've mentioned it in the commit message, but alright- I'll add a
comment in the file.

> > > 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...
> >
> > We should fix the race in svnrdump on the atomic-revprop branch as well.
> >
>
> No problem; r998622. (The svnsync patch isn't committed yet, but it
> should be easy to port it to svnrdump.)

Ok, let me know when it's done. I'll port it to svnrdump too.

-- Ram
Received on 2010-09-19 13:25:17 CEST