Re: svn commit: rev 2216 - trunk/subversion/include trunk/subversion/libsvn_client trunk/subversion/clients/cmdline trunk/subversion/tests/clients/cmdline/getopt_tests_data

On Fri, Jun 14, 2002 at 01:43:26PM -0500, rooneg@tigris.org wrote:
>...
> +++ trunk/subversion/libsvn_client/export.c Fri Jun 14 13:43:13 2002
>...
> +static svn_error_t *
> +remove_admin_dirs (const char *dir, apr_pool_t *pool)
> +{
> + apr_pool_t *subpool = svn_pool_create (pool);
> + apr_hash_t *dirents = apr_hash_make (subpool);

No need to do this. svn_io_get_dirents() creates and returns one to you.

> + const enum svn_node_kind *type;
> + apr_hash_index_t *hi;
> + const char *item;
> + void *key, *val;

We normally move variables to the innermost block possible. Thus: key, val,
item, and type would all move inside the hash iteration loop.

(and const for the key, as Philip points out)

>...
> + if (*type == svn_node_dir)
> + {
> + char *dir_path = svn_path_join (dir, key, subpool);

'const char *'. By leaving off the const, you imply that you might be
changing it.

> + if (strlen (item) == sizeof (SVN_WC_ADM_DIR_NAME) - 1
> + && strcmp (item, SVN_WC_ADM_DIR_NAME) == 0)

This is rather excessive. You're looping over the string twice. Just do the
strcmp().

>...
> +copy_versioned_files (const char *from,
> + const char *to,
> + apr_pool_t *pool)
> +{
> + apr_pool_t *subpool = svn_pool_create (pool);
> + apr_hash_t *dirents = apr_hash_make (subpool);

No need to make this.

> + const enum svn_node_kind *type;
> + svn_wc_entry_t *entry;
> + apr_hash_index_t *hi;
> + apr_status_t apr_err;
> + svn_error_t *err;
> + const char *item;
> + void *key, *val;

Moving variables again...

>...
> + if (*type == svn_node_dir)
> + {
> + if (strncmp (item, SVN_WC_ADM_DIR_NAME,
> + sizeof (SVN_WC_ADM_DIR_NAME) - 1) == 0)

That check is wrong. It will match '.svnfoo'. Just use strcmp().

>...
> + {
> + char *new_from = svn_path_join (from, key, subpool);
> + char *new_to = svn_path_join (to, key, subpool);

'const char *'

>...
> + else if (*type == svn_node_file)
> + {
> + char *copy_from = svn_path_join (from, item, subpool);
> + char *copy_to = svn_path_join (to, item, subpool);

ditto.

> + entry = NULL;
> +
> + err = svn_wc_entry (&entry, copy_from, FALSE, subpool);

You shouldn't need to set entry to NULL. svn_wc_entry() will do it.

>...
> +
> + svn_pool_destroy (subpool);

This subpool covers the whole function. I think you may want to just use the
caller's pool for the initial entry and hash table, then use the subpool for
the iteration. A directory with 10k files in it will cause problems with the
"whole function" subpool.

And in general, a subpool that just wraps what a function does is suspect.
Subpools are important for loops. There are *some* specific functions where
we know that its memory is so huge that we want to create/destroy a pool for
just that function, but it is rare.

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