Ramkumar Ramachandra wrote on Mon, Sep 20, 2010 at 10:35:34 +0530: > Hi Daniel, > > Daniel Shahaf writes: > > Ramkumar Ramachandra wrote on Sun, Sep 19, 2010 at 16:52:50 +0530: > > > 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: > > > > > > 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. > > > > > > > Please add a comment to svnsync's get_lock() as well. > > +1 needed :) >
+1 given, but please s/##/###/ for consistency :) > [[[ > * subversion/svnsync/main.c > (get_lock): Add a comment explaining what the function does along > with a note about it being duplicated in svnrdump. > > * subversion/svnrdump/load_editor.c > (get_lock): Add a comment explaining what the function does along > with a note about it being duplicated in svnsync. > ]]] > > Index: subversion/svnsync/main.c > =================================================================== > --- subversion/svnsync/main.c (revision 998652) > +++ subversion/svnsync/main.c (working copy) > @@ -285,7 +285,13 @@ check_lib_versions(void) > > > /* Acquire a lock (of sorts) on the repository associated with the > - * given RA SESSION. > + * given RA SESSION. This lock is just a revprop change attempt in a > + * time-delay loop. This function is duplicated by svnrdump in > + * load_editor.c. > + * > + * ## TODO: Make this function more generic and > + * expose it through a header for use by other Subversion > + * applications to avoid duplication. > */ > static svn_error_t * > get_lock(svn_ra_session_t *session, apr_pool_t *pool) > Index: subversion/svnrdump/load_editor.c > =================================================================== > --- subversion/svnrdump/load_editor.c (revision 998772) > +++ subversion/svnrdump/load_editor.c (working copy) > @@ -56,6 +56,14 @@ commit_callback(const svn_commit_info_t *commit_in > return SVN_NO_ERROR; > } > > +/* Acquire a lock (of sorts) on the repository associated with the > + * given RA SESSION. This lock is just a revprop change attempt in a > + * time-delay loop. This function is duplicated by svnsync in main.c. > + * > + * ## TODO: Make this function more generic and > + * expose it through a header for use by other Subversion > + * applications to avoid duplication. > + */ > static svn_error_t * > get_lock(svn_ra_session_t *session, apr_pool_t *pool) > { > > > Thanks for the offer. I can do that myself, though; svnrdump is > > a first-class citizen now, as is this duplication (per the direction > > this thread seems to be going), so AFAICT all cards say the branch > > maintainer should be adding that to svnrdump. > > Sure :) > > -- Ram