You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by kf...@collab.net on 2005/04/11 22:50:44 UTC
Re: svn commit: r14041 - in trunk/subversion: libsvn_ra_dav mod_dav_svn tests/clients/cmdline
cmpilato@tigris.org writes:
> Author: cmpilato
> Date: Fri Apr 8 10:23:36 2005
> New Revision: 14041
>
> Modified:
> trunk/subversion/libsvn_ra_dav/fetch.c
> trunk/subversion/mod_dav_svn/update.c
> trunk/subversion/tests/clients/cmdline/update_tests.py
> Log:
> Finish issue #2268 (svn update of xml-unsafe dir (from within) http error).
>
> * subversion/libsvn_ra_dav/fetch.c
> (make_reporter): XML-escape "src-path" before it hits the wire.
> While here, make all escapings of this sort use the same
> stringbuf.
>
> * subversion/mod_dav_svn/update.c
> (string_from_cdata_pieces, empty_cdata_error):
> (dav_svn__update_report): Go figure. Turns out the assumption we've
> been making all along about our XML CDATA all being in a single
> chunk just ain't valid anymore. Use the new helper functions to
> concatenate CDATA chunks and eliminate lots of duplicated code.
> Finally, re-organize some logic blocks.
I think you meant to say "New helper functions." up there.
> [... libsvn_ra_dav/fetch.c changes ...]
The fetch.c changes looked good to me. Too bad about the need to
constantly reinitialize 'xml_s' to NULL, oh well.
> --- trunk/subversion/mod_dav_svn/update.c (original)
> +++ trunk/subversion/mod_dav_svn/update.c Fri Apr 8 10:23:36 2005
> @@ -998,6 +998,40 @@
> }
>
>
> +static svn_stringbuf_t *
> +string_from_cdata_pieces(apr_xml_elem *element,
> + apr_pool_t *pool)
> +{
> + svn_stringbuf_t *buf;
> + struct apr_text *this_elem;
> +
> + /* No CDATA? That's badness. */
> + if (! element->first_cdata.first)
> + return NULL;
> +
> + buf = svn_stringbuf_create(element->first_cdata.first->text, pool);
> + this_elem = element->first_cdata.first->next;
> + while (this_elem)
> + {
> + svn_stringbuf_appendcstr(buf, this_elem->text);
> + this_elem = this_elem->next;
> + }
> + return buf;
> +}
Document?
> +static dav_error *
> +empty_cdata_error(const char *tagname,
> + apr_pool_t *pool)
> +{
> + const char *errstr = apr_pstrcat(pool, "The request's '", tagname,
> + "' element contains empty cdata; there "
> + "is a problem with the client.", NULL);
> + return dav_new_error_tag(pool, HTTP_BAD_REQUEST, 0, errstr,
> + SVN_DAV_ERROR_NAMESPACE, SVN_DAV_ERROR_TAG);
> +}
Also here, though I guess it's less important as this is so obvious.
> dav_error * dav_svn__update_report(const dav_resource *resource,
> const apr_xml_doc *doc,
> ap_filter_t *output)
> @@ -1069,175 +1103,85 @@
> Thus, the check for non-empty cdata in each of these cases
> cannot be moved to the top of the loop, because then it would
> wrongly catch other elements that do allow empty cdata. */
> -
> + svn_stringbuf_t *cdata;
> +
> if (child->ns == ns && strcmp(child->name, "target-revision") == 0)
> {
> - if (! child->first_cdata.first)
> - return dav_new_error_tag(resource->pool, HTTP_BAD_REQUEST, 0,
> - "The request's 'target-revision' element contains empty cdata; "
> - "there is a problem with the client.",
> - SVN_DAV_ERROR_NAMESPACE,
> - SVN_DAV_ERROR_TAG);
> -
> - /* ### assume no white space, no child elems, etc */
> - revnum = SVN_STR_TO_REV(child->first_cdata.first->text);
> + if (! ((cdata = string_from_cdata_pieces(child, resource->pool))))
> + return empty_cdata_error(child->name, resource->pool);
> + revnum = SVN_STR_TO_REV(cdata->data);
> }
Is it just me, or is there an extra level of paren-wrapping going on here?
> -
> if (child->ns == ns && strcmp(child->name, "src-path") == 0)
> {
> - /* ### assume no white space, no child elems, etc */
> dav_svn_uri_info this_info;
> -
> - if (! child->first_cdata.first)
> - return dav_new_error_tag(resource->pool, HTTP_BAD_REQUEST, 0,
> - "The request's 'src-path' element contains empty cdata; "
> - "there is a problem with the client.",
> - SVN_DAV_ERROR_NAMESPACE,
> - SVN_DAV_ERROR_TAG);
> -
> - /* split up the 1st public URL. */
> - if ((derr = dav_svn__test_canonical
> - (child->first_cdata.first->text, resource->pool)))
> + if (! ((cdata = string_from_cdata_pieces(child, resource->pool))))
> + return empty_cdata_error(child->name, resource->pool);
> + if ((derr = dav_svn__test_canonical(cdata->data,
> + resource->pool)))
And here.
> return derr;
> - serr = dav_svn_simple_parse_uri(&this_info, resource,
> - child->first_cdata.first->text,
> - resource->pool);
> - if (serr != NULL)
> - {
> - return dav_svn_convert_err(serr, HTTP_INTERNAL_SERVER_ERROR,
> - "Could not parse src-path URL.",
> - resource->pool);
> - }
> + if ((serr = dav_svn_simple_parse_uri(&this_info, resource,
> + cdata->data,
> + resource->pool)))
> + return dav_svn_convert_err(serr, HTTP_INTERNAL_SERVER_ERROR,
> + "Could not parse 'src-path' URL.",
> + resource->pool);
And here.
(Sorry, nothing more substantive to say. Write more bugs and I'll
write better critiques.)
> src_path = this_info.repos_path;
> }
> -
> if (child->ns == ns && strcmp(child->name, "dst-path") == 0)
> {
> - /* ### assume no white space, no child elems, etc */
> dav_svn_uri_info this_info;
> -
> - if (! child->first_cdata.first)
> - return dav_new_error_tag(resource->pool, HTTP_BAD_REQUEST, 0,
> - "The request's 'dst-path' element contains empty cdata; "
> - "there is a problem with the client. See "
> - "http://subversion.tigris.org/issues/show_bug.cgi?id=1055",
> - SVN_DAV_ERROR_NAMESPACE,
> - SVN_DAV_ERROR_TAG);
> -
> - /* split up the 2nd public URL. */
> - if ((derr = dav_svn__test_canonical
> - (child->first_cdata.first->text, resource->pool)))
> + if (! ((cdata = string_from_cdata_pieces(child, resource->pool))))
> + return empty_cdata_error(child->name, resource->pool);
> + if ((derr = dav_svn__test_canonical(cdata->data,
> + resource->pool)))
> return derr;
Same.
> - serr = dav_svn_simple_parse_uri(&this_info, resource,
> - child->first_cdata.first->text,
> - resource->pool);
> - if (serr != NULL)
> - {
> - return dav_svn_convert_err(serr, HTTP_INTERNAL_SERVER_ERROR,
> - "Could not parse dst-path URL.",
> - resource->pool);
> - }
> + if ((serr = dav_svn_simple_parse_uri(&this_info, resource,
> + cdata->data,
> + resource->pool)))
> + return dav_svn_convert_err(serr, HTTP_INTERNAL_SERVER_ERROR,
> + "Could not parse 'dst-path' URL.",
> + resource->pool);
"Guess who?"
> dst_path = this_info.repos_path;
> }
> -
> if (child->ns == ns && strcmp(child->name, "update-target") == 0)
> {
> - if (! child->first_cdata.first)
> - return dav_new_error_tag(resource->pool, HTTP_BAD_REQUEST, 0,
> - "The request's 'update-target' element contains empty cdata; "
> - "there is a problem with the client.",
> - SVN_DAV_ERROR_NAMESPACE,
> - SVN_DAV_ERROR_TAG);
> -
> - /* ### assume no white space, no child elems, etc */
> - if ((derr = dav_svn__test_canonical
> - (child->first_cdata.first->text, resource->pool)))
> + if (! ((cdata = string_from_cdata_pieces(child, resource->pool))))
> + return empty_cdata_error(child->name, resource->pool);
> + if ((derr = dav_svn__test_canonical(cdata->data,
> + resource->pool)))
> return derr;
> - target = child->first_cdata.first->text;
> + target = cdata->data;
> }
"It's me!"
(This code is so much more robust now, though, +relief+ .)
> if (child->ns == ns && strcmp(child->name, "recursive") == 0)
> {
> - if (! child->first_cdata.first)
> - return dav_new_error_tag(resource->pool, HTTP_BAD_REQUEST, 0,
> - "The request's 'recursive' element contains empty cdata; "
> - "there is a problem with the client.",
> - SVN_DAV_ERROR_NAMESPACE,
> - SVN_DAV_ERROR_TAG);
> -
> - /* ### assume no white space, no child elems, etc */
> - if (strcmp(child->first_cdata.first->text, "no") == 0)
> + if (! ((cdata = string_from_cdata_pieces(child, resource->pool))))
> + return empty_cdata_error(child->name, resource->pool);
> + if (strcmp(cdata->data, "no") == 0)
> recurse = FALSE;
> }
Is that a paren in your pocket or are you just happy to see me?
> if (child->ns == ns && strcmp(child->name, "ignore-ancestry") == 0)
> {
> - if (! child->first_cdata.first)
> - return dav_new_error_tag(resource->pool, HTTP_BAD_REQUEST, 0,
> - "The request's 'ignore-ancestry' element contains empty cdata; "
> - "there is a problem with the client.",
> - SVN_DAV_ERROR_NAMESPACE,
> - SVN_DAV_ERROR_TAG);
> -
> - /* ### assume no white space, no child elems, etc */
> - if (strcmp(child->first_cdata.first->text, "no") != 0)
> + if (! ((cdata = string_from_cdata_pieces(child, resource->pool))))
> + return empty_cdata_error(child->name, resource->pool);
> + if (strcmp(cdata->data, "no") != 0)
> ignore_ancestry = TRUE;
If I weren't basically a cruel person, I wouldn't quote every instance.
> }
> if (child->ns == ns && strcmp(child->name, "resource-walk") == 0)
> {
> - if (! child->first_cdata.first)
> - return dav_new_error_tag(resource->pool, HTTP_BAD_REQUEST, 0,
> - "The request's 'resource-walk' element contains empty cdata; "
> - "there is a problem with the client.",
> - SVN_DAV_ERROR_NAMESPACE,
> - SVN_DAV_ERROR_TAG);
> -
> - /* ### assume no white space, no child elems, etc */
> - if (strcmp(child->first_cdata.first->text, "no") != 0)
> + if (! ((cdata = string_from_cdata_pieces(child, resource->pool))))
> + return empty_cdata_error(child->name, resource->pool);
> + if (strcmp(cdata->data, "no") != 0)
> resource_walk = TRUE;
But, as it happens, I *am* a cruel person...
> }
> if (child->ns == ns && strcmp(child->name, "text-deltas") == 0)
> {
> - if (! child->first_cdata.first)
> - return dav_new_error_tag(resource->pool, HTTP_BAD_REQUEST, 0,
> - "The request's 'text-deltas' element contains empty cdata; "
> - "there is a problem with the client.",
> - SVN_DAV_ERROR_NAMESPACE,
> - SVN_DAV_ERROR_TAG);
> -
> - /* ### assume no white space, no child elems, etc */
> - if (strcmp(child->first_cdata.first->text, "no") == 0)
> + if (! ((cdata = string_from_cdata_pieces(child, resource->pool))))
> + return empty_cdata_error(child->name, resource->pool);
> + if (strcmp(cdata->data, "no") == 0)
> text_deltas = FALSE;
> }
> }
...and I can't help myself...
> -
> - if (revnum == SVN_INVALID_REVNUM)
> - {
> - serr = svn_fs_youngest_rev(&revnum, repos->fs, resource->pool);
> - if (serr != NULL)
> - {
> - return dav_svn_convert_err(serr, HTTP_INTERNAL_SERVER_ERROR,
> - "Could not determine the youngest "
> - "revision for the update process.",
> - resource->pool);
> - }
> - }
> -
> - editor = svn_delta_default_editor(resource->pool);
> - editor->set_target_revision = upd_set_target_revision;
> - editor->open_root = upd_open_root;
> - editor->delete_entry = upd_delete_entry;
> - editor->add_directory = upd_add_directory;
> - editor->open_directory = upd_open_directory;
> - editor->change_dir_prop = upd_change_xxx_prop;
> - editor->close_directory = upd_close_directory;
> - editor->absent_directory = upd_absent_directory;
> - editor->add_file = upd_add_file;
> - editor->open_file = upd_open_file;
> - editor->apply_textdelta = upd_apply_textdelta;
> - editor->change_file_prop = upd_change_xxx_prop;
> - editor->close_file = upd_close_file;
> - editor->absent_file = upd_absent_file;
> - editor->close_edit = upd_close_edit;
> -
> +
...though I'm not so cruel as to ask about the whitespace you added to
that line.
> /* If the client never sent a <src-path> element, it's old and
> sending a style of report that we no longer allow. */
> if (! src_path)
> @@ -1250,6 +1194,17 @@
> SVN_DAV_ERROR_TAG);
> }
>
> + /* If a revision for this operation was not dictated to us, this
> + means "update to whatever the current HEAD is now". */
> + if (revnum == SVN_INVALID_REVNUM)
> + {
> + if ((serr = svn_fs_youngest_rev(&revnum, repos->fs, resource->pool)))
> + return dav_svn_convert_err(serr, HTTP_INTERNAL_SERVER_ERROR,
> + "Could not determine the youngest "
> + "revision for the update process.",
> + resource->pool);
> + }
Paren-O-Mania, ladies and gentlemen! Get your parens here!
(You may hit me now.)
> uc.resource = resource;
> uc.output = output;
> uc.anchor = src_path;
> @@ -1300,6 +1255,22 @@
> dir_delta() between REPOS_PATH/TARGET and TARGET_PATH. In the
> case of an update or status, these paths should be identical. In
> the case of a switch, they should be different. */
> + editor = svn_delta_default_editor(resource->pool);
> + editor->set_target_revision = upd_set_target_revision;
> + editor->open_root = upd_open_root;
> + editor->delete_entry = upd_delete_entry;
> + editor->add_directory = upd_add_directory;
> + editor->open_directory = upd_open_directory;
> + editor->change_dir_prop = upd_change_xxx_prop;
> + editor->close_directory = upd_close_directory;
> + editor->absent_directory = upd_absent_directory;
> + editor->add_file = upd_add_file;
> + editor->open_file = upd_open_file;
> + editor->apply_textdelta = upd_apply_textdelta;
> + editor->change_file_prop = upd_change_xxx_prop;
> + editor->close_file = upd_close_file;
> + editor->absent_file = upd_absent_file;
> + editor->close_edit = upd_close_edit;
> if ((serr = svn_repos_begin_report(&rbaton, revnum, repos->username,
> repos->repos,
> src_path, target,
Hmm, I see from the context that this paren thing is some sort of
quaint local custom. Forget I mentioned it.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r14041 - in trunk/subversion: libsvn_ra_dav mod_dav_svn
tests/clients/cmdline
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Tue, 11 Apr 2005 kfogel@collab.net wrote:
> cmpilato@tigris.org writes:
> > --- trunk/subversion/mod_dav_svn/update.c (original)
> > +++ trunk/subversion/mod_dav_svn/update.c Fri Apr 8 10:23:36 2005
> > @@ -998,6 +998,40 @@
> > }
> >
> >
> > +static svn_stringbuf_t *
> > +string_from_cdata_pieces(apr_xml_elem *element,
> > + apr_pool_t *pool)
> > +{
> > + svn_stringbuf_t *buf;
> > + struct apr_text *this_elem;
> > +
> > + /* No CDATA? That's badness. */
> > + if (! element->first_cdata.first)
> > + return NULL;
> > +
> > + buf = svn_stringbuf_create(element->first_cdata.first->text, pool);
> > + this_elem = element->first_cdata.first->next;
> > + while (this_elem)
> > + {
> > + svn_stringbuf_appendcstr(buf, this_elem->text);
> > + this_elem = this_elem->next;
> > + }
> > + return buf;
> > +}
>
There is an dav_xml_get_cdata in mod_dav.h that we use in other places.
> > +static dav_error *
> > +empty_cdata_error(const char *tagname,
> > + apr_pool_t *pool)
> > +{
> > + const char *errstr = apr_pstrcat(pool, "The request's '", tagname,
> > + "' element contains empty cdata; there "
> > + "is a problem with the client.", NULL);
> > + return dav_new_error_tag(pool, HTTP_BAD_REQUEST, 0, errstr,
> > + SVN_DAV_ERROR_NAMESPACE, SVN_DAV_ERROR_TAG);
> > +}
>
The terminology is a little strange here. There is no such thing as "empty
cdata"; an element might be empty. If you use dav_xml_get_cdata, you'll
get an empty string back and avoid this kludge.
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r14041 - in trunk/subversion: libsvn_ra_dav mod_dav_svn
tests/clients/cmdline
Posted by Julian Foad <ju...@btopenworld.com>.
kfogel@collab.net wrote:
> cmpilato@tigris.org writes:
>>+ if (! ((cdata = string_from_cdata_pieces(child, resource->pool))))
>>+ return empty_cdata_error(child->name, resource->pool);
>
> Is it just me, or is there an extra level of paren-wrapping going on here?
>
>>+ if ((derr = dav_svn__test_canonical(cdata->data,
>>+ resource->pool)))
>
> And here.
[...]
>> if ((serr = svn_repos_begin_report(&rbaton, revnum, repos->username,
>> repos->repos,
>> src_path, target,
>
> Hmm, I see from the context that this paren thing is some sort of
> quaint local custom. Forget I mentioned it.
There is a (fairly) well known practice of putting an extra level of
parentheses around an assignment that is the whole test expression in an "if",
"while", etc., to indicate that, yes, it was intentionally an assignment rather
than a typo for an equality test. Some compilers co-operate with this
practice, giving a warning about assignment within a test only if it is not
within extra parentheses. (I think.)
However, that only accounts for some of the instances that you pointed out -
e.g. the second but not the first of those I quote above. The first one (and
all those starting with "not" ("!")) does not need extra parentheses, because
this practice does not apply to sub-expressions, only to the outer expression
that is the whole test. (I think.)
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org