On Tue, Jul 29, 2014 at 5:45 AM, Branko Čibej <br...@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
