Re: svn commit: r18486 - trunk/subversion/libsvn_ra_serf

On Thu, 16 Feb 2006, jerenkrantz@tigris.org wrote:
...
> --- trunk/subversion/libsvn_ra_serf/update.c (original)
> +++ trunk/subversion/libsvn_ra_serf/update.c Thu Feb 16 15:20:29 2006
> @@ -65,7 +65,10 @@
> /* our pool */
> apr_pool_t *pool;
>
> - /* the containing directory name */
> + /* the base name. */
> + const char *base_name;

Comment doesn't add a lot of additional value. I recommend either
removing it, or being a bit more verbose:

    /* the directory's base name (excluding parents) */

...
> @@ -119,11 +125,14 @@
> /* The enclosing directory. */
> report_dir_t *dir;
>
> - /* the name of the file. */
> - const char *file_name;
> + /* the base name of the file. */
> + const char *base_name;

Compare to above.

...
> @@ -283,6 +303,7 @@
> new_state->info->dir->parent_dir->ref_count++;
>
> new_state->info->dir->props = apr_hash_make(new_state->info->pool);
> + new_state->info->dir->ns_list = new_state->info->dir->ns_list;

Why point ns_list back at itself? (I can recommend a good
psychiatrist. ;) Should it actually reference a member of ctx?

> /* Point to the update_editor */
> new_state->info->dir->update_editor = ctx->update_editor;
...
> @@ -573,7 +613,7 @@
> svn_txdelta_op_t delta_op;
> svn_string_t window_data;
>
> - status = serf_bucket_read(response, 2048, &data, &len);
> + status = serf_bucket_read(response, 8000, &data, &len);

Need a #define for the magic number?

> if (SERF_BUCKET_READ_ERROR(status))
> {
> return status;
...
> @@ -1077,11 +1159,55 @@
...
> + /* We need to move the prop_ns, prop_name, and prop_val into the
> + * same lifetime as the dir->pool.
> + */
> + ns_t *ns, *ns_name_match;
> + int found = 0;

Why not use a svn_boolean_t here to represent the flag, rather than an
int?

> + report_dir_t *dir;
> +
> + dir = ctx->state->info->dir;
> +
> + /* We're going to be slightly tricky. We don't care what the ->url
> + * field is here at this point. So, we're going to stick a single
> + * copy of the property name inside of the ->url field.
> + */
> + ns_name_match = NULL;
> + for (ns = dir->ns_list; ns; ns = ns->next)
> + {
> + if (strcmp(ns->namespace, ctx->state->info->prop_ns) == 0)
> + {
> + ns_name_match = ns;
> + if (strcmp(ns->url, ctx->state->info->prop_name) == 0)
> + {
> + found = 1;

And the constant, TRUE, here.

> + break;
> + }
> + }
> + }
...
> --- trunk/subversion/libsvn_ra_serf/util.c (original)
> +++ trunk/subversion/libsvn_ra_serf/util.c Thu Feb 16 15:20:29 2006
> @@ -170,7 +170,7 @@
>
> while (1)

TRUE instead of 1.

> {
> - status = serf_bucket_read(response, 2048, &data, &len);
> + status = serf_bucket_read(response, 8000, &data, &len);

Here's another spot that (module-private?) #define could be used.

...
> --- trunk/subversion/libsvn_ra_serf/xml.c (original)
> +++ trunk/subversion/libsvn_ra_serf/xml.c Thu Feb 16 15:20:29 2006
> @@ -48,16 +48,29 @@
...
> + for (cur_ns = *ns_list; cur_ns; cur_ns = cur_ns->next)
> + {
> + if (strcmp(cur_ns->namespace, tmp_attrs[0]+6) == 0)

Some whitespace around that '+' operator would make this more easily
readable.

> + {
> + found = 1;

TRUE

> + break;
> + }
> + }
>
> - new_ns->namespace = apr_pstrdup(pool, tmp_attrs[0]+6);
> - new_ns->url = apr_pstrdup(pool, tmp_attrs[1]);
> + if (!found)
> + {
> + new_ns = apr_palloc(pool, sizeof(*new_ns));
> + new_ns->namespace = apr_pstrdup(pool, tmp_attrs[0]+6);
...

whitespace

-- 
Daniel Rall