You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by kf...@collab.net on 2004/11/29 16:21:45 UTC
Re: svn commit: r12063 - in trunk/subversion: libsvn_ra_svn svnserve
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
Re: svn commit: r12063 - in trunk/subversion: libsvn_ra_svn svnserve
Posted by kf...@collab.net.
"Peter N. Lundblad" <pe...@famlundblad.se> writes:
> I will. Karl, and all others, you want to forget about my last answer?
Never happened.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r12063 - in trunk/subversion: libsvn_ra_svn svnserve
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Mon, 29 Nov 2004 kfogel@collab.net wrote:
> Greg Hudson <gh...@MIT.EDU> writes:
> > Both should be using pool. The only reason to use entry->pool is if
> > you're creating an object which is going to live in the entry.
> >
> > The discrepancy is because editorp.c (the code which normally gets used)
> > was fixed, while editor.c (the pre-0.34 compatibility code) was not.
>
> Thanks. Peter, you want to take care of this?
>
I will. Karl, and all others, you want to forget about my last answer? I
got it backwards in some way. Greg, thanks for keeping an eye on me:-(
Best,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r12063 - in trunk/subversion: libsvn_ra_svn svnserve
Posted by kf...@collab.net.
Greg Hudson <gh...@MIT.EDU> writes:
> This is true of essentially every function we have which takes a pool,
> and of essentially every object we have which contains a pool. Why
> document it for every function? Should we also document what pools are
> and how they work, for every function?
I would say yes, we should document it for every function, as we do
elsewhere in Subversion. There are, or have been, objects in
Subversion which contain pool references where the pool's lifetime is
longer than the objects. These are usually batons, not entries, but
you get the point.
But it's not a big enough concern for me to complain further about it,
beyond answering your question, nor to do anything about it.
Obviously, the answer to your rhetorical second question is "No" :-).
> > 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.
>
> Both should be using pool. The only reason to use entry->pool is if
> you're creating an object which is going to live in the entry.
>
> The discrepancy is because editorp.c (the code which normally gets used)
> was fixed, while editor.c (the pre-0.34 compatibility code) was not.
Thanks. Peter, you want to take care of this?
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r12063 - in trunk/subversion: libsvn_ra_svn
svnserve
Posted by Greg Hudson <gh...@MIT.EDU>.
On Mon, 2004-11-29 at 11:21, kfogel@collab.net wrote:
> > + path = svn_path_canonicalize(path, entry->pool);
> 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.
It's not; "pool" would have been better.
> The lack of internal interface documentation in libsvn_ra_svn makes it
> hard to know the pool lifetimes, oh well.
pool lives for the lifetime of the function call and any object it
returns to the caller (which in this case is nothing). entry->pool
lives as long as the entry.
This is true of essentially every function we have which takes a pool,
and of essentially every object we have which contains a pool. Why
document it for every function? Should we also document what pools are
and how they work, for every function?
> 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.
Both should be using pool. The only reason to use entry->pool is if
you're creating an object which is going to live in the entry.
The discrepancy is because editorp.c (the code which normally gets used)
was fixed, while editor.c (the pre-0.34 compatibility code) was not.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r12063 - in trunk/subversion: libsvn_ra_svn svnserve
Posted by kf...@collab.net.
"Peter N. Lundblad" <pe...@famlundblad.se> writes:
> The reason to use subpools here is to save memory when doing large edits.
> When the file/dir is closed, the subpool is destroyed. There is no reason
> to keep the path around any longer either.
That makes sense to me (though entry->pool is sort of orthogonal to a
subpool, so I'm not sure how much it should be treated like one.)
You had already seen Greg Hudson's response when you wrote this, or
not?
> I'm pretty sure this is an oversight. I considered changing it, but did
> want to keep the whole change minimal for back-porting.
Understood. I agree it should be a separate change, if it is changed.
> What about adding something like "Paths should be in canonical form when
> sent over the protocol. However, as a matter of input validation, a
> protocol implementation should always canonicalize received paths if it
> needs them in canonical form internally." You could probably word it
> better, but you get the idea.
I like your wording, actually. Go for it...
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r12063 - in trunk/subversion: libsvn_ra_svn
svnserve
Posted by Greg Hudson <gh...@MIT.EDU>.
On Mon, 2004-11-29 at 15:01, Peter N. Lundblad wrote:
> > > + path = svn_path_canonicalize(path, entry->pool);
> > 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 reason to use subpools here is to save memory when doing large edits.
> When the file/dir is closed, the subpool is destroyed. There is no reason
> to keep the path around any longer either.
This argument doesn't make sense. The caller is already using a subpool
which is cleared between invocations of editor commands. "pool" is a
much better choice for temporary allocations than entry->pool, which may
live considerably longer.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r12063 - in trunk/subversion: libsvn_ra_svn svnserve
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Mon, 29 Nov 2004 kfogel@collab.net wrote:
> lundblad@tigris.org writes:
> > --- trunk/subversion/libsvn_ra_svn/editor.c (original)
> > 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 reason to use subpools here is to save memory when doing large edits.
When the file/dir is closed, the subpool is destroyed. There is no reason
to keep the path around any longer either.
> 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 :-).
>
Please read it. Then you could rewrite in Java - err, I mean port it. <g>
> > + 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.
>
I'm pretty sure this is an oversight. I considered changing it, but did
want to keep the whole change minimal for back-porting.
> > @@ -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.
>
[snip]
> 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.
>
What about adding something like "Paths should be in canonical form when
sent over the protocol. However, as a matter of input validation, a
protocol implementation should always canonicalize received paths if it
needs them in canonical form internally." You could probably word it
better, but you get the idea.
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org