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
Received on 2015-03-10 13:18:53 CET