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
Received on Mon Nov 29 21:02:51 2004