Re: Deprecating svn_fs_id_t

On 22.12.2013 13:56, Stefan Fuhrmann wrote:
> For some time now, this thing has been a nuisance to me
> in various situations. Recent discussions prompted me
> to have a closer look at this and now I want to see it
> removed from the API (maintaining it in the deprecated
> section). Here is why; further down is how.

First of all, we've been discussing at length using the FS IDs
(effectively, svn_fs_id_t) in mergeinfo, in order to avoid the mess that
arises from trying to track renames based on the mergeinfo source path.
If you throw them out, we'll just have to re-introduce them again.

> Why
> ---
>
> svn_fs_id_t is ill-conceived and unnecessary concept with
> an under-developed API:
>
> * Ill-conceived - as this is not used as an ID / key to
> anything in the rest of the API. The API looks like
>
> get_infoA(root, path)
> get_infoB(root, path)
>
> instead of
>
> get_ID(root, path)
> get_infoA(id)
> get_infoB(id)
>
> So, we rather exposed an implementation detail and had
> to deprecate svn_fs_parse_id() early on to prevent the
> most severe consequences.

That's not an argument for deprecating FS IDs; it's an argument for
deprecating the whole FS tree API.

> * Unnecessary - as it gets in the way instead of actually
> contributing to an operation. We use svn_fs_id_t to test
> for noderev identity and relationship but all code uses
> the following pattern:
>
> id_a = get_ID(rootA, pathA)
> id_b = get_ID(rootB, pathB)
> compare(id_a, id_b)
>
> Shorter and in keeping with the rest of the API, we should
> simply do
>
> compare(rootA, pathA, rootB, pathB)

Again, as above; this is an argument for adding and using "compare"
function variant. Or just designing a completely new API.

> There are two more uses. One for "debugging" (svnlook)
> which could use a similar private API call. The other
> one is as a basis for an optimization and there is an
> alternative implementation for that.
>
> * Under-developed - as it does not provide the API that
> it could.

Still not an argument for derpecation; it's an argument for writing new
APIs, if/when it turns out they're needed.

> For instance, it might return a root object
> (telling us the txn / rev that the noderev was created in).
> There is also no check on the copy id sub-struct that
> might answer a question like: refer these noderevs to
> the same node with no copy operation in between?
>
> id_a = get_ID(rootA, path)
> id_b = get_ID(rootB, path)
> if (same_node(id_a, id_b) && same_copy(id_a, id_b))
> doIt()
>
> Note that the IDs in this scenario are still derived
> from Subversion's versioned file system semantics and
> independent from any concrete implementation.

>
>
> How / what
> -----------
>
> * Introduce a root+path based svn_fs_compare

Makes sense ...

> and deprecate
> svn_fs_compare_ids as well as svn_fs_check_related.

... but this doesn't. See above about mergeinfo.

> * Deprecate svn_fs_dirent_t and replace it with svn_fs_dirent2_t
> containing no ID member. Bump svn_fs_dir_entries API as well.

I suspect you want this in order to reduce chache footprint. Maybe we
just shouldn't cache whole svn_fs_dirent_t structs?

> * Similarly bump svn_fs_path_change2_t plus related API.
>
> * Deprecate svn_fs_unparse_id and svn_fs_node_id. Introduce
> svn_fs__unparse_id and svn_fs__node_id as private API for
> debugging and tools like svnlook. Also, use these to
> implement the deprecated directory and changed path list APIs.

Again, see above about mergeinfo. Also, I can't see a good reason for
deprecating this at all. You're not required to use any of these
functions. You don't deprecate a public API just because you don't like it.

-- Brane

-- 
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane_at_wandisco.com