lundblad@tigris.org writes:
> --- trunk/subversion/libsvn_ra_svn/editor.c (original)
> +++ trunk/subversion/libsvn_ra_svn/editor.c Sun Nov 28 05:33:36 2004
> @@ -29,6 +29,7 @@
> #include "svn_types.h"
> #include "svn_string.h"
> #include "svn_error.h"
> +#include "svn_path.h"
> #include "svn_delta.h"
> #include "svn_ra_svn.h"
> #include "svn_pools.h"
> @@ -429,6 +430,7 @@
>
> SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "c(?r)c", &path, &rev, &token));
> SVN_ERR(lookup_token(ds, token, &entry, pool));
> + path = svn_path_canonicalize(path, entry->pool);
> SVN_CMD_ERR(ds->editor->delete_entry(path, rev, entry->baton, entry->pool));
> SVN_ERR(svn_ra_svn_write_cmd_response(conn, pool, ""));
> return SVN_NO_ERROR;
Just curious: why is it better to use entry->pool here instead of
pool? I've no reason to think it's wrong, would just like to
understand better.
The lack of internal interface documentation in libsvn_ra_svn makes it
hard to know the pool lifetimes, oh well. Of course, I could just
read the *code*, but then I'd have to learn C :-).
> --- trunk/subversion/libsvn_ra_svn/editorp.c (original)
> +++ trunk/subversion/libsvn_ra_svn/editorp.c Sun Nov 28 05:33:36 2004
> @@ -29,6 +29,7 @@
> #include "svn_types.h"
> #include "svn_string.h"
> #include "svn_error.h"
> +#include "svn_path.h"
> #include "svn_delta.h"
> #include "svn_ra_svn.h"
> #include "svn_pools.h"
> @@ -459,6 +460,7 @@
>
> SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "c(?r)c", &path, &rev, &token));
> SVN_ERR(lookup_token(ds, token, FALSE, &entry));
> + path = svn_path_canonicalize(path, entry->pool);
> SVN_CMD_ERR(ds->editor->delete_entry(path, rev, entry->baton, pool));
> return SVN_NO_ERROR;
> }
Same question applies here, but with a difference:
In the first case, in editor.c, the next call, to
ds->editor->delete_entry(), used 'entry->pool' too. But here, the
next call, also ds->editor->delete_entry(), uses 'pool' instead.
Do we know why there is this inconsistency? I realize it was not part
of your change, but you might be in a position to explain it.
> @@ -554,6 +560,9 @@
> &file_token, ©_path, ©_rev));
> SVN_ERR(lookup_token(ds, token, FALSE, &entry));
> ds->file_refs++;
> + path = svn_path_canonicalize(path, ds->file_pool);
> + if (copy_path)
> + copy_path = svn_path_canonicalize(copy_path, ds->file_pool);
> file_entry = store_token(ds, NULL, file_token, TRUE, ds->file_pool);
> SVN_CMD_ERR(ds->editor->add_file(path, entry->baton, copy_path, copy_rev,
> ds->file_pool, &file_entry->baton));
Similar question applies here.
> @@ -573,6 +582,7 @@
> &file_token, &rev));
> SVN_ERR(lookup_token(ds, token, FALSE, &entry));
> ds->file_refs++;
> + path = svn_path_canonicalize(path, ds->file_pool);
> file_entry = store_token(ds, NULL, file_token, TRUE, ds->file_pool);
> SVN_CMD_ERR(ds->editor->open_file(path, entry->baton, rev, ds->file_pool,
> &file_entry->baton));
And here, although in these cases at least the ds->editor->foo() call
is consistently using 'ds->file_pool' at least.
> --- trunk/subversion/svnserve/serve.c (original)
> +++ trunk/subversion/svnserve/serve.c Sun Nov 28 05:33:36 2004
> @@ -240,6 +240,7 @@
>
> SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "crb",
> &path, &rev, &start_empty));
> + path = svn_path_canonicalize(path, pool);
> if (!b->err)
> b->err = svn_repos_set_path(b->report_baton, path, rev, start_empty, pool);
> return SVN_NO_ERROR;
> @@ -252,6 +253,7 @@
> const char *path;
>
> SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "c", &path));
> + path = svn_path_canonicalize(path, pool);
> if (!b->err)
> b->err = svn_repos_delete_path(b->report_baton, path, pool);
> return SVN_NO_ERROR;
> @@ -267,7 +269,8 @@
>
> SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "ccrb",
> &path, &url, &rev, &start_empty));
> - url = svn_path_uri_decode(url, pool);
> + path = svn_path_canonicalize(path, pool);
> + url = svn_path_uri_decode(svn_path_canonicalize(url, pool), pool);
> if (!b->err)
> b->err = get_fs_path(b->repos_url, url, &fs_path, pool);
> if (!b->err)
> @@ -599,6 +602,7 @@
> /* Parse arguments. */
> SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "c(?r)bb", &path, &rev,
> &want_props, &want_contents));
> + path = svn_path_canonicalize(path, pool);
> SVN_ERR(trivial_auth_request(conn, pool, b));
> if (!SVN_IS_VALID_REVNUM(rev))
> SVN_CMD_ERR(svn_fs_youngest_rev(&rev, b->fs, pool));
> @@ -672,6 +676,7 @@
>
> SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "c(?r)bb", &path, &rev,
> &want_props, &want_contents));
> + path = svn_path_canonicalize(path, pool);
> SVN_ERR(trivial_auth_request(conn, pool, b));
> if (!SVN_IS_VALID_REVNUM(rev))
> SVN_CMD_ERR(svn_fs_youngest_rev(&rev, b->fs, pool));
> @@ -769,6 +774,7 @@
> /* Parse the arguments. */
> SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "(?r)cb", &rev, &target,
> &recurse));
> + target = svn_path_canonicalize(target, pool);
> SVN_ERR(trivial_auth_request(conn, pool, b));
> if (!SVN_IS_VALID_REVNUM(rev))
> SVN_CMD_ERR(svn_fs_youngest_rev(&rev, b->fs, pool));
> @@ -788,6 +794,8 @@
> /* Parse the arguments. */
> SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "(?r)cbc", &rev, &target,
> &recurse, &switch_url));
> + target = svn_path_canonicalize(target, pool);
> + switch_url = svn_path_canonicalize(switch_url, pool);
> SVN_ERR(trivial_auth_request(conn, pool, b));
> if (!SVN_IS_VALID_REVNUM(rev))
> SVN_CMD_ERR(svn_fs_youngest_rev(&rev, b->fs, pool));
> @@ -810,6 +818,7 @@
> /* Parse the arguments. */
> SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "cb?(?r)",
> &target, &recurse, &rev));
> + target = svn_path_canonicalize(target, pool);
> SVN_ERR(trivial_auth_request(conn, pool, b));
> if (!SVN_IS_VALID_REVNUM(rev))
> SVN_CMD_ERR(svn_fs_youngest_rev(&rev, b->fs, pool));
> @@ -828,6 +837,8 @@
> /* Parse the arguments. */
> SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "(?r)cbbc", &rev, &target,
> &recurse, &ignore_ancestry, &versus_url));
> + target = svn_path_canonicalize(target, pool);
> + versus_url = svn_path_canonicalize(versus_url, pool);
> SVN_ERR(trivial_auth_request(conn, pool, b));
> if (!SVN_IS_VALID_REVNUM(rev))
> SVN_CMD_ERR(svn_fs_youngest_rev(&rev, b->fs, pool));
> @@ -907,7 +918,10 @@
> if (elt->kind != SVN_RA_SVN_STRING)
> return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> "Log path entry not a string");
> - full_path = svn_path_join(b->fs_path, elt->u.string->data, pool);
> + full_path = svn_path_join(b->fs_path,
> + svn_path_canonicalize(elt->u.string->data,
> + pool),
> + pool);
> *((const char **) apr_array_push(full_paths)) = full_path;
> }
> SVN_ERR(trivial_auth_request(conn, pool, b));
> @@ -940,6 +954,7 @@
> svn_node_kind_t kind;
>
> SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "c(?r)", &path, &rev));
> + path = svn_path_canonicalize(path, pool);
> SVN_ERR(trivial_auth_request(conn, pool, b));
> if (!SVN_IS_VALID_REVNUM(rev))
> SVN_CMD_ERR(svn_fs_youngest_rev(&rev, b->fs, pool));
> @@ -971,6 +986,7 @@
> SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "crl", &relative_path,
> &peg_revision,
> &loc_revs_proto));
> + relative_path = svn_path_canonicalize(relative_path, pool);
>
> abs_path = svn_path_join(b->fs_path, relative_path, pool);
>
> @@ -1099,6 +1115,7 @@
> /* Parse arguments. */
> SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "c(?r)(?r)",
> &path, &start_rev, &end_rev));
> + path = svn_path_canonicalize(path, pool);
> SVN_ERR(trivial_auth_request(conn, pool, b));
> full_path = svn_path_join(b->fs_path, path, pool);
>
> @@ -1342,6 +1359,7 @@
> if (!success)
> return svn_ra_svn_flush(conn, pool);
> SVN_ERR(svn_ra_svn_read_tuple(conn, pool, "c", &client_url));
> + client_url = svn_path_canonicalize(client_url, pool);
> err = find_repos(client_url, params->root, &b, pool);
> if (!err && current_access(&b) == NO_ACCESS)
> err = svn_error_create(SVN_ERR_RA_NOT_AUTHORIZED, NULL,
> @@ -1360,6 +1378,7 @@
> * URL, and then we do an auth request. */
> SVN_ERR(svn_ra_svn_parse_tuple(item->u.list, pool, "nlc", &ver,
> &caplist, &client_url));
> + client_url = svn_path_canonicalize(client_url, pool);
> SVN_ERR(svn_ra_svn_set_capabilities(conn, caplist));
> err = find_repos(client_url, params->root, &b, pool);
> if (!err)
There's so much canonicalizing going on in here, that I think a
comment explaining why it's necessary would be appropriate. Just one
comment, in a prominent place, would be enough. Or maybe in two
places: top of client.c and top of serve.c? Hmmm, that's not really
ideal either. Well, if you can think of a good way to indicate in the
code that canonicalization serves a purpose, as a reminder for when
people add new code, that would be great.
Thanks for the fix, as always!
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Nov 29 17:26:37 2004