Re: [PATCH] Update to out-of-date code comments

On 27/12/12 17:24, Daniel Shahaf wrote:>> /* 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 svnrdump in
>> - * load_editor.c.
>> + * time-delay loop. A similar function used by svnrdump is defined in
>> + * svnrdump/load_editor.c
>
> Why did you s/duplicate/similar/? I think the former is better (for its
> "if you change me, change that other one too" implication).
>
> That's all. All in all I'm not sure whether to just apply it or wait
> for another iteration, I'll go for the latter. Thanks for the patch!
>
> Daniel
>
Hi Daniel,

Second iteration attached. I've copied the indentation style from one
of your commit messages.

With regard to the get_lock() functions, my reading was that the two
functions now had different arguments and different behaviour - to my
mind, in that case, "duplicate" isn't an appropriate description.

The relevant function bodies are attached at the bottom of this mail
for general convenience.

Apart from the arguments, the differences appear to be cosmetic until
the calls to svn_ra__get_operational_lock() where each call has
arguments has unique arguments provided. I haven't investigated the
significance of this difference.

Of course, the fact that I think this comment is improved by rewording
doesn't make it so - feel free to make the final call on this.

Gabriela

-- 
/* From subversion/svnrdump/load_editor.c */
static svn_error_t *
get_lock(const svn_string_t **lock_string_p,
          svn_ra_session_t *session,
          svn_cancel_func_t cancel_func,
          void *cancel_baton,
          apr_pool_t *pool)
{
   svn_boolean_t be_atomic;
   SVN_ERR(svn_ra_has_capability(session, &be_atomic,
                                 SVN_RA_CAPABILITY_ATOMIC_REVPROPS,
                                 pool));
   if (! be_atomic)
     {
       /* Pre-1.7 servers can't lock without a race condition.  (Issue 
#3546) */
       svn_error_t *err =
         svn_error_create(SVN_ERR_UNSUPPORTED_FEATURE, NULL,
                          _("Target server does not support atomic 
revision "
                            "property edits; consider upgrading it to 
1.7."));
       svn_handle_warning2(stderr, err, "svnrdump: ");
       svn_error_clear(err);
     }
   return svn_ra__get_operational_lock(lock_string_p, NULL, session,
                                       SVNRDUMP_PROP_LOCK, FALSE,
                                       10 /* retries */, 
lock_retry_func, NULL,
                                       cancel_func, cancel_baton, pool);
}
/* From subversion/svnsync/svnsync.c */
static svn_error_t *
get_lock(const svn_string_t **lock_string_p,
          svn_ra_session_t *session,
          svn_boolean_t steal_lock,
          apr_pool_t *pool)
{
   svn_error_t *err;
   svn_boolean_t be_atomic;
   const svn_string_t *stolen_lock;
   SVN_ERR(svn_ra_has_capability(session, &be_atomic,
                                 SVN_RA_CAPABILITY_ATOMIC_REVPROPS,
                                 pool));
   if (! be_atomic)
     {
       /* Pre-1.7 server.  Can't lock without a race condition.
          See issue #3546.
        */
       err = svn_error_create(
               SVN_ERR_UNSUPPORTED_FEATURE, NULL,
               _("Target server does not support atomic revision property "
                 "edits; consider upgrading it to 1.7 or using an external "
                 "locking program"));
       svn_handle_warning2(stderr, err, "svnsync: ");
       svn_error_clear(err);
     }
   err = svn_ra__get_operational_lock(lock_string_p, &stolen_lock, session,
                                      SVNSYNC_PROP_LOCK, steal_lock,
                                      10 /* retries */, lock_retry_func, 
NULL,
                                      check_cancel, NULL, pool);
   if (!err && stolen_lock)
     {
       return svn_cmdline_printf(pool,
                                 _("Stole lock previously held by '%s'\n"),
                                 stolen_lock->data);
     }
   return err;
}