Here, in the form of a patch, are many suggestions and queries I've
made on Ev2. I'm not intimately familiar with the design and goals of
it, I'm just responding to how it's currently written up in the header
file, with some memory of past discussions. Discussion welcomed.
[[[
Index: subversion/include/svn_editor.h
===================================================================
--- subversion/include/svn_editor.h (revision 1197094)
+++ subversion/include/svn_editor.h (working copy)
@@ -177,18 +177,25 @@
* \n\n
* Just before each callback invocation is carried out, the @a cancel_func
* that was passed to svn_editor_create() is invoked to poll any
* external reasons to cancel the sequence of operations. Unless it
* overrides the cancellation (denoted by #SVN_ERR_CANCELLED), the driver
* aborts the transmission by invoking the svn_editor_abort() callback.
* Exceptions to this are calls to svn_editor_complete() and
* svn_editor_abort(), which cannot be canceled externally.
*
+ * ### JAF: It should check for cancellation before 'complete'.
+ * Imagine the user runs 'svn commit large-new-file' and then
+ * tries to cancel it, and imagine svn_editor_add() does not
+ * internally respond to cancellation. If the driver then calls
+ * 'complete' without intercepting the cancellation, the user
+ * would not be happy to see that commit being completed.
+ *
* - @b Receive: While the driver invokes operations upon the editor, the
* receiver finds its callback functions called with the information
* to operate on its tree. Each actual callback function receives those
* arguments that the driver passed to the "driving" functions, plus these:
* - @a baton: This is the @a editor_baton pointer originally passed to
* svn_editor_create(). It may be freely used by the callback
* implementation to store information across all callbacks.
* - @a scratch_pool: This temporary pool is cleared directly after
* each callback returns. See "Pool Usage".
@@ -203,76 +210,111 @@
*
* <h3>Driving Order Restrictions</h3>
* In order to reduce complexity of callback receivers, the editor callbacks
* must be driven in adherence to these rules:
*
* - svn_editor_add_directory() -- Another svn_editor_add_*() call must
* follow for each child mentioned in the @a children argument of any
* svn_editor_add_directory() call.
*
+ * ### JAF: ... must follow at any time after the parent add_directory(),
+ * and not before it.
+ *
* - svn_editor_set_props()
* - The @a complete argument must be TRUE if no more calls will follow on
* the same path. @a complete must always be TRUE for directories.
* - If @a complete is FALSE, and:
* - if @a relpath is a file, this must (at some point) be followed by
* an svn_editor_set_text() call on the same path.
* - if @a relpath is a symlink, this must (at some point) be followed by
* an svn_editor_set_target() call on the same path.
*
* - svn_editor_set_text() and svn_editor_set_target() must always occur
* @b after an svn_editor_set_props() call on the same path, if any.
*
* In other words, if there are two calls coming in on the same path, the
* first of them has to be svn_editor_set_props().
*
+ * ### JAF: The set_* functions treat the properties of a node as
+ * separate from the node's text or symlink target, whereas the
+ * add_* functions treat the properties as an integral part of each
+ * kind of node. This seems like a needless difference. It would make
+ * sense for set_text() to take the properties with the text, and let
+ * the implementation worry about whether the props and/or the text
+ * have actually changed (as well as how to transmit the changes
+ * most efficiently). For symlinks, include the properties with the
+ * 'set target' call (rename to 'set symlink') and for directories
+ * introduce a 'set directory' function, and remove the set_props()
+ * function. This would eliminate the exception in the "Once Rule".
+ *
* - Other than the above two pairs of linked operations, a path should
* never be referenced more than once by the add_* and set_* and the
* delete operations (the "Once Rule"). The source path of a copy (and
* its children, if a directory) may be copied many times, and are
* otherwise subject to the Once Rule. The destination path of a copy
* or move may have set_* operations applied, but not add_* or delete.
* If the destination path of a copy or move is a directory, then its
* children are subject to the Once Rule. The source path of a move
* (and its child paths) may be referenced in add_*, or as the
* destination of a copy (where these new, copied nodes are subject to
* the Once Rule).
*
* - The ancestor of an added or modified node may not be deleted. The
* ancestor may not be moved (instead: perform the move, *then* the edits).
+ *
+ * ### JAF: Ancestor == parent dir?
+ *
+ * ### JAF: The ancestor of a node that is modified or added or copied-here
+ * or moved-here ...?
+ *
+ * ### JAF: The ancestor of ... [can | may not?] be copied or moved.
*
* - svn_editor_delete() must not be used to replace a path -- i.e.
* svn_editor_delete() must not be followed by an svn_editor_add_*() on
* the same path, nor by an svn_editor_copy() or svn_editor_move() with
* the same path as the copy/move target.
+ *
+ * ### JAF: Nor followed by set_*().
+ *
+ * ### JAF: Nor followed by copy() or move() with the same path as the
+ * copy/move source?
*
* Instead of a prior delete call, the add/copy/move callbacks should be
* called with the @a replaces_rev argument set to the revision number of
* the node at this path that is being replaced. Note that the path and
* revision number are the key to finding any other information about the
* replaced node, like node kind, etc.
* @todo say which function(s) to use.
+ *
+ * ### JAF: Can we have a way to refer to replacing a node in a tree that
+ * is not a committed revision and so doesn't have a revision number?
+ * This seems to be the only place where the editor definition requires
+ * that a target node is a (copy of a) committed repository node.
*
- * - svn_editor_delete() must not be used to move a path -- i.e.
- * svn_editor_delete() must not delete the source path of a previous
+ * - svn_editor_delete() should not be used to move a path -- i.e.
+ * svn_editor_delete() should not delete the source path of a previous
* svn_editor_copy() call. Instead, svn_editor_move() must be used.
* Note: if the desired semantics is one (or more) copies, followed
* by a delete... that is fine. It is simply that svn_editor_move()
* should be used to describe a semantic move.
*
* - One of svn_editor_complete() or svn_editor_abort() must be called
* exactly once, which must be the final call the driver invokes.
* Invoking svn_editor_complete() must imply that the set of changes has
* been transmitted completely and without errors, and invoking
* svn_editor_abort() must imply that the transformation was not completed
* successfully.
*
* - If any callback invocation returns with an error, the driver must
* invoke svn_editor_abort() and stop transmitting operations.
+ *
+ * - ### JAF: Some restriction analogous to the receiver's "All callbacks
+ * must complete their handling of a path before they return ..."?
* \n\n
*
* <h3>Receiving Restrictions</h3>
* All callbacks must complete their handling of a path before they
* return, except for the following pairs, where a change must be completed
* when receiving the second callback in each pair:
* - svn_editor_set_props() (if @a complete is FALSE) and
* svn_editor_set_text() (if the node is a file)
* - svn_editor_set_props() (if @a complete is FALSE) and
@@ -328,25 +370,45 @@
* calling any of the driving functions (e.g. svn_editor_add_directory()).
* As with any other error, the driver must then invoke svn_editor_abort()
* and abort the transformation sequence. See #svn_cancel_func_t.
*
* The @a cancel_baton argument to svn_editor_create() is passed
* unchanged to each poll of @a cancel_func.
*
* The cancellation function and baton are typically provided by the client
* context.
+ *
+ * ### JAF: Bring the section on cancellation under the Life-Cycle heading
+ * down to here and combine it.
*
*
* @todo ### TODO anything missing? -- allow text and prop change to follow
- * a move or copy. -- set_text() vs. apply_text_delta()? -- If a
+ * a move or copy. -- set_text() vs. apply_text_delta()?
+ *
+ * ### JAF: Those two seem to be done already.
+ *
+ * -- If a
* set_props/set_text/set_target/copy/move/delete in a merge source is
* applied to a different branch, which side will REVISION arguments reflect
* and is there still a problem?
+ *
+ * ### JAF: The 'revision' argument will always refer to the 'before'
+ * state of the source node that the editor is editing. The receiver
+ * must know about this source node to make sense of the edit. If
+ * the receiver is merging the source-node edits into some other
+ * node on a different target branch, the editor knows nothing about
+ * that other node (which may not even have a revision number if
+ * it's a working version that's been modified already by some
+ * previous merge or user intervention).
+ *
+ * ### JAF: As well as the text checksum for files, consider adding an
+ * (optional?) checksum for the full content of every node kind --
+ * text with props, symlink with props, dir with list of children.
*
* @since New in 1.8.
*/
typedef struct svn_editor_t svn_editor_t;
/** These function types define the callback functions a tree delta consumer
* implements.
*
@@ -698,57 +760,51 @@
*
* Create a new directory at @a relpath. The immediate parent of @a relpath
* is expected to exist.
*
* Set the properties of the new directory to @a props, which is an
* apr_hash_t holding key-value pairs. Each key is a const char* of a
* property name, each value is a const svn_string_t*. If no properties are
* being set on the new directory, @a props must be NULL.
*
- * If this add is expected to replace a previously existing file or
- * directory at @a relpath, the revision number of the node to be replaced
+ * If this add is expected to replace a previously existing node
+ * at @a relpath, the revision number of the node to be replaced
* must be given in @a replaces_rev. Otherwise, @a replaces_rev must be
* SVN_INVALID_REVNUM. Note: it is not allowed to call a "delete" followed
* by an "add" on the same path. Instead, an "add" with @a replaces_rev set
* accordingly MUST be used.
*
* A complete listing of the immediate children of @a relpath that will be
* added subsequently is given in @a children. @a children is an array of
* const char*s, each giving the basename of an immediate child.
*
* For all restrictions on driving the editor, see #svn_editor_t.
*/
svn_error_t *
svn_editor_add_directory(svn_editor_t *editor,
const char *relpath,
const apr_array_header_t *children,
apr_hash_t *props,
svn_revnum_t replaces_rev);
+/* ### JAF: Let's keep the 'relpath' and 'replaces_rev' params together,
+ * like they are in the delete/copy/move callbacks, because
+ * logically they're tightly related. Also in other add_*(). */
/** Drive @a editor's #svn_editor_cb_add_file_t callback.
*
* Create a new file at @a relpath. The immediate parent of @a relpath
* is expected to exist.
*
* The file's contents are specified in @a contents which has a checksum
* matching @a checksum.
*
- * Set the properties of the new file to @a props, which is an
- * apr_hash_t holding key-value pairs. Each key is a const char* of a
- * property name, each value is a const svn_string_t*. If no properties are
- * being set on the new file, @a props must be NULL.
- *
- * If this add is expected to replace a previously existing file or
- * directory at @a relpath, the revision number of the node to be replaced
- * must be given in @a replaces_rev. Otherwise, @a replaces_rev must be
- * SVN_INVALID_REVNUM. Note: it is not allowed to call a "delete" followed
- * by an "add" on the same path. Instead, an "add" with @a replaces_rev set
- * accordingly MUST be used.
+ * For descriptions of @a props and @a replaces_rev, see
+ * svn_editor_add_file().
*
* For all restrictions on driving the editor, see #svn_editor_t.
* @since New in 1.8.
*/
svn_error_t *
svn_editor_add_file(svn_editor_t *editor,
const char *relpath,
const svn_checksum_t *checksum,
svn_stream_t *contents,
@@ -772,18 +828,19 @@
const char *target,
apr_hash_t *props,
svn_revnum_t replaces_rev);
/** Drive @a editor's #svn_editor_cb_add_absent_t callback.
*
* Create an "absent" node of kind @a kind at @a relpath. The immediate
* parent of @a relpath is expected to exist.
* ### TODO @todo explain "absent".
+ * ### JAF: What are the allowed values of 'kind'?
*
* For a description of @a replaces_rev, see svn_editor_add_file().
*
* For all restrictions on driving the editor, see #svn_editor_t.
* @since New in 1.8.
*/
svn_error_t *
svn_editor_add_absent(svn_editor_t *editor,
const char *relpath,
@@ -794,18 +851,20 @@
*
* Set or change properties on the existing node at @a relpath. This
* function sends *all* properties, both existing and changes.
* ### TODO @todo What is REVISION for?
* ### HKW: This is puzzling to me as well...
* ###
* ### what about "entry props"? will these still be handled via
* ### the general prop function?
*
+ * For a description of @a props, see svn_editor_add_file().
+ *
* @a complete must be FALSE if and only if
* - @a relpath is a file and an svn_editor_set_text() call will follow on
* the same path, or
* - @a relpath is a symbolic link and an svn_editor_set_target() call will
* follow on the same path.
*
* For all restrictions on driving the editor, see #svn_editor_t.
* @since New in 1.8.
*/
@@ -815,18 +874,47 @@
svn_revnum_t revision,
apr_hash_t *props,
svn_boolean_t complete);
/** Drive @a editor's #svn_editor_cb_set_text_t callback.
*
* Set/change the text content of a file at @a relpath to @a contents
* with checksum @a checksum.
* ### TODO @todo Does this send the *complete* content, always?
+ * ### JAF: I think the idea is that 'contents' is the new full text and,
+ * if wanted, the driver and receiver implementations should diff and
+ * patch (respectively) it against the old full text.
+ *
+ * ### JAF: This may be inefficient for an implementation that already
+ * has a diff available but doesn't have a full text available
+ * (perhaps neither the 'before' nor the 'after' full text). We
+ * should consider providing an alternative driver-side API so
+ * that the driver can choose to bypass this full-text phase and
+ * send the new text directly in any representation agreed with
+ * the receiver, including as a diff against the old. On the
+ * receiver side the same consideration applies.
+ *
+ * ### JAF: For the driver: @a contents is a readable stream. The editor
+ * implementation may pull text from it as required, and will then
+ * close it before returning. The editor [### need not | will]
+ * pull all the text from the stream before closing it.
+ *
+ * ### JAF: For the receiver: @a contents is a readable stream. The
+ * receiver may pull text from it as required, and will then
+ * close it before returning. The receiver [### need not | will]
+ * pull all the text from the stream before closing it.
+ *
+ * ### JAF: Is it permissible for the text change to be a no-op? The
+ * driver may wish to avoid doing so for efficiency if it knows,
+ * but I'd say we should not forbid it. Either way we should
+ * consider how to be consistent in this regard across all the
+ * whole editor.
+ *
* ### TODO @todo What is REVISION for?
*
* For all restrictions on driving the editor, see #svn_editor_t.
* @since New in 1.8.
*/
svn_error_t *
svn_editor_set_text(svn_editor_t *editor,
const char *relpath,
svn_revnum_t revision,
]]]
- Julian
Received on 2011-11-03 19:18:06 CET