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