Re: svn commit: rev 5391 - in trunk/subversion: include libsvn_ra_dav mod_dav_svn

Woo! Looks really good. I've only got stylistic nits to make.

On Wed, Mar 19, 2003 at 01:04:51PM -0600, cmpilato@tigris.org wrote:
>...
> +++ trunk/subversion/libsvn_ra_dav/props.c Wed Mar 19 13:04:37 2003
>...
> @@ -279,31 +294,70 @@
> prop_ctx_t *pc = userdata;
> svn_ra_dav_resource_t *r = ne_propfind_current_private(pc->dph);
> const char *name;
> + const svn_string_t *value = NULL;
> + const elem_defn *parent_defn;
> + const elem_defn *defn;
> +
> + /* Reset the encoding. */
>
> - if (elm->id == NE_ELM_href)
> + switch (elm->id)
> {
> + case NE_ELM_href:
> /* use the parent element's name, not the href */
> - const elem_defn *parent_defn = defn_from_id(r->href_parent);
> + parent_defn = defn_from_id(r->href_parent);
> + name = parent_defn ? apr_pstrdup(pc->pool, parent_defn->name) : NULL;
> +
> + /* if name == NULL, then we don't know about this DAV:href. */
> + if (name == NULL)
> + return 0;

How about:

         /* if parent_defn == NULL, then we don't know about this DAV:href. */
         if (parent_defn == NULL)
           return 0;
           
         name = apr_pstrdup(pc->pool, parent_defn->name);

>...
> + value = svn_string_create(cdata, pc->pool);
> }
>
> - if (name != NULL)
> - apr_hash_set(r->propset, name, APR_HASH_KEY_STRING,
> - apr_pstrdup(pc->pool, cdata));
> + /* store VALUE in the property hash (keyed with NAME). */
> + apr_hash_set(r->propset, name, APR_HASH_KEY_STRING, value);

That value assignment happens a lot. Could possible refactor:

    /* special processing wasn't required, so we'll just turn cdata into
       an svn_string_t in the correct pool which we can store. */
    if (value == NULL)
      value = svn_string_create(cdata, pc->pool);
    
    /* store VALUE ...

>...
> @@ -666,13 +711,13 @@
> apr_pool_t *pool)
> {
> svn_ra_dav_resource_t *baseline_rsrc, *rsrc;
> - const char *my_bc_url;
> - svn_string_t my_bc_relative;
> + const svn_string_t *my_bc_url;
> + svn_string_t my_bc_rel;

The name change seemed a bit gratuitous; _rel isn't as obvious as _relative.
And to do the name change for the sake of an 80-col formatting change... urk.

>...
> if (latest_rev != NULL)
> {
> - const char *vsn_name;
> -
> - vsn_name = apr_hash_get(baseline_rsrc->propset,
> - SVN_RA_DAV__PROP_VERSION_NAME,
> - APR_HASH_KEY_STRING);
> + const svn_string_t *vsn_name= apr_hash_get(baseline_rsrc->propset,
> + SVN_RA_DAV__PROP_VERSION_NAME,
> + APR_HASH_KEY_STRING);

I avoided the initializer specifically to avoid the 80-col issue above.
You're also missing a space before the = symbol.

>...
> /* query the DAV:resourcetype of the full, assembled URL. */
> - const char *full_bc_url
> - = svn_path_url_add_component(my_bc_url, my_bc_relative.data, pool);
> + const char *full_bc_url = svn_path_url_add_component(my_bc_url->data,
> + my_bc_rel.data,
> + pool);

And the reason for the var rename :-)

