On 29.07.2014 12:27, Julian Foad wrote:
> I just looked at svn_client_mtcc.h, scheduled for 1.9 release
>
> I noticed it's basically an svn_delta_editor commit editor at the libsvn_client level, without the 'open' and 'close' methods.
>
> The header file doesn't say anything about the semantics and rules for the provided functions. I'm guessing it's basically the same as svn_delta_editor. For example:
>
> * You are making incremental edits to a 'current state'. The order of operations therefore matters.
>
> * Modify, delete, move(source) requires an existing node at the given path in the current state. Add_file, mkdir, copy, move(dest) requires there is no node at the given path in the current state.(To replace a node you must first delete it, then add at the same path.)
>
> * A sequence of changes need not be minimal. For example, after adding something you may delete it, and then that path is again free to have something added there.
>
> * 'Delete' is recursive.
>
> * After 'add_file' you MAY but SHOULD NOT (?) then use 'update_file' on it.
>
> * ... and so on.
>
> These rules are intuitive to someone familiar with svn_delta_editor_t and expecting it to work like that, but otherwise are not intuitive. We need to document them.
>
> It's not clear whether the copy source relpath is relative to the edit anchor URL of the edit or to the repository root. Does this mean you need authorization to open a commit session to the root directory of all paths, including copy source paths? If you want to make a tag of /trunk, do you need write access to the repo root?
>
> The 'move' API performs a copy and a delete. However,looking at the implementation, for the copy part the specified source path is taken to be a path in an existing revision, whereas for the delete part the same specified source path is taken to be a path in the current state of the edit. Thus, this won't work when trying to move a moved thing. It's broken: it only works for trivial changes.
>
> All of this and more is the easily overlooked complexity in an apparently trivial-looking new API.
>
> The advantages of using a standard interface to accomplish a task are well known. If this API is basically a simplified commit editor interface, we should embrace that. Change it to a standard editor interface, or define a new simpler interface and make this one *instance* of that interface.
>
> In what main ways is this simpler than svn_delta_editor?
>
> * Assumes a single-revision base state.
>
> * Relaxed tree traversal order: no explicit 'open' and 'close'.
>
> * Text delta sending is built in.
>
> OK, those are all convenient simplifications. (There are other smaller differences as well.)
>
> If we are going to define Yet Another Commit Editor API, let's do it right.
>
> Thoughts? (I'll gather my own thoughts later, but I would start with not releasing this as public in its current state.)
+1 to making the svn_mtcc API private in 1.9. I've made similar
objections before. It's currently used by svnmucc and a couple C test
cases, and I don't see any reason to make it public. "Convenience to
users" does not outweigh good API design, IMNSHO.
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco | Realising the impossibilities of Big Data
e. brane_at_wandisco.com
Received on 2014-07-29 12:45:30 CEST