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. br...@wandisco.com
