Julian Foad wrote:
> On Tue, 2009-11-10 at 20:53 +0530, Kannan wrote:
> > -----BEGIN PGP SIGNED MESSAGE-----
> > Hash: SHA1
> >
> > Julian Foad wrote:
> > > On Tue, 2009-11-10 at 19:34 +0530, Kannan wrote:
> > >> -----BEGIN PGP SIGNED MESSAGE-----
> > >> Hash: SHA1
> > >>
> > >> Julian Foad wrote:
> > >>> The value of "fs_path" there comes from
> > >>> svn_client__path_relative_to_root(..., include_leading_slash=TRUE) which
> > >>> says, "Return the path of ABSPATH_OR_URL relative to the repository root
> > >>> (REPOS_ROOT) in REL_PATH (URI-decoded)".
> > >>>
> > >>> The dirent_uri API defines "relpath" as never having a leading slash, so
> > >>> we can't use svn_relpath_join(). The API does not specify whether a
> > >>> "uri" is URI-encoded, so we suppose we can use svn_uri_join().
> > >>>
> > >>> I think that way of using the APIs is not ideal, but it will do for now.
> > >> Thank you for the clarification, Julian.
> > >>>> And for
> > >>>> the other case(from your comments) use svn_dirent_join() as it points
> > >>>> only to local absolute paths, or I'm still not getting you?. But the
> > >>>> 'ls' tests seem to pass for me for the patch I sent, which is odd?
> > >>> Not sure what "other case" you are referring to.
> > >> Sorry for the brevity. Referred to the change to svn_uri_join() in the
> > >> "if" block of the same patch.
> > >
> > > Index: subversion/libsvn_client/list.c
> > > ===================================================================
> > > --- subversion/libsvn_client/list.c (revision 40358)
> > > +++ subversion/libsvn_client/list.c (working copy)
> > > @@ -86,11 +86,11 @@
> > >
> > > svn_pool_clear(iterpool);
> > >
> > > - path = svn_path_join(dir, item->key, iterpool);
> > > + path = svn_relpath_join(dir, item->key, iterpool);
> > >
> > > if (locks)
> > > {
> > > - const char *abs_path = svn_path_join(fs_path, path,
> > > iterpool);
> > > + const char *abs_path = svn_uri_join(fs_path, path, iterpool);
> > > lock = apr_hash_get(locks, abs_path, APR_HASH_KEY_STRING);
> > > }
> > > else
> > >
> > > That "if" block? But that's the one we have been talking about, since
> > > Dan Rall asked "Why use svn_uri_join() rather than svn_dirent_join()?"
> > > in this message:
> > > <http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2415197>.
> >
> > Oops, thought you answered for the svn_relpath_join()! So, the
> > svn_relpath_join() is fine then? (Bit confused)
>
> In the message
> <http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2416141>, I was talking about that "if" block. (You can follow the quoted context back to this earlier message: <http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2415197>.)
>
> I said "we can't use svn_relpath_join() [...] we suppose we can use
> svn_uri_join()". I meant that svn_uri_join() does the right thing. I
> wrote "we suppose" because I don't like the design of svn_uri_join()
> being used for this purpose, but it is better than using
> svn_path_join(). So this change is correct:
>
> if (locks)
> {
> - const char *abs_path = svn_path_join(fs_path, path,
> iterpool);
> + const char *abs_path = svn_uri_join(fs_path, path, iterpool);
> lock = apr_hash_get(locks, abs_path, APR_HASH_KEY_STRING);
> }
Oops, you were asking about the svn_relpath_join() outside the "if"
block. In that case, (copying from IRC so everyone can see ...)
The base is the "dir" parameter, which is "relative to the root of the
RA session". Therefore svn_dirent_join() is definitely wrong. You should
use svn_relpath_join() if it doesn't start with a slash, or
svn_uri_join() if it might start with a slash. From what I can see, it
doesn't start with a slash so your patch is correct in using
svn_relpath_join().
Looking back, I see that those are the only two changes in the patch, so
that means the patch is correct! I will commit it.
Committed revision 40443. Thanks for the patch!
- Julian
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2416202
Received on 2009-11-10 17:10:41 CET