RE: [Patch] Make svn_tristate_t compatible with svn_boolean_t
> -Original Message- > From: Stefan Fuhrmann [mailto:stefanfuhrm...@alice-dsl.de] > Sent: donderdag 4 november 2010 0:21 > To: Subversion Development > Subject: [Patch] Make svn_tristate_t compatible with svn_boolean_t > > Hi there, > > after stumbling twice over this issue, I ran grep > and found that the current usage of svn_tristate_t > does not depend on the actual numerical values > used for its states. > > Therefore, I propose to define svn_tristate_false > equal to FALSE and svn_tristate_true equal to > TRUE. That way, we can compare booleans with > tristates and assign booleans to tristates. Without > that, we need to write code like > > if ((get_some_bool() ? svn_tristate_true : svn_tristate_false) == > get_a_tristate()) > ... > > Also, the following will compile without warning but > requires the patch to do what was intended: > > // FALSE becomes "unknown", TRUE becomes "false" > svn_tristate_t my_tristate = get_some_bool(); What do you try to get as result here? I'm not 100% sure if tests like (12 == 12) really always return the defined value TRUE and we most likely have other functions that return just some != 0 value as svn_boolean_t. And what if TRUE happens to be defined as -1? (In that case unknown and FALSE would have the same value) I don't think this change makes it easier to use the trystate. You still have to perform the checks yourself. Bert
RE: [Patch] Make svn_tristate_t compatible with svn_boolean_t
Bert Huijben wrote: > > -Original Message- > > From: Stefan Fuhrmann [mailto:stefanfuhrm...@alice-dsl.de] > > after stumbling twice over this issue, I ran grep > > and found that the current usage of svn_tristate_t > > does not depend on the actual numerical values > > used for its states. > > > > Therefore, I propose to define svn_tristate_false > > equal to FALSE and svn_tristate_true equal to > > TRUE. That way, we can compare booleans with > > tristates and assign booleans to tristates. Without > > that, we need to write code like > > > > if ((get_some_bool() ? svn_tristate_true : svn_tristate_false) == > > get_a_tristate()) > > ... > > > > Also, the following will compile without warning but > > requires the patch to do what was intended: > > > > // FALSE becomes "unknown", TRUE becomes "false" > > svn_tristate_t my_tristate = get_some_bool(); > > What do you try to get as result here? > > I'm not 100% sure if tests like (12 == 12) really always return the defined > value TRUE (I'm just addressing the C language facts in this response.) In C, when an operator such as "==" is defined to yield a Boolean value, it *always* return 0 for "false" or 1 for "true", never other values for "true". However, non-Boolean-valued expressions, such as "flags & 0x40", can be used where a Boolean value is wanted, such as "if (flags & 0x40)" or "override || (flags & 0x40)". Any non-zero is regarded as "true". > and we most likely have other functions that return just some != > 0 value as svn_boolean_t. Yes: there can be functions in our code or in libraries that we call that return other values than 1 to mean "true", although we tend to avoid doing so in our own code. > And what if TRUE happens to be defined as -1? It never will be. It's defined as 1 specifically so that it matches the C language definition of "true". > (In that case unknown and FALSE would have the same value) - Julian
Re: [Patch] Make svn_tristate_t compatible with svn_boolean_t
On Thu, 2010-11-04, Stefan Fuhrmann wrote: > Hi there, > > after stumbling twice over this issue, I ran grep > and found that the current usage of svn_tristate_t > does not depend on the actual numerical values > used for its states. > > Therefore, I propose to define svn_tristate_false > equal to FALSE and svn_tristate_true equal to > TRUE. That way, we can compare booleans with > tristates and assign booleans to tristates. Without > that, we need to write code like > > if ((get_some_bool() ? svn_tristate_true : svn_tristate_false) == > get_a_tristate()) > ... > > Also, the following will compile without warning but > requires the patch to do what was intended: > > // FALSE becomes "unknown", TRUE becomes "false" > svn_tristate_t my_tristate = get_some_bool(); > > -- Stefan^2. > > [[[ > Make svn_tristate_t mainly compatible to svn_boolean_t > by making its numerical values match the ones used for > svn_boolean_t. > > * subversion/include/svn_types.h >(svn_tristate_t): define *_true and *_false in terms of >the svn_boolean_t values TRUE and FALSE, respectively. > ]]] It seems sensible to have the set of values either match where they overlap (like you're suggesting), or be totally incompatible (e.g. the set of tristate values being 2/3/4, or the tristate data type being incompatible with "int"). If the values match, then there is no need for the enumerators "svn_tristate_false" and "svn_tristate_true", since "FALSE" and "TRUE" can be used instead. If the idea is that these values are compatible, then we *should* delete the "svn_tristate_true/false" names and use the existing names for them, and not have the ambiguity of which name to use. As Bert said, it's not always safe to assign or compare what we call a "Boolean" value directly with an svn_tristate_t value, because we can't always guarantee that "true" is represented by "1". But often it would be safe. :-/ - Julian
Re: [Patch] Make svn_tristate_t compatible with svn_boolean_t
On Thu, 2010-11-04 at 11:51 +, Julian Foad wrote: > On Thu, 2010-11-04, Stefan Fuhrmann wrote: > > Hi there, > > > > after stumbling twice over this issue, I ran grep > > and found that the current usage of svn_tristate_t > > does not depend on the actual numerical values > > used for its states. > > > > Therefore, I propose to define svn_tristate_false > > equal to FALSE and svn_tristate_true equal to > > TRUE. That way, we can compare booleans with > > tristates and assign booleans to tristates. Without > > that, we need to write code like > > > > if ((get_some_bool() ? svn_tristate_true : svn_tristate_false) == > > get_a_tristate()) > > ... > > > > Also, the following will compile without warning but > > requires the patch to do what was intended: > > > > // FALSE becomes "unknown", TRUE becomes "false" > > svn_tristate_t my_tristate = get_some_bool(); > > > > -- Stefan^2. > > > > [[[ > > Make svn_tristate_t mainly compatible to svn_boolean_t > > by making its numerical values match the ones used for > > svn_boolean_t. > > > > * subversion/include/svn_types.h > >(svn_tristate_t): define *_true and *_false in terms of > >the svn_boolean_t values TRUE and FALSE, respectively. > > ]]] > > It seems sensible to have the set of values either match where they > overlap (like you're suggesting), or be totally incompatible (e.g. the > set of tristate values being 2/3/4, or the tristate data type being > incompatible with "int"). > > If the values match, then there is no need for the enumerators > "svn_tristate_false" and "svn_tristate_true", since "FALSE" and "TRUE" > can be used instead. If the idea is that these values are compatible, > then we *should* delete the "svn_tristate_true/false" names and use the > existing names for them, and not have the ambiguity of which name to > use. > > As Bert said, it's not always safe to assign or compare what we call a > "Boolean" value directly with an svn_tristate_t value, because we can't > always guarantee that "true" is represented by "1". But often it would > be safe. :-/ Having said all that, +1 on removing the gratuitous inconsistency by applying this patch. Committed r1030909. Thanks. - Julian
Re: [Patch] Make svn_tristate_t compatible with svn_boolean_t
On Thu, Nov 4, 2010 at 7:43 AM, Julian Foad wrote: > On Thu, 2010-11-04 at 11:51 +, Julian Foad wrote: >> On Thu, 2010-11-04, Stefan Fuhrmann wrote: >> > Hi there, >> > >> > after stumbling twice over this issue, I ran grep >> > and found that the current usage of svn_tristate_t >> > does not depend on the actual numerical values >> > used for its states. >> > >> > Therefore, I propose to define svn_tristate_false >> > equal to FALSE and svn_tristate_true equal to >> > TRUE. That way, we can compare booleans with >> > tristates and assign booleans to tristates. Without >> > that, we need to write code like >> > >> > if ((get_some_bool() ? svn_tristate_true : svn_tristate_false) == >> > get_a_tristate()) >> > ... >> > >> > Also, the following will compile without warning but >> > requires the patch to do what was intended: >> > >> > // FALSE becomes "unknown", TRUE becomes "false" >> > svn_tristate_t my_tristate = get_some_bool(); >> > >> > -- Stefan^2. >> > >> > [[[ >> > Make svn_tristate_t mainly compatible to svn_boolean_t >> > by making its numerical values match the ones used for >> > svn_boolean_t. >> > >> > * subversion/include/svn_types.h >> > (svn_tristate_t): define *_true and *_false in terms of >> > the svn_boolean_t values TRUE and FALSE, respectively. >> > ]]] >> >> It seems sensible to have the set of values either match where they >> overlap (like you're suggesting), or be totally incompatible (e.g. the >> set of tristate values being 2/3/4, or the tristate data type being >> incompatible with "int"). >> >> If the values match, then there is no need for the enumerators >> "svn_tristate_false" and "svn_tristate_true", since "FALSE" and "TRUE" >> can be used instead. If the idea is that these values are compatible, >> then we *should* delete the "svn_tristate_true/false" names and use the >> existing names for them, and not have the ambiguity of which name to >> use. >> >> As Bert said, it's not always safe to assign or compare what we call a >> "Boolean" value directly with an svn_tristate_t value, because we can't >> always guarantee that "true" is represented by "1". But often it would >> be safe. :-/ > > Having said all that, +1 on removing the gratuitous inconsistency by > applying this patch. > > Committed r1030909. Gah. Can we please wait a little bit longer on this kind of stuff to allow people in other timezones a chance to weigh in? -Hyrum (who's own opinion on the matter is forthcoming)
Re: [Patch] Make svn_tristate_t compatible with svn_boolean_t
On Thu, Nov 4, 2010 at 6:51 AM, Julian Foad wrote: > On Thu, 2010-11-04, Stefan Fuhrmann wrote: >> Hi there, >> >> after stumbling twice over this issue, I ran grep >> and found that the current usage of svn_tristate_t >> does not depend on the actual numerical values >> used for its states. >> >> Therefore, I propose to define svn_tristate_false >> equal to FALSE and svn_tristate_true equal to >> TRUE. That way, we can compare booleans with >> tristates and assign booleans to tristates. Without >> that, we need to write code like >> >> if ((get_some_bool() ? svn_tristate_true : svn_tristate_false) == >> get_a_tristate()) >> ... >> >> Also, the following will compile without warning but >> requires the patch to do what was intended: >> >> // FALSE becomes "unknown", TRUE becomes "false" >> svn_tristate_t my_tristate = get_some_bool(); >> >> -- Stefan^2. >> >> [[[ >> Make svn_tristate_t mainly compatible to svn_boolean_t >> by making its numerical values match the ones used for >> svn_boolean_t. >> >> * subversion/include/svn_types.h >> (svn_tristate_t): define *_true and *_false in terms of >> the svn_boolean_t values TRUE and FALSE, respectively. >> ]]] > > It seems sensible to have the set of values either match where they > overlap (like you're suggesting), or be totally incompatible (e.g. the > set of tristate values being 2/3/4, or the tristate data type being > incompatible with "int"). > > If the values match, then there is no need for the enumerators > "svn_tristate_false" and "svn_tristate_true", since "FALSE" and "TRUE" > can be used instead. If the idea is that these values are compatible, > then we *should* delete the "svn_tristate_true/false" names and use the > existing names for them, and not have the ambiguity of which name to > use. > > As Bert said, it's not always safe to assign or compare what we call a > "Boolean" value directly with an svn_tristate_t value, because we can't > always guarantee that "true" is represented by "1". But often it would > be safe. :-/ I don't know how comfortable I am with this change. Having svn_tristate_t be a superset of svn_boolean_t just means that folks can/will use the former when they should b using the latter. Making them interchangable is a Bad Thing because it confuses the semantics, and doesn't force people to make a decision about which one to use. "Hmm, which should I use here, a Tristate or a Boolean? Oh well, it doesn't matter, because Tristate is just a superset of a Boolean, and they are compatible." My fears here may be overblown, but there *is* a difference between and Tristate and a Boolean, and there should be a difference between a Tristate-true and a Boolean-true. Maintaining the incompatibility in code between the two is a way to enforce this. -Hyrum
Re: [Patch] Make svn_tristate_t compatible with svn_boolean_t
On Thu, 2010-11-04 at 07:45 -0500, Hyrum K. Wright wrote: > On Thu, Nov 4, 2010 at 7:43 AM, Julian Foad wrote: > > On Thu, 2010-11-04 at 11:51 +, Julian Foad wrote: > >> On Thu, 2010-11-04, Stefan Fuhrmann wrote: > >> > Hi there, > >> > > >> > after stumbling twice over this issue, I ran grep > >> > and found that the current usage of svn_tristate_t > >> > does not depend on the actual numerical values > >> > used for its states. > >> > > >> > Therefore, I propose to define svn_tristate_false > >> > equal to FALSE and svn_tristate_true equal to > >> > TRUE. That way, we can compare booleans with > >> > tristates and assign booleans to tristates. Without > >> > that, we need to write code like > >> > > >> > if ((get_some_bool() ? svn_tristate_true : svn_tristate_false) == > >> > get_a_tristate()) > >> > ... > >> > > >> > Also, the following will compile without warning but > >> > requires the patch to do what was intended: > >> > > >> > // FALSE becomes "unknown", TRUE becomes "false" > >> > svn_tristate_t my_tristate = get_some_bool(); > >> > > >> > -- Stefan^2. > >> > > >> > [[[ > >> > Make svn_tristate_t mainly compatible to svn_boolean_t > >> > by making its numerical values match the ones used for > >> > svn_boolean_t. > >> > > >> > * subversion/include/svn_types.h > >> >(svn_tristate_t): define *_true and *_false in terms of > >> >the svn_boolean_t values TRUE and FALSE, respectively. > >> > ]]] > >> > >> It seems sensible to have the set of values either match where they > >> overlap (like you're suggesting), or be totally incompatible (e.g. the > >> set of tristate values being 2/3/4, or the tristate data type being > >> incompatible with "int"). > >> > >> If the values match, then there is no need for the enumerators > >> "svn_tristate_false" and "svn_tristate_true", since "FALSE" and "TRUE" > >> can be used instead. If the idea is that these values are compatible, > >> then we *should* delete the "svn_tristate_true/false" names and use the > >> existing names for them, and not have the ambiguity of which name to > >> use. > >> > >> As Bert said, it's not always safe to assign or compare what we call a > >> "Boolean" value directly with an svn_tristate_t value, because we can't > >> always guarantee that "true" is represented by "1". But often it would > >> be safe. :-/ > > > > Having said all that, +1 on removing the gratuitous inconsistency by > > applying this patch. > > > > Committed r1030909. > > Gah. Can we please wait a little bit longer on this kind of stuff to > allow people in other timezones a chance to weigh in? > > -Hyrum (who's own opinion on the matter is forthcoming) As regards your concern that they are subtly different: yes, I share the concern. I think as a generality we are unaccustomed to tri-state variables and are likely to make mistakes. So maybe making them incompatible is in fact better. My take on this was: whether we decide to make it intentionally compatible or intentionally incompatible, the current (before I committed that) definition was, IMO, gratuitously confusing. I thought that this commit was an OK step whatever the final decision. But I guess committing that has implicitly supported the view that they should be intentionally compatible. I'll *try* to wait a bit longer on things. - Julian
Re: [Patch] Make svn_tristate_t compatible with svn_boolean_t
On 04.11.2010 13:47, Hyrum K. Wright wrote: On Thu, Nov 4, 2010 at 6:51 AM, Julian Foad wrote: On Thu, 2010-11-04, Stefan Fuhrmann wrote: Hi there, after stumbling twice over this issue, I ran grep and found that the current usage of svn_tristate_t does not depend on the actual numerical values used for its states. Therefore, I propose to define svn_tristate_false equal to FALSE and svn_tristate_true equal to TRUE. That way, we can compare booleans with tristates and assign booleans to tristates. Without that, we need to write code like if ((get_some_bool() ? svn_tristate_true : svn_tristate_false) == get_a_tristate()) ... Also, the following will compile without warning but requires the patch to do what was intended: // FALSE becomes "unknown", TRUE becomes "false" svn_tristate_t my_tristate = get_some_bool(); -- Stefan^2. [[[ Make svn_tristate_t mainly compatible to svn_boolean_t by making its numerical values match the ones used for svn_boolean_t. * subversion/include/svn_types.h (svn_tristate_t): define *_true and *_false in terms of the svn_boolean_t values TRUE and FALSE, respectively. ]]] It seems sensible to have the set of values either match where they overlap (like you're suggesting), or be totally incompatible (e.g. the set of tristate values being 2/3/4, or the tristate data type being incompatible with "int"). If the values match, then there is no need for the enumerators "svn_tristate_false" and "svn_tristate_true", since "FALSE" and "TRUE" can be used instead. If the idea is that these values are compatible, then we *should* delete the "svn_tristate_true/false" names and use the existing names for them, and not have the ambiguity of which name to use. As Bert said, it's not always safe to assign or compare what we call a "Boolean" value directly with an svn_tristate_t value, because we can't always guarantee that "true" is represented by "1". But often it would be safe. :-/ I don't know how comfortable I am with this change. Having svn_tristate_t be a superset of svn_boolean_t just means that folks can/will use the former when they should b using the latter. Making them interchangable is a Bad Thing because it confuses the semantics, and doesn't force people to make a decision about which one to use. "Hmm, which should I use here, a Tristate or a Boolean? Oh well, it doesn't matter, because Tristate is just a superset of a Boolean, and they are compatible." My fears here may be overblown, but there *is* a difference between and Tristate and a Boolean, and there should be a difference between a Tristate-true and a Boolean-true. Maintaining the incompatibility in code between the two is a way to enforce this. Thanks all for the replies. Here is how I see it plus a couple of things I discovered in the meantime. * general consensus: overlapping definitions with inconsistent meaning are bad. * enums are always compatible with ints, so we can't make them incompatible at compile time That leaves us with two basic options. (1) Define that a tristate is a "nullable" boolean (.net speak). To me, this seems to be the way that it is being used right now. However, one might be tempted to (miss-)use tristates instead of booleans "just in case". That would be bad. (2) Define that tristates and booleans are similar but fundamentally different. Using values >1 for tristate values would make them "always true" in boolean expressions. So, the logic would probably fail early and the compiler might even generate a warning. I think there is no big risk associated with neither of these options. If people feel uncomfortable with (1), I'm happy to switch to (2). But I also discovered two issues with the current boolean type definitions. First, TRUE and FALSE are defined after I use them in svn_tristate_t which can be fixed easily and leads directly to the second issue. We only define TRUE and FALSE in case they have not been defined, yet. Since we rely on their numerical values, we should at test for these values in case the macros are pre-defined (#error if not). If people agree, I will implement (2) and the fix the other two issues. -- Stefan^2.
Re: [Patch] Make svn_tristate_t compatible with svn_boolean_t
On Sun, 2010-11-07, Stefan Fuhrmann wrote: [...] > Thanks all for the replies. Here is how I see it plus > a couple of things I discovered in the meantime. > > * general consensus: overlapping definitions with >inconsistent meaning are bad. > * enums are always compatible with ints, so we can't >make them incompatible at compile time We can't make *enums* incompatible, but we could make the tristate a different type, such as a struct or a pointer, which would be incompatible. That can be option (3). But it doesn't seem to be necessary, as your option (2) seems to provide good enough separation. > That leaves us with two basic options. > > (1) Define that a tristate is a "nullable" boolean (.net speak). > To me, this seems to be the way that it is being used > right now. However, one might be tempted to (miss-)use > tristates instead of booleans "just in case". That would > be bad. > > (2) Define that tristates and booleans are similar but > fundamentally different. Using values >1 for tristate > values would make them "always true" in boolean > expressions. So, the logic would probably fail early > and the compiler might even generate a warning. > > I think there is no big risk associated with neither > of these options. If people feel uncomfortable with (1), > I'm happy to switch to (2). That would be OK for me. > But I also discovered two issues with the current boolean > type definitions. First, TRUE and FALSE are defined > after I use them in svn_tristate_t which can be fixed easily > and leads directly to the second issue. > > We only define TRUE and FALSE in case they have > not been defined, yet. Since we rely on their numerical > values, we should at test for these values in case the > macros are pre-defined (#error if not). That's over-kill. Not necessary. > If people agree, I will implement (2) and the fix the other > two issues. - Julian
Re: [Patch] Make svn_tristate_t compatible with svn_boolean_t
On Thu, Nov 4, 2010 at 08:45, Hyrum K. Wright wrote: > On Thu, Nov 4, 2010 at 7:43 AM, Julian Foad wrote: >... >> Having said all that, +1 on removing the gratuitous inconsistency by >> applying this patch. >> >> Committed r1030909. > > Gah. Can we please wait a little bit longer on this kind of stuff to > allow people in other timezones a chance to weigh in? I have raised this before, Julian. For making changes with some impact, where feedback from the community is desired, then the standard Apache rule is 72 hours. And even if we don't worry about rules, it is simply *respectful* to give others a chance to speak up. Last time, you bumped the format with something like FOUR hours notice. That was really bad. I'm not sure about the extent of the badness here (tho changing something as central as svn_types.h seems pretty obvious as an "input-required" change), but given that another member of the community has said "woah. too quick", then you REALLY need to slow down. This isn't an attempt to slow down your work. This is an attempt (bordering on requirement) for you to work with the rest of the community on changes to our codebase. -g
Re: [Patch] Make svn_tristate_t compatible with svn_boolean_t
I would +1 everything you said... if Julian had changed svn_boolean_t. He changed svn_tristate_t, which is used exactly once in trunk, so I don't see the need to be as careful with it. The caveat? This assumes that --- as we ought to --- we not act on feedback differently because it comes after the change has been committed. Feedback that would have caused a "No, let's not commit this yet" reaction if provided early should cause a "OK, let's revert this for now then" reaction if provided later. Daniel Greg Stein wrote on Tue, Nov 09, 2010 at 00:47:27 -0500: > On Thu, Nov 4, 2010 at 08:45, Hyrum K. Wright > wrote: > > On Thu, Nov 4, 2010 at 7:43 AM, Julian Foad > > wrote: > >... > >> Having said all that, +1 on removing the gratuitous inconsistency by > >> applying this patch. > >> > >> Committed r1030909. > > > > Gah. Can we please wait a little bit longer on this kind of stuff to > > allow people in other timezones a chance to weigh in? > > I have raised this before, Julian. For making changes with some > impact, where feedback from the community is desired, then the > standard Apache rule is 72 hours. And even if we don't worry about > rules, it is simply *respectful* to give others a chance to speak up. > > Last time, you bumped the format with something like FOUR hours > notice. That was really bad. I'm not sure about the extent of the > badness here (tho changing something as central as svn_types.h seems > pretty obvious as an "input-required" change), but given that another > member of the community has said "woah. too quick", then you REALLY > need to slow down. > > This isn't an attempt to slow down your work. This is an attempt > (bordering on requirement) for you to work with the rest of the > community on changes to our codebase. > > -g
Re: [Patch] Make svn_tristate_t compatible with svn_boolean_t
It is used *once* ?! Bleh. Then I'd ask why we even have the type, especially why does it exist in svn_types.h. Your other points: quite fair. I assumed tristate_t was much more important due to its placement in svn_types. Cheers, -g On Tue, Nov 9, 2010 at 12:15, Daniel Shahaf wrote: > I would +1 everything you said... if Julian had changed svn_boolean_t. > > He changed svn_tristate_t, which is used exactly once in trunk, so > I don't see the need to be as careful with it. > > The caveat? This assumes that --- as we ought to --- we not act on > feedback differently because it comes after the change has been > committed. Feedback that would have caused a "No, let's not commit this > yet" reaction if provided early should cause a "OK, let's revert this > for now then" reaction if provided later. > > Daniel > > Greg Stein wrote on Tue, Nov 09, 2010 at 00:47:27 -0500: >> On Thu, Nov 4, 2010 at 08:45, Hyrum K. Wright >> wrote: >> > On Thu, Nov 4, 2010 at 7:43 AM, Julian Foad >> > wrote: >> >... >> >> Having said all that, +1 on removing the gratuitous inconsistency by >> >> applying this patch. >> >> >> >> Committed r1030909. >> > >> > Gah. Can we please wait a little bit longer on this kind of stuff to >> > allow people in other timezones a chance to weigh in? >> >> I have raised this before, Julian. For making changes with some >> impact, where feedback from the community is desired, then the >> standard Apache rule is 72 hours. And even if we don't worry about >> rules, it is simply *respectful* to give others a chance to speak up. >> >> Last time, you bumped the format with something like FOUR hours >> notice. That was really bad. I'm not sure about the extent of the >> badness here (tho changing something as central as svn_types.h seems >> pretty obvious as an "input-required" change), but given that another >> member of the community has said "woah. too quick", then you REALLY >> need to slow down. >> >> This isn't an attempt to slow down your work. This is an attempt >> (bordering on requirement) for you to work with the rest of the >> community on changes to our codebase. >> >> -g >
Re: [Patch] Make svn_tristate_t compatible with svn_boolean_t
On Wed, 2010-11-10, Greg Stein wrote: > It is used *once* ?! > > Bleh. Then I'd ask why we even have the type, especially why does it > exist in svn_types.h. I wondered the same. The answer appears to be that 'svn_log_changed_path2_t' (which was added to svn_types.h in 1.6) has now been extended with two new fields, 'text_modified' and 'props_modified', which are only valid in certain cases, as part of r877688: > r877688 | rhuijben | 2009-05-06 17:02:16 +0100 (Wed, 06 May 2009) | 46 lines > > Provide information on whether the text and/or properties were changed for > each changed path in a revision. This patch makes the information available > to api users and the svn log -v --xml output. > > * subversion/include/svn_types.h > (svn_tristate_t): New type. > (svn_tristate_to_word, svn_tristate_from_word): New function. > (svn_log_changed_path2_t): Add two new fields. > > * subversion/libsvn_ra_neon/log.c > (log_start_element): Parse text-mods and props-mods attributes. > > * subversion/libsvn_ra_serf/log.c > (read_changed_path_attributes): New function to parse node kind and > modification types. > (start_log): Move node kind parsing to read_changed_path_attributes. > > * subversion/libsvn_ra_svn/client.c > (optbool_to_tristate): New function. > (ra_svn_log): Add parsing of two new optional booleans. > > * subversion/libsvn_ra_svn/protocol > (changed-path-entry): Fix optional parts for node kind and add text-mods > and prop-mods. > > * subversion/libsvn_repos/log.c > (detect_changed): Copy text and property modified values from fs api. > > * subversion/libsvn_subr/kitchensink.c > (svn_tristate_to_word, svn_tristate_from_word): New function. > > * subversion/mod_dav_svn/reports/log.c > (log_receiver): Move common attribute generation code to a single place and > provide text-mods and prop-mods attributes. > > * subversion/svn/log-cmd.c > (log_entry_receiver_xml): Add text-mods and prop-mods attributes. > > * subversion/svn/schema/log.rnc > (attlist.path): Declare text-mods and prop-mods attributes. > > * subversion/svnserve/serve.c > (log_receiver): Provide two extra booleans, removing optional definition > from the node-kind as we always provide this. (Writing optional booleans > is not supported by the svn tuple code). The question is: would we prefer to represent the data another way? For example, we could perhaps make them booleans and say that if both are false when action = 'M', that means the specific data is NOT available, and if either or both is true then that means it IS available. Stefan2 has gone on to use the tristate type for two more purposes in his performance branch, but I'm not sure that we've yet demonstrated it's worth making it a (svn-)global public type. - Julian