The new svn_client_mtcc API

[Julian Foad <ju...@btopenworld.com>]

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.)

- Julian

Re: The new svn_client_mtcc API

[Greg Stein <gs...@gmail.com>]

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

Re: The new svn_client_mtcc API

[Branko Čibej <br...@wandisco.com>]

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

Re: The new svn_client_mtcc API

[Julian Foad <ju...@btopenworld.com>]

> On 29.07.2014 19:32, Julian Foad wrote:
>> I'll move the header file into the 'private' subdirectory, in preparation for the
>> 1.9 release, as there is consensus that this is not ready for prime time. Then we
>> can still use it and can work on improving it with no time restriction.

Done in r1614599.

Branko Čibej wrote:
> Don't forget to add the extra _ to the symbols defined in that header.

I hadn't forgotten; rather, I thought it wasn't a requirement since we didn't do it for some other private headers. But it seems that was just svn_editor_t (Ev2) and was an anomaly. Anyway, done.

- Julian

Re: The new svn_client_mtcc API

[Branko Čibej <br...@wandisco.com>]

On 29.07.2014 19:32, Julian Foad wrote:
> [Brane and Greg said +1.]
>
> Bert, I'm CC'ing to you as you added the code.
>
> I'll move the header file into the 'private' subdirectory, in preparation for the 1.9 release, as there is consensus that this is not ready for prime time. Then we can still use it and can work on improving it with no time restriction.

Don't forget to add the extra _ to the symbols defined in that header.

-- Brane


-- 
Branko Čibej | Director of Subversion
WANdisco | Realising the impossibilities of Big Data
e. brane@wandisco.com

Re: The new svn_client_mtcc API

[Julian Foad <ju...@btopenworld.com>]

[Brane and Greg said +1.]

Bert, I'm CC'ing to you as you added the code.

I'll move the header file into the 'private' subdirectory, in preparation for the 1.9 release, as there is consensus that this is not ready for prime time. Then we can still use it and can work on improving it with no time restriction.

- Julian


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.)