lundblad@tigris.org writes:
> + /** @since New in 1.1.
> + * Retrieve a subset of the interesting revisions of a file @a path
> + * as seen in revision @a end. Invoke @a handler with @a handler_baton
> + * as its first argument for each such revision. @a sesson_baton is
> + * an open RA session. @a pool is used for all allocations.
> + *
> + * If there is an interesting revision of the file that is less than or
> + * equal to start, the iteration will start at that revision. Else, the
> + * iteration will start at the first revision of the file in the \
repository,
> + * whic has to be less than or equal to end. Note that if the function
> + * succeeds, @a handler will have been called at least once.
> + *
> + * In a series of calls, the file contents for the first interesting \
revision
> + * will be provided as a text delta against the empty file. In the \
following
> + * calls, the delta will be against the contents for the previous call. */
> + svn_error_t *(*get_file_revs) (void *session_baton,
> + const char *path,
> + svn_revnum_t start,
> + svn_revnum_t end,
> + svn_ra_file_rev_handler_t handler,
> + void *handler_baton,
> apr_pool_t *pool);
Would it be good to document here the exact error returned if the
server does not support this RA function?
> } svn_ra_plugin_t;
>
> Added: trunk/subversion/libsvn_ra_dav/file_revs.c
> Url: http://svn.collab.net/viewcvs/svn/trunk/subversion/libsvn_ra_dav/file_revs.c?view=auto&rev=10188
> ==============================================================================
> --- (empty file)
> +++ trunk/subversion/libsvn_ra_dav/file_revs.c [...]
>
> [...]
>
> +static const char *
> +get_attr (const char **atts, const char *which)
> +{
> + for (; atts && *atts; atts += 2)
> + if (strcmp (*atts, which) == 0)
> + return atts[1];
> + return NULL;
> +}
Hmmm. I feel like we've got similar code to this elsewhere in ra_dav,
let's see... Yes, in fetch.c, *and* in log.c, the exact same code.
Before we had it in two places, now we have it in three.
This is getting ridiculous. I think it's time to factor this
somewhere. Of course, you're not the original sinner, just the most
recent :-), but would you like to fix this?
> +/* If an error occurs, store it and abort the XML parse. */
> +#define CHKERR(e) \
> +do { \
> + if ((rb->err = (e)) != NULL) \
> + return NE_XML_ABORT; \
> +} while(0)
Same factorization comment applies here.
> +svn_error_t *
> +svn_ra_dav__get_file_revs (void *session_baton,
> + const char *path,
> + svn_revnum_t start,
> + svn_revnum_t end,
> + svn_ra_file_rev_handler_t handler,
> + void *handler_baton,
> + apr_pool_t *pool)
> +{
> [...]
> + /* Map status 501: Method Not Implemented to our not implemented error.
> + 1.0.x servers and older don't support this report. */
> + if (http_status == 501)
> + return svn_error_create (SVN_ERR_RA_NOT_IMPLEMENTED, err,
> + _("get-file-revs REPORT not implemented"));
Ah hah -- presumably, this is the error I suggested documenting in the
prototype?
> --- trunk/subversion/libsvn_ra_dav/ra_dav.h (original)
> +++ trunk/subversion/libsvn_ra_dav/ra_dav.h Thu Jul 8 18:11:03 2004
> @@ -273,6 +273,15 @@
> svn_node_kind_t *kind,
> apr_pool_t *pool);
>
> +svn_error_t *svn_ra_dav__get_file_revs (void *session_baton,
> + const char *path,
> + svn_revnum_t start,
> + svn_revnum_t end,
> + svn_ra_file_rev_handler_t handler,
> + void *handler_baton,
> + apr_pool_t *pool);
> +
> +
I realize we have been inconsistent (or worse) about this elsewhere in
ra_dav, but might be good to say explicitly that this implements the
'svn_ra_plugin_t.get_file_revs' prototype, so people can easily find
the right doc string.
> +static svn_error_t *
> +svn_ra_local__get_file_revs (void *session_baton,
> + const char *path,
> + svn_revnum_t start,
> + svn_revnum_t end,
> + svn_ra_file_rev_handler_t handler,
> + void *handler_baton,
> + apr_pool_t *pool)
Same thing about mentioning where to find the doc string.
> --- trunk/subversion/libsvn_ra_svn/client.c (original)
> +++ trunk/subversion/libsvn_ra_svn/client.c Thu Jul 8 18:11:03 2004
> @@ -175,6 +175,30 @@
> return SVN_NO_ERROR;
> }
>
> +/* Convert property diffs received from the server to an array of svn_prop_. */
> +static svn_error_t *parse_prop_diffs(apr_array_header_t *list,
> + apr_pool_t *pool,
> + apr_array_header_t **diffs)
Did you mean 'svn_prop_t' here? More precisely, did you mean
'svn_prop_t *'? In general, you should say the exact types of input
and output elements.
Also, there's a small confusion between the doc string and the
prototype: The doc string implies that 'diffs' is input and 'list' is
output. Yet diffs is the double-star type, which implies it is the
*output*. I presume this function sets '*diffs' to a new
'apr_array_header_t *', allocated in pool, etc.
Generally, we document such functions this way:
/* Set *OUTPUT to an array of 'svn_blah_t *' objects, based on the
information in INPUT. Allocate *OUTPUT and its elements in POOL. */
You know, something like that.
> +{
> + svn_ra_svn_item_t *elt;
> + svn_prop_t *prop;
> + int i;
> +
> + *diffs = apr_array_make(pool, list->nelts, sizeof(svn_prop_t));
> +
> + for (i = 0; i < list->nelts; i++)
> + {
> + elt = &APR_ARRAY_IDX(list, i, svn_ra_svn_item_t);
> + if (elt->kind != SVN_RA_SVN_LIST)
> + return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> + _("Prop diffs element not a list"));
> + prop = apr_array_push(*diffs);
> + SVN_ERR(svn_ra_svn_parse_tuple(elt->u.list, pool, "c(?s)", &prop->name,
> + &prop->value));
> + }
> + return SVN_NO_ERROR;
> +}
Okay, I can see that the types are behaving as expected here; it is
the doc string which is confusing.
> +static svn_error_t *ra_svn_get_file_revs(void *session_baton, const char *path,
> + svn_revnum_t start, svn_revnum_t end,
> + svn_ra_file_rev_handler_t handler,
> + void *handler_baton, apr_pool_t *pool)
> +{
Same thing about mentioning what prototype this implements.
> + /* Servers before 1.1 don't support this command. Check for this here. */
> + if (err && err->apr_err == SVN_ERR_RA_SVN_UNKNOWN_CMD)
> + return svn_error_create(SVN_ERR_RA_NOT_IMPLEMENTED, err,
> + _("get-file-revs not implemented"));
> + SVN_ERR(err);
Same error, good :-).
> + while (1)
> + {
> + svn_pool_clear(rev_pool);
> + svn_pool_clear(chunk_pool);
> + SVN_ERR(svn_ra_svn_read_item(sess->conn, rev_pool, &item));
> + if (item->kind == SVN_RA_SVN_WORD && strcmp(item->u.word, "done") == 0)
> + break;
> + /* Either we've got a correct revision or we will error out below. */
> + had_revision = TRUE;
> + if (item->kind != SVN_RA_SVN_LIST)
> + return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> + _("Revision entry not a list"));
> +
> + SVN_ERR(svn_ra_svn_parse_tuple(item->u.list, rev_pool,
> + "crll", &p, &rev, &rev_proplist,
> + &proplist));
> + SVN_ERR(parse_proplist(rev_proplist, rev_pool, &rev_props));
> + SVN_ERR(parse_prop_diffs(proplist, rev_pool, &props));
> +
> + /* Get the first delta chunk so we know if there is a delta. */
> + SVN_ERR(svn_ra_svn_read_item(sess->conn, chunk_pool, &item));
> + if (item->kind != SVN_RA_SVN_STRING)
> + return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> + _("Text delta chunk not a string"));
> + has_txdelta = item->u.string->len > 0;
> +
> + SVN_ERR(handler(handler_baton, p, rev, rev_props,
> + has_txdelta ? &d_handler : NULL, &d_baton,
> + props, rev_pool));
> +
> + /* Process the text delta if any. */
> + if (has_txdelta)
> + {
> + if (d_handler)
> + stream = svn_txdelta_parse_svndiff(d_handler, d_baton, TRUE,
> + rev_pool);
> + else
> + stream = NULL;
> + while (item->u.string->len > 0)
> + {
> + size = item->u.string->len;
> + if (stream)
> + SVN_ERR(svn_stream_write(stream, item->u.string->data, &size));
> + svn_pool_clear(chunk_pool);
> +
> + SVN_ERR(svn_ra_svn_read_item(sess->conn, chunk_pool, &item));
> + if (item->kind != SVN_RA_SVN_STRING)
> + return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> + _("Text delta chunk not a string"));
> + }
> + if (stream)
> + SVN_ERR(svn_stream_close(stream));
> + }
> + }
> +
> + SVN_ERR(svn_ra_svn_read_cmd_response(sess->conn, pool, ""));
> +
> + /* Return error if we didn't get any revisions. */
> + if (!had_revision)
> + return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> + _("The get-file-revs command didn't return "
> + "any revisions."));
> +
> + svn_pool_destroy(chunk_pool);
> + svn_pool_destroy(rev_pool);
> +
> + return SVN_NO_ERROR;
> +}
At first, I couldn't figure out why you had both rev_pool and
chunk_pool being created and destroyed at the same level; but looking
at the code, I see it makes sense.
> +/* Create a writable generic stream that will encode its output to base64
> + and send it to the Apache filter OUTPUT using BB. */
> +svn_stream_t * dav_svn_make_base64_output_stream(apr_bucket_brigade *bb,
> + ap_filter_t *output,
> + apr_pool_t *pool);
Say "Return", not just "Create". Might also want to point out that
this allocates the stream in POOL.
> +static svn_error_t *send_xml(struct file_rev_baton *frb,
> + const char *fmt, ...)
> +{
> + apr_status_t apr_err;
> + va_list ap;
> +
> + va_start(ap, fmt);
> + apr_err = apr_brigade_vprintf(frb->bb, ap_filter_flush,
> + frb->output, fmt, ap);
> + va_end(ap);
> + if (apr_err)
> + return svn_error_create(apr_err, 0, NULL);
> + /* ### check for an aborted connection, since the brigade functions
> + don't appear to be return useful errors when the connection is
> + dropped. */
> + if (frb->output->c->aborted)
> + return svn_error_create(SVN_ERR_APMOD_CONNECTION_ABORTED, 0, NULL);
> + return SVN_NO_ERROR;
> +}
Again, we've gone from code duplication in two places to three
places. All of these send_xml() functions (here, in log.c, and in
update.c) seem to do basically the same thing. It's true they look
inside the specific batons, but the things they extract from those
batons are always the same things: 'bb', 'output', and 'c->aborted',
which are presumably the same types wherever they occur.
I wonder if we can't factor this code into one place, taking those
three things as parameters, instead of extracting them from the baton
every time? If we really want to, we can have a wrapper function in
each file to do the extracting, but at least the bulk of the
send_xml() code could be factored...
> +/* If FRB->needs_header is true, send the "<S:file-revs-report>" start
> + tag and set FRB->needs_header to zero. Else do nothing. */
> +static svn_error_t *maybe_send_header(struct file_rev_baton *frb)
> +{
> + if (frb->needs_header)
> + {
> + SVN_ERR(send_xml(frb,
> + DAV_XML_HEADER DEBUG_CR
> + "<S:file-revs-report xmlns:S=\"" SVN_XML_NAMESPACE "\" "
> + "xmlns:D=\"DAV:\">" DEBUG_CR));
> + frb->needs_header = FALSE;
> + }
> + return SVN_NO_ERROR;
> +}
This is basically duplicated from log.c, but on the other hand, I can
see how it would be hard to factor, since the header it sends out is
specific to the baton type anyway.
In situations like that, I sometimes add a comment to both places,
saying "Note that this is essentially duplicated in log.c [or
file_revs.c]. If duplicating again, then consider factoring." In
other words, the factorization overhead is not worth it if it's only
two places, but if it's three (or five, or whatever), then it's
starting to be worth it.
> +/* Send a property named NAME with value VAL in an element named ELEM_NAME.
> + NAME is properly quoted and VAL is encoded if necessary. */
The passive voice in the second sentence obscures the calling
discipline here -- it's not instantly clear whether this is saying
that the caller must make sure NAME is properly quoted and VAL
encoded, or that this function does the quoting and encoding. Of
course, after a moment's thought, one realizes that this function does
the quoting/encoding, but if it used the active voice, the reader
wouldn't ever wonder.
> +/* This implements the svn_repos_file_rev_handler_t intefrace. */
^^^^^^^^^
Looks like you've got a tranpsosition there :-).
> +dav_error *
> +dav_svn__file_revs_report(const dav_resource *resource,
> + const apr_xml_doc *doc,
> + ap_filter_t *output)
This doesn't appear to be documented in dav_svn.h either... (I realize
that ra_dav and mod_dav_svn are spotty in their documentation, for
historical reasons, but new code should always be documented).
> +/* Write out a list of property diffs. PROPDIFFS is an array of svn_prop_t
> + * values. */
> +static svn_error_t *write_prop_diffs(svn_ra_svn_conn_t *conn,
> + apr_pool_t *pool,
> + apr_array_header_t *propdiffs)
Is it really 'svn_prop_t', or is it 'svn_prop_t *'?
> +{
> + int i;
> +
> + for (i = 0; i < propdiffs->nelts; ++i)
> + {
> + const svn_prop_t *prop = &APR_ARRAY_IDX(propdiffs, i, svn_prop_t);
> +
> + SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "c(?s)",
> + prop->name, prop->value));
> + }
> +
> + return SVN_NO_ERROR;
> +}
> +
Wow, so it's really 'svn_prop_t'. Okay. But then why bother to take
its address? Why not just do
const svn_prop_t prop = APR_ARRAY_IDX(propdiffs, i, svn_prop_t);
SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "c(?s)",
prop.name, prop.value));
in that case?
> +static svn_error_t *svndiff_handler(void *baton, const char *data,
> + apr_size_t *len)
> +{
> + file_revs_baton_t *b = baton;
> + svn_string_t str;
> +
> + str.data = data;
> + str.len = *len;
> + return svn_ra_svn_write_string(b->conn, b->pool, &str);
> +}
> +
> +static svn_error_t *svndiff_close_handler(void *baton)
> +{
> + file_revs_baton_t *b = baton;
> +
> + SVN_ERR(svn_ra_svn_write_cstring(b->conn, b->pool, ""));
> + return SVN_NO_ERROR;
> +}
> +
> +static svn_error_t *file_rev_handler(void *baton, const char *path,
> + svn_revnum_t rev, apr_hash_t *rev_props,
> + svn_txdelta_window_handler_t *d_handler,
> + void **d_baton,
> + apr_array_header_t *prop_diffs,
> + apr_pool_t *pool)
> +{
This entire file doesn't seem to use doc strings, so I can't really
fault you for not writing them for your new code, but... I wish I knew
why we do not have doc strings for internal functions in svnserve. :-(
Comments like the ones above are only to be expected in a change this
large, please don't take them as my reaction to the change as a whole.
It's a very solid piece of work -- bravo!
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Jul 12 16:17:09 2004