Re: [PATCH] Remove unnecessary props read in svn_wc_get_prop_diffs()

Malcolm Rowe <malcolm-svn-dev@farside.org.uk> writes:

> Two notes about the patch:
> 1. I thought that it was clearer to return early from the function if
> propchanges was NULL, rather than wrap the second half of the function
> in the body of an if() statement, but the disadvantage is that there
> are now two exit points from the function. Let me know if you want it
> the other way.

Both styles are used in the code.

> 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.

> [[[
> * subversion/libsvn_wc/props.c
> (svn_wc_get_prop_diffs): Rearrange, and exit early if the caller doesn't
> want to receive property changes, avoiding an unnecessary read of
> the WORKING properties file.
> ]]]

Looks good.

>
> Regards,
> Malcolm
> Index: subversion/libsvn_wc/props.c
> ===================================================================
> --- subversion/libsvn_wc/props.c (revision 17144)
> +++ subversion/libsvn_wc/props.c (working copy)
> @@ -1807,26 +1807,28 @@ svn_wc_get_prop_diffs (apr_array_header_
> apr_hash_t *localprops = apr_hash_make (pool);
> apr_hash_t *baseprops = apr_hash_make (pool);
>
> -
> - SVN_ERR (svn_wc__prop_path (&prop_path, path, adm_access, FALSE, pool));
> + /* 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.

> 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;
>
> + /* If we don't care about the propchanges, we can quit now. */
> + if (propchanges == NULL)
> + return SVN_NO_ERROR;
> +
> + /* Load the WORKING props. */

That one is also trivial.

> + SVN_ERR (svn_wc__prop_path (&prop_path, path, adm_access, FALSE, pool));
> + SVN_ERR (svn_wc__load_prop_file (prop_path, localprops, pool));
> +
> /* At this point, if either of the propfiles are non-existent, then
> the corresponding hash is simply empty. */
> -
> - if (propchanges != NULL)
> - {
> - SVN_ERR (svn_prop_diffs (&local_propchanges, localprops,
> - baseprops, pool));
> - *propchanges = local_propchanges;
> - }
> +
> + SVN_ERR (svn_prop_diffs (&local_propchanges, localprops,
> + baseprops, pool));
> + *propchanges = local_propchanges;
>
> return SVN_NO_ERROR;
> }

-- 
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org