You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Stefan Fuhrmann <st...@apache.org> on 2017/12/21 14:14:25 UTC
svn_dirent_t.size API inconsistency
I think I found an API documentation bug.
svn_types.h specifies for svn_dirent_t.size:
/** length of file text, or 0 for directories */
svn_filesize_t size;
However, (almost?) all implementations set it to
SVN_INVALID_FILESIZE for directories. This is also
what we do for svn_io_dirent_t.size.
I discovered this as r1818578 broke our Perl tests.
The same test makes it clear that no API user could
have successfully used the API as documented.
Therefore, I suggest to change the API doc to match
the implementation, check the code for consistency
and add an entry to our errata list.
-- Stefan^2.
RE: svn_dirent_t.size API inconsistency
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Stefan Fuhrmann [mailto:stefan2@apache.org]
> Sent: donderdag 21 december 2017 16:01
> To: dev@subversion.apache.org
> Subject: Re: svn_dirent_t.size API inconsistency
>
> On 21.12.2017 15:14, Stefan Fuhrmann wrote:
> > I think I found an API documentation bug.
> > svn_types.h specifies for svn_dirent_t.size:
> >
> > /** length of file text, or 0 for directories */
> > svn_filesize_t size;
> >
> > However, (almost?) all implementations set it to
> > SVN_INVALID_FILESIZE for directories. This is also
> > what we do for svn_io_dirent_t.size.
> >
> > I discovered this as r1818578 broke our Perl tests.
> > The same test makes it clear that no API user could
> > have successfully used the API as documented.
> >
> > Therefore, I suggest to change the API doc to match
> > the implementation, check the code for consistency
> > and add an entry to our errata list.
+1 on the change.
Given that we already have all existing callers do the right thing (so no behavior change), I'm not sure if adding something to our errata list is necessary... But I don't see a problem with adding it anyway.
Bert
> >
> > -- Stefan^2.
> >
> Below the necessary patch w/o erratum.
>
> -- Stefan^2.
>
> [[[
> Fix inconsistent handling of svn_dirent_t.size for directories.
>
> Most code uses SVN_INVALID_FILESIZE for them, so change the docs to
> match
> that fact and update all users to consistently follow the new docstring.
>
> * subversion/include/svn_types.h
> (svn_dirent_t): Change documentation for SIZE.
>
> * subversion/libsvn_ra_local/ra_plugin.c
> (svn_ra_local__get_dir): Return the new default for directories.
>
> * subversion/libsvn_ra_serf/list.c
> (item_closed): Be sure to default to SVN_INVALID_FILESIZE.
>
> * subversion/libsvn_repos/list.c
> (fill_dirent): Set SIZE consistently with other RA layers.
>
> --This line, and those below, will be ignored--
>
> Index: subversion/include/svn_types.h
> ===============================================================
> ====
> --- subversion/include/svn_types.h (revision 1818804)
> +++ subversion/include/svn_types.h (working copy)
> @@ -652,7 +652,7 @@ typedef struct svn_dirent_t
> /** node kind */
> svn_node_kind_t kind;
>
> - /** length of file text, or 0 for directories */
> + /** length of file text, otherwise SVN_INVALID_FILESIZE */
> svn_filesize_t size;
>
> /** does the node have props? */
> Index: subversion/libsvn_ra_local/ra_plugin.c
> ===============================================================
> ====
> --- subversion/libsvn_ra_local/ra_plugin.c (revision 1818804)
> +++ subversion/libsvn_ra_local/ra_plugin.c (working copy)
> @@ -1387,7 +1387,7 @@ svn_ra_local__get_dir(svn_ra_session_t *
> {
> /* size */
> if (fs_entry->kind == svn_node_dir)
> - entry->size = 0;
> + entry->size = SVN_INVALID_FILESIZE;
> else
> SVN_ERR(svn_fs_file_length(&(entry->size), root,
> fullpath, iterpool));
> Index: subversion/libsvn_ra_serf/list.c
> ===============================================================
> ====
> --- subversion/libsvn_ra_serf/list.c (revision 1818932)
> +++ subversion/libsvn_ra_serf/list.c (working copy)
> @@ -139,6 +139,8 @@ item_closed(svn_ra_serf__xml_estate_t *x
>
> if (size)
> SVN_ERR(svn_cstring_atoi64(&dirent.size, size));
> + else
> + dirent.size = SVN_INVALID_FILESIZE;
>
> if (crev)
> SVN_ERR(svn_revnum_parse(&dirent.created_rev, crev, NULL));
> Index: subversion/libsvn_repos/list.c
> ===============================================================
> ====
> --- subversion/libsvn_repos/list.c (revision 1818804)
> +++ subversion/libsvn_repos/list.c (working copy)
> @@ -51,7 +51,7 @@ fill_dirent(svn_dirent_t *dirent,
> if (dirent->kind == svn_node_file)
> SVN_ERR(svn_fs_file_length(&(dirent->size), root, path,
> scratch_pool));
> else
> - dirent->size = 0;
> + dirent->size = SVN_INVALID_FILESIZE;
>
> SVN_ERR(svn_fs_node_has_props(&dirent->has_props, root, path,
> scratch_pool));
> ]]]
>
Re: svn_dirent_t.size API inconsistency
Posted by Stefan Fuhrmann <st...@apache.org>.
On 21.12.2017 15:14, Stefan Fuhrmann wrote:
> I think I found an API documentation bug.
> svn_types.h specifies for svn_dirent_t.size:
>
> /** length of file text, or 0 for directories */
> svn_filesize_t size;
>
> However, (almost?) all implementations set it to
> SVN_INVALID_FILESIZE for directories. This is also
> what we do for svn_io_dirent_t.size.
>
> I discovered this as r1818578 broke our Perl tests.
> The same test makes it clear that no API user could
> have successfully used the API as documented.
>
> Therefore, I suggest to change the API doc to match
> the implementation, check the code for consistency
> and add an entry to our errata list.
>
> -- Stefan^2.
>
Below the necessary patch w/o erratum.
-- Stefan^2.
[[[
Fix inconsistent handling of svn_dirent_t.size for directories.
Most code uses SVN_INVALID_FILESIZE for them, so change the docs to match
that fact and update all users to consistently follow the new docstring.
* subversion/include/svn_types.h
(svn_dirent_t): Change documentation for SIZE.
* subversion/libsvn_ra_local/ra_plugin.c
(svn_ra_local__get_dir): Return the new default for directories.
* subversion/libsvn_ra_serf/list.c
(item_closed): Be sure to default to SVN_INVALID_FILESIZE.
* subversion/libsvn_repos/list.c
(fill_dirent): Set SIZE consistently with other RA layers.
--This line, and those below, will be ignored--
Index: subversion/include/svn_types.h
===================================================================
--- subversion/include/svn_types.h (revision 1818804)
+++ subversion/include/svn_types.h (working copy)
@@ -652,7 +652,7 @@ typedef struct svn_dirent_t
/** node kind */
svn_node_kind_t kind;
- /** length of file text, or 0 for directories */
+ /** length of file text, otherwise SVN_INVALID_FILESIZE */
svn_filesize_t size;
/** does the node have props? */
Index: subversion/libsvn_ra_local/ra_plugin.c
===================================================================
--- subversion/libsvn_ra_local/ra_plugin.c (revision 1818804)
+++ subversion/libsvn_ra_local/ra_plugin.c (working copy)
@@ -1387,7 +1387,7 @@ svn_ra_local__get_dir(svn_ra_session_t *
{
/* size */
if (fs_entry->kind == svn_node_dir)
- entry->size = 0;
+ entry->size = SVN_INVALID_FILESIZE;
else
SVN_ERR(svn_fs_file_length(&(entry->size), root,
fullpath, iterpool));
Index: subversion/libsvn_ra_serf/list.c
===================================================================
--- subversion/libsvn_ra_serf/list.c (revision 1818932)
+++ subversion/libsvn_ra_serf/list.c (working copy)
@@ -139,6 +139,8 @@ item_closed(svn_ra_serf__xml_estate_t *x
if (size)
SVN_ERR(svn_cstring_atoi64(&dirent.size, size));
+ else
+ dirent.size = SVN_INVALID_FILESIZE;
if (crev)
SVN_ERR(svn_revnum_parse(&dirent.created_rev, crev, NULL));
Index: subversion/libsvn_repos/list.c
===================================================================
--- subversion/libsvn_repos/list.c (revision 1818804)
+++ subversion/libsvn_repos/list.c (working copy)
@@ -51,7 +51,7 @@ fill_dirent(svn_dirent_t *dirent,
if (dirent->kind == svn_node_file)
SVN_ERR(svn_fs_file_length(&(dirent->size), root, path,
scratch_pool));
else
- dirent->size = 0;
+ dirent->size = SVN_INVALID_FILESIZE;
SVN_ERR(svn_fs_node_has_props(&dirent->has_props, root, path,
scratch_pool));
]]]