You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Kannan <ka...@collab.net> on 2010/02/04 17:01:54 UTC
Re: [PATCH] Fix failing ci caused in r40202
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Julian Foad wrote:
[..]
> I committed the patch (with that fix) in 901797.
>
> Thanks for the patch.
>
> Now I am running the test suite on your patch to use
> svn_path_url_add_component2(),
> <http://mail-archives.apache.org/mod_mbox/subversion-dev/200912.mbox/%
> 3C4B2B12EA.5000208@collab.net%3E>, that depends on this one.
Hi Julian,
Assertion failure occurred due to a non-canonical base URL passed to
`svn_path_url_add_component2()'. Despite having canonicalized them
wherever they are generated, the reason for this was that in
`end_element()' of props.c, canonicalization was done where the url was
assigned:
<snip>
return assign_rsrc_url(pc->rsrc, svn_uri_canonicalize(cdata, pc->pool),
pc->pool);
</snip>
But there's one more place(which I missed to notice) where the value of
`cdata'(non-canonical url) is used to assigned the URL, for those
files/dirs who've parent info(cp/mv operations). So the non-canonical
URL was this one and hence the failure.
Attached herewith is the patch which canonicalizes `cdata' where its
initialized as proposed in [1]. Though there's one more place:
<snip>
case ELEM_status:
/* Parse the <status> tag's CDATA for a status code. */
if (ne_parse_statusline(cdata, &status))
return svn_error_create(SVN_ERR_XML_MALFORMED, NULL, NULL);
</snip>
that does not need the canonicalized value, thought its better to do the
canonicalization in just one place.
[[[
Log:
Follow-up r901797. Canonicalization of BASE URL doesn't reflect for
resources having parent info, till now.
* subversion/libsvn_ra_neon/props.c
(end_element): Canonicalize the BASE URL initially itself in order to
reflect the same for resources having parent info and otherwise.
Patch by: Kannan R <ka...@collab.net>
]]]
With this, the upgrade to ..add_component2() could be performed(hope),
whose link is also found in [1].
P.S: `make davautocheck` passed :)
[1]-http://mail-archives.apache.org/mod_mbox/subversion-dev/201001.mbox/%3C4B45D651.7060904@collab.net%3E
- --
Thanks & Regards,
Kannan
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iQEVAwUBS2r9gnlTqcY7ytmIAQJqbAf/Y+0Zbv1K4bsB/Q5Zv522rR+p/+HV7yoq
aucj91ZydB0yqjcq1Il6v33+14Y7vKcfVVg2RMZ7a0RbMYj04Fx2z5fF+L4JPZel
GoU0YtxIgxR7iwdwkjtjcTIROY9g7tH+SolQdpa9gIXrn3Gk7XQR6LA63z0fHZal
kGrS1dWXR5k/8aXrjGM4Hl3Z6rrgc79PoNqAUNT08VzGWD34Is9xEF14uYGUEr6Z
5vnjDMF3jabV8WOgrU6L39sodGur1BvmaJknMlJJNE7QYbVWXxIsAORsgxV5NMAL
8LKgnmSZjwHgSpD8JTPOJY8LhlN0YjfG7UQFWjSBU3+arluuTDjBDw==
=Uti2
-----END PGP SIGNATURE-----
Re: [PATCH] Fix failing ci caused in r40202
Posted by Julian Foad <ju...@btopenworld.com>.
On Fri, 2010-02-05, Kannan wrote:
> Follow-up r901797. Canonicalization of BASE URL doesn't reflect for
> elements having parent info, till now.
>
> * subversion/libsvn_ra_neon/props.c
> (end_element): Canonicalize the BASE URL for elements having parent
> info.
>
> Patch by: Kannan R <ka...@collab.net>
> Suggested by: julianfoad
> ]]]
I committed this follow-up in r906897. I re-wrote the log message; it's
not particularly about elements "having parent info", it's about any
<href> element whose parent element is not <response>.
[[[
Follow-up r901797: ensure the URLs in libsvn_ra_neon are always
canonical.
* subversion/libsvn_ra_neon/props.c
(end_element): Canonicalize the URL read from an "href" element in the
other code path. (One code path was fixed in r901797.)
[...]
]]]
Thanks.
- Julian
Re: [PATCH] Fix failing ci caused in r40202
Posted by Kannan <ka...@collab.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Julian Foad wrote:
[..]
> Yes.
>
> Thanks.
>
Attaching the updated patch herewith. Thank you for the comments, Julian.
[[[
Log:
Follow-up r901797. Canonicalization of BASE URL doesn't reflect for
elements having parent info, till now.
* subversion/libsvn_ra_neon/props.c
(end_element): Canonicalize the BASE URL for elements having parent
info.
Patch by: Kannan R <ka...@collab.net>
Suggested by: julianfoad
]]]
- --
Thanks & Regards,
Kannan
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iQEVAwUBS2vkxHlTqcY7ytmIAQJH+wgArJ99jbGvChLLzpT0zpjA8ra+aauBkOeI
1eFY5B+jzksub8fU5Bb6aaTtpsuKevH2a6749fNC+qAG17zrLsJMAIdJ3JmONYUx
ew98gHd61MOFvjolBVJZIFrJN6cuctP/998HU8nQRSKwldTb45ZwIgml+fAZnqrz
TQbRkqEzQY/dJq1cIh8jGwozf8dshM6mEi/XQTVRiOEK42uWpOpQt3L4pkUQoi9l
OSkam0iDurURopu8QewRqnYI4vAsX3ANxQItgJIlImSbwHVXuq3OCm+l+dwnxTAr
m4ispuQfAdlT5Dn214PoF+4Gw6iFIlZijCC+jlG+VciOYy6gkBOynA==
=D9+G
-----END PGP SIGNATURE-----
Re: [PATCH] Fix failing ci caused in r40202
Posted by Julian Foad <ju...@btopenworld.com>.
Kannan wrote:
> Julian Foad wrote:
> > But that's not right. Like I said before, the variable holds different
> > kinds of data at different times. (If we this variable was intended to
> > always hold a URL, we would want the variable's name to indicate so.)
> >
> > It is wrong to call svn_uri_canonicalize() on a string that is not known
> > to be a URL. Depending on what the string is, that might change it in
> > some undesired way.
> >
>
> Thought cdata->data only holds a URL(after chasing their values in gdb).
> Thanks for the clarification. So, canonicalization should be done in the
> two places where required rather than doing it initially? If so, will
> send the updated patch.
Yes.
Thanks.
- Julian
Re: [PATCH] Fix failing ci caused in r40202
Posted by Kannan <ka...@collab.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Julian Foad wrote:
> But that's not right. Like I said before, the variable holds different
> kinds of data at different times. (If we this variable was intended to
> always hold a URL, we would want the variable's name to indicate so.)
>
> It is wrong to call svn_uri_canonicalize() on a string that is not known
> to be a URL. Depending on what the string is, that might change it in
> some undesired way.
>
Thought cdata->data only holds a URL(after chasing their values in gdb).
Thanks for the clarification. So, canonicalization should be done in the
two places where required rather than doing it initially? If so, will
send the updated patch.
>> Though there's one more place:
>>
>> <snip>
>>
>> case ELEM_status:
>> /* Parse the <status> tag's CDATA for a status code. */
>> if (ne_parse_statusline(cdata, &status))
>> return svn_error_create(SVN_ERR_XML_MALFORMED, NULL, NULL);
>>
>> </snip>
>>
>> that does not need the canonicalized value, thought its better to do the
>> canonicalization in just one place.
>
> No. (And there are several other places where the variable is used in
> that function, where the value it holds is something other than a URL.)
Yup, saw them after sending the mail :( Thank you Julian.
- --
Thanks & Regards,
Kannan
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iQEVAwUBS2url3lTqcY7ytmIAQJLcgf/TcgXB4fxNTZ47zRuRm2XiX3aIp3kMd7y
eBFTozasjSKVTak0KydXZZkyK/9vMv1IprHy08eWU+jq9ryEc/d4vXm5PKZgyQ3e
J9XQ5+Vq48WtpDoIIGVKvuYgp9a6bdIw9Ho2s5LoYmiEEaqHBRuW2mgrlBUxK1D1
FQfyD21gVZdptxb4wzaPKYl5vQHJ0zodxTRHsjxJNFhMZV6uPJeN6z7qUf554+z7
7gcqLkD8gLjnMGTLJWUMAh9syQ7pkKvPTNt4HmiqlJIvugIgPvdv0gbQrU7j+adt
5GfoKQnDRnVHGmamEgIavtPK/X5fy4HbEWY+n/V/YZJ8uXH6CTzeCA==
=McpW
-----END PGP SIGNATURE-----
Re: [PATCH] Fix failing ci caused in r40202
Posted by Julian Foad <ju...@wandisco.com>.
Kannan wrote:
> Assertion failure occurred due to a non-canonical base URL passed to
> `svn_path_url_add_component2()'. Despite having canonicalized them
> wherever they are generated, the reason for this was that in
> `end_element()' of props.c, canonicalization was done where the url was
> assigned:
>
> <snip>
>
> return assign_rsrc_url(pc->rsrc, svn_uri_canonicalize(cdata, pc->pool),
> pc->pool);
Yes, because in this code path we know that the value of the variable
'cdata' is expected to be a URL.
> </snip>
>
> But there's one more place(which I missed to notice) where the value of
> `cdata'(non-canonical url) is used to assigned the URL, for those
> files/dirs who've parent info(cp/mv operations). So the non-canonical
> URL was this one and hence the failure.
OK, that's good: you've found the other place where it needs to be
canonicalized.
> Attached herewith is the patch which canonicalizes `cdata' where its
> initialized as proposed in [1].
But that's not right. Like I said before, the variable holds different
kinds of data at different times. (If we this variable was intended to
always hold a URL, we would want the variable's name to indicate so.)
It is wrong to call svn_uri_canonicalize() on a string that is not known
to be a URL. Depending on what the string is, that might change it in
some undesired way.
> Though there's one more place:
>
> <snip>
>
> case ELEM_status:
> /* Parse the <status> tag's CDATA for a status code. */
> if (ne_parse_statusline(cdata, &status))
> return svn_error_create(SVN_ERR_XML_MALFORMED, NULL, NULL);
>
> </snip>
>
> that does not need the canonicalized value, thought its better to do the
> canonicalization in just one place.
No. (And there are several other places where the variable is used in
that function, where the value it holds is something other than a URL.)
- Julian