On Wed, 2011-06-15, Stefan Fuhrmann wrote:
> On 15.06.2011 15:29, Julian Foad wrote:
> > A heads-up: I'm renaming svn_hash_get_bool() to
> > svn_config_hash_get_bool(), and svn_hash_get_cstring() to
> > svn_config_hash_get(), moving them from svn_hash.h to svn_config.h.
> > They are being used only for config hashes, and don't seem like
> > general-purpose hash functions. In particular, _get_bool() looks for a
> > variety of human-oriented boolean-like strings (on, off, yes, no, etc.).
> >
> > Note that we already have svn_config_get_bool() and svn_config_get(),
> > which are similar but operate on a structured config file object,
> > whereas the functions I'm renaming operate on a simple hash.
> The reason why I explicitly didn't follow that direct in the first place
> is that svn_hash is very different from svn_config. Moving the
> get_bool function to svn_config would basically make it an orphan:
> There is no other hash-related function in there let alone a set of
> functions that take hashes as kind of an alternative to svn_config
> structures.
Current usage may not be a reliable guide to future usage, but it gives
us a starting point. The _get_bool() function is used in two places:
svn_repos_create(..., apr_hash_t *fs_config, ...):
if (svn_hash_get_bool(fs_config,
SVN_FS_CONFIG_PRE_1_4_COMPATIBLE,
FALSE))
and in libsvn_fs_fs/caching.c:
read_config():
*cache_txdeltas = svn_hash_get_bool(fs->config,
SVN_FS_CONFIG_FSFS_CACHE_DELTAS,
FALSE);
*cache_fulltexts = svn_hash_get_bool(fs->config,
SVN_FS_CONFIG_FSFS_CACHE_FULLTEXTS,
TRUE);
Notice that in both cases we are reading from a hash that is named
"config". Certainly those are not in the same format as svn_config_t,
but semantically these usages have more in common with
svn_config_get_bool()
svn_config_get_yes_no_ask()
svn_config_get_server_setting_bool()
than they have with generic hash tables.
The first usage, in svn_repos_create(), may be correct, but in other
places (create_repos_structure, svn_fs_fs__create, base_create) we
simply test whether "apr_hash_get(fs->config,
SVN_FS_CONFIG_PRE_1_4_COMPATIBLE, APR_HASH_KEY_STRING)" is non-null.
That is inconsistent. Which do we want?
The _get_cstring() function is similarly used with "config" hashes,
again in two places:
svn_fs_create(..., apr_hash_t *fs_config, ...)
fs_type = svn_hash_get_cstring(fs_config,
SVN_FS_CONFIG_FS_TYPE, DEFAULT_FS_TYPE);
svn_repos_create(..., apr_hash_t *fs_config, ...)
repos->fs_type = svn_hash_get_cstring(fs_config,
SVN_FS_CONFIG_FS_TYPE, DEFAULT_FS_TYPE);
Unlike _get_bool(), this function is not interpreting the string values
at all, so is suitable for use in other (non-config) contexts.
However, as a pair these functions are clearly aimed at being used with
"config" hashes. Whether that means they should be in svn_config.h or
svn_hash.h I don't know, but it does strongly say to me that their names
should have "config" in them. Either "svn_hash_config_..." or
"svn_config_hash_...".
p.s. I have several times thought it might be nice to have a set of
functions that operate on a { cstring key => cstring value } hash,
without having to specify "APR_HASH_KEY_STRING" to each invocation of
apr_hash_get() and similar functions. That's one of the benefits of this
_get_cstring() function over plain apr_hash_get; the other benefit is
substituting a default value.
- Julian
> So, I'm -0 on the rename and -0 on the move.
>
> -- Stefan^2.
> >
> > Current APIs:
> > [[[
> > # In svn_hash.h:
> >
> > /** Find the value of a @a key in @a hash, return the value.
> > *
> > * If @a hash is @c NULL or if the @a key cannot be found, the
> > * @a default_value will be returned.
> > *
> > * @since New in 1.7.
> > */
> > const char *
> > svn_hash_get_cstring(apr_hash_t *hash,
> > const char *key,
> > const char *default_value);
> >
> > /** Like svn_hash_get_cstring(), but for boolean values.
> > *
> > * Parses the value as a boolean value. The recognized representations
> > * are 'TRUE'/'FALSE', 'yes'/'no', 'on'/'off', '1'/'0'; case does not
> > * matter.
> > *
> > * @since New in 1.7.
> > */
> > svn_boolean_t
> > svn_hash_get_bool(apr_hash_t *hash,
> > const char *key,
> > svn_boolean_t default_value);
> >
> > # In svn_config.h:
> >
> > /** Find the value of a (@a section, @a option) pair in @a cfg, set @a
> > * *valuep to the value.
> > *
> > * If @a cfg is @c NULL, just sets @a *valuep to @a default_value. If
> > * the value does not exist, expand and return @a default_value. @a
> > * default_value can be NULL.
> > *
> > * The returned value will be valid at least until the next call to
> > * svn_config_get(), or for the lifetime of @a default_value. It is
> > * safest to consume the returned value immediately.
> > *
> > * This function may change @a cfg by expanding option values.
> > */
> > void
> > svn_config_get(svn_config_t *cfg,
> > const char **valuep,
> > const char *section,
> > const char *option,
> > const char *default_value);
> >
> > /** Like svn_config_get(), but for boolean values.
> > *
> > * Parses the option as a boolean value. The recognized representations
> > * are 'TRUE'/'FALSE', 'yes'/'no', 'on'/'off', '1'/'0'; case does not
> > * matter. Returns an error if the option doesn't contain a known string.
> > */
> > svn_error_t *
> > svn_config_get_bool(svn_config_t *cfg,
> > svn_boolean_t *valuep,
> > const char *section,
> > const char *option,
> > svn_boolean_t default_value);
> > ]]]
> >
> > Let me know if you have any concerns. As I don't anticipate any, I'll
> > probably make the change before waiting for replies.
> >
> > - Julian
