Neels J Hofmeyr wrote:
> Sorry Julian, I'm late with this reply...
No problem; it's good to hear your reply.
Julian Foad wrote:
> > Neels J Hofmeyr wrote:
> >> Julian Foad wrote:
> >>> On Thu, 2009-07-09 at 15:53 -0700, Neels Janosch Hofmeyr wrote:
> >>>> Enable an atomic replace in editor v2.
> >>>>
> >>>> * subversion/include/svn_editor.h
> >>>> (svn_editor_add_directory_t, svn_editor_add_directory,
> >>>> svn_editor_add_file_t, svn_editor_add_file,
> >>>> svn_editor_add_symlink_t, svn_editor_add_symlink,
> >>>> svn_editor_add_absent_t, svn_editor_add_absent):
> >>>> Add parameter REPLACES_REV.
> >>> Where is this parameter documented?
> >
> > [...]
> >> It felt kind of silly to only document that parameter in the midst of void.
> >> And I am too lazy right now to document all of them.
> >
> > But it's not silly. I can't review the change without knowing what it
> > means. I'm sure it seems totally obvious to you, but I have questions
> > about it.
>
> Yes I fully understand that. We could, too, ask gstein to add proper
> documentation for the rest of the parameters for the same reasons. I had the
> same kind of "hm" moment when I saw the docs missing pretty much completely.
We should document all of the parameters, yes. The reason I picked up on
this one in particular is that the others are (I think) pretty much
copied from the old equivalent API, so their semantics are (I hope) well
established and documented and we can transfer the documentation
relatively easily; but the new parameter doesn't have this route
available.
> My intention is to follow up on this once the time has come. I haven't added
> the parameter just so, I do have a specific application of it in mind (in
> the diff callbacks), so probably I won't forget about it.
>
> >
> >> However, I added a rough description in notes/editor-v2.txt in r38396.
> >
> > [[[
> > add_directory(path, children, props, replaces_rev)
> > -- name all children. receiver can put on "pending" list
> > -- MUST be followed (at some point) by add_*() calls for each child
> > -- if replaces_rev isn't SVN_INVALID_REVNUM, this is an atomic
> > replace.
> > Use a revnum because it implies all other info like node kind etc.
> > ]]]
> >
> > That doesn't actually say what the parameter means. Is it something like
> > this:
>
> Yeah, in fact, I wrote that stuff because it answers a question stsp had.
>
> > [[[
> > If replaces_rev is not SVN_INVALID_REVNUM:
> > (a) This 'add' atomically replaces the node (of kind 'file' or
> > 'directory') that would have existed (if this call and any associated
> > 'delete' call were ignored) at 'path' in the target revision of this
> > editor drive.
>
> There are different callbacks for file, dir, symlink and absent ... it
> doesn't make sense to speak of "'file' or 'directory'", right?
OK. What I really meant when I wrote "'file' or 'directory'" was "a node
that exists".
> > (b) The replaced node-rev must have unbroken history back to revision
> > 'replaces_rev', with no modifications of any kind.
>
> I think it doesn't *have to* must. It depends on the application of the
> editor: A diff may handle it one way, a merge another. Also, passing the
> revision is not because the revision was that important for a replace. The
> really important info is whether the replaced node is a directory, file, or
> whatever. The revision number just is the minimal bit of information that
> enables the receiver to find out *anything* it wants to know about the
> replaced node.
>
> > (c) A separate 'delete' of the node that previously existed should not
> > be issued by the driver, but must be accepted by the receiver (before or
> > after this call) for backward compatibility.
>
> I think it can't be after. The current code works around the non-atomic
> replace by caching the deletes and looking up each add. It wouldn't work if
> the order was reversed, AFAIR.
>
> > ]]]
> >
> > ?
>
> My version:
> [[[
> During a simple 'add', REPLACES_REV is SVN_INVALID_REVNUM.
Great. Instead of "a simple 'add'" we could spell out "adding a node
where there was none before this edit, or where a 'delete' has been
issued earlier in this edit".
> If REPLACES_REV is *not* SVN_INVALID_REVNUM, this is an atomic replace: The
> driver of the callback intends this add to atomically replace the node of
> the same name at revision REPLACES_REV. The receiver of the callback may
> trigger a tree-conflict if the receiving side has a different node kind,
> revision, etc., for a node with the same name (e.g. during merge), but that
> depends on what makes sense in the specific application.
>
> The REPLACES_REV argument is an svn_revnum_t, because by the node's path and
> its revision, the receiving side can find out *anything* it wants to know
> about the replaced node; first and foremost, its node kind.
>
> The driver is required to use add() with REPLACES_REV instead of a separate
> delete() call before this add(). Receivers of this callback in general are
> not required to support a separate delete() call for a 'replace'.
I think it would be clearer to state the editor requirements separately
from the usage recommendations. (This last paragraph and the following
one seem to mix the two.)
> If you intend to implement a separate delete() call when driving this
> editor, make sure you really have to and put up "big red signs" in the
> documentation of your driver. Be aware that, in general, the driver should
> do all of the (possibly hard) work of combining a delete() with an add() to
> form an atomic replace, taking that burden off the various receivers. So if
> you write a driver that doesn't do that, you have to make sure that all your
> receivers (preferably very few) can handle a non-atomic replace.
> Furthermore, a delete() call should occur always before, never after an
> add() call for the same node.
> ]]]
>
> What do you tink about this text?
Much better!
Thanks.
- Julian
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2372447
Received on 2009-07-19 19:54:07 CEST