On Mon, 2011-06-20, Julian Foad wrote:
> 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_...".
svn_hash.h is (at least was originally) supposed to be for Subversion's
hash serialization functions. Now it has some other hash-related
functions too.
svn_config.h is (at least was originally) supposed to be for handling
the svn_config_t structure.
For now, I have simply renamed the functions to contain a double
underscore (svn_hash__get_bool, svn_hash__get_cstring) so I can move on
to other things; now we can come back to this any time.
- Julian
> 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.
