On Wed, Nov 02, 2005 at 09:53:17PM +0000, Philip Martin wrote:
> > 2. We could also move the initialisation of the localprops local
> > variable further down, but I assume that the call to apr_hash_make()
> > is inexpensive.
>
> If you used the "big if" style it could go inside the if.
>
You're right. I moved local_propchanges as well.
> > + /* Load the BASE props. */
>
> I'm not a fan of "trivial" comments, ones that state what the code
> does rather than why, and I think that one comes into the trivial
> category.
>
> > + /* Load the WORKING props. */
>
> That one is also trivial.
>
Okay, agreed. I used them when I wasn't sure of what was going on,
but they don't impart anything useful now.
Updated patch attached. I also realised that the calls to
svn_wc__prop_path() and svn_wc__load_prop_file() to read the WORKING
properties file exactly duplicated the logic in svn_wc_props_list(),
so I replaced them with a call to that instead.
[[[
* subversion/libsvn_wc/props.c
(svn_wc_get_prop_diffs): Rearrange, avoiding an unnecessary read of
the WORKING properties file if the caller doesn't want to receive
property changes. Replace calls to read the properties file with
a single call to svn_wc_props_list().
]]]
Regards,
Malcolm
Index: subversion/libsvn_wc/props.c
===================================================================
--- subversion/libsvn_wc/props.c (revision 17151)
+++ subversion/libsvn_wc/props.c (working copy)
@@ -1799,33 +1799,32 @@ svn_error_t *
svn_wc_get_prop_diffs (apr_array_header_t **propchanges,
apr_hash_t **original_props,
const char *path,
svn_wc_adm_access_t *adm_access,
apr_pool_t *pool)
{
- const char *prop_path, *prop_base_path;
- apr_array_header_t *local_propchanges;
- apr_hash_t *localprops = apr_hash_make (pool);
+ const char *prop_base_path;
apr_hash_t *baseprops = apr_hash_make (pool);
-
- SVN_ERR (svn_wc__prop_path (&prop_path, path, adm_access, FALSE, pool));
SVN_ERR (svn_wc__prop_base_path (&prop_base_path, path, adm_access, FALSE,
pool));
-
- SVN_ERR (svn_wc__load_prop_file (prop_path, localprops, pool));
SVN_ERR (svn_wc__load_prop_file (prop_base_path, baseprops, pool));
if (original_props != NULL)
*original_props = baseprops;
- /* At this point, if either of the propfiles are non-existent, then
- the corresponding hash is simply empty. */
-
if (propchanges != NULL)
{
+ apr_hash_t *localprops;
+ apr_array_header_t *local_propchanges;
+
+ SVN_ERR (svn_wc_prop_list (&localprops, path, adm_access, pool));
+
+ /* At this point, if either of the propfiles are non-existent, then
+ the corresponding hash is simply empty. */
+
SVN_ERR (svn_prop_diffs (&local_propchanges, localprops,
baseprops, pool));
*propchanges = local_propchanges;
}
return SVN_NO_ERROR;
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Nov 2 23:46:51 2005