You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@wandisco.com> on 2011/06/15 15:29:37 UTC
API change - rename svn_hash_get_cstring/bool() to
svn_config_hash_get_*()
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.
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
Re: API change - rename svn_hash_get_cstring/bool() to svn_config_hash_get_*()
Posted by Stefan Fuhrmann <eq...@web.de>.
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.
>
>
>
Re: API change - rename svn_hash_get_cstring/bool() to
svn_config_hash_get_*()
Posted by Julian Foad <ju...@wandisco.com>.
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.
Re: API change - rename svn_hash_get_cstring/bool() to
svn_config_hash_get_*()
Posted by Julian Foad <ju...@wandisco.com>.
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
Re: API change - rename svn_hash_get_cstring/bool() to svn_config_hash_get_*()
Posted by Stefan Fuhrmann <eq...@web.de>.
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.
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
>
>
>
Re: API change - rename svn_hash_get_cstring/bool() to
svn_config_hash_get_*()
Posted by Julian Foad <ju...@wandisco.com>.
n.b. These two functions are new in 1.7.
- Julian
On Wed, 2011-06-15 at 14:29 +0100, 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.
>
>
> 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
>
>