On Tue, Jul 29, 2014 at 5:45 AM, Branko Čibej <brane_at_wandisco.com> wrote:
> 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.
>
>
Similarly, I raised an issue about the API, and its maturity (back in
January). I believe it would be prudent to keep it private for 1.9.
Cheers,
-g
Received on 2014-07-29 13:13:32 CEST