[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: CVS update: subversion/subversion/tests/libsvn_repos repos-test.c

From: Greg Stein <gstein_at_lyra.org>
Date: 2001-04-25 01:28:36 CEST

On Mon, Apr 23, 2001 at 02:43:32PM -0000, cmpilato@tigris.org wrote:
>...
> + The caller must call editor->close_edit on EDIT_BATON;
> + svn_fs_dir_delta does not close the edit itself.
> +
> + Do any allocation necessary for the delta computation in POOL.
> + This function's maximum memory consumption is at most roughly
> + proportional to the greatest depth of SOURCE_PATH under
> + TARGET_ROOT, not the total size of the delta. */
> +svn_error_t *
> +svn_repos_update (svn_fs_root_t *target_root,
> + svn_fs_root_t *source_root,
> + svn_string_t *parent_dir,
> + svn_string_t *entry,
> + apr_hash_t *source_rev_diffs,
> + const svn_delta_edit_fns_t *editor,
> + void *edit_baton,
> + apr_pool_t *pool);

I don't understand the rationale for this function. Why do we have it, and
what is the key difference from svn_repos_dir_delta? Can that be clarified
in the documentation? They both seem to do the same thing, but with slightly
different arguments.

And *WHAT* is the reason for neither of them calling close_edit? Both of
them call set_target_revision and replace_root, so why not close_edit? It is
a pain in the butt for users of these functions to never touch the editor
*except* to have to go and clean up after the function, using a close_edit.
Why?

>...
> --- merge.c 2001/04/18 14:03:45 1.6
> +++ merge.c 2001/04/23 14:43:31 1.7
> @@ -402,9 +402,12 @@
> mrc.root = committed_root;
> mrc.repos = repos;
>
> - serr = svn_repos_dir_delta(previous_root, "/", revs,
> - committed_root, "/",
> + serr = svn_repos_dir_delta(previous_root,
> + svn_string_create ("/", pool), revs,
> + committed_root,
> + svn_string_create ("/", pool),
> editor, &mrc, pool);

I created a utility variable to hold a single "/" string here, rather than
needing to create two. Also, careful on the style (spaces).

Further, this whole thing for creating an svn_string_t for a *CONSTANT* is
*really* started to peeve me. The darned function isn't going to do any
modification of the string, so why use an svn_string_t?

I really think we need to reconsider our use of svn_string_t everywhere. It
is kind of silly to use it for constants. In many cases, we're just wrapping
these darn constants in the svn_string_t header plus copying the value
again. That is a lot less efficient than the occasional strlen().

The number of places where I need to do svn_string_create because of some
dumb API, when I *know* it doesn't need the svn_string_t memory buffering
gunk is growing, and is becoming somewhat silly.

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/
Received on Sat Oct 21 14:36:29 2006

This is an archived mail posted to the Subversion Dev mailing list.