[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r33043 - trunk/subversion/libsvn_client

From: Hyrum K. Wright <hyrum_wright_at_mail.utexas.edu>
Date: Sat, 13 Sep 2008 13:21:29 -0500

Philip Martin wrote:
> hwright_at_tigris.org writes:
>
>> Author: hwright
>> Date: Fri Sep 12 12:10:13 2008
>> New Revision: 33043
>>
>> Log:
>> Directly return some errors, removing superfluous invocations of SVN_ERR().
>> This only handles libsvn_client; the other libraries could be similarly
>> audited.
>>
>> I tried not to be too agressive when doing this. There's a balance between
>> maintainability and efficiency: there are probably other places where we can
>> directly return, but tracking those down is tricky, and can introduce subtle
>> bugs for future maintainers.
>
> Looks like pointless code churn to me; every active branch is now
> facing a massive merge. What is the gain?

Aside from the slight performance gain, this also adds some consistency.
Instead of making these changes piecemeal, usually along with unrelated changes
in the same files, I thought it better to do an audit of the code and do them
all at once.

Branches have faced similar massive merges and survived, such as when
SVN_DEPRECATED was recently introduced, or when doing EOL-whitespace
elimination. I don't anticipate merging being that much of a problem here, and
I don't think ease-of-merging should drive code improvements. If you have a
branch you'll be merging too, and would like help resolving any conflicts, let
me know.

>> @@ -1939,26 +1922,23 @@ setup_copy(svn_commit_info_t **commit_in
>> if ((! srcs_are_urls) && (! dst_is_url))
>> {
>> *commit_info_p = NULL;
>> - SVN_ERR(wc_to_wc_copy(copy_pairs, is_move, make_parents,
>> - ctx, pool));
>> + return wc_to_wc_copy(copy_pairs, is_move, make_parents, ctx, pool);
>> }
>> else if ((! srcs_are_urls) && (dst_is_url))
>> {
>> - SVN_ERR(wc_to_repos_copy(commit_info_p, copy_pairs, make_parents,
>> - revprop_table, ctx, pool));
>> + return wc_to_repos_copy(commit_info_p, copy_pairs, make_parents,
>> + revprop_table, ctx, pool);
>> }
>> else if ((srcs_are_urls) && (! dst_is_url))
>> {
>> *commit_info_p = NULL;
>> - SVN_ERR(repos_to_wc_copy(copy_pairs, make_parents, ctx, pool));
>> + return repos_to_wc_copy(copy_pairs, make_parents, ctx, pool);
>> }
>> else
>> {
>> - SVN_ERR(repos_to_repos_copy(commit_info_p, copy_pairs, make_parents,
>> - revprop_table, ctx, is_move, pool));
>> + return repos_to_repos_copy(commit_info_p, copy_pairs, make_parents,
>> + revprop_table, ctx, is_move, pool);
>> }
>> -
>> - return SVN_NO_ERROR;
>> }
>
> It might be an improvement in short functions, but in cases like the
> code above it's not clear that your new code is better--if pushed I
> probably prefer the original code.

In this case, and others like it, I disagree. By directly returning, we help
show the organization of the code: setup_copy() performs some common work and
then dispatches the remainder of the processing to the respective helper functions.

But, that's just my feeling, if there is strong sentiment to the contrary, we
can revert it.

-Hyrum

Received on 2008-09-13 20:42:01 CEST

This is an archived mail posted to the Subversion Dev mailing list.