On Sat, 12 Nov 2005, Daniel Berlin wrote:
> [[[
> Add caching of property existence for a few select properties.
>
I start by saying "On wc-propcaching branch: ", so people can choose to
ignore this if they want to:-) Seriously, when working on a feature
branch, I think that's good practice.
> * subversion/include/svn_wc.h
> (struct svn_wc_entry_t): Add has_properties member.
>
> * subversion/libsvn_wc/props.c
> (svn_wc__install_props): Set has_properties from the
> from the props we are going to install.
"from the from the":-)
> (svn_wc_prop_get): Short circuit the cached properties
> by checking if they exist before reading the props file.
> (svn_wc__cached_properties): New function.
> (svn_wc__build_has_properties): New function.
> (svn_is_boolean_property): New function.
You could group these three together if you wanted to, as a
comma-separated list.
> (svn_value_of_set_prop): New function.
Ok, four:-)
>
> * subversion/libsvn_wc/props.h
> (svn_wc__cached_properties): New prototype.
> (svn_wc__build_has_properties): New prototype.
>
> * subversion/libsvn_wc/entries.c
> (svn_wc__atts_to_entry): Add code to handle has_properties.
> (write_entry): Ditto.
> (fold_entry): Ditto.
> (svn_wc_entry_dup): Ditto.
>
Ah, looks like a GCC changelog I read sometimes ("dito"). We often use
grouping for this instead, but I really have no strong feelings.
> Index: subversion/include/svn_wc.h
> ===================================================================
> --- subversion/include/svn_wc.h (revision 17313)
> +++ subversion/include/svn_wc.h (working copy)
> @@ -1216,6 +1216,10 @@ typedef struct svn_wc_entry_t
> * @since New in 1.4. */
> svn_boolean_t prop_mods;
>
> + /** Cached property existence for this entry.
> + * @since New in 1.4. */
> + const char *has_properties;
> +
The docstrings here are all a little terse, but I think this coud live with a short explanation that what props are cached is WC dependent. Just so callers don't start using this with the wrong expectations. Also, can this be NULL?
And say how tokens are separated.
> /* IMPORTANT: If you extend this structure, check svn_wc_entry_dup() to see
> if you need to extend that as well. */
> } svn_wc_entry_t;
> Index: subversion/libsvn_wc/props.c
> ===================================================================
> --- subversion/libsvn_wc/props.c (revision 17313)
> +++ subversion/libsvn_wc/props.c (working copy)
> @@ -299,6 +299,35 @@ svn_wc__get_existing_prop_reject_file (c
>
> /*---------------------------------------------------------------------*/
>
> +
> +char *
> +svn_wc__build_has_properties (apr_hash_t *props, apr_pool_t *pool)
Why not return const char *?
> +{
> + apr_array_header_t *cached_props;
> + char *has_properties_string = NULL;
> + int i;
> +
> + if (!props || apr_hash_count (props) == 0)
> + return "";
> +
> + cached_props = svn_cstring_split (svn_wc__cached_properties (), "|",
> + TRUE, pool);
> + for (i = 0; i < cached_props->nelts; i++)
> + {
> + const char *proptolookfor = APR_ARRAY_IDX (cached_props, i,
> + const char *);
> +
> + if (apr_hash_get (props, proptolookfor, APR_HASH_KEY_STRING) != NULL)
> + {
> + if (!has_properties_string)
> + has_properties_string = "";
> + has_properties_string = apr_pstrcat (pool, has_properties_string,
> + proptolookfor, "|", NULL);
OK, we are not doing massive allocations here, but why not just use a
stringbuf? This is for code clarity rather than efficiency.
> + }
> + }
> + return has_properties_string;
> +}
> +
Why can this return either empty string or NULL? Is the difference
important or just an accident?
> /*** Installing new properties. ***/
> svn_error_t *
> svn_wc__install_props (svn_stringbuf_t **log_accum,
> @@ -341,12 +370,15 @@ svn_wc__install_props (svn_stringbuf_t *
> /* Check if the props are modified. */
> SVN_ERR (svn_prop_diffs (&prop_diffs, working_props, base_props, pool));
> tmp_entry.prop_mods = (prop_diffs->nelts > 0);
> + tmp_entry.has_properties = svn_wc__build_has_properties (working_props,
> + pool);
>
> /* ### TODO: Check props in working props and update flags for specific
> props when we have such flags. */
>
You're just killing the above TODO, so you can safely put it to rest
> somewhere else now.
> SVN_ERR (svn_wc__loggy_entry_modify (log_accum, adm_access, name, &tmp_entry,
> - SVN_WC__ENTRY_MODIFY_PROP_MODS, pool));
> + SVN_WC__ENTRY_MODIFY_PROP_MODS
> + | SVN_WC__ENTRY_MODIFY_HAS_PROPERTIES, pool));
Woops, long line!
>
> /* Write our property hashes into temporary files. Notice that the
> paths computed are ABSOLUTE pathnames, which is what our disk
> @@ -401,6 +433,14 @@ svn_wc__install_props (svn_stringbuf_t *
>
> SVN_ERR (svn_wc__loggy_set_readonly (log_accum, adm_access,
> real_prop_base, pool));
> +
> + memset (&tmp_entry, 0, sizeof (tmp_entry));
No need to clear the entry first. That will just confuse readers.
> + tmp_entry.has_properties = svn_wc__build_has_properties (base_props,
> + pool);
> + SVN_ERR (svn_wc__loggy_entry_modify (log_accum, adm_access, name,
> + &tmp_entry,
> + SVN_WC__ENTRY_MODIFY_HAS_PROPERTIES,
> + pool));
> }
>
> return SVN_NO_ERROR;
> @@ -1289,9 +1329,32 @@ svn_wc_prop_list (apr_hash_t **props,
> return svn_wc__load_prop_file (prop_path, *props, pool);
> }
>
> +static svn_boolean_t
> +svn_is_boolean_property (const char *name)
Missing docstring (yes, the name is nearly a docstring, but we try to
have at least a short docstring per function). And don't use the svn_
prefix for static functions.
> +static svn_string_t *
> +svn_value_of_set_prop (const char *name, apr_pool_t *pool)
Same as above, but here the docstring feels a little more urgent:-)
> +{
> + if (strcmp (name, SVN_PROP_EXECUTABLE) == 0)
> + return svn_string_create (SVN_PROP_EXECUTABLE_VALUE, pool);
> + else if (strcmp (name, SVN_PROP_NEEDS_LOCK) == 0)
> + return svn_string_create (SVN_PROP_NEEDS_LOCK_VALUE, pool);
> + else if (strcmp (name, SVN_PROP_SPECIAL) == 0)
> + return svn_string_create (SVN_PROP_SPECIAL_VALUE, pool);
> + return NULL;
> +}
>
Hmmm, I wonder why we have these three values instead of one value for
> a true boolean property, but that's not part of this patch. It is
> just illy:-)
> svn_error_t *
> svn_wc_prop_get (const svn_string_t **value,
> @@ -1304,6 +1367,35 @@ svn_wc_prop_get (const svn_string_t **va
> apr_hash_t *prophash;
>
> enum svn_prop_kind kind = svn_property_kind (NULL, name);
> + char *tolookfor = apr_pstrcat (pool, name, "|", NULL);
> +
> + if (strstr (svn_wc__cached_properties (), tolookfor))
> + {
> + const svn_wc_entry_t *entry;
> + SVN_ERR (svn_wc_entry (&entry, path, adm_access, TRUE, pool));
> +
svn_wc_entry may return a NULL entry. To preserve the current
semantics, I think you should just return a NULL value in this case.
> + /* We separate these two cases so that we can return the correct
> + value for booleans if they exist in the string. */
> + if (!entry->has_properties)
> + {
> + *value = NULL;
> + return SVN_NO_ERROR;
> + }
> + if (!strstr (entry->has_properties, tolookfor))
> + {
> + *value = NULL;
> + return SVN_NO_ERROR;
> + }
How does this work with entries in the old format? (Note that we only
bump the WC format when we have write access, so we have to be able to
read the old format.)
> + if (svn_is_boolean_property (name))
> + {
> + *value = svn_value_of_set_prop (name, pool);
> + if (*value == NULL)
> + return svn_error_createf (SVN_ERR_BAD_PROP_KIND, NULL,
> + _("Boolean property without set value"),
> + name);
Isn't this error just the result of an internal inconsistency in this
file? Should be an assert in that case.
> + return SVN_NO_ERROR;
> + }
> + }
>
> if (kind == svn_prop_wc_kind)
> {
> @@ -2124,3 +2216,9 @@ svn_wc__has_magic_property (apr_array_he
> }
> return FALSE;
> }
> +
> +const char *
> +svn_wc__cached_properties (void)
> +{
> + return SVN_PROP_SPECIAL "|" SVN_PROP_EXTERNALS "|" SVN_PROP_NEEDS_LOCK "|";
> +}
> Index: subversion/libsvn_wc/props.h
> ===================================================================
> --- subversion/libsvn_wc/props.h (revision 17313)
> +++ subversion/libsvn_wc/props.h (working copy)
> @@ -159,6 +159,15 @@ svn_error_t *svn_wc__install_props (svn_
> svn_boolean_t write_base_props,
> apr_pool_t *pool);
>
> +/* Return a '|' separated string containing the names of all of the
> + properties that the working copy caches the existence of. */
> +const char *svn_wc__cached_properties (void);
> +
What is "the working copy" in this case? When we release this, we have
WCs that cache no properties and WCs that cache these three
properties.
> +/* Return a '|' separated string containing the names of all cached
> + properties that are set in PROPS. This string is suitable for
> + using as the "has_properties" member of the entry structure. */
> +char * svn_wc__build_has_properties (apr_hash_t *props, apr_pool_t *pool);
Space after *.
Say that the returned list is allocated in POOL.
Rest looks fine. On IRC; we discussed that | will be changed back to
space and the revert_admin_things change will not be necessary any
longer.
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Nov 14 23:53:20 2005