Sorry Julian, I'm late with this 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.
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?
> (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.
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'.
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?
~Neels
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2372335
Received on 2009-07-18 23:20:40 CEST