On Thu, Nov 3, 2011 at 14:10, Julian Foad <julian.foad_at_wandisco.com> wrote:
>...
> @@ -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.
That would be a bug in the add, which is not checking. I see no reason
to work around a bug. If the caller wants to abort the editor, then it
should be able to do so, regardless of the cancellation setting.
>...
> @@ -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.
Huh? The statement already says the children must follow. I don't
understand this seeming redundancy.
>...
> * 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
Sure, that makes sense. Then the set_props() can lose the COMPLETE
argument, and we add the restriction that set_text() and set_target()
MUST NOT be called before/after the set_props() call.
We could also state that props==NULL means "no change". "No
properties" (the empty set) would be designated by an empty hash.
> + * 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
Keep it set target. You are not setting the symlink, you are setting
the *target* of the symlink. This is the nomenclature used everywhere
already.
> + * introduce a 'set directory' function, and remove the set_props()
> + * function. This would eliminate the exception in the "Once Rule".
Sure.
>...
> * - 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?
Any ancestor, not just the parent.
> + *
> + * ### JAF: The ancestor of a node that is modified or added or copied-here
> + * or moved-here ...?
Sure.
> + *
> + * ### JAF: The ancestor of ... [can | may not?] be copied or moved.
The statement says the ancestor may not be moved. Since it says
nothing about copying: sure, that is allowed (as you'd expect, so we
don't have to call it out).
> * - 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_*().
You cannot call set_*() on a missing node, so there is no reason to state this.
> + * ### JAF: Nor followed by copy() or move() with the same path as the
> + * copy/move source?
You cannot copy/move a missing node.
The point of the above statement is to enforce that replacement is not
allowed using a delete, followed by something that (re)constructs a
node at that path. You don't need to (re)define the fact that set,
copy, move must have a valid 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.
Makes sense. We can state that SVN_INVALID_REVNUM means "uncommitted"
(rather than "wildcard").
> *
> - * - 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
MUST NOT. Why the change?
>...
> * - 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 ..."?
I don't see the analogy.
>...
> @@ -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.
*shrug*
> *
> *
> * @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.
Yup.
> + *
> + * -- 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).
Right. svn_editor changes a tree from one state to another. "Merge"
doesn't come into play whatsoever. "Make the tree look like <X>".
Effectively, the driver computes the merge result, and then applies
that to the target tree via svn_editor.
> + *
> + * ### 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.
Huh? We've never talking about adding more checksums. I don't think
this is the place to start.
>...
> 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_*(). */
*shrug*
>...
> + * For descriptions of @a props and @a replaces_rev, see
> + * svn_editor_add_file().
Sure.
>...
> @@ -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'?
Not sure. My initial assumption was {dir, file, symlink}.
Do we need to be able to set server-excluded, and excluded? I believe
these latter two concepts are derived from the notion of "this $kind
node should be marked absent", and it just depends on who receives
such a notice. ie. the update receiver takes an absent marker as
"server-excluded".
>...
> @@ -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().
Sure.
>...
> @@ -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.
The CONTENTS param is always the new full text. Always.
If the receiver is an RA committing editor, then it MAY choose to
generate a diff to send to the server. But that is private to the
receiver. If the driver is an RA update driver, then it MAY need to
apply a diff received from the server against the (known) base text
producing the new full text.
No other formats should be used within the CONTENTS stream (e.g never
an svndiff).
> + * ### JAF: This may be inefficient for an implementation that already
> + * has a diff available but doesn't have a full text available
Don't worry about this. In all cases that I identified, the base text
was available to apply a patch against to derive the desired new full
text.
If you can identify a counter-example, then we can discuss. But
otherwise, this isn't an issue.
>...
> + * ### 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.
Somewhere in the doc, it should say that the receiver MAY close the
stream before reading anything from it. It MAY choose to close the
stream before reading all text from it, but that is unlikely. The
typical scenario is that the receiver chooses to not use the contents,
or that it will use the entire contents.
Because receivers may close the stream before reading anything,
drivers may choose to implement a "lazy-open" stream. ie. only on the
first read, would the driver make the necessary calls to wc_db to get
the pristine stream.
Note: this lazy-open concept is in the Ev2 notes, but it looks like it
didn't make it to svn_editor.h.
> + *
> + * ### 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.
This is basically above. The editor framework will never touch the
stream, so we're only talking about the receiver's callback here.
> + * ### 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.
It is permissible, but stupid. I would be fine with saying "not
allowed". Or at least making a statement like "set_text() SHOULD make
changes to the contents."
>...
Cheers,
-g
Received on 2011-11-03 21:05:35 CET