On 15.06.2011 16:45, Daniel Shahaf wrote:
> [ bcc stefan2 ]
>
> Julian Foad wrote on Wed, Jun 15, 2011 at 15:14:53 +0100:
>> New in 1.7 in svn_types.h:
>> svn_tristate_t
>> svn_tristate_to_word() # yields "true"/"false"/NULL
>> svn_tristate_from_word() # takes "true"/"yes"/"on"/.../
>>
>> The _to/from_word() functions don't seem canonically purposed: some
>> users want a single representation of the values, for use in on-the-wire
>> protocols and XML output, while other users want a more flexible,
>> human-readable interpretation that accepts multiple different words
>> ("true" = "on" = "yes").
Well, these look like reasonable conversion utilities to me.
They simply don't make assumptions about the context that
they might be used in.
However, one might argue whether they need to be public.
Before making them private, we need to extend svn_cmdline
to do the conversion. Otherwise, a svn tool (svnserve in that
case) would need to call private API.
>> The doc string of _from_word() is wrong: it says any input other than
>> "true" and "false" is unknown.
Yep. Forgot to update that.
>> The current users are:
>>
>> * The main use is in svn_log_changed_path2_t.text_modified
>> and .props_modified. For these, we use _to_word() in mod_dav_svn to
>> write a protocol string (assumes the value is not svn_tristate_unknown),
>> use _from_word() in ra_neon and ra_serf to interpret a protocol string,
>> and use _to_word() in "svn log --xml" to output an XML attribute, using
>> an undocumented feature of svn_xml_make_open_tag() to allow the
>> attribute string to be NULL. All of these need only "true"/"false", not
>> yes/no/off/on.
But they won't be hurt by allowing those alternative values.
>> * implementation of svn_config_get_bool() and svn_hash_get_bool(),
>> which uses _tristate_from_word() to flexibly read "on"/"off"/"yes"/etc.
The is where the more flexible implementation comes from.
It looked a little duplicated.
>> * svnserve:main.c which uses it simply to detect whether a
>> command-line option is "true"/"yes"/"on", and doesn't actually care
>> about the tri-state-ness.
>>
Well, not specifying the parameter is equivalent to the 3rd state.
But this check actually prompted me *not* to duplicate the
comparison code again but to factor it out to some common
utility function. Even 3rd party coders might find it helpful.
> Why does that svnserve option take a string argument? If it's just
> a boolean why not do
>
> - {"cache-txdeltas", SVNSERVE_OPT_CACHE_TXDELTAS, 1,
> + {"cache-txdeltas", SVNSERVE_OPT_CACHE_TXDELTAS, 0,
>
> ?
Because the default can be "true" / "on" and that default
may change in later versions. Furthermore, both options
should have the same UI.
>> At least, and at first, we should remove the second and third usages
>> listed above.
And replace them by what?
>> Those usages are more akin to "svn_cmdline" or
>> "svn_config" utilities, or could well be private.
>>
>> Further, I consider moving/renaming these two functions to be a bit more
>> private even for their main use in svn_log_changed_path2_t.
>> Fair enough. I don't see that any of the above uses require third
>> parties API consumers to parse tristates (from string to enum), so no
>> objection to making _from_word() private.
Making them private would be fine with me but that requires
svn_cmdline to be extended with some equivalent public function.
-- Stefan^2.
Received on 2011-06-15 21:29:59 CEST