Re: Editor v2 - suggestions and queries

Only a couple of nit-pick comments.

Generally, since there is already discussion in the file itself, I'd
support committing additional discussion directly, and then discussing it
on the mailing list if needed.

-Hyrum

On Thu, Nov 3, 2011 at 1:10 PM, Julian Foad <julian.foad_at_wandisco.com>wrote:

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

These should be left as-is, per RFC 2119.

> * 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
>

Use the more specific "directory" rather than "node". (This is a doctoring
which only applies to directories.)

> + * 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.
>

Please, please, please, please, no secret backdoors! Let's get this
implementation figured out, and then worry about potential optimizations.

> + *
> + * ### 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.
>

No-op text changes are permissible.

> + *
> * ### 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
>

-- 
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com/