On 10 March 2015 at 15:18, Julian Foad <julianfoad_at_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
Received on 2015-03-13 14:47:16 CET