You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Stefan Sperling <st...@elego.de> on 2009/10/26 12:57:11 UTC
r40202 broke commit over ra_neon (Re: svn commit: r40202 -
trunk/subversion/libsvn_ra_neon)
On Fri, Oct 23, 2009 at 08:59:04AM -0700, Julian Foad wrote:
> Author: julianfoad
> Date: Fri Oct 23 08:59:03 2009
> New Revision: 40202
>
> Log:
> Use new dirent/URI/path functions to resolve some deprecation warnings.
> Also fix a bit of indentation.
>
> * subversion/libsvn_ra_neon/commit.c
> (get_version_url, create_activity, commit_delete_entry, commit_add_dir,
> commit_add_file, commit_close_file): Use `svn_path_url_add_component2()'.
> (add_child): Use `svn_path_url_add_component2()' and `svn_uri_join()'.
> (checkout_resource): Use `svn_relpath_local_style()'.
> (get_child_tokens): Use `svn_uri_is_child()'.
> @@ -325,8 +325,8 @@ static svn_error_t * create_activity(com
> the activity, and create the activity. The URL for our activity
> will be ACTIVITY_COLL/UUID */
> SVN_ERR(get_activity_collection(cc, &activity_collection, FALSE, pool));
> - url = svn_path_url_add_component(activity_collection->data,
> - uuid_buf, pool);
> + url = svn_path_url_add_component2(activity_collection->data,
> + uuid_buf, pool);
> SVN_ERR(svn_ra_neon__simple_request(&code, cc->ras,
> "MKACTIVITY", url, NULL, NULL,
> 201 /* Created */,
I had to revert the above hunk in r40224, to fix the following
assertion failure during commit over ra_neon:
(gdb) bt
#0 0x0c264451 in kill () from /usr/lib/libc.so.51.1
#1 0x0c2bbdb3 in abort () at /usr/src/lib/libc/stdlib/abort.c:68
#2 0x0c243c5b in __assert2 (file=0x25d608c2 "subversion/libsvn_subr/path.c",
line=100, func=0x25d60884 "svn_path_join",
failedexpr=0x25d608a0 "svn_path_is_canonical(base, pool)")
at /usr/src/lib/libc/gen/assert.c:52
#3 0x05d81be6 in svn_path_join (base=0x7ea78a48 "/repos/svn/!svn/act/",
component=0x7ea78898 "bb3146ba-c229-11de-9977-6303f13ecd5f",
pool=0x8b5bd018) at subversion/libsvn_subr/path.c:100
#4 0x05d83115 in svn_path_url_add_component2 (
url=0x7ea78a48 "/repos/svn/!svn/act/",
component=0x7ea78898 "bb3146ba-c229-11de-9977-6303f13ecd5f",
pool=0x8b5bd018) at subversion/libsvn_subr/path.c:927
#5 0x01f331f6 in create_activity (cc=0x7ea78800, pool=0x8b5bd018)
at subversion/libsvn_ra_neon/commit.c:328
#6 0x01f35400 in svn_ra_neon__get_commit_editor (session=0x8347da38,
editor=0xcfbe6dcc, edit_baton=0xcfbe6dc8, revprop_table=0x7ea78670,
callback=0x7a4dcf3 <svn_client__commit_callback>,
callback_baton=0x7ea787f8, lock_tokens=0x8347d7a8, keep_locks=0,
pool=0x8b5bd018) at subversion/libsvn_ra_neon/commit.c:1477
#7 0x06a1ce04 in svn_ra_get_commit_editor3 (session=0x8347da38,
editor=0xcfbe6dcc, edit_baton=0xcfbe6dc8, revprop_table=0x7ea78670,
callback=0x7a4dcf3 <svn_client__commit_callback>,
callback_baton=0x7ea787f8, lock_tokens=0x8347d7a8, keep_locks=0,
pool=0x8b5bd018) at subversion/libsvn_ra/ra_loader.c:586
#8 0x07a48d1e in get_ra_editor (ra_session=0xcfbe6dc4, editor=0xcfbe6dcc,
edit_baton=0xcfbe6dc8, ctx=0x8a5dc718,
base_url=0x8347d760 "https://svn.collab.net/repos/svn/trunk/subversion/libsvn_subr", base_dir=0x852ae280 "/home/stsp/svn/svn-trunk",
log_msg=0x8347d610 "Resolve \"format not a string literal and no format arguments found\" warning.\n\n* subversion/libsvn_subr/io.c\n (do_io_file_wrapper_cleanup): Add the format specifier \"%s\", which\n fixes the warning.\n\nPa"..., commit_items=0x84361fb0, revprop_table=0x0, commit_info_p=0xcfbe6e30,
is_commit=1, lock_tokens=0x8347d7a8, keep_locks=0, pool=0x8b5bd018)
at subversion/libsvn_client/commit.c:641
#9 0x07a4a832 in svn_client_commit4 (commit_info_p=0xcfbe6e30,
targets=0x852ad778, depth=svn_depth_infinity, keep_locks=0,
keep_changelists=0, changelists=0x8a5dc160, revprop_table=0x0,
ctx=0x8a5dc718, pool=0x8b5bd018) at subversion/libsvn_client/commit.c:1646
#10 0x1c006a49 in svn_cl__commit (os=0x8b5bd1c0, baton=0xcfbe6f1c,
pool=0x8b5bd018) at subversion/svn/commit-cmd.c:119
#11 0x1c010925 in main (argc=4, argv=0xcfbe7110) at subversion/svn/main.c:2195
(gdb) up
#1 0x0c2bbdb3 in abort () at /usr/src/lib/libc/stdlib/abort.c:68
68 (void)kill(getpid(), SIGABRT);
(gdb) up
#2 0x0c243c5b in __assert2 (file=0x25d608c2 "subversion/libsvn_subr/path.c",
line=100, func=0x25d60884 "svn_path_join",
failedexpr=0x25d608a0 "svn_path_is_canonical(base, pool)")
at /usr/src/lib/libc/gen/assert.c:52
52 abort();
(gdb) up
#3 0x05d81be6 in svn_path_join (base=0x7ea78a48 "/repos/svn/!svn/act/",
component=0x7ea78898 "bb3146ba-c229-11de-9977-6303f13ecd5f",
pool=0x8b5bd018) at subversion/libsvn_subr/path.c:100
100 assert(svn_path_is_canonical(base, pool));
(gdb)
The problem is that base has a trailing slash, so "/repos/svn/!svn/act/"
is not considered canonical.
The old version canonicalised the URL while the new version does not:
const char *
svn_path_url_add_component(const char *url,
const char *component,
apr_pool_t *pool)
{
/* URL can have trailing '/' */
url = svn_path_canonicalize(url, pool);
return svn_path_url_add_component2(url, component, pool);
}
const char *
svn_path_url_add_component2(const char *url,
const char *component,
apr_pool_t *pool)
{
/* = svn_path_uri_encode() but without always copying */
component = uri_escape(component, uri_char_validity, pool);
return svn_path_join(url, component, pool);
}
The docstring of the new version documents this difference, so this
was done on purpose.
To fix this properly, we need to canonicalise the base somewhere.
Please also check any other add_component->add_component2 upgrades
made in r40202 for similar issues.
Thanks,
Stefan
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2411374
Re: r40202 broke commit over ra_neon (Re: svn commit: r40202 - trunk/subversion/libsvn_ra_neon)
Posted by Kannan <ka...@collab.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
> Stefan Sperling wrote:
>> The problem is that base has a trailing slash, so "/repos/svn/!svn/act/"
>> is not considered canonical.
[..]
>> The old version canonicalised the URL while the new version does not:
>> To fix this properly, we need to canonicalise the base somewhere.
>> Please also check any other add_component->add_component2 upgrades
>> made in r40202 for similar issues.
> Attached herewith is the patch which fixes the failing commit owing to
> the above case.
>
> [[[
> 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; and a minor indentation fix.
>
> [in subversion/libsvn_ra_neon]
>
> * commit.c
> (get_version_url, create_activity, commit_add_dir, commit_add_file
> commit_close_file, add_child): Use `svn_path_url_add_component2()'.
> (commit_delete_entry): Use `svn_path_url_add_component2()' and a minor
> indentation fix in comment.
> (svn_ra_neon__get_commit_editor, checkout_resource): 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()'.
>
> Patch by: Kannan R <ka...@collab.net>
> ]]]
Julian/Stefan: Ping?
- --
Thanks & Regards,
Kannan
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iQEVAwUBSw95/nlTqcY7ytmIAQIZzwf/UC+USf1ILjSjsKGa8KbdvPUKq4ErvApp
QpqKhM7Mis76gZULPOff8aIrjBFizgGo87hk9mFvD4BSFvVzAbXdy/h2As85InFQ
CC8JJLJ8nqQKffb2LcGJD/r+C6eEeKYA/ELO7ZbM5DePzebywNG9NsYbCXN6qEdr
5McA78YawAw6FE08DQ1siRz5D0JesYiTcYto7Wq6PFpIFFkNQxon2MV48s+nb/IH
dnpDCIU9q2FcqS5I28KxdLcBhlGbfLGAYlB/FWrGNzfrekA6ujZiIqyKuf1fqJ2j
1B9fuqyOmKMnwW6exYZCOGmKao0r3M5N69XA8inEGtZ5r8O0aHmkbA==
=TPbk
-----END PGP SIGNATURE-----
Re: r40202 broke commit over ra_neon (Re: svn commit: r40202 - trunk/subversion/libsvn_ra_neon)
Posted by Kannan <ka...@collab.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Hyrum K. Wright wrote:
> On Dec 8, 2009, at 7:07 AM, Kannan wrote:
>
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> Hyrum K. Wright wrote:
>>> Any chance patch submissions can include [PATCH] in the subject, per HACKING guidelines?
>>>
>> Thank you, Hyrum. Just sent them as a part of discussion, sorry for
>> that. I'll look to it hereafter.
>
> Oh, I didn't mean to imply that you need to start a new thread, just that when transitioning from "discussion" to "patch" it would be nice to prepend [PATCH] on the subject line for those of us that filter on such things.
> </orange-speckled>
Oh, ok. I'll look to that hereafter. Thank you.
- --
Thanks & Regards,
Kannan
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iQEVAwUBSx5daHlTqcY7ytmIAQJeNwf9HE9BhY+CiDDDzQCA5LplqXctTfvaMQae
Z5iMbv4xHRTxidF8vFLPdShrpEGTN4sWCueVyuBrEMa1nwSFk1inHVNY6+KiVcW8
3vTVAr4QyRgHzIfdoLw/Mi+Dn+hhDUT9ezc8dKDOvyTlgDkzXNgN799Of2sfN6iC
IOn6wYyZARvek+6Qh5Mi7r7eSysc7uRLYq8SEja2dSJIiezYmePo083/8UECZ2o6
KMxYjbHQaiDMf4lUqLvAvmH75oqe0Lx97cSjDMQ7/N8Vo4ABdDgreDBBs1Op8Qxp
+8BtmeRZ6ZNoJ9QjAn6/QNf1MR/ZElmonC7tHMQ+iqfWvN2edtK+ug==
=bs1X
-----END PGP SIGNATURE-----
Re: r40202 broke commit over ra_neon (Re: svn commit: r40202 - trunk/subversion/libsvn_ra_neon)
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
On Dec 8, 2009, at 7:07 AM, Kannan wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Hyrum K. Wright wrote:
>> Any chance patch submissions can include [PATCH] in the subject, per HACKING guidelines?
>>
> Thank you, Hyrum. Just sent them as a part of discussion, sorry for
> that. I'll look to it hereafter.
Oh, I didn't mean to imply that you need to start a new thread, just that when transitioning from "discussion" to "patch" it would be nice to prepend [PATCH] on the subject line for those of us that filter on such things.
</orange-speckled>
-Hyrum
Re: r40202 broke commit over ra_neon (Re: svn commit: r40202 - trunk/subversion/libsvn_ra_neon)
Posted by Kannan <ka...@collab.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Hyrum K. Wright wrote:
> Any chance patch submissions can include [PATCH] in the subject, per HACKING guidelines?
>
Thank you, Hyrum. Just sent them as a part of discussion, sorry for
that. I'll look to it hereafter.
- --
Thanks & Regards,
Kannan
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iQEVAwUBSx5PnXlTqcY7ytmIAQIw7AgArNtZbIQcJyjSsYcXxVIqkavh004hEcDv
kuN6Z3FFtuDmxbm/uSccdKB7a909iAuHuwNPKNP6/d8J2nt5qiN35nY//6oL/+bZ
8ZGhma9tOgliYm4jQZ+r8ldbx47Rmw9Ae9CvJaYuO1pu0RUxuGshCCJV2TaJKpRp
qrRw+GP+QACthFfeOSeXzeng7lJsCGgbQ4qTE3e/+2/hyoWttUEEMxqILk0OY1ra
J+dMSgRo2mXk2OlkxLw+SzBiE0ysMAd7LZc9jIgFvQ7VwQeYas7djMRGIGvFuLc5
h356L+OFVof6LjaUlex6J2qozfOWQlqlaD3Krpjwh2iYUf1qHqIGvg==
=S0zk
-----END PGP SIGNATURE-----
Re: r40202 broke commit over ra_neon (Re: svn commit: r40202 - trunk/subversion/libsvn_ra_neon)
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
Any chance patch submissions can include [PATCH] in the subject, per HACKING guidelines?
Thanks,
-Hyrum
On Dec 8, 2009, at 12:14 AM, Kannan wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Stefan Sperling wrote:
> [..]
>> You're correct, the place you point out is the place where my_bc_url
>> is initialised. If we could canonicalise it right there without causing
>> breakage elsewhere (make check over ra_neon should tell us) then please
>> send an updated patch. I'd say if changing this ends up causing breakage
>> we can apply your original patch, and then try to fix the breakage and
>> canonicalise my_bc_url earlier in a separate patch.
>
> Thank you Stefan. `apr_hash_get()' returns a struct comprising of two
> values(data, len), of which we need to canonicalise 'data' to store it
> in `my_bc_url'. So, canonicalisation right there fails (AFAICS).
> Attaching the updated original 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>
> ]]]
>
>
> - --
> Thanks & Regards,
> Kannan
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.6 (GNU/Linux)
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
>
> iQEVAwUBSx3u33lTqcY7ytmIAQLkpgf/Ye/v0jRCE65QFxYh5dr/oR5n/DoijIhO
> fVVzsu/k9Cc/6SuKbCg6eCPr9rHaDRVvBCQy+kOHC3HYyNOdv37wUT0LGqc94LHg
> RVyTq4R9EcIBFB05z3C7OD3UWso6ngaOteUD4+fe/PvPql51GiqQ+gPKzpmiZaJN
> 23IN13gadZOXg8079N6yLv1ui+9OIG8i3UQ4ON9v6s5h8+euxMYv2A3qe6pjVaW6
> qXZKuAiL4/Qj7YMfOtEG7rmNsGtbukPFJ2kZ7p+Xu9B13Bxv2tOaGohlVf31Ypj7
> fJKiKunAbGJM4mN6L/cwlnQHIWIxSMvNzEex9ULkCym0AUTZrxtuHA==
> =d3CC
> -----END PGP SIGNATURE-----
> Index: subversion/libsvn_ra_neon/commit.c
> ===================================================================
> --- subversion/libsvn_ra_neon/commit.c (revision 885339)
> +++ subversion/libsvn_ra_neon/commit.c (working copy)
> @@ -203,9 +203,9 @@
> the version resource URL of RSRC. */
> if (parent && parent->vsn_url && parent->revision == rsrc->revision)
> {
> - rsrc->vsn_url = svn_path_url_add_component(parent->vsn_url,
> - rsrc->name,
> - rsrc->pool);
> + rsrc->vsn_url = svn_path_url_add_component2(parent->vsn_url,
> + rsrc->name,
> + rsrc->pool);
> return SVN_NO_ERROR;
> }
>
> @@ -231,7 +231,7 @@
> rsrc->revision,
> pool));
>
> - url = svn_path_url_add_component(bc_url.data, bc_relative.data, pool);
> + url = svn_path_url_add_component2(bc_url.data, bc_relative.data, pool);
> }
>
> /* Get the DAV:checked-in property, which contains the URL of the
> @@ -325,8 +325,8 @@
> the activity, and create the activity. The URL for our activity
> will be ACTIVITY_COLL/UUID */
> SVN_ERR(get_activity_collection(cc, &activity_collection, FALSE, pool));
> - url = svn_path_url_add_component(activity_collection->data,
> - uuid_buf, pool);
> + url = svn_path_url_add_component2(activity_collection->data,
> + uuid_buf, pool);
> SVN_ERR(svn_ra_neon__simple_request(&code, cc->ras,
> "MKACTIVITY", url, NULL, NULL,
> 201 /* Created */,
> @@ -338,8 +338,8 @@
> if (code == 404)
> {
> SVN_ERR(get_activity_collection(cc, &activity_collection, TRUE, pool));
> - url = svn_path_url_add_component(activity_collection->data,
> - uuid_buf, pool);
> + url = svn_path_url_add_component2(activity_collection->data,
> + uuid_buf, pool);
> SVN_ERR(svn_ra_neon__simple_request(&code, cc->ras,
> "MKACTIVITY", url, NULL, NULL,
> 201, 0, pool));
> @@ -373,7 +373,7 @@
> rsrc->pool = pool;
> rsrc->revision = revision;
> rsrc->name = name;
> - rsrc->url = svn_path_url_add_component(parent->url, name, pool);
> + rsrc->url = svn_path_url_add_component2(parent->url, name, pool);
> rsrc->local_path = svn_path_join(parent->local_path, name, pool);
>
> /* Case 1: the resource is truly "new". Either it was added as a
> @@ -382,7 +382,7 @@
> URL by the rules of deltaV: "copy structure is preserved below
> the WR you COPY to." */
> if (created || (parent->vsn_url == NULL))
> - rsrc->wr_url = svn_path_url_add_component(parent->wr_url, name, pool);
> + rsrc->wr_url = svn_path_url_add_component2(parent->wr_url, name, pool);
>
> /* Case 2: the resource is already under version-control somewhere.
> This means it has a VR URL already, and the WR URL won't exist
> @@ -517,8 +517,7 @@
> return svn_error_createf(SVN_ERR_RA_DAV_MALFORMED_DATA, NULL,
> _("Unable to parse URL '%s'"), locn);
> }
> -
> - rsrc->wr_url = apr_pstrdup(rsrc->pool, parse.path);
> + rsrc->wr_url = svn_uri_canonicalize(parse.path, rsrc->pool);
> ne_uri_free(&parse);
>
> return SVN_NO_ERROR;
> @@ -714,7 +713,7 @@
> SVN_ERR(checkout_resource(parent->cc, parent->rsrc, TRUE, NULL, pool));
>
> /* create the URL for the child resource */
> - child = svn_path_url_add_component(parent->rsrc->wr_url, name, pool);
> + child = svn_path_url_add_component2(parent->rsrc->wr_url, name, pool);
>
> /* Start out assuming that we're deleting a file; try to lookup the
> path itself in the token-hash, and if found, attach it to the If:
> @@ -729,8 +728,8 @@
> const char *token_header_val;
> const char *token_uri;
>
> - token_uri = svn_path_url_add_component(parent->cc->ras->url->data,
> - path, pool);
> + token_uri = svn_path_url_add_component2(parent->cc->ras->url->data,
> + path, pool);
> token_header_val = apr_psprintf(pool, "<%s> (<%s>)",
> token_uri, token);
> extra_headers = apr_hash_make(pool);
> @@ -888,9 +887,9 @@
> "source" argument to the COPY request. The "Destination:"
> header given to COPY is simply the wr_url that is already
> part of the child object. */
> - copy_src = svn_path_url_add_component(bc_url.data,
> - bc_relative.data,
> - workpool);
> + copy_src = svn_path_url_add_component2(bc_url.data,
> + bc_relative.data,
> + workpool);
>
> /* Have neon do the COPY. */
> SVN_ERR(svn_ra_neon__copy(parent->cc->ras,
> @@ -1088,9 +1087,9 @@
> "source" argument to the COPY request. The "Destination:"
> header given to COPY is simply the wr_url that is already
> part of the file_baton. */
> - copy_src = svn_path_url_add_component(bc_url.data,
> - bc_relative.data,
> - workpool);
> + copy_src = svn_path_url_add_component2(bc_url.data,
> + bc_relative.data,
> + workpool);
>
> /* Have neon do the COPY. */
> SVN_ERR(svn_ra_neon__copy(parent->cc->ras,
> @@ -1271,9 +1270,9 @@
> svn_ra_neon__set_header
> (extra_headers, "If",
> apr_psprintf(pool, "<%s> (<%s>)",
> - svn_path_url_add_component(cc->ras->url->data,
> - file->rsrc->url,
> - request->pool),
> + svn_path_url_add_component2(cc->ras->url->data,
> + file->rsrc->url,
> + request->pool),
> file->token));
>
> if (pb->base_checksum)
> @@ -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);
>
> /* To set the revision properties, we must checkout the latest baseline
> and get back a mutable "working" baseline. */
> @@ -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);
> +
> /*
> ** Create an Activity. This corresponds directly to an FS transaction.
> ** We will check out all further resources within the context of this
> Index: subversion/libsvn_ra_neon/props.c
> ===================================================================
> --- subversion/libsvn_ra_neon/props.c (revision 885339)
> +++ subversion/libsvn_ra_neon/props.c (working copy)
> @@ -991,7 +991,10 @@
>
> /* maybe return bc_url to the caller */
> if (bc_url)
> - *bc_url = *my_bc_url;
> + {
> + bc_url->data = svn_uri_canonicalize(my_bc_url->data, pool);
> + bc_url->len = my_bc_url->len;
> + }
>
> if (latest_rev != NULL)
> {
Re: r40202 broke commit over ra_neon (Re: svn commit: r40202 - trunk/subversion/libsvn_ra_neon)
Posted by Kannan <ka...@collab.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Stefan Sperling wrote:
[..]
> You're correct, the place you point out is the place where my_bc_url
> is initialised. If we could canonicalise it right there without causing
> breakage elsewhere (make check over ra_neon should tell us) then please
> send an updated patch. I'd say if changing this ends up causing breakage
> we can apply your original patch, and then try to fix the breakage and
> canonicalise my_bc_url earlier in a separate patch.
Thank you Stefan. `apr_hash_get()' returns a struct comprising of two
values(data, len), of which we need to canonicalise 'data' to store it
in `my_bc_url'. So, canonicalisation right there fails (AFAICS).
Attaching the updated original 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>
]]]
- --
Thanks & Regards,
Kannan
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iQEVAwUBSx3u33lTqcY7ytmIAQLkpgf/Ye/v0jRCE65QFxYh5dr/oR5n/DoijIhO
fVVzsu/k9Cc/6SuKbCg6eCPr9rHaDRVvBCQy+kOHC3HYyNOdv37wUT0LGqc94LHg
RVyTq4R9EcIBFB05z3C7OD3UWso6ngaOteUD4+fe/PvPql51GiqQ+gPKzpmiZaJN
23IN13gadZOXg8079N6yLv1ui+9OIG8i3UQ4ON9v6s5h8+euxMYv2A3qe6pjVaW6
qXZKuAiL4/Qj7YMfOtEG7rmNsGtbukPFJ2kZ7p+Xu9B13Bxv2tOaGohlVf31Ypj7
fJKiKunAbGJM4mN6L/cwlnQHIWIxSMvNzEex9ULkCym0AUTZrxtuHA==
=d3CC
-----END PGP SIGNATURE-----
Re: r40202 broke commit over ra_neon (Re: svn commit: r40202 -
trunk/subversion/libsvn_ra_neon)
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Dec 07, 2009 at 11:33:40AM +0530, Kannan wrote:
> Kannan wrote:
> > Stefan Sperling wrote:
> >> On Mon, Nov 30, 2009 at 09:30:58PM +0530, Kannan wrote:
> >>> Stefan Sperling wrote:
> >>> @@ -991,7 +991,10 @@
> >>>
> >>> /* maybe return bc_url to the caller */
> >>> if (bc_url)
> >>> - *bc_url = *my_bc_url;
> >>> + {
> >>> + bc_url->data = svn_uri_canonicalize(my_bc_url->data, pool);
> >>> + bc_url->len = my_bc_url->len;
> >>> + }
> >> It would be nicer to have svn_ra_neon__get_baseline_props()
> >> do the canonicalisation, at this spot:
> >
> >> /* don't forget to tack on the parts we lopped off in order to find
> >> the VCC... We are expected to return a URI decoded relative
> >> path, so decode the lopped path first. */
> >> my_bc_relative = svn_path_join(relative_path->data,
> >> svn_path_uri_decode(lopped_path, pool),
> >> pool);
> >
> >> Then the caller would not need to worry about canonicalisation
> >> and your above change would not be needed.
> >
> >> Also we could replace the svn_path_join() while there.
> >
> >
> > The above code initialises `my_bc_rel' right? And `my_bc_url' is
> > getting initialised in `svn_ra_neon__get_baseline_info()' here :
> >
> > <snip>
> > /* Allocate our own copy of bc_url regardless. */
> > my_bc_url = apr_hash_get(baseline_rsrc->propset,
> > SVN_RA_NEON__PROP_BASELINE_COLLECTION,
> > APR_HASH_KEY_STRING);
> > </snip>
> >
> > Please correct me if I'm wrong. Thank you for your feedback. If the
> > above case seems ok, then shall I send the updated patch?
>
> Any updates on this thread?
You're correct, the place you point out is the place where my_bc_url
is initialised. If we could canonicalise it right there without causing
breakage elsewhere (make check over ra_neon should tell us) then please
send an updated patch. I'd say if changing this ends up causing breakage
we can apply your original patch, and then try to fix the breakage and
canonicalise my_bc_url earlier in a separate patch.
Thanks,
Stefan
Re: r40202 broke commit over ra_neon (Re: svn commit: r40202 - trunk/subversion/libsvn_ra_neon)
Posted by Kannan <ka...@collab.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Kannan wrote:
> Stefan Sperling wrote:
>> On Mon, Nov 30, 2009 at 09:30:58PM +0530, Kannan wrote:
>>> Stefan Sperling wrote:
>>>>> @@ -773,7 +777,7 @@
>>>>>
>>>>> Note that we're not sending the locks in the If: header, for
>>>>> the same reason we're not sending in MERGE's headers: httpd has
>>>>> - limits on the amount of data it's willing to receive in headers. */
>>>>> + limits on the amount of data it's willing to receive in headers. */
>>>> Why was this changed?
>>> Indentation fix in the comment.
>> It's better to make unrelated changes in separate patches.
>
> Ok, I'll make it under a separate one.
>
>> I found one more nit:
>
>>> Index: subversion/libsvn_ra_neon/props.c
>>> ===================================================================
>>> --- subversion/libsvn_ra_neon/props.c (revision 885339)
>>> +++ subversion/libsvn_ra_neon/props.c (working copy)
>>> @@ -991,7 +991,10 @@
>>>
>>> /* maybe return bc_url to the caller */
>>> if (bc_url)
>>> - *bc_url = *my_bc_url;
>>> + {
>>> + bc_url->data = svn_uri_canonicalize(my_bc_url->data, pool);
>>> + bc_url->len = my_bc_url->len;
>>> + }
>> It would be nicer to have svn_ra_neon__get_baseline_props()
>> do the canonicalisation, at this spot:
>
>> /* don't forget to tack on the parts we lopped off in order to find
>> the VCC... We are expected to return a URI decoded relative
>> path, so decode the lopped path first. */
>> my_bc_relative = svn_path_join(relative_path->data,
>> svn_path_uri_decode(lopped_path, pool),
>> pool);
>
>> Then the caller would not need to worry about canonicalisation
>> and your above change would not be needed.
>
>> Also we could replace the svn_path_join() while there.
>
>
> The above code initialises `my_bc_rel' right? And `my_bc_url' is
> getting initialised in `svn_ra_neon__get_baseline_info()' here :
>
> <snip>
> /* Allocate our own copy of bc_url regardless. */
> my_bc_url = apr_hash_get(baseline_rsrc->propset,
> SVN_RA_NEON__PROP_BASELINE_COLLECTION,
> APR_HASH_KEY_STRING);
> </snip>
>
> Please correct me if I'm wrong. Thank you for your feedback. If the
> above case seems ok, then shall I send the updated patch?
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/
iQEVAwUBSxyavHlTqcY7ytmIAQKyCwf7BC1yOZhDoWcTi1b086+hWCcl12761xSX
bM1tVKIFkU+4L8vNmVw4iXSehIzKaHF5X1XmlJ7O5tKIrig9xB52RkWxJh0R52G3
EsHOJcpK/NyMlAseIht+OEu1iLPice02pgZrFMXKAtE46pbsOmZDjS44RHoVQKX7
AgPch86RhP3/MmDcr/bhT+gWAV3OhQZczakCDhkL5vrGOMyVwTGPcGjELKoTmJRc
UbLtLAGp72fp8BZf7VpmjkYOVwy8F5SSeTiDJHmQYkTiTTrVK254tmFcgvXnTMyc
9aAtv0m6F2KKcCAAiWo7hGPtJlvSU9qhtnjisFGSB/5jgigUSxTXrg==
=xV3e
-----END PGP SIGNATURE-----
Re: r40202 broke commit over ra_neon (Re: svn commit: r40202 - trunk/subversion/libsvn_ra_neon)
Posted by Kannan <ka...@collab.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Stefan Sperling wrote:
> On Mon, Nov 30, 2009 at 09:30:58PM +0530, Kannan wrote:
>> Stefan Sperling wrote:
>>>> @@ -773,7 +777,7 @@
>>>>
>>>> Note that we're not sending the locks in the If: header, for
>>>> the same reason we're not sending in MERGE's headers: httpd has
>>>> - limits on the amount of data it's willing to receive in headers. */
>>>> + limits on the amount of data it's willing to receive in headers. */
>>> Why was this changed?
>> Indentation fix in the comment.
>
> It's better to make unrelated changes in separate patches.
Ok, I'll make it under a separate one.
> I found one more nit:
>
>> Index: subversion/libsvn_ra_neon/props.c
>> ===================================================================
>> --- subversion/libsvn_ra_neon/props.c (revision 885339)
>> +++ subversion/libsvn_ra_neon/props.c (working copy)
>> @@ -991,7 +991,10 @@
>>
>> /* maybe return bc_url to the caller */
>> if (bc_url)
>> - *bc_url = *my_bc_url;
>> + {
>> + bc_url->data = svn_uri_canonicalize(my_bc_url->data, pool);
>> + bc_url->len = my_bc_url->len;
>> + }
>
> It would be nicer to have svn_ra_neon__get_baseline_props()
> do the canonicalisation, at this spot:
>
> /* don't forget to tack on the parts we lopped off in order to find
> the VCC... We are expected to return a URI decoded relative
> path, so decode the lopped path first. */
> my_bc_relative = svn_path_join(relative_path->data,
> svn_path_uri_decode(lopped_path, pool),
> pool);
>
> Then the caller would not need to worry about canonicalisation
> and your above change would not be needed.
>
> Also we could replace the svn_path_join() while there.
>
The above code initialises `my_bc_rel' right? And `my_bc_url' is
getting initialised in `svn_ra_neon__get_baseline_info()' here :
<snip>
/* Allocate our own copy of bc_url regardless. */
my_bc_url = apr_hash_get(baseline_rsrc->propset,
SVN_RA_NEON__PROP_BASELINE_COLLECTION,
APR_HASH_KEY_STRING);
</snip>
Please correct me if I'm wrong. Thank you for your feedback. If the
above case seems ok, then shall I send the updated patch?
- --
Thanks & Regards,
Kannan
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iQEVAwUBSxSqUXlTqcY7ytmIAQLLDgf/V6z5tCCVXj5LzbbzMFWkx9EG28/C2ei3
0gMfvSq/QzS4o9TI2nYmBa1rnlldSq/0eVdRSRWm0giJRmdj/APYoL+CL4wsh9ox
GJryEd5s+P1wxo9oPPpYpmvAqrwGWeKxUymW0zWonSQt1RRsONBil4glRZsy1EqL
NpxPlZ+BtkUAq1L7q5keXnXDqqiR1aFcPPjZH0H+eCc+lgdKkOWOzaUDQBx2Ll+u
tCiUp7i9dmhqvrELGylwFGczM2mA1dam0yd/PpAH+tCsLJDec0ADHkptzKSUGNYl
l72kEVXql+YjJGvRAeiIovqcRP5J65KasQf57vqWPQ6z1nz2bpwANA==
=Y+5/
-----END PGP SIGNATURE-----
Re: r40202 broke commit over ra_neon (Re: svn commit: r40202 -
trunk/subversion/libsvn_ra_neon)
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Nov 30, 2009 at 09:30:58PM +0530, Kannan wrote:
> Stefan Sperling wrote:
> >> @@ -773,7 +777,7 @@
> >>
> >> Note that we're not sending the locks in the If: header, for
> >> the same reason we're not sending in MERGE's headers: httpd has
> >> - limits on the amount of data it's willing to receive in headers. */
> >> + limits on the amount of data it's willing to receive in headers. */
> >
> > Why was this changed?
>
> Indentation fix in the comment.
It's better to make unrelated changes in separate patches.
I found one more nit:
> Index: subversion/libsvn_ra_neon/props.c
> ===================================================================
> --- subversion/libsvn_ra_neon/props.c (revision 885339)
> +++ subversion/libsvn_ra_neon/props.c (working copy)
> @@ -991,7 +991,10 @@
>
> /* maybe return bc_url to the caller */
> if (bc_url)
> - *bc_url = *my_bc_url;
> + {
> + bc_url->data = svn_uri_canonicalize(my_bc_url->data, pool);
> + bc_url->len = my_bc_url->len;
> + }
It would be nicer to have svn_ra_neon__get_baseline_props()
do the canonicalisation, at this spot:
/* don't forget to tack on the parts we lopped off in order to find
the VCC... We are expected to return a URI decoded relative
path, so decode the lopped path first. */
my_bc_relative = svn_path_join(relative_path->data,
svn_path_uri_decode(lopped_path, pool),
pool);
Then the caller would not need to worry about canonicalisation
and your above change would not be needed.
Also we could replace the svn_path_join() while there.
The rest looks good now.
Thanks,
Stefan
Re: r40202 broke commit over ra_neon (Re: svn commit: r40202 - trunk/subversion/libsvn_ra_neon)
Posted by Kannan <ka...@collab.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Stefan Sperling wrote:
> On Mon, Nov 30, 2009 at 08:28:19PM +0530, Kannan wrote:
>> @@ -518,7 +518,11 @@
>> _("Unable to parse URL '%s'"), locn);
>> }
>>
>> - rsrc->wr_url = apr_pstrdup(rsrc->pool, parse.path);
>> + /* Canonicalize the base here as `svn_path_url_add_component2()'
>> + won't handle it */
>
> You don't need the above comment. We always canonicalise paths
> received from input before using them.
>
>> + rsrc->wr_url = svn_uri_canonicalize
>> + (apr_pstrdup(rsrc->pool, parse.path), pool);
>> +
>
> This duplicates the string twice.
> You don't need to duplicate the string before canonicalising it.
> Canonicalisation will also create a copy.
> Just do:
> rsrc->wr_url = svn_uri_canonicalize(parse.path, rsrc->pool);
[..]
>> @@ -773,7 +777,7 @@
>>
>> Note that we're not sending the locks in the If: header, for
>> the same reason we're not sending in MERGE's headers: httpd has
>> - limits on the amount of data it's willing to receive in headers. */
>> + limits on the amount of data it's willing to receive in headers. */
>
> Why was this changed?
Indentation fix in the comment.
Thank you Stefan. As per your comments, posting the updated 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; and a minor indentation fix.
[in subversion/libsvn_ra_neon]
* commit.c
(get_version_url, create_activity, commit_add_dir, commit_add_file
commit_close_file, add_child): Use `svn_path_url_add_component2()'.
(commit_delete_entry): Use `svn_path_url_add_component2()' and a minor
indentation fix in comment.
(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>
]]]
- --
Thanks & Regards,
Kannan
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iQEVAwUBSxPsOXlTqcY7ytmIAQKCMwf/Ztx95DJDo/yNWfW4XWuDt9pCsxN9VAQg
TqsGnkuQ25QzpPrR7SYpbu+u/8+Jx6eBBhKqaiYogw1oRp4+PfxGuGd3wE4xGP9e
XAEiCzvRiGEWQYrKGpMjkcfyOQpR78A3hPCSUY6m3lcRQy7wm9+L9R2EML7UtHph
TD1vW0Eyc/RoU/sqsp31Pem6DF7ep0SknL5OP22JrpLAb57HClIrXodR0Xq426YW
+QkjoN6fkY9llVsFSfhvU3LRTIT1Sbw9S1cOZkaUS7Ap5xwpXnn4OPZlJeg/mnWC
TXMXr4DpX3/Si0kbhB7uydiUnKJB5rUNzShUyh7m5OfUKy1cOS45Pw==
=SyUL
-----END PGP SIGNATURE-----
Re: r40202 broke commit over ra_neon (Re: svn commit: r40202 -
trunk/subversion/libsvn_ra_neon)
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Nov 30, 2009 at 08:28:19PM +0530, Kannan wrote:
> @@ -518,7 +518,11 @@
> _("Unable to parse URL '%s'"), locn);
> }
>
> - rsrc->wr_url = apr_pstrdup(rsrc->pool, parse.path);
> + /* Canonicalize the base here as `svn_path_url_add_component2()'
> + won't handle it */
You don't need the above comment. We always canonicalise paths
received from input before using them.
> + rsrc->wr_url = svn_uri_canonicalize
> + (apr_pstrdup(rsrc->pool, parse.path), pool);
> +
This duplicates the string twice.
You don't need to duplicate the string before canonicalising it.
Canonicalisation will also create a copy.
Just do:
rsrc->wr_url = svn_uri_canonicalize(parse.path, rsrc->pool);
> ne_uri_free(&parse);
>
> return SVN_NO_ERROR;
> @@ -773,7 +777,7 @@
>
> Note that we're not sending the locks in the If: header, for
> the same reason we're not sending in MERGE's headers: httpd has
> - limits on the amount of data it's willing to receive in headers. */
> + limits on the amount of data it's willing to receive in headers. */
Why was this changed?
> apr_hash_t *child_tokens = NULL;
> svn_ra_neon__request_t *request;
> @@ -1389,8 +1393,11 @@
> vcc, NULL,
> &svn_ra_neon__checked_in_prop, pool));
> baseline_rsrc.pool = pool;
> - baseline_rsrc.vsn_url = baseline_url->data;
>
> + /* Canonicalize the base here as `svn_path_url_add_component2()'
> + won't handle it. */
Again, the comment is not necessary.
> + baseline_rsrc.vsn_url = svn_uri_canonicalize(baseline_url->data, pool);
> +
> /* To set the revision properties, we must checkout the latest baseline
> and get back a mutable "working" baseline. */
> err = checkout_resource(cc, &baseline_rsrc, FALSE, NULL, pool);
> @@ -1452,6 +1459,10 @@
> /* ### should we perform an OPTIONS to validate the server we're about
> ### to talk to? */
>
> + /* Canonicalize the base here as `svn_path_url_add_component2()'
> + won't handle it */
Same.
> + cc->ras->act_coll = svn_uri_canonicalize(cc->ras->act_coll, pool);
> +
> /*
> ** Create an Activity. This corresponds directly to an FS transaction.
> ** We will check out all further resources within the context of this
> Index: subversion/libsvn_ra_neon/props.c
> ===================================================================
> --- subversion/libsvn_ra_neon/props.c (revision 885339)
> +++ subversion/libsvn_ra_neon/props.c (working copy)
> @@ -991,7 +991,12 @@
>
> /* maybe return bc_url to the caller */
> if (bc_url)
> - *bc_url = *my_bc_url;
> + {
> + /* Canonicalize the base here as `svn_path_url_add_component2()'
> + won't handle it */
Same.
Thanks,
Stefan
> + bc_url->data = svn_uri_canonicalize(my_bc_url->data, pool);
> + bc_url->len = my_bc_url->len;
> + }
>
> if (latest_rev != NULL)
> {
Re: r40202 broke commit over ra_neon (Re: svn commit: r40202 - trunk/subversion/libsvn_ra_neon)
Posted by Kannan <ka...@collab.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Stefan Sperling wrote:
[..]
> Canonicalising in place looks suspicous. Why do we let these
> strings run around in non-canonicalised form for a while, and then
> later canonicalise them? There is code above these lines that also
> uses rsrc->vsn_url, for instance.
> Isn't it possible to canonicalise these strings earlier, when rsrc
> is allocated and initialised?
Thank you for the comments, Stefan. Canonicalization of the base is
now done, where they're getting initialized. rsrc->wr_url is
initialized in the same place, but in place canonicalization has been
changed.
>> Index: ../../subversion/libsvn_ra_neon/props.c
>> ===================================================================
>> --- ../../subversion/libsvn_ra_neon/props.c (revision 882427)
>> +++ ../../subversion/libsvn_ra_neon/props.c (working copy)
>> @@ -991,7 +991,12 @@
>>
>> /* maybe return bc_url to the caller */
>> if (bc_url)
>> - *bc_url = *my_bc_url;
>> + {
>> + /* Canonicalize the base here as `svn_path_url_add_component2()'
>> + won't handle it */
>> + bc_url->data = svn_uri_canonicalize(my_bc_url->data, pool);
>> + bc_url->len = my_bc_url->len;
>
> Here, my_bc_url() comes from svn_ra_neon__get_baseline_props().
> Why doesn't svn_ra_neon__get_baseline_props() canonicalise its result?
`my_bc_rel' gets its value from `svn_ra_neon__get_baseline_props()'
whereas `my_bc_url' is initialized in the same place.
Attaching the updated 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; and a minor indentation fix.
[in subversion/libsvn_ra_neon]
* commit.c
(get_version_url, create_activity, commit_add_dir, commit_add_file
commit_close_file, add_child): Use `svn_path_url_add_component2()'.
(commit_delete_entry): Use `svn_path_url_add_component2()' and a minor
indentation fix in comment.
(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()'.
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/
iQEVAwUBSxPdi3lTqcY7ytmIAQL4FQf7BAFuQz9brjUtGNFKWb/PIcz3srbfsbPh
p50XX91GLSEBjy4PKdQwTJyuDdyOT3UFc0WFSjmy8hFcoAANvyAEWuqOWU+2F4Qk
BQs+n8XLTHfmAT/W3XV5pyUs8eJzQRTAd/Il2Zux3V6gAXihuNEUDshO+1rMBupi
dx8/H5kbmViOB/3/1JaAEXpU5rjM+rKrjylKTPDgqMyeFOahVLcUikDYykgC+pMi
uxgZERdQ+iaYuefxcTBWXMyUIpYFVCSVrQjse28ASmewlWkSm0fZ8Ruzt1cRRmni
/z3AUbRtuGFMvVwXYJ+tGY1OmV7uDl/awVS3x2VufF00xcgHjCvmpg==
=C2dF
-----END PGP SIGNATURE-----
Re: r40202 broke commit over ra_neon (Re: svn commit: r40202 -
trunk/subversion/libsvn_ra_neon)
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Nov 23, 2009 at 11:32:51AM +0530, Kannan wrote:
> [[[
> 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; and a minor indentation fix.
>
> [in subversion/libsvn_ra_neon]
>
> * commit.c
> (get_version_url, create_activity, commit_add_dir, commit_add_file
> commit_close_file, add_child): Use `svn_path_url_add_component2()'.
> (commit_delete_entry): Use `svn_path_url_add_component2()' and a minor
> indentation fix in comment.
> (svn_ra_neon__get_commit_editor, checkout_resource): 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()'.
>
> Patch by: Kannan R <ka...@collab.net>
> ]]]
Sorry for the delay.
> Index: ../../subversion/libsvn_ra_neon/commit.c
> ===================================================================
> --- ../../subversion/libsvn_ra_neon/commit.c (revision 882427)
> +++ ../../subversion/libsvn_ra_neon/commit.c (working copy)
> @@ -519,6 +519,12 @@
> }
>
> rsrc->wr_url = apr_pstrdup(rsrc->pool, parse.path);
> +
> + /* Canonicalize the base here as `svn_path_url_add_component2()'
> + won't handle it */
> + rsrc->vsn_url = svn_uri_canonicalize(rsrc->vsn_url, pool);
> + rsrc->wr_url = svn_uri_canonicalize(rsrc->wr_url, pool);
Canonicalising in place looks suspicous. Why do we let these
strings run around in non-canonicalised form for a while, and then
later canonicalise them? There is code above these lines that also
uses rsrc->vsn_url, for instance.
Isn't it possible to canonicalise these strings earlier, when rsrc
is allocated and initialised?
> Index: ../../subversion/libsvn_ra_neon/props.c
> ===================================================================
> --- ../../subversion/libsvn_ra_neon/props.c (revision 882427)
> +++ ../../subversion/libsvn_ra_neon/props.c (working copy)
> @@ -991,7 +991,12 @@
>
> /* maybe return bc_url to the caller */
> if (bc_url)
> - *bc_url = *my_bc_url;
> + {
> + /* Canonicalize the base here as `svn_path_url_add_component2()'
> + won't handle it */
> + bc_url->data = svn_uri_canonicalize(my_bc_url->data, pool);
> + bc_url->len = my_bc_url->len;
Here, my_bc_url() comes from svn_ra_neon__get_baseline_props().
Why doesn't svn_ra_neon__get_baseline_props() canonicalise its result?
Thanks,
Stefan
Re: r40202 broke commit over ra_neon (Re: svn commit: r40202 - trunk/subversion/libsvn_ra_neon)
Posted by Kannan <ka...@collab.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Stefan Sperling wrote:
> On Fri, Oct 23, 2009 at 08:59:04AM -0700, Julian Foad wrote:
>> Author: julianfoad
>> Date: Fri Oct 23 08:59:03 2009
>> New Revision: 40202
>>
>> Log:
>> Use new dirent/URI/path functions to resolve some deprecation warnings.
>> Also fix a bit of indentation.
>>
>> * subversion/libsvn_ra_neon/commit.c
>> (get_version_url, create_activity, commit_delete_entry, commit_add_dir,
>> commit_add_file, commit_close_file): Use `svn_path_url_add_component2()'.
>> (add_child): Use `svn_path_url_add_component2()' and `svn_uri_join()'.
>> (checkout_resource): Use `svn_relpath_local_style()'.
>> (get_child_tokens): Use `svn_uri_is_child()'.
>
>> @@ -325,8 +325,8 @@ static svn_error_t * create_activity(com
>> the activity, and create the activity. The URL for our activity
>> will be ACTIVITY_COLL/UUID */
>> SVN_ERR(get_activity_collection(cc, &activity_collection, FALSE, pool));
>> - url = svn_path_url_add_component(activity_collection->data,
>> - uuid_buf, pool);
>> + url = svn_path_url_add_component2(activity_collection->data,
>> + uuid_buf, pool);
>> SVN_ERR(svn_ra_neon__simple_request(&code, cc->ras,
>> "MKACTIVITY", url, NULL, NULL,
>> 201 /* Created */,
>
> I had to revert the above hunk in r40224, to fix the following
> assertion failure during commit over ra_neon:
> The problem is that base has a trailing slash, so "/repos/svn/!svn/act/"
> is not considered canonical.
> The old version canonicalised the URL while the new version does not:
>
> const char *
> svn_path_url_add_component(const char *url,
> const char *component,
> apr_pool_t *pool)
> {
> /* URL can have trailing '/' */
> url = svn_path_canonicalize(url, pool);
>
> return svn_path_url_add_component2(url, component, pool);
> }
>
> const char *
> svn_path_url_add_component2(const char *url,
> const char *component,
> apr_pool_t *pool)
> {
> /* = svn_path_uri_encode() but without always copying */
> component = uri_escape(component, uri_char_validity, pool);
>
> return svn_path_join(url, component, pool);
> }
>
> The docstring of the new version documents this difference, so this
> was done on purpose.
>
> To fix this properly, we need to canonicalise the base somewhere.
> Please also check any other add_component->add_component2 upgrades
> made in r40202 for similar issues.
Attached herewith is the patch which fixes the failing commit owing to
the above case.
[[[
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; and a minor indentation fix.
[in subversion/libsvn_ra_neon]
* commit.c
(get_version_url, create_activity, commit_add_dir, commit_add_file
commit_close_file, add_child): Use `svn_path_url_add_component2()'.
(commit_delete_entry): Use `svn_path_url_add_component2()' and a minor
indentation fix in comment.
(svn_ra_neon__get_commit_editor, checkout_resource): 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()'.
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/
iQEVAwUBSwoli3lTqcY7ytmIAQLv5Qf/dWtRI9zjcFdAmaM0FWUFxOn5snETImgZ
QGGo9nx+S/YMK9gbjT0XWSwGTIThciRtz92hdL9wIhSNDlclrPShzA8S03xLG2jl
cMH31aUIvXqcDigVYjx2t6+6Ho8kB0rn2ciuYRypDkGjcK/A9pVWsWy8SgcBJL/h
Jt0f4YfXhSllEcVscd8Z5swl3nvg0bVT6ZDM+D3rKBBofEaO7pemuChwWUMcA/wk
CcNnoRV5+lhaHxmvS4KynBOvToVszLk6VdnjwwuEXH2bdhB15oMdqZRZIrHml3/N
rNMq/fsjnpJH79eZvoRgW95vSffUDb2NEKtvrrTESD1TEJyp/uXFfg==
=doxg
-----END PGP SIGNATURE-----