You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Branko Čibej <br...@wandisco.com> on 2013/03/11 11:25:11 UTC
Confusion to svn_kind_t!
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.
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.
-- Brane
--
Branko Čibej
Director of Subversion | WANdisco | www.wandisco.com
Re: Confusion to svn_kind_t!
Posted by Branko Čibej <br...@wandisco.com>.
On 11.03.2013 11:46, Daniel Shahaf wrote:
> Branko Čibej wrote on Mon, Mar 11, 2013 at 11:25:11 +0100:
>> 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.
>>
>> 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.
> Don't know about java, but we've confused the two kinds before, so
> perhaps:
>
> Index: subversion/include/svn_types.h
> ===================================================================
> --- subversion/include/svn_types.h (revision 1455090)
> +++ subversion/include/svn_types.h (working copy)
> @@ -223,7 +223,7 @@ svn__apr_hash_index_val(const apr_hash_index_t *hi
> typedef enum svn_kind_t
> {
> /** something's here, but we don't know what */
> - svn_kind_unknown,
> + svn_kind_unknown = 100, /* do not overlap svn_node_kind_t */
>
> /** absent */
> svn_kind_none,
That would work, too. The thing is, I'd rather not invent a whole new
kind type in Java and would much prefer to create a consistent mapping
of both enumerations to the same Java enum.
-- Brane
--
Branko Čibej
Director of Subversion | WANdisco | www.wandisco.com
Re: Confusion to svn_kind_t!
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Branko Čibej wrote on Mon, Mar 11, 2013 at 11:25:11 +0100:
> 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.
>
> 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.
Don't know about java, but we've confused the two kinds before, so
perhaps:
Index: subversion/include/svn_types.h
===================================================================
--- subversion/include/svn_types.h (revision 1455090)
+++ subversion/include/svn_types.h (working copy)
@@ -223,7 +223,7 @@ svn__apr_hash_index_val(const apr_hash_index_t *hi
typedef enum svn_kind_t
{
/** something's here, but we don't know what */
- svn_kind_unknown,
+ svn_kind_unknown = 100, /* do not overlap svn_node_kind_t */
/** absent */
svn_kind_none,
Re: Confusion to svn_kind_t!
Posted by Branko Čibej <br...@wandisco.com>.
So I've started the merge of svn_kind_t into svn_node_kind_t and have
run across a more fundamental kind of conflict that I can't resolve
without the help of the authors of the tree-conflict storage.
Here's the problem: the WC code has a mechanism for converting
enumeration values used in code to tokens used by the database. This
mechanism uses a conversion table. For the vast majority of uses, the
table looks like this:
[from subversion/libsvn_wc/token-map.h]
static const svn_token_map_t kind_map[] = {
{ "file", svn_kind_file }, /* MAP_FILE */
{ "dir", svn_kind_dir }, /* MAP_DIR */
{ "symlink", svn_kind_symlink }, /* MAP_SYMLINK */
{ "unknown", svn_kind_unknown }, /* MAP_UNKNOWN */
{ NULL }
};
However, for tree conflict storage, we have:
[from subversion/libsvn_wc/tree_conflicts.c]
static const svn_token_map_t node_kind_map[] =
{
{ "none", svn_node_none },
{ "file", svn_node_file },
{ "dir", svn_node_dir },
{ "", svn_node_unknown },
{ NULL }
};
Note that the first table is for svn_kind_t, whereas the second is for
svn_node_kind_t (which supposedly shouldn't have been used in the WC
code in the first place); but that's not the issue, the biggest problem
is the difference in mapping svn_kind_unknown and svn_node_unknown.
I could leave the difference as-is, but frankly it seems silly to be
inconsistent in the way we represent essentially the same things in the
database. In my opinion, this calls for a WC-version bump before we
branch 1.8.
Other opinions please?
-- Brane
--
Branko Čibej
Director of Subversion | WANdisco | www.wandisco.com
Re: Confusion to svn_kind_t!
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Mar 11, 2013 at 01:44:50PM +0100, Branko Čibej wrote:
> 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.
Absolutely fine with me.
Re: Confusion to svn_kind_t!
Posted by Branko Čibej <br...@wandisco.com>.
On 11.03.2013 13:14, Bert Huijben wrote:
>> -----Original Message-----
>> From: Stefan Sperling [mailto:stsp@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
RE: Confusion to svn_kind_t!
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Stefan Sperling [mailto:stsp@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.
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.
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?
>
> This diff might break existing 1.8 working copies so we should probably
> bump the format number as well...
>
> [[[
> * subversion/include/svn_types.h
> (svn_kind_t): Map kind values that already existed in svn_node_kind_t
> to the same enumeration constants. This makes it easier to transition
> from svn_node_kind_t to svn_kind_t, since svn_kind_t values are now
> backwards compatible with node_kind_t.
> ]]]
We don't need a format bump as the working copy stores a word, not an int.
Bert
>
> Index: subversion/include/svn_types.h
> ==========================================================
> =========
> --- subversion/include/svn_types.h (revision 1455102)
> +++ subversion/include/svn_types.h (working copy)
> @@ -222,9 +222,6 @@ svn__apr_hash_index_val(const apr_hash_index_t
> *hi
> */
> typedef enum svn_kind_t
> {
> - /** something's here, but we don't know what */
> - svn_kind_unknown,
> -
> /** absent */
> svn_kind_none,
>
> @@ -234,6 +231,9 @@ typedef enum svn_kind_t
> /** directory */
> svn_kind_dir,
>
> + /** something's here, but we don't know what */
> + svn_kind_unknown,
> +
> /** symbolic link */
> svn_kind_symlink
>
Re: Confusion to svn_kind_t!
Posted by Stefan Sperling <st...@elego.de>.
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.
This diff might break existing 1.8 working copies so we should probably
bump the format number as well...
[[[
* subversion/include/svn_types.h
(svn_kind_t): Map kind values that already existed in svn_node_kind_t
to the same enumeration constants. This makes it easier to transition
from svn_node_kind_t to svn_kind_t, since svn_kind_t values are now
backwards compatible with node_kind_t.
]]]
Index: subversion/include/svn_types.h
===================================================================
--- subversion/include/svn_types.h (revision 1455102)
+++ subversion/include/svn_types.h (working copy)
@@ -222,9 +222,6 @@ svn__apr_hash_index_val(const apr_hash_index_t *hi
*/
typedef enum svn_kind_t
{
- /** something's here, but we don't know what */
- svn_kind_unknown,
-
/** absent */
svn_kind_none,
@@ -234,6 +231,9 @@ typedef enum svn_kind_t
/** directory */
svn_kind_dir,
+ /** something's here, but we don't know what */
+ svn_kind_unknown,
+
/** symbolic link */
svn_kind_symlink