Re: svn commit: rev 1672 - trunk/subversion/libsvn_ra_dav

On Thu, Apr 11, 2002 at 04:05:35PM -0500, sussman@tigris.org wrote:
>...
> +++ trunk/subversion/libsvn_ra_dav/props.c Thu Apr 11 16:05:34 2002
> @@ -458,6 +458,7 @@
> svn_ra_dav_resource_t *rsrc;
> const char *vcc;
> struct uri parsed_url;
> + svn_string_t *my_bc_url, *my_bc_relative;

These should just be 'const char *' rather than svn_string_t. There isn't a
need to have the length unless you happen to return it to the caller.

>...
> @@ -522,9 +528,6 @@
>
> uri_free(&parsed_url);
>
> - if (is_dir != NULL)
> - *is_dir = rsrc->is_collection;

This should stay here *IF* lopped_path is empty. We already have the answer,
so we don't want to be required to make another round trip.

>...
> @@ -536,33 +539,39 @@
> "resource.");
> }
>
> + /* Allocate our own bc_relative path. */
> + my_bc_relative = svn_string_create ("", pool);

Two problems here: 1) you allocate an svn_string_t on the heap, even though
it could have been a structure on the stack; 2) as mentioned before, we
don't need a string -- just a 'const char *'

>...
> + bc_relative->data = my_bc_relative->data;
> + bc_relative->len = my_bc_relative->len;

Compute the length here, where you actually need it.

> /* shortcut: no need to do more work if the data isn't needed. */
> - if (bc_url == NULL && latest_rev == NULL)
> + if (bc_url == NULL && latest_rev == NULL && is_dir == NULL)
> return SVN_NO_ERROR;

You can also return if "is_dir != NULL && *lopped_path == '\0'". IOW, the
condition could look like:

    if (bc_url == NULL && latest_rev == NULL
        && (is_dir == NULL || *lopped_path == '\0'))

>...
> + /* Allocate our own copy of bc_url regardless. */
> + my_bc_url = svn_string_create ("", pool);
> + my_bc_url->data = apr_hash_get(rsrc->propset,
> + SVN_RA_DAV__PROP_BASELINE_COLLECTION,
> + APR_HASH_KEY_STRING);
> + if (my_bc_url->data == NULL)
> + {
> + /* ### better error reporting... */
> + /* ### need an SVN_ERR here */
> + return svn_error_create(APR_EGENERAL, 0, NULL, pool,
> + "DAV:baseline-collection was not present "
> + "on the baseline resource.");
> + }
> + my_bc_url->len = strlen(my_bc_url->data);

No need for the length right here.

>...
> + bc_url->data = my_bc_url->data;
> + bc_url->len = my_bc_url->len;

Compute it here.

>...
> + if (is_dir != NULL)
> + {
> + /* query the DAV:resourcetype of the full, assembled URL. */
> + char *full_bc_url = svn_path_join(my_bc_url->data,
> + my_bc_relative->data,
> + pool);

'const char *' is better; it tells the reader that you don't plan to modify
the path from the join() call.

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