>...
> +/* Helper function for svn_ra_dav__do_proppatch() below. */
> +static void
> +do_setprop(ne_buffer *body,
> + const char *name,
> + const svn_string_t *value,
> + apr_pool_t *pool)
> +{
> + const char *encoding = "";
> + const char *xml_safe;
> + const char *xml_tag_name;
> +
> + /* Map property names to namespaces */
> +#define NSLEN (sizeof(SVN_PROP_PREFIX) - 1)
> + if (strncmp(name, SVN_PROP_PREFIX, NSLEN) == 0)
> + {
> + xml_tag_name = apr_pstrcat(pool, "S:", name + NSLEN, NULL);
> + }
> +#undef NSLEN
> + else
> + {
> + xml_tag_name = apr_pstrcat(pool, "C:", name, NULL);
> + }
> +
> + /* If there is no value, just generate an empty tag and get outta
> + here. */
> + if (! value)

Can improve this a bit with:

  if (value == NULL || *value == '\0')

I just reviewed the mod_dav related code and the prop storage works fine for
empty tags.

>...
> +#ifdef SVN_DAV_FEATURE_BINARY_PROPS
> + else
> + {
> + const svn_string_t *base64ed = svn_base64_encode_string(value, pool);
> + encoding = " V:encoding=\"base64\"";
> + xml_safe = base64ed->data;
> + }
> +#endif /* SVN_DAV_FEATURE_BINARY_PROPS */

I know the above code got changed, but Philip's point about issuing a
warning for binary properties would be Goodness.

>...
> +++ trunk/subversion/mod_dav_svn/deadprops.c Wed Mar 19 13:04:37 2003
>...
> else
> {
> - /* add <prefix:name>value</prefix:name> */
> + /* add <prefix:name [V:encoding="base64"]>value</prefix:name> */
> + const char *xml_safe;
> + const char *encoding = "";
> +
> + /* Ensure XML-safety of our property values before sending them
> + across the wire. */
> +#ifdef SVN_DAV_FEATURE_BINARY_PROPS
> + if (! svn_xml_is_xml_safe(propval->data, propval->len))
> + {
> + propval = (svn_string_t *)svn_base64_encode_string(propval, pool);
> + xml_safe = propval->data;
> + encoding = apr_pstrcat(pool, " V:encoding=\"base64\"", NULL);

No need to copy that string into a pool. Even if you *did* want to, then
apr_pstrdup() woulda been easier.

A comment on the reason for the cast would be good.

>...
> @@ -290,10 +305,9 @@
> const apr_xml_elem *elem,
> dav_namespace_map *mapping)
> {
> - svn_string_t propval;
> -
> - /* ### oops. apr_xml is busted: it doesn't allow for binary data at the
> - ### moment. thankfully, we aren't using binary props yet. */
> + svn_string_t *propval = apr_pcalloc(db->p, sizeof(*propval));

Rather than putting that on the stack, I like to do:

  svn_string_t propval_struct;
  svn_string_t *propval = &propval_struct;

That pattern will work for this function, where you want to switch the
propval pointer. Another alternative is noted below.

>...
> + apr_pool_t *pool = db->p;
> + apr_xml_attr *attr = elem->attr;

'attr' can be 'const apr_xml_attr *attr'

>...
> + /* Check for special encodings of the property value. */
> + while (attr)
> + {
> + if (strcmp (attr->name, "encoding") == 0) /* ### namespace check? */
> + {
> + const char *enc_type = attr->value;
> +
> + /* Handle known encodings here. */
> + if (enc_type && (strcmp (enc_type, "base64") == 0))
> + propval = (svn_string_t *)svn_base64_decode_string(propval, pool);

This could have been:

  propval = *svn_base64_decode_string(&propval, pool);

I'm not sure that I like this as much because somebody might miss the * that
is occurring. While the assignment and & usage is legal, the notion of
execution points is always a bit shaky, so the original pattern is a bit
nicer (with the mod of a structure, noted above). Assuming so, then a
comment explaining the cast would be good.

> + else
> + return dav_new_error (pool, HTTP_INTERNAL_SERVER_ERROR, 0,
> + "Unknown property encoding");

Spaces in after func name :-)

> + break;
> + }
> + /* Next attribute, please. */
> + attr = attr->next;

Oof. If somebody adds a "continue", they might miss this part. The loop is
safer as a for-loop:

  for (attr = elem->attr; attr != NULL; attr = attr->next)

>...

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