On Tue, Mar 10, 2015 at 1:18 PM, 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?
>
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 <brane_at_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 <ivan_at_visualsvn.com> wrote:
> 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.
>
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.
Received on 2015-03-16 13:44:56 CET