Hyrum K Wright wrote:
> On Tue, Feb 21, 2012 at 5:54 PM, Greg Stein <gstein_at_gmail.com> wrote:
>> On Tue, Feb 21, 2012 at 16:44, <hwright_at_apache.org> wrote:
>>> Ev2 shims: Register the appropriate callbacks for use with the Ev2
>>> shims every time we fetch a commit editor from within the client.
>>
>> I think you've leaked the "hidden shims" out too far with
>> this change.
>> Can't you put it back where you just registered the shims for every RA
>> session. It certainly doesn't hurt to put them into the session, and
>> let them be unused.
>>
>> On the baton, you're passing NULL for all of these, so it would seem
A practical observation first. It appears that the client shim callbacks that you're registering will all crash immediately if this abspath is null, in their current implementations. That would appear to mean that there is currently no *practical* benefit to registering them, at the moment, except in the one place where a real abspath is given.
>> that you could do the same when creating the RA session and installing
>> shims. If you *do* need the baton sometimes, then pass the baton into
>> the client-internal RA session creation.
>>
>> ... something. It just seems that we shouldn't be seeing this
>> registration everywhere.
>
> I agree in principle that keeping the shims and their corresponding
> "magic" as hidden as possible is a good thing.
Conceptually, I'm not sure that this should be hidden. Where the client code gets and uses an editor (v1), it of course does what is necessary to communicate through Ev1 about the current state and the desired changes, which may include setting up all its own callback functions and a baton to access all the required data. Where we're updating the client code to make use of Ev2, even if only through shims, it seems reasonable that the client code should be responsible for passing some sort of handle (callbacks and paths, or whatever) to enable the additional editors to access the parts of the client's state and desired changes that aren't accessible through the Ev1 API.
So I don't see a problem with having some explicit client code at the point where it invokes (even implicitly) the new editor.
Maybe it would make even more sense if the insertion of shims were explicit at that point rather than hidden?
[...]
> To complicate matters further, even if a root abspath is know when the
> ra session is opened--say because the ra session is opened to a
> certain location--it could potentially change by reparenting the
> session or through some other means. [...]
That, to me, just confirms that the information access provided by the callbacks belongs conceptually to the editor and not to the RA session.
BTW on reading through some of the shim code I made a few comments for your consideration, pasted below in diff format. It was quite a shallow reading so don't read too much into it.
- Julian
[[[
Index: subversion/libsvn_delta/compat.c
===================================================================
--- subversion/libsvn_delta/compat.c (revision 1292704)
+++ subversion/libsvn_delta/compat.c (working copy)
@@ -996,14 +996,15 @@ struct operation {
OP_ADD_ABSENT,
OP_PROPSET /* only for files for which no other operation is
occuring; directories are OP_OPEN with non-empty
props */
} operation;
- const char *path;
+ const char *path; /* ### relpath? relative to? */
svn_kind_t kind; /* to copy, mkdir, put or set revprops */
+ /* ### Can you reference concrete terms such as OP_*? */
svn_revnum_t base_revision; /* When committing, the base revision */
svn_revnum_t copyfrom_revision; /* to copy, valid for add and replace */
svn_checksum_t *new_checksum; /* An MD5 hash of the new contents, if any */
const char *copyfrom_url; /* to copy, valid for add and replace */
const char *src_file; /* for put, the source file for contents */
apr_hash_t *children; /* const char *path -> struct operation * */
@@ -1058,12 +1059,15 @@ get_operation(const char *path,
apr_hash_set(operation->children, apr_pstrdup(result_pool, path),
APR_HASH_KEY_STRING, child);
}
/* If an operation has a child, it must of necessity be a directory,
so ensure this fact. */
+ /* ### Shouldn't we be *checking* rather than overwriting? */
+ /* ### What is the KIND for an OP_REPLACE operation that replaces a file
+ * with a dir (or any other differing node kinds)? */
operation->kind = svn_kind_dir;
return child;
}
/* Add PATH to the operations tree rooted at OPERATION, creating any
@@ -1074,24 +1078,28 @@ get_operation(const char *path,
------------ ----- ------- -------- --------
ACTION_MKDIR NULL invalid NULL NULL
ACTION_COPY valid valid NULL NULL
ACTION_PUT NULL invalid valid NULL
ACTION_DELETE NULL invalid NULL NULL
ACTION_PROPSET valid invalid NULL valid
+ ### Not used --^ ^-- Is used... but what for?
Node type information is obtained for any copy source (to determine
whether to create a file or directory) and for any deleted path (to
ensure it exists since svn_delta_editor_t->delete_entry doesn't
- return an error on non-existent nodes). */
+ return an error on non-existent nodes).
+ ### is obtained ... by calling EB->fetch_kind_func.
+ ### The deleted path check doesn't seem to be implemented.
+*/
static svn_error_t *
build(struct editor_baton *eb,
enum action_code_t action,
const char *relpath,
svn_kind_t kind,
- const char *url,
- svn_revnum_t rev,
+ const char *url, /* is copyfrom url? */
+ svn_revnum_t rev, /* is copyfrom rev? */
apr_hash_t *props,
const char *src_file,
svn_checksum_t *checksum,
svn_revnum_t head,
apr_pool_t *scratch_pool)
{
@@ -1120,15 +1128,17 @@ build(struct editor_baton *eb,
{
apr_hash_t *current_props;
apr_array_header_t *propdiffs;
operation->kind = kind;
+ /* Fetch the CURRENT_PROPS and calculate the PROPDIFFS. */
if (operation->operation == OP_REPLACE)
current_props = apr_hash_make(scratch_pool);
else if (operation->copyfrom_url)
+ /* ### passing COPYFROM_URL where the callee expects a relpath */
SVN_ERR(eb->fetch_props_func(¤t_props, eb->fetch_props_baton,
operation->copyfrom_url,
operation->copyfrom_revision,
scratch_pool, scratch_pool));
else
SVN_ERR(eb->fetch_props_func(¤t_props, eb->fetch_props_baton,
@@ -1157,12 +1167,13 @@ build(struct editor_baton *eb,
(operation->operation == OP_REPLACE)))
{
if ((operation->kind == svn_kind_file)
&& (operation->operation == OP_OPEN))
operation->operation = OP_PROPSET;
}
+ /* ### What's this? */
if (!operation->copyfrom_revision)
operation->copyfrom_revision = rev;
return SVN_NO_ERROR;
}
if (action == ACTION_DELETE)
]]]
Received on 2012-02-23 12:36:35 CET