On Fri, 27 Jan 2006 jerenkrantz@tigris.org wrote:
> Author: jerenkrantz
> Date: Fri Jan 27 00:10:02 2006
> New Revision: 18260
>
> Modified:
> trunk/subversion/libsvn_ra_serf/serf.c
>
> Log:
> ra_serf: Perform the REPORT request for update, but do not parse the correctly
> returned response yet.
>
> This code is reaching the point where there are obvious opportunities to
> refactor, optimize, and move code around, but we're not ready to do that until
> we can sucessfully drive a checkout.
>
Yeah, I'll try to avoid making points about that below:-)
> Modified: trunk/subversion/libsvn_ra_serf/serf.c
> Url: http://svn.collab.net/viewcvs/svn/trunk/subversion/libsvn_ra_serf/serf.c?rev=18260&p1=trunk/subversion/libsvn_ra_serf/serf.c&p2=trunk/subversion/libsvn_ra_serf/serf.c&r1=18259&r2=18260
> ==============================================================================
> --- trunk/subversion/libsvn_ra_serf/serf.c (original)
> +++ trunk/subversion/libsvn_ra_serf/serf.c Fri Jan 27 00:10:02 2006
> @@ -27,6 +27,7 @@
> #include "svn_pools.h"
> #include "svn_ra.h"
> #include "svn_dav.h"
> +#include "svn_xml.h"
> #include "../libsvn_ra/ra_loader.h"
> #include "svn_config.h"
> #include "svn_delta.h"
> @@ -136,8 +137,7 @@
> apr_sockaddr_t *address;
>
> serf_sess = apr_pcalloc(pool, sizeof(*serf_sess));
> - /* todo: create a subpool? */
> - serf_sess->pool = pool;
> + apr_pool_create(&serf_sess->pool, pool);
Should be svn_pool_create, but why do you need a subpool?
> +
> +static void add_tag_buckets(serf_bucket_t *agg_bucket, const char *tag,
> + const char *value, serf_bucket_alloc_t *bkt_alloc)
I would use _element_ instead of _tag_ in the name, since it adds a whole
element, not just a tag.
Also, a docstring here (and on the other functions) would be really
valuable, because to me it is not obvious exactly what this function does
and I don't think you should have to read the code to know what a function
does. One can guess what this function does, but does it, for example,
escape the value string with XML entities? Note that we have functions to
create XML tags already in svn_xml.h, but they work on stringbufs, and
maybe it is more efficient to work on serf buckets directly (or some other
good reason) in which case this is fine of course.
> +{
...
> + tmp = SERF_BUCKET_SIMPLE_STRING(value, bkt_alloc);
> + serf_bucket_aggregate_append(agg_bucket, tmp);
> +
OK, it does not escape the element content. Would it be an improvement to
do so? Escaping and encoding bugs are quite common...
> +
> +static svn_error_t *
> +set_path(void *report_baton,
> + const char *path,
> + svn_revnum_t revision,
> + svn_boolean_t start_empty,
> + const char *lock_token,
> + apr_pool_t *pool)
> +{
> + report_context_t *report = report_baton;
> + serf_bucket_t *tmp;
> +
> + tmp = SERF_BUCKET_SIMPLE_STRING_LEN("<S:entry rev=\"",
> + sizeof("<S:entry rev=\"")-1,
> + report->sess->bkt_alloc);
> + serf_bucket_aggregate_append(report->buckets, tmp);
> +
> + tmp = SERF_BUCKET_SIMPLE_STRING(apr_ltoa(pool, revision),
> + report->sess->bkt_alloc);
> + serf_bucket_aggregate_append(report->buckets, tmp);
> +
> + tmp = SERF_BUCKET_SIMPLE_STRING_LEN("\"", sizeof("\"")-1,
> + report->sess->bkt_alloc);
> + serf_bucket_aggregate_append(report->buckets, tmp);
> +
> + if (lock_token)
> + {
> + tmp = SERF_BUCKET_SIMPLE_STRING_LEN(" lock-token=\"",
> + sizeof(" lock-token=\"")-1,
> + report->sess->bkt_alloc);
> + serf_bucket_aggregate_append(report->buckets, tmp);
> +
> + tmp = SERF_BUCKET_SIMPLE_STRING(lock_token,
> + report->sess->bkt_alloc);
Need to escape the lock token, even if it is unlikely that failing to do
so will cause problems just in this case.
> + serf_bucket_aggregate_append(report->buckets, tmp);
> +
> + tmp = SERF_BUCKET_SIMPLE_STRING_LEN("\"", sizeof("\"")-1,
> + report->sess->bkt_alloc);
> + serf_bucket_aggregate_append(report->buckets, tmp);
> + }
> +
> + if (start_empty)
> + {
> + tmp = SERF_BUCKET_SIMPLE_STRING_LEN(" start-empty=\"true\"",
> + sizeof(" start-empty=\"true\"")-1,
> + report->sess->bkt_alloc);
> + serf_bucket_aggregate_append(report->buckets, tmp);
> + }
> +
> + tmp = SERF_BUCKET_SIMPLE_STRING_LEN(">", sizeof(">")-1,
> + report->sess->bkt_alloc);
> + serf_bucket_aggregate_append(report->buckets, tmp);
> +
...
> + tmp = SERF_BUCKET_SIMPLE_STRING(path, report->sess->bkt_alloc);
Here, it's more important.
> + vcc_url = fetch_prop(props, sess->repos_url.path, "DAV:",
> + "version-controlled-configuration");
> +
Wouldn't it be better to have #defines for these string constants, making
it harder to misspell them?
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Jan 27 09:43:47 2006