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.f...@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/
