On 23.06.2011 12:48, Julian Foad wrote:
> 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
>
Hi Julian,
I'm fine with the rename. Sorry that I haven't found time
to respond earlier. I'm still in the aftermath of moving.
If my luck holds, I will get reliable internet access tomorrow.
-- Stefan^2.
>> 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.
>
>
>
Received on 2011-06-26 22:41:21 CEST