On Tue, 2004-12-14 at 12:10, kfogel@collab.net wrote:
> VK Sameer <sameer@collab.net> writes:
>
> I guess the warning doesn't need to be so strict anymore, since the
> patch is getting closer and closer :-).
OK, removed.
> > * subversion/tests/clients/cmdline/commit_tests.py
> > (tab_test): New test.
> > (test_list): Run it XFail.
> > (commit_uri_unsafe): Removed tab test
>
> For the last line, say "tab test portion moved to new test above" or
> something, to make it clear what's going on here.
OK.
> > +svn_boolean_t svn_path_is_valid (const char *path,
> > + apr_pool_t *pool,
> > + svn_boolean_t check_utf8);
> > +
>
> Oops, I think that just slipped in there :-).
Yup, gone.
> Although normally we assume UTF8 strings as input, here it might be
> good to explicitly state that the string must be UTF8. Because this
> is a validation function, and people might otherwise assume that it
> will validate that the string is valid UTF8, among other things.
OK.
> > --- subversion/libsvn_wc/update_editor.c (revision 12257)
> > +++ subversion/libsvn_wc/update_editor.c (working copy)
> > @@ -1014,6 +1014,7 @@
> > || ((! copyfrom_path) && (SVN_IS_VALID_REVNUM (copyfrom_revision))))
> > abort();
> >
> > + SVN_ERR (svn_path_check_valid (db->path, db->pool));
> > /* There should be nothing with this name. */
> > SVN_ERR (svn_io_check_path (db->path, &kind, db->pool));
> > if (kind != svn_node_none)
> > @@ -1431,10 +1432,12 @@
> > const svn_wc_entry_t *entry;
> > svn_node_kind_t kind;
> > svn_wc_adm_access_t *adm_access;
> > + apr_pool_t *subpool;
> >
> > /* the file_pool can stick around for a *long* time, so we want to use
> > a subpool for any temporary allocations. */
> > - apr_pool_t *subpool = svn_pool_create (pool);
> > + subpool = svn_pool_create (pool);
> > + SVN_ERR (svn_path_check_valid (path, subpool));
Sorry, I meant to use pool instead. The thought was that this would
avoid increasing memory usage just before erroring out. Updated in
patch.
> Everything else looks good. If any of the call sites look like they
> should be doing UTF8 testing as well, then this would be a good time
> to add that.
OK, will do.
> > +def tab_test(sbox):
> > + "tab testing"
> > +
> > + # ripped out of commit_uri_unsafe - currently must be used with an XFail
> > + # add test for directory
>
> Why must it be used with XFail?
Only because I haven't figured out the testing code yet :) The easiest
way to test was to check whether a commit of a pathname with tab failed.
I'll fix that before my final version.
> You don't need to mention the source of the code, btw (unless the
> source has some special pertinence here?). Everything is
> cut-and-pasted from somewhere else.
It was more of a reminder for myself. But I'll take it out.
> So, is there any way we can test that a forbidden path gets the right
> error? Or would that make the test suite too unportable?
I should think so, just have to spend some time with the tests.
> If you moved the code, why leave commented-out bits? Just remove them
> entirely. It's all under version control anyway :-).
Yeah, just sloppy with test code :) Done.
> Why the commented-out "#tab_test" there?
Removed.
Thanks again for the review.
Regards
Sameer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Dec 14 12:09:08 2004