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 2009/12/08 13:16:17 UTC
[PATCH] Fix failing ci caused in r40202
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
As per discussion in the thread [1], attaching the patch herewith.
Log:
Make a proper fix to resolve some deprecation warnings using
`svn_path_url_add_component2()' by canonicalizing the base before
passing to the above method.
[in subversion/libsvn_ra_neon]
* commit.c
(get_version_url, create_activity, commit_add_dir, commit_add_file
commit_close_file, add_child, commit_delete_entry): Use
`svn_path_url_add_component2()'.
(svn_ra_neon__get_commit_editor, checkout_resource, apply_revprops):
Canonicalize the base before passing to
`svn_path_url_add_component2()'.
* props.c
(svn_ra_neon__get_baseline_info): Canonicalize the base before
passing to `svn_path_url_add_component2()'.
Found and Suggested by: stsp
Patch by: Kannan R <ka...@collab.net>
[1]-http://mail-archives.apache.org/mod_mbox/subversion-dev/200912.mbox/%3C4B1DEEE0.6010505@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/
iQEVAwUBSx5RoHlTqcY7ytmIAQIzzAf+M10mK7sUcb78KzV5pSK4EgDHBjENz9AS
rR9Vd90GEdnU5+qSz5r4ibK41OsxzaY3cV/M0SIXpD4pxTuuaUroe4KxxppC25kS
LX0G0/MeTCWfOZEPbX1ZbACGgH56jEzIGYEgavZDzXvBT48V7W5G/AoGeP/L9bNG
VH5NydGQ68LvHsWcokXZaE/6Hxt6ow079CFphfR9kSD/84RmtLf6GCWw/1+zuksQ
Ba1TlrsW8e5L6XhoajPVYd2sCfeadnsXEfkv5diKeSkPAO5jftLBpywuz8fGi8Wj
FuvUyBfXn4BAvnLrdF7NaYrO+nZbkFnDSZy9u7rtSZ3P1/L3BT+Alw==
=EdNW
-----END PGP SIGNATURE-----
Re: [PATCH] Fix failing ci caused in r40202
Posted by Kannan <ka...@collab.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Kannan wrote:
>
> As per discussion in the thread [1], attaching the patch herewith.
>
> Log:
> Make a proper fix to resolve some deprecation warnings using
> `svn_path_url_add_component2()' by canonicalizing the base before
> passing to the above method.
>
> [in subversion/libsvn_ra_neon]
>
> * commit.c
> (get_version_url, create_activity, commit_add_dir, commit_add_file
> commit_close_file, add_child, commit_delete_entry): Use
> `svn_path_url_add_component2()'.
> (svn_ra_neon__get_commit_editor, checkout_resource, apply_revprops):
> Canonicalize the base before passing to
> `svn_path_url_add_component2()'.
>
> * props.c
> (svn_ra_neon__get_baseline_info): Canonicalize the base before
> passing to `svn_path_url_add_component2()'.
>
> Found and Suggested by: stsp
> Patch by: Kannan R <ka...@collab.net>
>
> [1]-http://mail-archives.apache.org/mod_mbox/subversion-dev/200912.mbox/%3C4B1DEEE0.6010505@collab.net%3E
>
Any updates on this thread?
- --
Thanks & Regards,
Kannan
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iQEVAwUBSyEKaXlTqcY7ytmIAQJXFgf+NXjR1QyQYx5O5RUxilNj8IAu98KAJ03u
DD7hYQo7iYoGOrnk4AG8QgEgS1x7sOmmih6ZKzkoavPDmPLXpSA/BrcLq9LXQ6LU
2MvCtzNFXU8s3aPp3QLSAXlJ/yPKCkY+yMWlICNQrTHITIXlsgXwIRNMEqioHaPv
5kCpNaW4XYYWK/IldnhcifurGVTDKx9bKscKwTbI4odRMIzLRsFPWqzRwLCulowy
xnHOsi0Mbqj7ZlZqrwFF8tklOIuiZejmrYFx+O5kZhXf3nccjsxODDHDwYq4HxB1
W+sTL0/uKqMkCcGsQTUbhaYsl//vvdI3AQWwPU15K3wOxD8/pB6NPg==
=Igjf
-----END PGP SIGNATURE-----
Re: [PATCH] Fix failing ci caused in r40202-upgrade to new func
Posted by Kannan <ka...@collab.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Julian Foad wrote:
> I (Julian Foad) wrote:
>> Hi Kannan.
>>
>> I cannot process these patches at the moment, sorry. I hope somebody
>> else can.
>
> ... but I did have a quick scan through and they look right.
>
> I should be able to commit these tomorrow.
>
> Thanks for your patience.
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/
iQEVAwUBSy+ZRnlTqcY7ytmIAQJfKAf+O8jjbUeFUTyIG/xQl/JfqondFjjBZb7j
veEXsPX6ryx6oAHVkYiW6o8VDS3jhTosk7IqlunYbOB0TV6Cf7cGOMHgHo1Bi+BE
ixxzM+SM/YQZ2oMh9GpSXz0iGFa49I80RDH6quYAHTiKp001uxcigAHagQICqqZA
bUU+C60Rtpr+bHSigVgFkBSluHXSPygIAoxrK6ZT1Ptv9PkD+cL1EEn5r9+sl+g/
2rcRf0uEFON8FD6J3pJyfe+DH8xdIvaQSMWiMt1xzoNJ9hbj616EZCp3E25wJKV4
1ryOT4LovfMA19RIZcYKh4s2lP4BTaGsJe9oSL611AOmejP7DYv1Hw==
=hUe2
-----END PGP SIGNATURE-----
Re: [PATCH] Fix failing ci caused in r40202-upgrade to new func
Posted by Julian Foad <ju...@wandisco.com>.
I (Julian Foad) wrote:
> Hi Kannan.
>
> I cannot process these patches at the moment, sorry. I hope somebody
> else can.
... but I did have a quick scan through and they look right.
I should be able to commit these tomorrow.
Thanks for your patience.
- Julian
> On Fri, 2009-12-18, Kannan wrote:
> > This patch upgrades ..add_component() to ..add_component2() which
> > depends on the previous uncommitted patch [1] to canonicalize the URLs.
> >
> > [[[
> > Log:
> > Resolve some deprecation warnings.
> >
> > * subversion/libsvn_ra_neon/commit.c
> > (get_version_url, create_activity, commit_add_dir, commit_add_file
> > commit_close_file, add_child, commit_delete_entry): Use
> > `svn_path_url_add_component2()'.
> >
> > Patch by: Kannan R <ka...@collab.net>
> > ]]]
> >
> > [1]-http://mail-archives.apache.org/mod_mbox/subversion-dev/200912.mbox/%3C4B29CF6C.6080308@collab.net%3E
>
>
Re: [PATCH] Fix failing ci caused in r40202-upgrade to new func
Posted by Julian Foad <ju...@wandisco.com>.
Hi Kannan.
I cannot process these patches at the moment, sorry. I hope somebody
else can.
- Julian
On Fri, 2009-12-18, Kannan wrote:
> This patch upgrades ..add_component() to ..add_component2() which
> depends on the previous uncommitted patch [1] to canonicalize the URLs.
>
> [[[
> Log:
> Resolve some deprecation warnings.
>
> * subversion/libsvn_ra_neon/commit.c
> (get_version_url, create_activity, commit_add_dir, commit_add_file
> commit_close_file, add_child, commit_delete_entry): Use
> `svn_path_url_add_component2()'.
>
> Patch by: Kannan R <ka...@collab.net>
> ]]]
>
> [1]-http://mail-archives.apache.org/mod_mbox/subversion-dev/200912.mbox/%3C4B29CF6C.6080308@collab.net%3E
Re: [PATCH] Fix failing ci caused in r40202-upgrade to new func
Posted by Julian Foad <ju...@wandisco.com>.
On Fri, 2009-12-18, Kannan wrote:
> This patch upgrades ..add_component() to ..add_component2() which
> depends on the previous uncommitted patch [1] to canonicalize the URLs.
>
> [[[
> Log:
> Resolve some deprecation warnings.
>
> * subversion/libsvn_ra_neon/commit.c
> (get_version_url, create_activity, commit_add_dir, commit_add_file
> commit_close_file, add_child, commit_delete_entry): Use
> `svn_path_url_add_component2()'.
>
> Patch by: Kannan R <ka...@collab.net>
> ]]]
Committed revision 907615. Thanks for persisting with this patch!
- Julian
> [1]-http://mail-archives.apache.org/mod_mbox/subversion-dev/200912.mbox/%3C4B29CF6C.6080308@collab.net%3E
Re: [PATCH] Fix failing ci caused in r40202-upgrade to new func
Posted by Kannan <ka...@collab.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Kannan wrote:
[..]
This patch upgrades ..add_component() to ..add_component2() which
depends on the previous uncommitted patch [1] to canonicalize the URLs.
[[[
Log:
Resolve some deprecation warnings.
* subversion/libsvn_ra_neon/commit.c
(get_version_url, create_activity, commit_add_dir, commit_add_file
commit_close_file, add_child, commit_delete_entry): Use
`svn_path_url_add_component2()'.
Patch by: Kannan R <ka...@collab.net>
]]]
[1]-http://mail-archives.apache.org/mod_mbox/subversion-dev/200912.mbox/%3C4B29CF6C.6080308@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/
iQEVAwUBSysS6nlTqcY7ytmIAQKmdQf/XPF+FFIIapsA/XrO4X9yAmCp76gS1KpF
pMqhJomM1+o73Eijamg97tAwj0U7dlg1rQJx0bWdz772FsljWS0lhq/DTLIhD+FU
jyIjioU/GEMMVriFjscItCYxdd8VFgmx1/5Va22n/GLp36wAuSAGtCtpYsy6LU/s
75nvxpRpBR6IdHqwt8ZW/LDgwYgV5WQY2kFSBFm3NOsZeXHYZhHnCQc2cf+7lcl9
bT1b9IQ9RBrjwpG2UIYRMV0Hhlb3Nt/igoHprnHIjZ6QPPoPtxcN6mIVjm6Q8anP
ldMaLW8aGlCv03hsqxJLFyweL5TPXZE32A2j6osd/nKhoc6YdPdgvQ==
=XUL1
-----END PGP SIGNATURE-----
Re: [PATCH] Fix failing ci caused in r40202
Posted by Gavin 'Beau' Baumanis <ga...@thespidernet.com>.
Ping.
This thread has received no new comments.
Gavin.
On 23/12/2009, at 01:59 , Kannan wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> I (Kannan) wrote:
> [..]
>>>> (svn_ra_neon__exchange_capabilities): Same.
>>> And this?
>
>> Trying to find them out.
>
> With regard to this change, AFAICS the `activity_collection' value is
> set in `svn_ra_neon__request_dispatch' where its obtained in the body of
> the response as xml(once the session is established). It's assignment to
> the our variable `oc' couldn't be traced (rather implicit) :( Hope it
> should be canonicalized as in the patch, please CMIIW.
>
>>> By the way, in the log message, the reason for this change is no longer
>>> specifically to do with svn_path_url_add_component2(), but rather to fix
>>> violations of our "URLs are canonical" rule.
>>
>> Thank you Julian, will correct it the next time.
>>
>>
>
> - --
> Thanks & Regards,
> Kannan
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.6 (GNU/Linux)
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
>
> iQEVAwUBSzDex3lTqcY7ytmIAQIZLwgAhGpw2V2okB+KPmkptVw62UZOitqJheri
> N5dWqlbT9RXLgQN4gwZBrBLTfphYTeQB9AiTI+NKnoSBJoAsVGFMgOp0bumwwG7l
> UrzxK1giQzmEwPs8gmepCtvYNbV1o1+5XCqIs2pay7eVcf6TFhRA8l+gNs1YG2En
> G6Zi0RNPngmhB7f6jtqy/GwTFv7LddZobniYqr5txHFe2OD/bextekQxXSKtBjDn
> 9/62XDyO7oJLB4xPuULafxkH1xrCcgj7iTe4YCs5FX2Jz3TusdQjpk3O5wZkNQDg
> o9e4sP6gyV7lP5/c6XPK1jr8649xC0b6Ae0DuyvedT9/TEYpLq5OwQ==
> =t3dX
> -----END PGP SIGNATURE-----
Re: [PATCH] Fix failing ci caused in r40202
Posted by Gavin 'Beau' Baumanis <ga...@thespidernet.com>.
Ping.
This thread has received no new comments.
Gavin.
On 23/12/2009, at 01:59 , Kannan wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> I (Kannan) wrote:
> [..]
>>>> (svn_ra_neon__exchange_capabilities): Same.
>>> And this?
>
>> Trying to find them out.
>
> With regard to this change, AFAICS the `activity_collection' value is
> set in `svn_ra_neon__request_dispatch' where its obtained in the body of
> the response as xml(once the session is established). It's assignment to
> the our variable `oc' couldn't be traced (rather implicit) :( Hope it
> should be canonicalized as in the patch, please CMIIW.
>
>>> By the way, in the log message, the reason for this change is no longer
>>> specifically to do with svn_path_url_add_component2(), but rather to fix
>>> violations of our "URLs are canonical" rule.
>>
>> Thank you Julian, will correct it the next time.
>>
>>
>
> - --
> Thanks & Regards,
> Kannan
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.6 (GNU/Linux)
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
>
> iQEVAwUBSzDex3lTqcY7ytmIAQIZLwgAhGpw2V2okB+KPmkptVw62UZOitqJheri
> N5dWqlbT9RXLgQN4gwZBrBLTfphYTeQB9AiTI+NKnoSBJoAsVGFMgOp0bumwwG7l
> UrzxK1giQzmEwPs8gmepCtvYNbV1o1+5XCqIs2pay7eVcf6TFhRA8l+gNs1YG2En
> G6Zi0RNPngmhB7f6jtqy/GwTFv7LddZobniYqr5txHFe2OD/bextekQxXSKtBjDn
> 9/62XDyO7oJLB4xPuULafxkH1xrCcgj7iTe4YCs5FX2Jz3TusdQjpk3O5wZkNQDg
> o9e4sP6gyV7lP5/c6XPK1jr8649xC0b6Ae0DuyvedT9/TEYpLq5OwQ==
> =t3dX
> -----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
Re: [PATCH] Fix failing ci caused in r40202
Posted by Kannan <ka...@collab.net>.
-----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>.
Kannan wrote:
> [[[
> Log:
> Ensure the URLs in libsvn_ra_neon are always canonical.
[...]
> ]]]
Thanks. I found one more problem when I tried to compile it:
subversion/libsvn_ra_neon/options.c:110: error: implicit declaration of
function ‘svn_uri_canonicalize’
I fixed this, by #including "svn_dirent_uri.h".
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.
- Julian
Re: [PATCH] Fix failing ci caused in r40202
Posted by Kannan <ka...@collab.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Julian Foad wrote:
[..]
> In the second place it is used, we know it holds a URL:
>
>> case ELEM_href:
>> /* Special handling for <href> that belongs to the <response> tag. */
>> if (rsrc->href_parent == ELEM_response)
>> return assign_rsrc_url(pc->rsrc, cdata, pc->pool);
>
> That's where we can canonicalize it. And are there any other places
> where the "cdata" variable is known to hold a URL? I'm looking... No,
> there are no others. In all the rest of the places, it is providing a
> generic "property value".
>
> With that fix, I believe the patch will be correct.
Thank you for the comments, Julian. I thought it's better to
canonicalize in the first place, but should be otherwise! Attaching the
updated patch as per your feedback, herewith.
[[[
Log:
Ensure the URLs in libsvn_ra_neon are always canonical.
[ in subversion/libsvn_ra_neon ]
* ra_neon.h
(svn_ra_neon__request_get_location): Update doc string to reflect that
it returns a canonicalized URL.
* util.c
(svn_ra_neon__request_get_location): Canonicalize the 'BASE URL' as
per the rule.
* props.c
(end_element): Same.
* options.c
(end_element): Same.
Found by: stsp
Suggested by: stsp, julianfoad
Patch by: Kannan R <ka...@collab.net>
]]]
- --
Thanks & Regards,
Kannan
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iQEVAwUBS1hA93lTqcY7ytmIAQIKLgf+P5n39GaVhl53zJ4oaEQOVMMX0ovSerqQ
Taakt6wLMuQREhnCoLzivk8phn3zyFXRV4wiqFly94X2wKFcKCX/K3I4nheecM5W
j/+DPQo4MLnFZDh8eJXuI4RpCR6+iP5Vz0wciE2nsVYt3+EJFGtqm+ZQTkgmj2iX
QnuVe55/GfNjIcsrqmJnjhwJ8W/YbKeBUq2iueeUPcC0m6T60WfK6Fy/3aEVVCYn
W/7TshgnFClyLdfAXDF8byrf5+fYDRjH49LBv7IMcd8A4krALCZ4+whYIIQNS2A7
byBGrTdViBtWr5vOO0POQEWwYMX8J9pLc5dNP/3LlzaM8CGReXb2AA==
=Fqzw
-----END PGP SIGNATURE-----
Re: [PATCH] Fix failing ci caused in r40202
Posted by Julian Foad <ju...@wandisco.com>.
On Thu, 2010-01-07, Kannan wrote:
> Tracing these back further, the value gets set in `end_element()'. This
> fixes one more instance of non-canonical URL in `get_version_url()',
> thus fixing all instances of non-canonical URLs AFAICS. Thank you for
> the feedback, Julian. Attaching the updated patch herewith.
Thanks for the patch, Kannan, and sorry for forgetting to deal with it.
Comments below.
> [[[
> Log:
> Ensure the URLs are always canonical.
A minor comment: Let's write that as,
Ensure URLs in libsvn_ra_neon are always canonical.
to make clear what "the URLs" refers to.
> [ in subversion/libsvn_ra_neon ]
>
> * util.c
> (svn_ra_neon__request_get_location): Canonicalize the 'BASE URL' as
> per the rule.
>
> * props.c
> (end_element): Same.
>
> * options.c
> (end_element): Same.
>
> Found by: stsp
> Suggested by: stsp, julianfoad
> Patch by: Kannan R <ka...@collab.net>
> ]]]
> Index: subversion/libsvn_ra_neon/util.c
> ===================================================================
> --- subversion/libsvn_ra_neon/util.c (revision 896759)
> +++ subversion/libsvn_ra_neon/util.c (working copy)
> @@ -1490,5 +1490,5 @@
> apr_pool_t *pool)
> {
> const char *val = ne_get_response_header(request->ne_req, "Location");
> - return val ? apr_pstrdup(pool, val) : NULL;
> + return val ? svn_uri_canonicalize(val, pool) : NULL;
> }
That looks good. I would update its doc string to mention that it
canonicalizes the URL, because presently it just says it returns the
header:
Index: subversion/libsvn_ra_neon/ra_neon.h
===================================================================
--- subversion/libsvn_ra_neon/ra_neon.h (revision 899581)
+++ subversion/libsvn_ra_neon/ra_neon.h (working copy)
@@ -973,6 +973,7 @@
apr_pool_t *pool);
/* Return the Location HTTP header or NULL if none was sent.
+ * (Return a canonical URL even if the header ended with a slash.)
*
* Do allocations in POOL.
*/
> Index: subversion/libsvn_ra_neon/options.c
> ===================================================================
> --- subversion/libsvn_ra_neon/options.c (revision 896759)
> +++ subversion/libsvn_ra_neon/options.c (working copy)
> @@ -107,7 +107,9 @@
> options_ctx_t *oc = baton;
>
> if (state == ELEM_href)
> - oc->activity_coll = svn_string_create_from_buf(oc->cdata, oc->pool);
> + oc->activity_coll = svn_string_create(svn_uri_canonicalize(oc->cdata->data,
> + oc->pool),
> + oc->pool);
>
> return SVN_NO_ERROR;
> }
That looks good, because the "activity_coll" field is always meant to
hold a URL.
> Index: subversion/libsvn_ra_neon/props.c
> ===================================================================
> --- subversion/libsvn_ra_neon/props.c (revision 896759)
> +++ subversion/libsvn_ra_neon/props.c (working copy)
> @@ -359,7 +359,7 @@
> const elem_defn *parent_defn;
> const elem_defn *defn;
> ne_status status;
> - const char *cdata = pc->cdata->data;
> + const char *cdata = svn_uri_canonicalize(pc->cdata->data, pc->pool);
>
> switch (state)
> {
This does not look good. The name of the variable "cdata" makes me think
it is meant to hold any kind of value, not always a URL. ("CDATA" is the
XML term for "any kind of printable data".) Let's look at where the
variable is used:
> 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);
Here is the first place it is used. It holds some kind of status
indication, in XML syntax. We don't want to have called
svn_uri_canonicalize() on this value.
In the second place it is used, we know it holds a URL:
> case ELEM_href:
> /* Special handling for <href> that belongs to the <response> tag. */
> if (rsrc->href_parent == ELEM_response)
> return assign_rsrc_url(pc->rsrc, cdata, pc->pool);
That's where we can canonicalize it. And are there any other places
where the "cdata" variable is known to hold a URL? I'm looking... No,
there are no others. In all the rest of the places, it is providing a
generic "property value".
With that fix, I believe the patch will be correct.
- Julian
Re: [PATCH] Fix failing ci caused in r40202
Posted by Kannan <ka...@collab.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Kannan wrote:
[..]
> [[[
> Log:
> Ensure the URLs are always canonical.
>
> [ in subversion/libsvn_ra_neon ]
>
> * util.c
> (svn_ra_neon__request_get_location): Canonicalize the 'BASE URL' as
> per the rule.
>
> * props.c
> (end_element): Same.
>
> * options.c
> (end_element): Same.
>
> Found by: stsp
> Suggested by: stsp, julianfoad
> Patch by: Kannan R <ka...@collab.net>
> ]]]
>
> If the above patch seems fine, the patch to upgrade ..add_component() to
> ..add_component2() can be found in [1].
>
> [1]-http://mail-archives.apache.org/mod_mbox/subversion-dev/200912.mbox/%3C4B2B12EA.5000208@collab.net%3E
>
Any updates on this thread?
- --
Thanks & Regards,
Kannan
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iQEVAwUBS0xZInlTqcY7ytmIAQLBgAf/dqopEc2tk2A0COS0OXDElKNAWpRGV6eC
zvb3pHjE0GP5Fi3cL2ugJGr/37pxLPonem25RT8Tn0UO8V1i2ONkd5WwnusjsR/F
ZbSvZ/eBcsP0rpY0n4toAzTmYQywJQE7NFT3s0O+oAg/U69Mus78aKfpdymT9rLT
m8CIT/2z/gwJMSlqj0+nh1pfaT5JKlAlndw6yrukjsg2Q3FeeO/43+8HcYsrsbDd
spoGaTdEbXdcscLOtB/NotEvAhPDMQmDP6g0mLyDDv+S5wJ8tPRq5FcN7ycnSzbx
aDP6ajF4ErvTuE0+oZGDGjaJBaggKwnKAbDIhoysV42mX83Q3sd6fw==
=iZbp
-----END PGP SIGNATURE-----
Re: [PATCH] Fix failing ci caused in r40202
Posted by Gavin Beau Baumanis <ga...@thespidernet.com>.
Apart from Kannan pinging his own update;
This submission update has received no new comments.
Gavin "Beau" Baumanis
On 07/01/2010, at 11:40 PM, Kannan wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Julian Foad wrote:
> [..]
>> I looked for assignments to the "activity_coll" field, and found it in
>> the function end_element(), where it copies it from the "cdata" field. I
>> think that is where the canonicalization should be done.
>
> My bad, misinterpreted `end_element()' as neon's method!
>
>> And here:
>>
>>> * props.c
>>> (svn_ra_neon__get_baseline_info, svn_ra_neon__get_one_prop): Same.
>>
>> The function ..._get_baseline_info() gets its bc_url from a hash created
>> by the function svn_ra_neon__get_baseline_props(), so I think that's
>> where the change needs to be.
>>
>> The function "get_one_prop" is not specifically for getting a baseline
>> URL, it is a generic function for getting any property, so I think you
>> should not be changing that function.
>
> Tracing these back further, the value gets set in `end_element()'. This
> fixes one more instance of non-canonical URL in `get_version_url()',
> thus fixing all instances of non-canonical URLs AFAICS. Thank you for
> the feedback, Julian. Attaching the updated patch herewith.
>
> [[[
> Log:
> Ensure the URLs are always canonical.
>
> [ in subversion/libsvn_ra_neon ]
>
> * util.c
> (svn_ra_neon__request_get_location): Canonicalize the 'BASE URL' as
> per the rule.
>
> * props.c
> (end_element): Same.
>
> * options.c
> (end_element): Same.
>
> Found by: stsp
> Suggested by: stsp, julianfoad
> Patch by: Kannan R <ka...@collab.net>
> ]]]
>
> If the above patch seems fine, the patch to upgrade ..add_component() to
> ..add_component2() can be found in [1].
>
> [1]-http://mail-archives.apache.org/mod_mbox/subversion-dev/200912.mbox/%3C4B2B12EA.5000208@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/
>
> iQEVAwUBS0XWUHlTqcY7ytmIAQIRnwf+LBmMhAKRPwCbWO2tmtzkhuQ9Bx7P1wlK
> lGFhyhUf82InW6uEYqMP9UaihxPBeSDr5Cqec92HN8xV/O1j0366pBdSJ/1sXvrn
> hg8U3toh9GSbIm5FqQ9WDWgOjYPG0VSr9pz/rFi+6fL9DkrSWqfcfHLZOJ4YIYQW
> jPpLhRFVuypQDxpeoOpSuh3PHPg+zovaEsGCNepfqpwiVITvK3B74r8zq/sYd6SL
> MF56QKDjgfFJAbD8W9SlA+y6aYJQm1ZxUMV9B2FdBSx8hv+i7SM3U6UTinQizUs8
> 8zutd2q7i7TA1jyqyZGRKx+4pProUbeIO1mUsRJrpwjaYlzssjRzRA==
> =8XvR
> -----END PGP SIGNATURE-----
> Index: subversion/libsvn_ra_neon/util.c
> ===================================================================
> --- subversion/libsvn_ra_neon/util.c (revision 896759)
> +++ subversion/libsvn_ra_neon/util.c (working copy)
> @@ -1490,5 +1490,5 @@
> apr_pool_t *pool)
> {
> const char *val = ne_get_response_header(request->ne_req, "Location");
> - return val ? apr_pstrdup(pool, val) : NULL;
> + return val ? svn_uri_canonicalize(val, pool) : NULL;
> }
> Index: subversion/libsvn_ra_neon/options.c
> ===================================================================
> --- subversion/libsvn_ra_neon/options.c (revision 896759)
> +++ subversion/libsvn_ra_neon/options.c (working copy)
> @@ -107,7 +107,9 @@
> options_ctx_t *oc = baton;
>
> if (state == ELEM_href)
> - oc->activity_coll = svn_string_create_from_buf(oc->cdata, oc->pool);
> + oc->activity_coll = svn_string_create(svn_uri_canonicalize(oc->cdata->data,
> + oc->pool),
> + oc->pool);
>
> return SVN_NO_ERROR;
> }
> Index: subversion/libsvn_ra_neon/props.c
> ===================================================================
> --- subversion/libsvn_ra_neon/props.c (revision 896759)
> +++ subversion/libsvn_ra_neon/props.c (working copy)
> @@ -359,7 +359,7 @@
> const elem_defn *parent_defn;
> const elem_defn *defn;
> ne_status status;
> - const char *cdata = pc->cdata->data;
> + const char *cdata = svn_uri_canonicalize(pc->cdata->data, pc->pool);
>
> switch (state)
> {
Re: [PATCH] Fix failing ci caused in r40202
Posted by Kannan <ka...@collab.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Julian Foad wrote:
[..]
> I looked for assignments to the "activity_coll" field, and found it in
> the function end_element(), where it copies it from the "cdata" field. I
> think that is where the canonicalization should be done.
My bad, misinterpreted `end_element()' as neon's method!
> And here:
>
>> * props.c
>> (svn_ra_neon__get_baseline_info, svn_ra_neon__get_one_prop): Same.
>
> The function ..._get_baseline_info() gets its bc_url from a hash created
> by the function svn_ra_neon__get_baseline_props(), so I think that's
> where the change needs to be.
>
> The function "get_one_prop" is not specifically for getting a baseline
> URL, it is a generic function for getting any property, so I think you
> should not be changing that function.
Tracing these back further, the value gets set in `end_element()'. This
fixes one more instance of non-canonical URL in `get_version_url()',
thus fixing all instances of non-canonical URLs AFAICS. Thank you for
the feedback, Julian. Attaching the updated patch herewith.
[[[
Log:
Ensure the URLs are always canonical.
[ in subversion/libsvn_ra_neon ]
* util.c
(svn_ra_neon__request_get_location): Canonicalize the 'BASE URL' as
per the rule.
* props.c
(end_element): Same.
* options.c
(end_element): Same.
Found by: stsp
Suggested by: stsp, julianfoad
Patch by: Kannan R <ka...@collab.net>
]]]
If the above patch seems fine, the patch to upgrade ..add_component() to
..add_component2() can be found in [1].
[1]-http://mail-archives.apache.org/mod_mbox/subversion-dev/200912.mbox/%3C4B2B12EA.5000208@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/
iQEVAwUBS0XWUHlTqcY7ytmIAQIRnwf+LBmMhAKRPwCbWO2tmtzkhuQ9Bx7P1wlK
lGFhyhUf82InW6uEYqMP9UaihxPBeSDr5Cqec92HN8xV/O1j0366pBdSJ/1sXvrn
hg8U3toh9GSbIm5FqQ9WDWgOjYPG0VSr9pz/rFi+6fL9DkrSWqfcfHLZOJ4YIYQW
jPpLhRFVuypQDxpeoOpSuh3PHPg+zovaEsGCNepfqpwiVITvK3B74r8zq/sYd6SL
MF56QKDjgfFJAbD8W9SlA+y6aYJQm1ZxUMV9B2FdBSx8hv+i7SM3U6UTinQizUs8
8zutd2q7i7TA1jyqyZGRKx+4pProUbeIO1mUsRJrpwjaYlzssjRzRA==
=8XvR
-----END PGP SIGNATURE-----
Re: [PATCH] Fix failing ci caused in r40202
Posted by Julian Foad <ju...@btopenworld.com>.
On Tue, 2009-12-22, Kannan wrote:
> I (Kannan) wrote:
> [..]
> >>> (svn_ra_neon__exchange_capabilities): Same.
> >> And this?
>
> > Trying to find them out.
>
> With regard to this change, AFAICS the `activity_collection' value is
> set in `svn_ra_neon__request_dispatch' where its obtained in the body of
> the response as xml(once the session is established). It's assignment to
> the our variable `oc' couldn't be traced (rather implicit) :( Hope it
> should be canonicalized as in the patch, please CMIIW.
I looked for assignments to the "activity_coll" field, and found it in
the function end_element(), where it copies it from the "cdata" field. I
think that is where the canonicalization should be done.
And here:
> * props.c
> (svn_ra_neon__get_baseline_info, svn_ra_neon__get_one_prop): Same.
The function ..._get_baseline_info() gets its bc_url from a hash created
by the function svn_ra_neon__get_baseline_props(), so I think that's
where the change needs to be.
The function "get_one_prop" is not specifically for getting a baseline
URL, it is a generic function for getting any property, so I think you
should not be changing that function.
- Julian
Re: [PATCH] Fix failing ci caused in r40202
Posted by Kannan <ka...@collab.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
I (Kannan) wrote:
[..]
>>> (svn_ra_neon__exchange_capabilities): Same.
>> And this?
> Trying to find them out.
With regard to this change, AFAICS the `activity_collection' value is
set in `svn_ra_neon__request_dispatch' where its obtained in the body of
the response as xml(once the session is established). It's assignment to
the our variable `oc' couldn't be traced (rather implicit) :( Hope it
should be canonicalized as in the patch, please CMIIW.
>> By the way, in the log message, the reason for this change is no longer
>> specifically to do with svn_path_url_add_component2(), but rather to fix
>> violations of our "URLs are canonical" rule.
>
> Thank you Julian, will correct it the next time.
>
>
- --
Thanks & Regards,
Kannan
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iQEVAwUBSzDex3lTqcY7ytmIAQIZLwgAhGpw2V2okB+KPmkptVw62UZOitqJheri
N5dWqlbT9RXLgQN4gwZBrBLTfphYTeQB9AiTI+NKnoSBJoAsVGFMgOp0bumwwG7l
UrzxK1giQzmEwPs8gmepCtvYNbV1o1+5XCqIs2pay7eVcf6TFhRA8l+gNs1YG2En
G6Zi0RNPngmhB7f6jtqy/GwTFv7LddZobniYqr5txHFe2OD/bextekQxXSKtBjDn
9/62XDyO7oJLB4xPuULafxkH1xrCcgj7iTe4YCs5FX2Jz3TusdQjpk3O5wZkNQDg
o9e4sP6gyV7lP5/c6XPK1jr8649xC0b6Ae0DuyvedT9/TEYpLq5OwQ==
=t3dX
-----END PGP SIGNATURE-----
Re: [PATCH] Fix failing ci caused in r40202
Posted by Kannan <ka...@collab.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Julian Foad wrote:
[..]
> Great.
>
> Did you look to see if there are any other similar calls in Subversion
> where we make the same mistake?
Couldn't find any, as of now.
[..]
>> * props.c
>> (svn_ra_neon__get_baseline_info, svn_ra_neon__get_one_prop): Same.
>
> Now let's move on to these places. Can you trace these back further?
>> * options.c
>> (svn_ra_neon__exchange_capabilities): Same.
>
> And this?
Trying to find them out.
> By the way, in the log message, the reason for this change is no longer
> specifically to do with svn_path_url_add_component2(), but rather to fix
> violations of our "URLs are canonical" rule.
Thank you Julian, will correct it the next time.
- --
Thanks & Regards,
Kannan
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iQEVAwUBSzDXS3lTqcY7ytmIAQLq8Af/RJHgaXRD0jiqGpQeP8DrtVijykfPFJa8
C0Jr3u+AHXUicELFtep1incvr12M3yYFgC7jPPrs++bw0pgbRA/VlvS5Po8irvOd
IV12mJEx67+AwbuCXwISly6c42bvNzHFzF4QJuX6EH1bVDNkTPsfCNQ1gkM7Iu1f
Dj6cyrayr2u04faAhOYNkAqtG4u0xmmnRJYk/Tw659bpXHHcocjEvJXu+wSL0vhT
ZPzv4+sv+HZ50z9rxiRmeTZWTKMQgFquz24UaiouuOLEQ9TlYH1HXBGaEKIKoL9z
eFXuCQnT9xoMbgdyO1smmQ5cHeLj9A4OnAhVj+sVytyiM0KzggkDyg==
=LmD1
-----END PGP SIGNATURE-----
Re: [PATCH] Fix failing ci caused in r40202
Posted by Julian Foad <ju...@btopenworld.com>.
On Tue, 2009-12-22, Kannan wrote:
> Julian Foad wrote:
> [..]
> >> No, it should create a canonical URL. The fix we need is inside
> >> svn_ra_neon__request_get_location(), or at least we need to look inside
> >> that function to see where the non-canonical URL is coming from, and
> >> maybe trace further back to find the cause of the problem.
> >
> > Oops, typo. Meant to ask for canonical. I'll give a shot to try where
> > the non-canonicalized is possibly created in ..get_location().
>
> `svn_ra_neon__request_get_location()' returns the non-canonical URL
> which is obtained from the `ne_get_response_header()'(third party
> function).
Great.
Did you look to see if there are any other similar calls in Subversion
where we make the same mistake?
> Attaching the updated patch to canonicalize the URLs.
>
> [[[
> Log:
> Ensure the URLs are canonical before passing to the new
> `svn_path_url_add_component2()'.
>
> [ in subversion/libsvn_ra_neon ]
>
> * util.c
> (svn_ra_neon__request_get_location): Canonicalize the 'BASE URL' as
> `svn_path_url_add_component2()' won't handle it.
>
> * props.c
> (svn_ra_neon__get_baseline_info, svn_ra_neon__get_one_prop): Same.
Now let's move on to these places. Can you trace these back further?
> * options.c
> (svn_ra_neon__exchange_capabilities): Same.
And this?
By the way, in the log message, the reason for this change is no longer
specifically to do with svn_path_url_add_component2(), but rather to fix
violations of our "URLs are canonical" rule.
- Julian
Re: [PATCH] Fix failing ci caused in r40202
Posted by Kannan <ka...@collab.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Julian Foad wrote:
[..]
>> No, it should create a canonical URL. The fix we need is inside
>> svn_ra_neon__request_get_location(), or at least we need to look inside
>> that function to see where the non-canonical URL is coming from, and
>> maybe trace further back to find the cause of the problem.
>
> Oops, typo. Meant to ask for canonical. I'll give a shot to try where
> the non-canonicalized is possibly created in ..get_location().
`svn_ra_neon__request_get_location()' returns the non-canonical URL
which is obtained from the `ne_get_response_header()'(third party
function). Attaching the updated patch to canonicalize the URLs.
[[[
Log:
Ensure the URLs are canonical before passing to the new
`svn_path_url_add_component2()'.
[ in subversion/libsvn_ra_neon ]
* util.c
(svn_ra_neon__request_get_location): Canonicalize the 'BASE URL' as
`svn_path_url_add_component2()' won't handle it.
* props.c
(svn_ra_neon__get_baseline_info, svn_ra_neon__get_one_prop): Same.
* options.c
(svn_ra_neon__exchange_capabilities): Same.
Found by: stsp
Suggested by: stsp, julianfoad
Patch by: Kannan R <ka...@collab.net>
]]]
- --
Thanks & Regards,
Kannan
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iQEVAwUBSzDONnlTqcY7ytmIAQJGfwf9FfnMJJbYYEg3uimSYpvv2fOjwM5pf9DL
A7tWd3UxPq4gHza2mZYoXRZELnVo9xbKGozoxpQLUqcVmrS9dDZd+C/SUtypxMMz
QwNyC9VKre2DVvI9n9+VSQNU11eAwIVf86OHyln4fBpmqf9czKkqcjxphGntrhSa
iGpKHgFnILekScqIgvsqVu9Rs1tr89daQnlQVaDtHo9UVTm3MvyuOshZLLIEezUA
LmA1EyB0V/Y/uiE3JgXfY7hXm7o/b0fF1coQ9UDeqkR/Z8zADHh64y6dClxpKhVM
SU9e95JTNG1+XXmoCg1NC9VsOuQNDJRphEx/FpdSHWpmFa9U+Chfjw==
=GGUJ
-----END PGP SIGNATURE-----
Re: [PATCH] Fix failing ci caused in r40202
Posted by Kannan <ka...@collab.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Julian Foad wrote:
[..]
> OK. The policy in Subversion is that the paths and URLs passed in and
> out of all APIs are canonical unless otherwise stated. Therefore we
> should make do_checkout() set its "locn" output parameter to a canonical
> URL.
>
>>> If the former, we should look for all other uses of ne_uri_parse() and
>>> canonicalize after them as well. If the latter, we should fix the
>>> do_checkout() function and look for all other uses of
>>> svn_ra_neon__request_get_location().
>
> Oops - when I wrote that, I was thinking
> svn_ra_neon__request_get_location() was an external (non-Subversion)
> function, but it is a Subversion function so it should return a
> canonical URL.
>
>> Does this imply to make `svn_ra_neon__request_get_location()' create a
>> non-canonical URL by default?
>
> No, it should create a canonical URL. The fix we need is inside
> svn_ra_neon__request_get_location(), or at least we need to look inside
> that function to see where the non-canonical URL is coming from, and
> maybe trace further back to find the cause of the problem.
Oops, typo. Meant to ask for canonical. I'll give a shot to try where
the non-canonicalized is possibly created in ..get_location().
>>> (What does the non-canonical URL or URI look like?)
>> http://localhost/repos/devrepo/!svn/wrk/b4ec0dff-9abf-42ee-8860-a6a9630218e4/
>>
>> (which is generated by do_checkout() and parse.path value would be:
>>
>> /repos/devrepo/!svn/wrk/b4ec0dff-9abf-42ee-8860-a6a9630218e4/ )
>
> Thanks.
Thank you.
- --
Thanks & Regards,
Kannan
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iQEVAwUBSzCygXlTqcY7ytmIAQIEtwf+I6hYI5boCQk5bL7Fw9sxatDn7V6YD3t8
LIqRWaxlWCmQ6vAdPJY2LqzRAaxxgQD8yLi09ngWv1itPxqiJak0Gd8OjRoof5LK
F9XO6XZ6kx221AGhmSLrFaSiVlbREOwbYve1U7OF3eY7QocK6X5uN1EINlsQJJSd
rTfGLvQte3XFj9p+9vIcYLKMuiKdw0IcaMarSakg+DodMOcGsTXSyaHKZr/V8A98
BpmnJ0H7NJrnmvTVllFCgR6O3SeMKaeklnjClnS3Zg+3Gw+cJKOP0+6WG+k5lLvt
bIO7jZNoC8QXEPUGpW1bII6vJWx2p17+OK4hYqLuYt7tYGWQeM7jFg==
=W9M9
-----END PGP SIGNATURE-----
Re: [PATCH] Fix failing ci caused in r40202
Posted by Julian Foad <ju...@btopenworld.com>.
Kannan wrote:
> Julian Foad wrote:
> > do_checkout():
> > ...
> > *locn = svn_ra_neon__request_get_location(request, pool);
> >
> > checkout_resource():
> > do_checkout(cc, rsrc->vsn_url, allow_404, token, &code, &locn, pool);
> > ne_uri_parse(locn, &parse);
> > rsrc->wr_url = svn_uri_canonicalize(parse.path, rsrc->pool);
> >
> > Does the "ne_uri_parse" function create a non-canonical ".path", or does
> > the do_checkout() function create a non-canonical URL?
>
> `do_checkout()' creates the non-canonical URL, after which the scheme,
> host, port and path of the URL are stored as separate members in 'parse'
> using `ne_uri_parse()'.
OK. The policy in Subversion is that the paths and URLs passed in and
out of all APIs are canonical unless otherwise stated. Therefore we
should make do_checkout() set its "locn" output parameter to a canonical
URL.
> > If the former, we should look for all other uses of ne_uri_parse() and
> > canonicalize after them as well. If the latter, we should fix the
> > do_checkout() function and look for all other uses of
> > svn_ra_neon__request_get_location().
Oops - when I wrote that, I was thinking
svn_ra_neon__request_get_location() was an external (non-Subversion)
function, but it is a Subversion function so it should return a
canonical URL.
> Does this imply to make `svn_ra_neon__request_get_location()' create a
> non-canonical URL by default?
No, it should create a canonical URL. The fix we need is inside
svn_ra_neon__request_get_location(), or at least we need to look inside
that function to see where the non-canonical URL is coming from, and
maybe trace further back to find the cause of the problem.
> > (What does the non-canonical URL or URI look like?)
>
> http://localhost/repos/devrepo/!svn/wrk/b4ec0dff-9abf-42ee-8860-a6a9630218e4/
>
> (which is generated by do_checkout() and parse.path value would be:
>
> /repos/devrepo/!svn/wrk/b4ec0dff-9abf-42ee-8860-a6a9630218e4/ )
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:
[..]
> Hi Kannan.
>
> With your patch, the code in "commit.c" now looks like this (ignoring
> all the error handling):
>
> do_checkout():
> ...
> *locn = svn_ra_neon__request_get_location(request, pool);
>
> checkout_resource():
> do_checkout(cc, rsrc->vsn_url, allow_404, token, &code, &locn, pool);
> ne_uri_parse(locn, &parse);
> rsrc->wr_url = svn_uri_canonicalize(parse.path, rsrc->pool);
>
> Does the "ne_uri_parse" function create a non-canonical ".path", or does
> the do_checkout() function create a non-canonical URL?
`do_checkout()' creates the non-canonical URL, after which the scheme,
host, port and path of the URL are stored as separate members in 'parse'
using `ne_uri_parse()'.
> If the former, we should look for all other uses of ne_uri_parse() and
> canonicalize after them as well. If the latter, we should fix the
> do_checkout() function and look for all other uses of
> svn_ra_neon__request_get_location().
Does this imply to make `svn_ra_neon__request_get_location()' create a
non-canonical URL by default?
> (What does the non-canonical URL or URI look like?)
http://localhost/repos/devrepo/!svn/wrk/b4ec0dff-9abf-42ee-8860-a6a9630218e4/
(which is generated by do_checkout() and parse.path value would be:
/repos/devrepo/!svn/wrk/b4ec0dff-9abf-42ee-8860-a6a9630218e4/ )
- --
Thanks & Regards,
Kannan
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iQEVAwUBSzCpunlTqcY7ytmIAQIu5Qf+ImE68BJEJ2nOxMzpo0s/bfR8wE2QORip
F9PK1rkZTPMwYAS5aLnkTgnXFu2wtfk8ZGEHdbEaQYoieQ4VypqKjQ1OPhAVY2K9
JUsCtDzrbUDtBbp/wXZptiCbnoxRFty2nSahNBcz0A+yS9Naxqx2mbfM9HW06v5C
72u65407Q6lXFPSzG1/QVLFuNSQseQkmZXzesHnnbfij5CGUa8xYwYLU7sX5OzHM
P+YQqQrFDLgL2dlU2NeWvJxWPookOklNaC/vMhndFWfqwvcMU9VWOl5kHP4CX3Fp
LveUAMN70XhHOkQ/ILRDINYUY2ev0E30OGnxrYbhJ7tv/UBr60Bxwg==
=C9YY
-----END PGP SIGNATURE-----
Re: [PATCH] Fix failing ci caused in r40202
Posted by Julian Foad <ju...@btopenworld.com>.
On Thu, 2009-12-17, Kannan wrote:
> Julian Foad wrote:
> [..]
> > It would be great if you could trace the calls back to wherever the
> > non-canonical paths are generated, and fix them at that point.
>
> Thank you Julian, for the comments. Attached herewith is the patch to
> canonicalize the URLs. If this seems fine, I shall send the patch to
> upgrade ..add_component() to ..add_component2().
>
> [[[
> Log:
> Ensure the URLs are canonical before passing to the new
> `svn_path_url_add_component2()'.
>
> [ in subversion/libsvn_ra_neon ]
>
> * commit.c
> (checkout_resource): Canonicalize the 'BASE' as
> `svn_path_url_add_component2()' won't handle it.
>
> * props.c
> (svn_ra_neon__get_baseline_info, svn_ra_neon__get_one_prop): Same.
>
> * options.c
> (svn_ra_neon__exchange_capabilities): Same.
Hi Kannan.
With your patch, the code in "commit.c" now looks like this (ignoring
all the error handling):
do_checkout():
...
*locn = svn_ra_neon__request_get_location(request, pool);
checkout_resource():
do_checkout(cc, rsrc->vsn_url, allow_404, token, &code, &locn, pool);
ne_uri_parse(locn, &parse);
rsrc->wr_url = svn_uri_canonicalize(parse.path, rsrc->pool);
Does the "ne_uri_parse" function create a non-canonical ".path", or does
the do_checkout() function create a non-canonical URL?
If the former, we should look for all other uses of ne_uri_parse() and
canonicalize after them as well. If the latter, we should fix the
do_checkout() function and look for all other uses of
svn_ra_neon__request_get_location().
(What does the non-canonical URL or URI look like?)
- Julian
Re: [PATCH] Fix failing ci caused in r40202
Posted by Kannan <ka...@collab.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Julian Foad wrote:
[..]
> It would be great if you could trace the calls back to wherever the
> non-canonical paths are generated, and fix them at that point.
Thank you Julian, for the comments. Attached herewith is the patch to
canonicalize the URLs. If this seems fine, I shall send the patch to
upgrade ..add_component() to ..add_component2().
[[[
Log:
Ensure the URLs are canonical before passing to the new
`svn_path_url_add_component2()'.
[ in subversion/libsvn_ra_neon ]
* commit.c
(checkout_resource): Canonicalize the 'BASE' as
`svn_path_url_add_component2()' won't handle it.
* props.c
(svn_ra_neon__get_baseline_info, svn_ra_neon__get_one_prop): Same.
* options.c
(svn_ra_neon__exchange_capabilities): Same.
Found by: stsp
Suggested by: stsp,julianfoad
Patch by: Kannan R <ka...@collab.net>
]]]
- --
Thanks & Regards,
Kannan
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iQEVAwUBSynPbHlTqcY7ytmIAQJf7QgAoHk0HePERuKpKd9fwV93r2S7818zRx9U
4RIsHab++HGX7G4JKv3xp0nL+yqP2nHeCV9Gmfxr3vDppgxGFzsFyQf14WGTNFsK
rX0V9p7Mis2uqcRPon8+DFeBKikG9PHSFkhKiiDhTTIlQ2re7+ZVHjPyTlLDf4W6
szs5Kw9YzZrVjdPifB1sOD9tuAxTwiTgpqJGHiEnmV6DWKi73mjMOYPYpEGuV+6o
1xg3nv6sEUxJBmehFhnEY6/HS0WKVMgXGuXEYUD60iVDbhcWoRzXGboVMa7IEKFt
pve8hYvUrR9fGToT3V7FvIFk6SQ6tt7tYfqoUR5PmN2yiPbypc+l1A==
=PAQw
-----END PGP SIGNATURE-----
Re: [PATCH] Fix failing ci caused in r40202
Posted by Julian Foad <ju...@wandisco.com>.
On Tue, 2009-12-08, Kannan wrote:
> Make a proper fix to resolve some deprecation warnings using
> `svn_path_url_add_component2()' by canonicalizing the base before
> passing to the above method.
Please split this patch into its two logical changes:
1. Ensure paths are canonical.
2. Upgrade ...add_component() to ...2().
> [in subversion/libsvn_ra_neon]
>
> * commit.c
> (get_version_url, create_activity, commit_add_dir, commit_add_file
> commit_close_file, add_child, commit_delete_entry): Use
> `svn_path_url_add_component2()'.
> (svn_ra_neon__get_commit_editor, checkout_resource, apply_revprops):
> Canonicalize the base before passing to
> `svn_path_url_add_component2()'.
>
> * props.c
> (svn_ra_neon__get_baseline_info): Canonicalize the base before
> passing to `svn_path_url_add_component2()'.
In subversion/libsvn_ra_neon/commit.c:
> @@ -1389,7 +1388,7 @@
> vcc, NULL,
> &svn_ra_neon__checked_in_prop, pool));
> baseline_rsrc.pool = pool;
> - baseline_rsrc.vsn_url = baseline_url->data;
> + baseline_rsrc.vsn_url = svn_uri_canonicalize(baseline_url->data, pool);
This is the wrong place to canonicalize the path. This path is coming
from the function svn_ra_neon__get_one_prop() so it should already be
canonical when it comes out of there.
> @@ -1452,6 +1451,8 @@
> /* ### should we perform an OPTIONS to validate the server we're about
> ### to talk to? */
>
> + cc->ras->act_coll = svn_uri_canonicalize(cc->ras->act_coll, pool);
> +
Same here... this should be made canonical where it is generated, before
it comes in to this function.
It would be great if you could trace the calls back to wherever the
non-canonical paths are generated, and fix them at that point.
Thanks,
- Julian