You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Gabriela Gibson <ga...@gmail.com> on 2012/12/27 15:13:01 UTC
[PATCH] Update to out-of-date code comments
[[[
Update code comments in a number of files which refer to "main.c", where
the file has been renamed.
Update to comments in subversion/svnsync/svnsync.c and
subversion/svnrdump/load_editor.c which referred to the function
(get_lock) being duplicated, where the prototypes differ slightly.
* subversion/svnrdump/load_editor.c:
(get_lock): Changed comment to refer to "similar" function.
in subversion/svnsync/svnsync.c
Changed comment to fix reference to "main.c
* subversion/svnsync/svnsync.c:
(get_lock): Changed comment to refer to "similar" function.
in subversion/svnsync/svnsync.c
* subversion/tests/svn_test_main.c:
(Opening comment): File name fixed
* subversion/svndumpfilter/svndumpfilter.c:
(Opening comment): File name fixed
* subversion/svn/cl.h:
(svn_cl__cmd_table[]): File name fixed in comment
(svn_cl__global_options[]): File name fixed in comment
(apr_getopt_option_t svn_cl__options[]): File name fixed in comment
* subversion/svn/svn.c:
(Opening comment): File name fixed
* subversion/svnadmin/svnadmin.c:
(Opening comment): File name fixed
* subversion/svnlook/svnlook.c:
(Opening comment): File name fixed
* subversion/svnserve/svnserve.c:
(Opening comment): File name fixed
Patch by Gabriela Gibson <gabriela.gibson_at_gmail.com>
]]]
Re: [PATCH] Update to out-of-date code comments
Posted by Daniel Shahaf <da...@elego.de>.
Gabriela Gibson wrote on Fri, Dec 28, 2012 at 11:47:08 +0000:
> Corrections attached, thanks for the advice!
Applie in r1426480 with minimal tweaks to the log message. Thanks!
Re: [PATCH] Update to out-of-date code comments
Posted by Gabriela Gibson <ga...@gmail.com>.
Corrections attached, thanks for the advice!
(please pardon the thread mess, I had mail issues)
Re: [PATCH] Update to out-of-date code comments
Posted by Daniel Shahaf <da...@elego.de>.
Gabriela Gibson wrote on Thu, Dec 27, 2012 at 22:09:40 +0000:
> 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.
>
Wrong attachment? It seems you re-attached the first iteration.
> 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.
>
True. But to me, "duplicate" has a subtext of "if you update one of
them, update the other too", that's why I prefer it. (Also, the
functions really are duplicates: the svnrdump one is identical to the
svnsync one with a curried steal_lock=FALSE.)
> 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.
Well, between two opinions, I'll fall back to "Status quo wins" --- at
least until there's a third opinion.
Re: [PATCH] Update to out-of-date code comments
Posted by Gabriela Gibson <ga...@gmail.com>.
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;
}
Re: [PATCH] Update to out-of-date code comments
Posted by Daniel Shahaf <da...@elego.de>.
Gabriela Gibson wrote on Thu, Dec 27, 2012 at 14:13:01 +0000:
> [[[
> Update code comments in a number of files which refer to "main.c", where
> the file has been renamed.
>
> Update to comments in subversion/svnsync/svnsync.c and
> subversion/svnrdump/load_editor.c which referred to the function
> (get_lock) being duplicated, where the prototypes differ slightly.
>
"the function get_lock()"
> * subversion/svnrdump/load_editor.c:
> (get_lock): Changed comment to refer to "similar" function.
Two spaces indent, not four. Also we prefer imperative to past tense
here.
> Patch by Gabriela Gibson <gabriela.gibson_at_gmail.com>
Colon after 'by'.
> Index: subversion/tests/svn_test_main.c
> ===================================================================
> --- subversion/tests/svn_test_main.c (revision 1426182)
> +++ subversion/tests/svn_test_main.c (working copy)
> @@ -1,5 +1,5 @@
> /*
> - * tests-main.c: shared main() & friends for SVN test-suite programs
> + * svn_tests_main.c: shared main() & friends for SVN test-suite programs
Typo in filename ("tests" singular).
> /* 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