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 16:14:53 UTC
API change - make svn_tristate_to_word/_from_word() private?
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").
The doc string of _from_word() is wrong: it says any input other than
"true" and "false" is unknown.
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.
* implementation of svn_config_get_bool() and svn_hash_get_bool(),
which uses _tristate_from_word() to flexibly read "on"/"off"/"yes"/etc.
* 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.
At least, and at first, we should remove the second and third usages
listed above. 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.
- Julian
Re: API change - make svn_tristate_to_word/_from_word() private?
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Fuhrmann wrote on Wed, Jun 15, 2011 at 21:29:32 +0200:
> On 15.06.2011 16:45, Daniel Shahaf wrote:
> >> * 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.
The code only cares whether the parameter was specified-and-"yes" or no.
It doesn't behave in three different ways for the three different values
of the parameter.
> 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.
I hadn't considered that. Fair enough, assuming the documentation
agrees (ie, we don't promise what the default is).
> Furthermore, both options should have the same UI.
+1
Re: API change - make svn_tristate_to_word/_from_word() private?
Posted by Julian Foad <ju...@wandisco.com>.
On Wed, 2011-06-15, Stefan Fuhrmann wrote:
> On 15.06.2011 16:45, Daniel Shahaf wrote:
> > 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.
As *implementations* they work fine, serving both single-representation
and human-friendly purposes for their current callers. But the
*interface* definitions are "hinky" as Greg put it, or in my words "not
"canonically purposed": they are designed to serve two or more logically
different purposes. That's bad design.
As the software develops over time, for the human-friendly purpose it
would be perfectly reasonable for us to want to change the set of
accepted words from time to time, maybe adding "1" and "0" as recognized
inputs or changing the default output to "yes" and "no" instead of
"true" and "false". Or localize the human language used for input and
output. Or return some readable string instead of NULL for the
"unknown" value. Or even remove "true" and "false" from the set of
recognized inputs. (Yes, that would mean changing the config file
specification and the command-line option specification, and dealing
with associated back-compat issues.)
For the single-representation purposes (used for XML output and
server-client protocol), we would probably never change the text values
used, and if we ever did we would need to co-ordinate that change as
part of a protocol change, and we would never want this to happen as a
side effect of changing the human-friendly parsing of command-line
options.
For single-representation purposes, _from_word() serves adequately as a
complement to _to_word(), accepting "true", "false" and NULL. But for
human-friendly purposes, _to_word() does not serve as a clear logical
complement to _from_word(). It does not provide *the* way to convert a
tristate value to a human-readable string, it just provides *one* way
which some callers might want, and other callers might want a different
way. And it outputs NULL whereas some callers might well want a
human-readable word such as "unknown" or "default".
These are some of the reasons why, even if the interfaces for these
different purposes share some implementation, the interfaces should be
separate.
> 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.
OK, thanks for the heads-up on that point.
> >> 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.
Correct.
> >> * 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.
Reducing duplication is good, yes. But not if the API suffers.
> >> * 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.
Maybe parsing of command-line option boolean values and the config-file
boolean values should be considered logically the same; that makes sense
to me. Great - these two purposes can share a parser.
Note that neither of these two purposes currently wants an output
function, although it is conceivable that at some time they may do, in
order to print out help or examples (of command-line usage or config
file settings). If and when we do want an output function for these
purposes, is it certain that we would want the function to output
"true"/"false"/NULL? No, I don't think so. For example, for the
"unknown" state we might well want it to output a default value string
that is provided as an input parameter.
> >> At least, and at first, we should remove the second and third usages
> >> listed above.
> And replace them by what?
Replace the use of svn_tristate_from_word() by an equivalent but more
local API.
> >> 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.
Great, that's what I'll do then.
Thanks.
- Julian
Re: API change - make svn_tristate_to_word/_from_word() private?
Posted by Stefan Fuhrmann <eq...@web.de>.
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.
Re: API change - make svn_tristate_to_word/_from_word() private?
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
[ 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").
>
> The doc string of _from_word() is wrong: it says any input other than
> "true" and "false" is unknown.
>
> 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.
>
> * implementation of svn_config_get_bool() and svn_hash_get_bool(),
> which uses _tristate_from_word() to flexibly read "on"/"off"/"yes"/etc.
>
> * 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.
>
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,
?
>
> At least, and at first, we should remove the second and third usages
> listed above. 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.
> - Julian
>
>
Thanks,
Re: API change - make svn_tristate_to_word/_from_word() private?
Posted by Greg Stein <gs...@gmail.com>.
On Wed, Jun 15, 2011 at 10:14, Julian Foad <ju...@wandisco.com> wrote:
> 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"/.../
>...
> At least, and at first, we should remove the second and third usages
> listed above. 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.
Concur. Those things always made me a little hinky.
Cheers,
-g