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
Received on 2010-09-20 11:38:49 CEST