Re: svn commit: rev 2074 - trunk/subversion/include trunk/subversion/libsvn_wc

On Mon, Jun 03, 2002 at 05:41:01PM -0500, kfogel@tigris.org wrote:
>...
> Note that this code is not useful until the editor->close_edit() calls
> have been moved out of the RA layer and into libsvn_client. If anyone
> has feedback on that, please comment now. My overall plan is to

I don't agree with that change. The RA layer calls replace_root() to start a
change, and should call close_edit() to single its termination. The fact
that something "returns from a func call" should not be the appropriate
control point.

If the WC and/or client wants to do more work, then empty out your
close_edit() function and move the work outside of the editor (to be
performed when RA->do_FOO returns). But do not alter the *usage* of the
editor to fit your scenario.

>...
> +++ trunk/subversion/include/svn_wc.h Mon Jun 3 17:40:57 2002
> @@ -752,6 +752,39 @@
> apr_pool_t *pool);
>
>
> +/* Set *EXTERNALS_NEW and *EXTERNALS_OLD to hash tables representing
> + * changes to values of the svn:externals property on directories
> + * traversed by EDIT_BATON.
> + *
> + * EDIT_BATON must be obtained from svn_wc_get_update_editor(),
> + * svn_wc_get_checkout_editor(), or svn_wc_get_switch_editor().
> + *
> + * Each hash maps `const char *' directory names onto `const char *'
> + * values of the externals property for that directory. The dir names

How do you plan to represent revisions? It would seem that you would want to
map directory names onto (URL, revision) pairs.

Oh: also, I seem to recall a checkin somewhere saying that the dir names in
the svn:externals property are *single* components. IMO, that is wrong. They
should be relative paths. This allows you to do something like:

  svn http://svn.collab.net/...
  svn/neon http://svn.webdav.org/...
  svn/apr http://svn.apache.org/...
  svn/apr-util http://svn.apache.org/...

>...
> + * The hashes, keys, and values have the same lifetime as EDIT_BATON.
> + *
> + * ### Ben and I were talking about whether it's tasteful to have a
> + * public function that acts on the editor baton like this. I think

I don't think that you should be structuring the code such that the edit
baton is still alive (e.g. close_edit has not been called). Thus, I think
you're going to be using an entirely different structure here than an
edit_baton (since libsvn_client will be using it, and it does not have
access to an open edit_baton since RA has closed it).

I suspect that you're going to have some kind of update_baton structure that
will hold your state, and this externals function will operate on that.
You'll fetch the update_baton at the same time that you fetch the editor and
edit_baton. What is currently in the edit baton will move over (because you
won't want to close it up during the close_edit).

And yes, this is more work, but telling the RA layer that it cannot close an
edit that it *began* is just wrong (in terms of the "spirit" of the editor
API).

>...
> + {
> + const char *new_value = change->value->data;
> + const char *old_value;
> + const svn_string_t *old_value_s;
> +
> + SVN_ERR (svn_wc_prop_get
> + (&old_value_s, SVN_PROP_EXTERNALS,
> + db->path, db->pool));
> +
> + old_value = old_value_s->data;
> +
> + if ((new_value == NULL) && (old_value == NULL))
> + ; /* No value before, no value after... so do nothing. */
> + else if (new_value && old_value
> + && (strcmp (old_value, new_value) == 0))

Micro-optimization nit: you've got the length of these two values. Comparing
their length first, and (if they're equal) using memcmp() will be faster
than a strcmp().

>...
> + {
> + struct edit_baton *eb = db->edit_baton;
> +
> + if (old_value)
> + apr_hash_set (eb->externals_old,
> + apr_pstrdup (eb->pool, db->path),
> + APR_HASH_KEY_STRING,
> + apr_pstrdup (eb->pool, old_value));

You've got the length of old_value, so using:

  apr_pstrmemdup(eb->pool, old_value, old_value_s->len)

will be faster.

> + if (new_value)
> + apr_hash_set (eb->externals_new,
> + apr_pstrdup (eb->pool, db->path),
> + APR_HASH_KEY_STRING,
> + apr_pstrdup (eb->pool, new_value));

Same, but with value->data->len.

>...
> +void
> +svn_wc_edited_externals (apr_hash_t **externals_new,
> + apr_hash_t **externals_old,
> + void *edit_baton)
> +{
> + struct edit_baton *eb = edit_baton;
> +
> + *externals_new = eb->externals_new;
> + *externals_old = eb->externals_old;
> +}

Bleck. I think what you'll be doing is something like:

struct edit_baton {
  ...
  struct svn_wc_update_baton *ub;
  ...
};

And just referring to eb->ub->externals_new and _old. After RA returns,
libsvn_client will call svn_wc_edited_externals(&n, &o, ub). Do its work.
Then call the (new) function svn_wc_finalize_update(ub).

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org