Re: revert_tests 15 and 17 fail in svn_ra_local__get_session_url

C. Michael Pilato wrote:
> Philip Martin wrote:
>> I haven't been running the tests much recently, but revert_tests 15
>> and 17 have been failing over ra_local for a couple of days at least.
>> The is_canonical assertion is failing in svn_path_join like this:
>>
>> (gdb) up
>> #4 0x00002ae7bcfe56aa in svn_ra_local__get_session_url (session=0x5435b0,
>> url=0x7fffeedefbe8, pool=0x5432f8)
>> at ../src/subversion/libsvn_ra_local/ra_plugin.c:330
>> 330 *url = svn_path_join(baton->repos_url,
>> (gdb) list
>> 325 apr_pool_t *pool)
>> 326 {
>> 327 svn_ra_local__session_baton_t *baton = session->priv;
>> 328 const char *fs_path = apr_pstrmemdup(pool, baton->fs_path->data,
>> 329 baton->fs_path->len);
>> 330 *url = svn_path_join(baton->repos_url,
>> 331 svn_path_uri_encode(fs_path + 1, pool),
>> 332 pool);
>> 333 return SVN_NO_ERROR;
>> 334 }
>> (gdb) p baton->fs_path
>> $1 = (svn_stringbuf_t *) 0x56b900
>> (gdb) p baton->fs_path[0]
>> $2 = {pool = 0x5432f8, data = 0x56b8f8 "", len = 0, blocksize = 1}
>>
>> Note that baton->fs_path is "" and yet fs_path+1 is being passed to
>> svn_path_uri_encode, that's obviously wrong. The apr_pstrmemdup call
>> looks unnecessary as well. Should this be fixed in ra_local or in tha
>> caller? Are other people seeing these failures?
>
> FS_PATH either should or should not have a leading slash -- inconsistency
> here is madness. If it has a leading slash, then the current code should be
> correct, and some caller is perhaps passing something bogus to the library.
> If it does *not* have a leading slash, then we should of course never try
> to advance past such a thing with a '+ 1' (which means your patch is incorrect).
>
> I'm not seeing any failures, and the documentation doesn't say for sure, but
> I think FS_PATH today *does* have a leading slash. Many RA-API-implementing
> functions in libsvn_ra_local build an "absolute filesystem path" by joining
> the relative path provided via the RA API to the FS_PATH stored in the
> session baton. Some of them just do the join; some do the join but first
> transform an empty FS_PATH into "/". It's all very inconsistent, and this
> inconsistency is not aided by the fact that the FS and REPOS APIs accept
> filesystem paths with or without leading slashes.
>
> My suggestion: decide whether or not FS_PATH does or doesn't have a leading
> slash, document in the session baton that decision, and enforce this in
> svn_ra_local__split_URL() and svn_ra_local__reparent().
>

Hrm... I wonder if svn_ra_local__reparent() is to blame for this problem.
It does this:

  svn_stringbuf_set(baton->fs_path,
                    svn_path_uri_decode(url + strlen(baton->repos_url),
                                        pool));

But in the case where we are asking to reparent to the same URL we were
already at, won't that result in an empty fs_path? This function should be
checking the result for emptiness, and making sure that fs_path has a
leading slash.

-- 
C. Michael Pilato <cmpilato@collab.net>
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand