You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2015/03/10 11:10:31 UTC
svn commit: r1665437 - /subversion/trunk/subversion/include/svn_fs.h
Author: stefan2
Date: Tue Mar 10 10:10:30 2015
New Revision: 1665437
URL: http://svn.apache.org/r1665437
Log:
Correct and complete docstrings in the FS API after 1.9 review.
No functional change.
* subversion/include/svn_fs.h
(svn_fs_backend_names): New Doxygen group allowing us to refer to the
pre-defined set of backends. Move to front due
to issues with group nesting.
(SVN_FS_CONFIG_COMPATIBLE_VERSION): Works for the current release as well.
(svn_fs_open2): Correct cleanup behavior description for RESULT_POOL.
Document SCRATCH_POOL.
(svn_fs_upgrade_notify_t): Grammar fix.
(svn_fs_upgrade2): Refer to latest repos API.
(svn_fs_hotcopy3): BDB is the only backend not sending notifications.
Document CANCEL_FUNC.
(svn_fs_compare_ids,
svn_fs_check_related): "Now" is 1.9.
(svn_fs_props_changed): Fix confusing wording.
(svn_fs_info_format,
svn_fs_lock_target_t): For clarity, remove '()' from function names in
@see line.
(svn_fs_fsfs_info_t): Make comment visible to doxygen.
(svn_fs_info_placeholder_t): For clarity, remove '()' from function names
in @see line.
(svn_fs_info): Mention FSX alongside FSFS.
(svn_fs_info_dup): Update pool usage documentation.
Modified:
subversion/trunk/subversion/include/svn_fs.h
Modified: subversion/trunk/subversion/include/svn_fs.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_fs.h?rev=1665437&r1=1665436&r2=1665437&view=diff
==============================================================================
--- subversion/trunk/subversion/include/svn_fs.h (original)
+++ subversion/trunk/subversion/include/svn_fs.h Tue Mar 10 10:10:30 2015
@@ -65,6 +65,30 @@ svn_fs_version(void);
/** An object representing a Subversion filesystem. */
typedef struct svn_fs_t svn_fs_t;
+/**
+ * @defgroup svn_fs_backend_names Built-in back-ends
+ * Constants defining the currently supported built-in filesystem backends.
+ *
+ * @see svn_fs_type
+ * @{
+ */
+/** @since New in 1.1. */
+#define SVN_FS_TYPE_BDB "bdb"
+/** @since New in 1.1. */
+#define SVN_FS_TYPE_FSFS "fsfs"
+
+/**
+ * EXPERIMENTAL filesystem backend.
+ *
+ * It is not ready for general production use. Please consult the
+ * respective release notes on suggested usage scenarios.
+ *
+ * @since New in 1.9.
+ */
+#define SVN_FS_TYPE_FSX "fsx"
+
+/** @} */
+
/**
* @name Filesystem configuration options
@@ -139,23 +163,10 @@ typedef struct svn_fs_t svn_fs_t;
/* Note to maintainers: if you add further SVN_FS_CONFIG_FSFS_CACHE_* knobs,
update fs_fs.c:verify_as_revision_before_current_plus_plus(). */
-/* See also svn_fs_type(). */
-/** @since New in 1.1. */
-#define SVN_FS_CONFIG_FS_TYPE "fs-type"
-/** @since New in 1.1. */
-#define SVN_FS_TYPE_BDB "bdb"
-/** @since New in 1.1. */
-#define SVN_FS_TYPE_FSFS "fsfs"
-
-/**
- * EXPERIMENTAL filesystem backend.
+/** Select the filesystem type. See also #svn_fs_type().
*
- * It is not ready for general production use. Please consult the
- * respective release notes on suggested usage scenarios.
- *
- * @since New in 1.9.
- */
-#define SVN_FS_TYPE_FSX "fsx"
+ * @since New in 1.1. */
+#define SVN_FS_CONFIG_FS_TYPE "fs-type"
/** Create repository format compatible with Subversion versions
* earlier than 1.4.
@@ -185,9 +196,9 @@ typedef struct svn_fs_t svn_fs_t;
*/
#define SVN_FS_CONFIG_PRE_1_8_COMPATIBLE "pre-1.8-compatible"
-/** Create repository format compatible with Subversion versions
- * earlier than 1.9. The value must be a version in the same format
- * as #SVN_VER_NUMBER.
+/** Create repository format compatible with the specified Subversion
+ * release. The value must be a version in the same format as
+ * #SVN_VER_NUMBER and cannot exceed the current version.
*
* @note The @c patch component would often be ignored, due to our forward
* compatibility promises within minor release lines. It should therefore
@@ -291,8 +302,9 @@ svn_fs_create(svn_fs_t **fs_p,
* return a pointer to it in @a *fs_p. If @a fs_config is not @c
* NULL, the options it contains modify the behavior of the
* filesystem. The interpretation of @a fs_config is specific to the
- * filesystem back-end. The opened filesystem may be closed by
- * destroying @a result_pool.
+ * filesystem back-end. The opened filesystem will be allocated in
+ * @a result_pool may be closed by clearing or destroying that pool.
+ * Use @a scratch_pool for temporary allocations.
*
* @note The lifetime of @a fs_config must not be shorter than @a
* result_pool's. It's a good idea to allocate @a fs_config from
@@ -346,7 +358,7 @@ typedef enum svn_fs_upgrade_notify_actio
svn_fs_upgrade_format_bumped
} svn_fs_upgrade_notify_action_t;
-/** The type of a upgrade notification function. @a number is specifc
+/** The type of an upgrade notification function. @a number is specifc
* to @a action (see #svn_fs_upgrade_notify_action_t); @a action is the
* type of action being performed. @a baton is the corresponding baton
* for the notification function, and @a pool can be used for temporary
@@ -374,7 +386,7 @@ typedef svn_error_t *(*svn_fs_upgrade_no
* the user to preempt this potentially lengthy operation.
*
* @note You probably don't want to use this directly. Take a look at
- * svn_repos_upgrade() instead.
+ * svn_repos_upgrade2() instead.
*
* @note Canceling an upgrade is legal but may leave remnants of previous
* format data that may not be cleaned up automatically by later calls.
@@ -497,9 +509,13 @@ typedef void (*svn_fs_hotcopy_notify_t)(
* For each revision range copied, @a notify_func will be called with
* staring and ending revision numbers (both inclusive and not necessarily
* different) and with the @a notify_baton. Currently, this notification
- * is only supported in the FSFS backend. @a notify_func may be @c NULL
+ * is not triggered by the BDB backend. @a notify_func may be @c NULL
* if this notification is not required.
*
+ * The optional @a cancel_func callback will be invoked with
+ * @a cancel_baton as usual to allow the user to preempt this potentially
+ * lengthy operation.
+ *
* Use @a scratch_pool for temporary allocations.
*
* @since New in 1.9.
@@ -904,9 +920,9 @@ typedef struct svn_fs_id_t svn_fs_id_t;
/** Return -1, 0, or 1 if node revisions @a a and @a b are respectively
* unrelated, equivalent, or otherwise related (part of the same node).
*
- * @note Using FS ID based functions is now discouraged and may be fully
- * deprecated in future releases. New code should use #svn_fs_node_relation()
- * and #svn_fs_node_relation_t instead.
+ * @note Using FS ID based functions is discouraged since 1.9 and may be
+ * fully deprecated in future releases. New code should use
+ * #svn_fs_node_relation() and #svn_fs_node_relation_t instead.
*/
int
svn_fs_compare_ids(const svn_fs_id_t *a,
@@ -917,9 +933,9 @@ svn_fs_compare_ids(const svn_fs_id_t *a,
/** Return TRUE if node revisions @a id1 and @a id2 are related (part of the
* same node), else return FALSE.
*
- * @note Using FS ID based functions is now discouraged and may be fully
- * deprecated in future releases. New code should use #svn_fs_node_relation()
- * and #svn_fs_node_relation_t instead.
+ * @note Using FS ID based functions is discouraged since 1.9 and may be
+ * fully deprecated in future releases. New code should use
+ * #svn_fs_node_relation() and #svn_fs_node_relation_t instead.
*/
svn_boolean_t
svn_fs_check_related(const svn_fs_id_t *id1,
@@ -1847,7 +1863,7 @@ svn_fs_props_different(svn_boolean_t *di
* at all.
*
* @note Prior to Subversion 1.9, this function would return false negatives
- * as well for FSFS: If @a root1 and @a root2 were both transaction roots
+ * for FSFS: If @a root1 and @a root2 were both transaction roots
* and the proplists of both paths had been changed in their respective
* transactions, @a changed_p would be set to #FALSE.
*/
@@ -2434,7 +2450,7 @@ svn_fs_youngest_rev(svn_revnum_t *younge
* Set @a *supports_version to the version number of the minimum Subversion GA
* release that can read and write @a fs.
*
- * @see svn_repos_info_format()
+ * @see svn_repos_info_format
*
* @since New in 1.9.
*/
@@ -2615,7 +2631,7 @@ svn_fs_set_uuid(svn_fs_t *fs,
/** Lock information for use with svn_fs_lock_many() [and svn_repos_fs_...].
*
- * @see svn_fs_lock_target_create().
+ * @see svn_fs_lock_target_create
*
* @since New in 1.9.
*/
@@ -3045,7 +3061,7 @@ typedef struct svn_fs_fsfs_info_t {
* @note Zero (0) if (but not iff) the format does not support packing. */
svn_revnum_t min_unpacked_rev;
- /* TRUE if logical addressing is enabled for this repository.
+ /** TRUE if logical addressing is enabled for this repository.
* FALSE if repository uses physical addressing. */
svn_boolean_t log_addressing;
/* ### TODO: information about fsfs.conf? rep-cache.db? write locks? */
@@ -3080,10 +3096,10 @@ typedef struct svn_fs_fsx_info_t {
} svn_fs_fsx_info_t;
-/** @see svn_fs_info()
+/** @see svn_fs_info
* @since New in 1.9. */
typedef struct svn_fs_info_placeholder_t {
- /** @see svn_fs_type() */
+ /** @see svn_fs_type */
const char *fs_type;
/* Do not add new fields here, to maintain compatibility with the first
@@ -3093,10 +3109,11 @@ typedef struct svn_fs_info_placeholder_t
/**
* Set @a *fs_info to a struct describing @a fs. The type of the
* struct depends on the backend: for #SVN_FS_TYPE_FSFS, the struct will be
- * of type #svn_fs_fsfs_info_t; otherwise, the struct is guaranteed to be
+ * of type #svn_fs_fsfs_info_t; for #SVN_FS_TYPE_FSX, it will be of type
+ * #svn_fs_fsx_info_t; otherwise, the struct is guaranteed to be
* (compatible with) #svn_fs_info_placeholder_t.
*
- * @see #svn_fs_fsfs_info_t
+ * @see #svn_fs_fsfs_info_t, #svn_fs_fsx_info_t
*
* @since New in 1.9.
*/
@@ -3107,10 +3124,11 @@ svn_fs_info(const svn_fs_info_placeholde
apr_pool_t *scratch_pool);
/**
- * Return a duplicate of @a info, allocated in @a pool. The returned struct
- * will be of the same type as the passed-in struct, which itself must have
- * been returned from svn_fs_info() or svn_fs_info_dup(). No part of the new
- * structure will be shared with @a info (except static string constants).
+ * Return a duplicate of @a info, allocated in @a result_pool. The returned
+ * struct will be of the same type as the passed-in struct, which itself
+ * must have been returned from svn_fs_info() or svn_fs_info_dup(). No part
+ * of the new structure will be shared with @a info (except static string
+ * constants). Use @a scratch_pool for temporary allocations.
*
* @see #svn_fs_info_placeholder_t, #svn_fs_fsfs_info_t
*
Re: svn commit: r1665437 - /subversion/trunk/subversion/include/svn_fs.h
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Tue, Mar 10, 2015 at 1:18 PM, Julian Foad <ju...@btopenworld.com>
wrote:
> Ivan Zhakov wrote:
> >> URL: http://svn.apache.org/r1665437
> >> Modified: subversion/trunk/subversion/include/svn_fs.h
> >> - * @note Using FS ID based functions is now discouraged and may be
> fully
> >> - * deprecated in future releases. New code should use
> #svn_fs_node_relation()
> >> - * and #svn_fs_node_relation_t instead.
> >> + * @note Using FS ID based functions is discouraged since 1.9 and may
> be
> >> + * fully deprecated in future releases. New code should use
> >> + * #svn_fs_node_relation() and #svn_fs_node_relation_t instead.
> >> */
> >> int
> >> svn_fs_compare_ids(const svn_fs_id_t *a,
> (and svn_fs_check_related)
> >
> > Stefan,
> >
> > You have proposed to deprecate the FS ID functions [1], but got well
> > justified objections [2].
> >
> > Are you going to remove these "future deprecation" clauses from
> > svn_fs.h or you have alternative ideas regarding this matter?
>
I have, indeed. My plan is to post an RFC to dev@ once
1.9 RC is out. The basic idea is to introduce svn_fs_node_t
that represents "a path under a svn_fs_root_t". It addresses
a multitude of issues but also makes all dealings with noderev
IDs a backend-specific implementation detail.
> > [1] http://svn.haxx.se/dev/archive-2013-12/0127.shtml
> > [2] http://svn.haxx.se/dev/archive-2013-12/0132.shtml
>
> My thoughts:
>
> The argument that we would want to use these ids for mergeinfo is, in my
> opinion, 99% unlikely.
>
> It doesn't make much sense to deprecate just the id comparison functions
> without deprecating all parts of the FS API that deal with node-rev ids:
> svn_fs_dirent_t, svn_fs_path_change2_t and svn_fs_node_id().
>
There is a fair chance that 1.10 will deprecate the first two of
them (each for different reasons). svn_fs_node_id() is used
by svnlook to display "debugging" info. We may or may not
want to replace that API function.
It would be much clearer to write "node-revision" instead of just "node"
> where the doc string says things like "if it is the same node".
Yes, that is problematic. My thinking is that "noderev" is not
a FS API concept but a backend implementation detail. The
FS API deals with layered trees, paths under a root. I'd call
those things tree nodes, or "node" for short, and they are a
different concept from the FS-internal node concept.
> I suggest we also consider renaming the symbols: s/node/noderev/. The
> symbol 'svn_fs_node_same' in particular is confusing.
>
Yes, that symbol is confusing. That is one of those points
where FS internals leak through the API. Based on what the
client wants to ask it should be something like
"svn_fs_node_no_changes_in_between"
Would be nice to have something shorter there. Also, we
must then document that the function may return
svn_fs_node_common_ancestor instead, e.g. if the nodes
got "touched" but not actually changed.
> This code appears in the FSFS implementation, fs_node_relation():
>
> /* Nodes from different transactions are never related. */
> if (root_a->is_txn_root && root_b->is_txn_root
> && strcmp(root_a->txn, root_b->txn))
> {
> *relation = svn_fs_node_unrelated;
> return SVN_NO_ERROR;
> }
>
> I don't understand this. In the FS-BASE implementation,
> base_node_relation(), node-revs are related iff they share the same
> node-id, regardless of what txns they may be in. Why is it different here?
>
My bad. I misread a part of the old 1.8.x "id equal" code to
mean that there is some idiosyncrasy demanding sibling txn
data to be unrelated. Fixed in r1665894.
> I suggest the following comment changes:
> [[[
> Index: subversion/include/svn_fs.h
> ===================================================================
> --- subversion/include/svn_fs.h (revision 1665169)
> +++ subversion/include/svn_fs.h (working copy)
> @@ -871,25 +871,25 @@ svn_fs_access_add_lock_token(svn_fs_acce
> * @{
> */
>
> -/** Defines the possible ways two arbitrary nodes may be related.
> +/** Defines the possible ways two arbitrary node-revisions may be related.
> *
> * @since New in 1.9.
> */
> typedef enum svn_fs_node_relation_t
> {
> - /** The nodes are not related.
> - * Nodes from different repositories are always unrelated.
> + /** The node-revisions are not related.
> + * Node-revisions from different repositories are always unrelated.
> * #svn_fs_compare_ids would return the value -1 in this case.
> */
> svn_fs_node_unrelated = 0,
>
> - /** They are the same physical node, i.e. there is no intervening
> change.
> + /** They are the same node-revision, i.e. there is no intervening
> change.
> * However, due to lazy copying, there may be part of different parent
> * copies. #svn_fs_compare_ids would return the value 0 in this case.
> */
> svn_fs_node_same,
>
> - /** The nodes have a common ancestor (which may be one of these nodes)
> + /** The node-revisions have a common ancestor (which may be one of them)
> * but are not the same.
> * #svn_fs_compare_ids would return the value 1 in this case.
> */
>
Although noderev is not an FS API concept, those docstring
changes are o.k. for now. Once there is a proper, well-defined
svn_fs_node_t, we can rephrase them in terms of new abstraction.
> @@ -904,9 +904,7 @@ typedef struct svn_fs_id_t svn_fs_id_t;
> /** Return -1, 0, or 1 if node revisions @a a and @a b are respectively
> * unrelated, equivalent, or otherwise related (part of the same node).
> *
> - * @note Using FS ID based functions is now discouraged and may be fully
> - * deprecated in future releases. New code should use
> #svn_fs_node_relation()
> - * and #svn_fs_node_relation_t instead.
> + * @see svn_fs_node_relation() for an alternative.
> */
> int
> svn_fs_compare_ids(const svn_fs_id_t *a,
> @@ -917,9 +915,7 @@ svn_fs_compare_ids(const svn_fs_id_t *a,
> /** Return TRUE if node revisions @a id1 and @a id2 are related (part of
> the
> * same node), else return FALSE.
> *
> - * @note Using FS ID based functions is now discouraged and may be fully
> - * deprecated in future releases. New code should use
> #svn_fs_node_relation()
> - * and #svn_fs_node_relation_t instead.
> + * @see svn_fs_node_relation() for an alternative.
> */
> svn_boolean_t
> svn_fs_check_related(const svn_fs_id_t *id1,
> ]]]
>
I did something similar in r1665896.
On Tue, Mar 10, 2015 at 1:54 PM, Branko Čibej <br...@wandisco.com> wrote:
> We do not, as a rule, deprecate, or warn about possible deprecation of,
> APIs that we do not provide replacements for in the same release. "May
> be deprecated in the future" is true for *all* of our public APIs. I see
> no good reason to put this in docstrings for these particular functions.
>
Yah, that was silly. All I wanted to express is that those
functions are all kinds of "nasty".
IMO, either actually deprecate the APIs, or leave out that text.
>
With r1665896, the docstring actual says what the user
should do.
On Fri, Mar 13, 2015 at 2:46 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 10 March 2015 at 15:18, Julian Foad <ju...@btopenworld.com> wrote:
> > Ivan Zhakov wrote:
> >>> URL: http://svn.apache.org/r1665437
> >>> Modified: subversion/trunk/subversion/include/svn_fs.h
> >>> - * @note Using FS ID based functions is now discouraged and may be
> fully
> >>> - * deprecated in future releases. New code should use
> #svn_fs_node_relation()
> >>> - * and #svn_fs_node_relation_t instead.
> >>> + * @note Using FS ID based functions is discouraged since 1.9 and may
> be
> >>> + * fully deprecated in future releases. New code should use
> >>> + * #svn_fs_node_relation() and #svn_fs_node_relation_t instead.
> >>> */
> >>> int
> >>> svn_fs_compare_ids(const svn_fs_id_t *a,
> > (and svn_fs_check_related)
> >>
> >> Stefan,
> >>
> >> You have proposed to deprecate the FS ID functions [1], but got well
> >> justified objections [2].
> >>
> >> Are you going to remove these "future deprecation" clauses from
> >> svn_fs.h or you have alternative ideas regarding this matter?
> >>
> >> [1] http://svn.haxx.se/dev/archive-2013-12/0127.shtml
> >> [2] http://svn.haxx.se/dev/archive-2013-12/0132.shtml
> >
> > My thoughts:
> >
> > The argument that we would want to use these ids for mergeinfo is, in my
> opinion, 99% unlikely.
> >
> > It doesn't make much sense to deprecate just the id comparison functions
> without
> > deprecating all parts of the FS API that deal with node-rev ids:
> svn_fs_dirent_t,
> > svn_fs_path_change2_t and svn_fs_node_id().
> >
> My original point was mostly about procedure: deprecation was proposed
> during 1.9 development and got well justified objection. But "future
> deprecation" was still documented in svn_fs.h, despite of objections.
>
That one has been fixed now.
Regarding the svn_fs_id_t concept itself: I think node IDs is very
> powerful mechanism and we should use them more instead of deprecation.
> For example extend getters like svn_fs_file_checksum() to accept node
> id as hint, this will help us to avoid a lot of DAG walks. Because if
> application want to list directory entries and get checksum for every
> file FSFS do the following:
> 1. svn_fs_dirent() returns list of directory entries with name and node id
> 2. for every entry applications calls svn_fs_file_checksum(root, path)
> and FSFS has to perform DAG walk to find node for this path. It's very
> expensive.
>
And I fully agree with that intend. My approach is to
introduce svn_fs_node_t as "svn_fs_id_t done right",
specifying / defining all the bits that we can't change
retroactively for the plain IDs.
A reasonable implementation would use noderev IDs
internally to prevent exactly those redundant DAG
walks, path constructions, validity checks ("am I a
node under a txn root?"). svn_fs_dirent & friends
will need to be updated as well, of course.
-- Stefan^2.
Re: svn commit: r1665437 - /subversion/trunk/subversion/include/svn_fs.h
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 10 March 2015 at 15:18, Julian Foad <ju...@btopenworld.com> wrote:
> Ivan Zhakov wrote:
>>> URL: http://svn.apache.org/r1665437
>>> Modified: subversion/trunk/subversion/include/svn_fs.h
>>> - * @note Using FS ID based functions is now discouraged and may be fully
>>> - * deprecated in future releases. New code should use #svn_fs_node_relation()
>>> - * and #svn_fs_node_relation_t instead.
>>> + * @note Using FS ID based functions is discouraged since 1.9 and may be
>>> + * fully deprecated in future releases. New code should use
>>> + * #svn_fs_node_relation() and #svn_fs_node_relation_t instead.
>>> */
>>> int
>>> svn_fs_compare_ids(const svn_fs_id_t *a,
> (and svn_fs_check_related)
>>
>> Stefan,
>>
>> You have proposed to deprecate the FS ID functions [1], but got well
>> justified objections [2].
>>
>> Are you going to remove these "future deprecation" clauses from
>> svn_fs.h or you have alternative ideas regarding this matter?
>>
>> [1] http://svn.haxx.se/dev/archive-2013-12/0127.shtml
>> [2] http://svn.haxx.se/dev/archive-2013-12/0132.shtml
>
> My thoughts:
>
> The argument that we would want to use these ids for mergeinfo is, in my opinion, 99% unlikely.
>
> It doesn't make much sense to deprecate just the id comparison functions without
> deprecating all parts of the FS API that deal with node-rev ids: svn_fs_dirent_t,
> svn_fs_path_change2_t and svn_fs_node_id().
>
My original point was mostly about procedure: deprecation was proposed
during 1.9 development and got well justified objection. But "future
deprecation" was still documented in svn_fs.h, despite of objections.
Regarding the svn_fs_id_t concept itself: I think node IDs is very
powerful mechanism and we should use them more instead of deprecation.
For example extend getters like svn_fs_file_checksum() to accept node
id as hint, this will help us to avoid a lot of DAG walks. Because if
application want to list directory entries and get checksum for every
file FSFS do the following:
1. svn_fs_dirent() returns list of directory entries with name and node id
2. for every entry applications calls svn_fs_file_checksum(root, path)
and FSFS has to perform DAG walk to find node for this path. It's very
expensive.
> It would be much clearer to write "node-revision" instead of just "node" where the doc string
> says things like "if it is the same node". I suggest we also consider renaming the
> symbols: s/node/noderev/. The symbol 'svn_fs_node_same' in particular is confusing.
>
> This code appears in the FSFS implementation, fs_node_relation():
>
> /* Nodes from different transactions are never related. */
> if (root_a->is_txn_root && root_b->is_txn_root
> && strcmp(root_a->txn, root_b->txn))
> {
> *relation = svn_fs_node_unrelated;
> return SVN_NO_ERROR;
> }
>
> I don't understand this. In the FS-BASE implementation, base_node_relation(), node-revs are
> related iff they share the same node-id, regardless of what txns they may be in. Why is it different here?
>
> I suggest the following comment changes:
The proposed patch makes sense for me.
--
Ivan Zhakov
Re: svn commit: r1665437 - /subversion/trunk/subversion/include/svn_fs.h
Posted by Branko Čibej <br...@wandisco.com>.
On 10.03.2015 13:18, Julian Foad wrote:
> Ivan Zhakov wrote:
>>> URL: http://svn.apache.org/r1665437
>>> Modified: subversion/trunk/subversion/include/svn_fs.h
>>> - * @note Using FS ID based functions is now discouraged and may be fully
>>> - * deprecated in future releases. New code should use #svn_fs_node_relation()
>>> - * and #svn_fs_node_relation_t instead.
>>> + * @note Using FS ID based functions is discouraged since 1.9 and may be
>>> + * fully deprecated in future releases. New code should use
>>> + * #svn_fs_node_relation() and #svn_fs_node_relation_t instead.
>>> */
>>> int
>>> svn_fs_compare_ids(const svn_fs_id_t *a,
> (and svn_fs_check_related)
>> Stefan,
>>
>> You have proposed to deprecate the FS ID functions [1], but got well
>> justified objections [2].
>>
>> Are you going to remove these "future deprecation" clauses from
>> svn_fs.h or you have alternative ideas regarding this matter?
>>
>> [1] http://svn.haxx.se/dev/archive-2013-12/0127.shtml
>> [2] http://svn.haxx.se/dev/archive-2013-12/0132.shtml
> My thoughts:
>
> The argument that we would want to use these ids for mergeinfo is, in my opinion, 99% unlikely.
Doesn't matter.
We do not, as a rule, deprecate, or warn about possible deprecation of,
APIs that we do not provide replacements for in the same release. "May
be deprecated in the future" is true for *all* of our public APIs. I see
no good reason to put this in docstrings for these particular functions.
IMO, either actually deprecate the APIs, or leave out that text.
> It doesn't make much sense to deprecate just the id comparison functions without deprecating all parts of the FS API that deal with node-rev ids: svn_fs_dirent_t, svn_fs_path_change2_t and svn_fs_node_id().
>
> It would be much clearer to write "node-revision" instead of just "node" where the doc string says things like "if it is the same node". I suggest we also consider renaming the symbols: s/node/noderev/. The symbol 'svn_fs_node_same' in particular is confusing.
Indeed. We've tried, for 15 years, to be clear about the difference
between a "node" and a "node revision". These names are a huge step
backwards.
-- Brane
Re: svn commit: r1665437 - /subversion/trunk/subversion/include/svn_fs.h
Posted by Julian Foad <ju...@btopenworld.com>.
Ivan Zhakov wrote:
>> URL: http://svn.apache.org/r1665437
>> Modified: subversion/trunk/subversion/include/svn_fs.h
>> - * @note Using FS ID based functions is now discouraged and may be fully
>> - * deprecated in future releases. New code should use #svn_fs_node_relation()
>> - * and #svn_fs_node_relation_t instead.
>> + * @note Using FS ID based functions is discouraged since 1.9 and may be
>> + * fully deprecated in future releases. New code should use
>> + * #svn_fs_node_relation() and #svn_fs_node_relation_t instead.
>> */
>> int
>> svn_fs_compare_ids(const svn_fs_id_t *a,
(and svn_fs_check_related)
>
> Stefan,
>
> You have proposed to deprecate the FS ID functions [1], but got well
> justified objections [2].
>
> Are you going to remove these "future deprecation" clauses from
> svn_fs.h or you have alternative ideas regarding this matter?
>
> [1] http://svn.haxx.se/dev/archive-2013-12/0127.shtml
> [2] http://svn.haxx.se/dev/archive-2013-12/0132.shtml
My thoughts:
The argument that we would want to use these ids for mergeinfo is, in my opinion, 99% unlikely.
It doesn't make much sense to deprecate just the id comparison functions without deprecating all parts of the FS API that deal with node-rev ids: svn_fs_dirent_t, svn_fs_path_change2_t and svn_fs_node_id().
It would be much clearer to write "node-revision" instead of just "node" where the doc string says things like "if it is the same node". I suggest we also consider renaming the symbols: s/node/noderev/. The symbol 'svn_fs_node_same' in particular is confusing.
This code appears in the FSFS implementation, fs_node_relation():
/* Nodes from different transactions are never related. */
if (root_a->is_txn_root && root_b->is_txn_root
&& strcmp(root_a->txn, root_b->txn))
{
*relation = svn_fs_node_unrelated;
return SVN_NO_ERROR;
}
I don't understand this. In the FS-BASE implementation, base_node_relation(), node-revs are related iff they share the same node-id, regardless of what txns they may be in. Why is it different here?
I suggest the following comment changes:
[[[
Index: subversion/include/svn_fs.h
===================================================================
--- subversion/include/svn_fs.h (revision 1665169)
+++ subversion/include/svn_fs.h (working copy)
@@ -871,25 +871,25 @@ svn_fs_access_add_lock_token(svn_fs_acce
* @{
*/
-/** Defines the possible ways two arbitrary nodes may be related.
+/** Defines the possible ways two arbitrary node-revisions may be related.
*
* @since New in 1.9.
*/
typedef enum svn_fs_node_relation_t
{
- /** The nodes are not related.
- * Nodes from different repositories are always unrelated.
+ /** The node-revisions are not related.
+ * Node-revisions from different repositories are always unrelated.
* #svn_fs_compare_ids would return the value -1 in this case.
*/
svn_fs_node_unrelated = 0,
- /** They are the same physical node, i.e. there is no intervening change.
+ /** They are the same node-revision, i.e. there is no intervening change.
* However, due to lazy copying, there may be part of different parent
* copies. #svn_fs_compare_ids would return the value 0 in this case.
*/
svn_fs_node_same,
- /** The nodes have a common ancestor (which may be one of these nodes)
+ /** The node-revisions have a common ancestor (which may be one of them)
* but are not the same.
* #svn_fs_compare_ids would return the value 1 in this case.
*/
@@ -904,9 +904,7 @@ typedef struct svn_fs_id_t svn_fs_id_t;
/** Return -1, 0, or 1 if node revisions @a a and @a b are respectively
* unrelated, equivalent, or otherwise related (part of the same node).
*
- * @note Using FS ID based functions is now discouraged and may be fully
- * deprecated in future releases. New code should use #svn_fs_node_relation()
- * and #svn_fs_node_relation_t instead.
+ * @see svn_fs_node_relation() for an alternative.
*/
int
svn_fs_compare_ids(const svn_fs_id_t *a,
@@ -917,9 +915,7 @@ svn_fs_compare_ids(const svn_fs_id_t *a,
/** Return TRUE if node revisions @a id1 and @a id2 are related (part of the
* same node), else return FALSE.
*
- * @note Using FS ID based functions is now discouraged and may be fully
- * deprecated in future releases. New code should use #svn_fs_node_relation()
- * and #svn_fs_node_relation_t instead.
+ * @see svn_fs_node_relation() for an alternative.
*/
svn_boolean_t
svn_fs_check_related(const svn_fs_id_t *id1,
]]]
- Julian
Re: svn commit: r1665437 - /subversion/trunk/subversion/include/svn_fs.h
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 10 March 2015 at 13:10, <st...@apache.org> wrote:
> Author: stefan2
> Date: Tue Mar 10 10:10:30 2015
> New Revision: 1665437
>
> URL: http://svn.apache.org/r1665437
> Log:
> Correct and complete docstrings in the FS API after 1.9 review.
> No functional change.
>
> * subversion/include/svn_fs.h
> (svn_fs_backend_names): New Doxygen group allowing us to refer to the
> pre-defined set of backends. Move to front due
> to issues with group nesting.
> (SVN_FS_CONFIG_COMPATIBLE_VERSION): Works for the current release as well.
> (svn_fs_open2): Correct cleanup behavior description for RESULT_POOL.
> Document SCRATCH_POOL.
> (svn_fs_upgrade_notify_t): Grammar fix.
> (svn_fs_upgrade2): Refer to latest repos API.
> (svn_fs_hotcopy3): BDB is the only backend not sending notifications.
> Document CANCEL_FUNC.
> (svn_fs_compare_ids,
> svn_fs_check_related): "Now" is 1.9.
> (svn_fs_props_changed): Fix confusing wording.
> (svn_fs_info_format,
> svn_fs_lock_target_t): For clarity, remove '()' from function names in
> @see line.
> (svn_fs_fsfs_info_t): Make comment visible to doxygen.
> (svn_fs_info_placeholder_t): For clarity, remove '()' from function names
> in @see line.
> (svn_fs_info): Mention FSX alongside FSFS.
> (svn_fs_info_dup): Update pool usage documentation.
>
> Modified:
> subversion/trunk/subversion/include/svn_fs.h
>
> Modified: subversion/trunk/subversion/include/svn_fs.h
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_fs.h?rev=1665437&r1=1665436&r2=1665437&view=diff
[...]
> - * @note Using FS ID based functions is now discouraged and may be fully
> - * deprecated in future releases. New code should use #svn_fs_node_relation()
> - * and #svn_fs_node_relation_t instead.
> + * @note Using FS ID based functions is discouraged since 1.9 and may be
> + * fully deprecated in future releases. New code should use
> + * #svn_fs_node_relation() and #svn_fs_node_relation_t instead.
> */
> int
> svn_fs_compare_ids(const svn_fs_id_t *a,
> @@ -917,9 +933,9 @@ svn_fs_compare_ids(const svn_fs_id_t *a,
> /** Return TRUE if node revisions @a id1 and @a id2 are related (part of the
> * same node), else return FALSE.
> *
> - * @note Using FS ID based functions is now discouraged and may be fully
> - * deprecated in future releases. New code should use #svn_fs_node_relation()
> - * and #svn_fs_node_relation_t instead.
> + * @note Using FS ID based functions is discouraged since 1.9 and may be
> + * fully deprecated in future releases. New code should use
> + * #svn_fs_node_relation() and #svn_fs_node_relation_t instead.
> */
Stefan,
You have proposed to deprecate the FS ID functions [1], but got well
justified objections [2].
Are you going to remove these "future deprecation" clauses from
svn_fs.h or you have alternative ideas regarding this matter?
[1] http://svn.haxx.se/dev/archive-2013-12/0127.shtml
[2] http://svn.haxx.se/dev/archive-2013-12/0132.shtml
--
Ivan Zhakov