Re: Confusion to svn_kind_t!

On 11.03.2013 13:14, Bert Huijben wrote:
>> -----Original Message-----
>> From: Stefan Sperling [mailto:stsp_at_elego.de]
>> Sent: maandag 11 maart 2013 12:59
>> To: Branko Čibej
>> Cc: Subversion Development
>> Subject: Re: Confusion to svn_kind_t!
>>
>> On Mon, Mar 11, 2013 at 11:25:11AM +0100, Branko Čibej wrote:
>>> As I've been updating JavaHL for 1.8, I tripped over the new svn_kind_t
>>> enum. Since it not only replaces svn_node_kind_t but also changes the
>>> values of the equivalent enumeration constants, maintaining backward
>>> compatibility in JavaHL is going to be a bit of a pain.
>> There have also been (still are?) several bugs on trunk where the wrong
>> kind is being used because code erroneously uses a node_kind_t where a
>> kind_t is expected and vice versa.
>>
>> I agree that keeping the existing enumeration values consistent between
>> the two types would ease transition and might also prevent bugs.
>>
>>> Do we really need a new enumeration type just to add a new enum
>>> constant? I'd prefer to extend svn_node_kind_t with the symlink value
>>> instead.
>> At this point use of svn_kind_t is very much engrained in libsvn_wc.
>> It is not going to be trivial to undo that. So I would say we keep
>> the new type but make the enum values for existing node_kind_t values
>> consistent.
> The original reasoning for adding a new enum was from gstein: Third party tools linking to libsvn_client don't expect new values.

The versioning guidelines do not mention enumerations, but IMO it
follows from "new functions" and "new constants" that new enumeration
values are allowed, too. In fact we already have cases where we've
extended enums, see e.g., svn_repos_notify_action_t in svn_repos.h and
svn_wc_notify_action_t in svn_wc.h.

> But 1.8 will be the second version with the new enum (it was svn_wc__db_kind_t in 1.7) and we never use svn_kind_symlink the way it was intended.

How was it intended to be used? I see lots of uses, but they don't tell
me anything about intentions.

> Mapping the enum values will probably only increase the confusion whenever we really start using the symlink or other future values.
>
> If that is the problem we try to solve, why not just remove svn_kind_t and go back to svn_node_kind_t?

I find it extremely confusing and frankly bad API design that we have
two public enums that mean essentially the same thing. Either svn_kind_t
should remain private to libsvn_wc, or we should extend and use
svn_node_kind_t everywhere. The situation as I see it now is worse than
either of these alternatives.

If the problem is mapping the (new) notion of symlink back to the (old)
notion of file, then we can add predicate functions (e.g.,
svn_node_kind_is_file()). That won't help old clients that decided not
to deal with out-of-bounds enumeration values, but since svn_kind_t only
exposed in *one* public API in svn_delta.h[1], which is new in 1.8, I
don't find that to be a problem. I'd much rather rev the pre-1.8 APIs
that can return svn_node_kind_t and add backward-compat shims to the
deprecated implementations to map symlink to file, with the new versions
exposing the whole range of the enum.

Unless someone can thing of a really good reason why that wouldn't work,
I propose to implement that plan and merge svn_node_kind_t with svn_kind_t.

-- Brane

[1] There's another direct use of svn_kind_t in svn_wc.h, which (a) I
don't really see as a public api, and (b) even if it is, that use is
also new in 1.8 and needs no backward-compat shims.

-- 
Branko Čibej
Director of Subversion | WANdisco | www.wandisco.com