[PATCH] Replace svn_path_join() in libsvn_client/mergeinfo.c

[Daniel Näslund]

[[[
Replace svn_path_join() since there are conflicting definitions of a
path.

* subversion/libsvn_client/mergeinfo.c
  (svn_client__adjust_mergeinfo_source_paths): Replace svn_path_join()
    with svn_dirent_join(). The base is a key in svn_mergeinfo_t.
  (svn_client__get_wc_mergeinfo): Replace svn_path_join() with
    svn_relpath_join(). The base is created with svn_dirent_basename()
    that returns a relative path.
  (svn_client_mergeinfo_get_merged): Replace svn_path_join() with
    svn_uri_join(). We want to create full urls of svn_mergeinfo_t keys.

Patch by: Daniel Näslund <daniel@...>
]]]

Some background: In subversion/include/svn_mergeinfo.h svn_mergeinfo_t
is just a typedef for a apr_hash_t. The keys are described like this:

 * (c) @c svn_mergeinfo_t, called "mergeinfo".  A hash mapping merge
 *     source paths (@c const char *, starting with slashes) to
 *     non-empty rangelist arrays.  A @c NULL hash is used to represent
 *     no mergeinfo and an empty hash is used to represent empty
 *     mergeinfo.

I'm assuming that means a dirent.

If the keys in svn_mergeinfo_t is a dirent then in
svn_client_mergeinfo_get_merged() we create an url from a dirent.

svn_dirent_uri.h says:

 * When translating between local paths (dirents) and uris code should
 * always go via the relative path format.
 * E.g.
 *  - by truncating a parent portion from a path with svn_*_skip_ancestor(),
 *  - or by converting portions to basenames and then joining to existing paths.

But in svn_wc__node_get_base_rev() we just skip the leading '/' in the
join. It's shorter and easier to understand than using
svn_dirent_skip_ancestor() and svn_dirent_basename. So I let it stay
that way.

/Daniel

[mergeinfo_path_join.diff]

---

Index: subversion/libsvn_client/mergeinfo.c
===================================================================
--- subversion/libsvn_client/mergeinfo.c (revision 40402)
+++ subversion/libsvn_client/mergeinfo.c (arbetskopia)
@@ -145,7 +145,7 @@
 
       /* Copy inherited mergeinfo into our output hash, adjusting the
          merge source as appropriate. */
-      path = svn_path_join(merge_source, rel_path, pool);
+      path = svn_dirent_join(merge_source, rel_path, pool);
       copied_rangelist = svn_rangelist_dup(rangelist, pool);
       apr_hash_set(adjusted_mergeinfo, path, APR_HASH_KEY_STRING,
                    copied_rangelist);
@@ -222,9 +222,9 @@
 
           /* No explicit mergeinfo on this path.  Look higher up the
              directory tree while keeping track of what we've walked. */
-          walk_relpath = svn_path_join(svn_dirent_basename(local_abspath,
-                                                           iterpool),
-                                       walk_relpath, result_pool);
+          walk_relpath = svn_relpath_join(svn_dirent_basename(local_abspath,
+                                                              iterpool),
+                                          walk_relpath, result_pool);
           local_abspath = svn_dirent_dirname(local_abspath, scratch_pool);
 
           SVN_ERR(svn_wc__node_get_base_rev(&parent_base_rev,
@@ -1587,7 +1587,7 @@
           const char *source_url;
 
           source_url = svn_path_uri_encode(key, pool);
-          source_url = svn_path_join(repos_root, source_url + 1, pool);
+          source_url = svn_uri_join(repos_root, source_url + 1, pool);
           apr_hash_set(full_path_mergeinfo, source_url,
                        APR_HASH_KEY_STRING, val);
         }

---

[Daniel Näslund]

On Fri, Nov 06, 2009 at 07:39:28PM +0100, Daniel Näslund wrote:

> [[[
> Replace svn_path_join() since there are conflicting definitions of a
> path.
>
> * subversion/libsvn_client/mergeinfo.c
>   (svn_client__adjust_mergeinfo_source_paths): Replace svn_path_join()
>     with svn_dirent_join(). The base is a key in svn_mergeinfo_t.
>   (svn_client__get_wc_mergeinfo): Replace svn_path_join() with
>     svn_relpath_join(). The base is created with svn_dirent_basename()
>     that returns a relative path.
>   (svn_client_mergeinfo_get_merged): Replace svn_path_join() with
>     svn_uri_join(). We want to create full urls of svn_mergeinfo_t keys.
>
> Patch by: Daniel Näslund <daniel@...>
> ]]]

[...]

Make check passed.

/Daniel

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2415260

[Daniel Rall]

On Fri, Nov 6, 2009 at 10:39 AM, Daniel Näslund <daniel@...> wrote:

> [[[
> Replace svn_path_join() since there are conflicting definitions of a
> path.
>
> * subversion/libsvn_client/mergeinfo.c
>  (svn_client__adjust_mergeinfo_source_paths): Replace svn_path_join()
>    with svn_dirent_join(). The base is a key in svn_mergeinfo_t.
>  (svn_client__get_wc_mergeinfo): Replace svn_path_join() with
>    svn_relpath_join(). The base is created with svn_dirent_basename()
>    that returns a relative path.
>  (svn_client_mergeinfo_get_merged): Replace svn_path_join() with
>    svn_uri_join(). We want to create full urls of svn_mergeinfo_t keys.
>
> Patch by: Daniel Näslund <daniel@...>
> ]]]
>
> Some background: In subversion/include/svn_mergeinfo.h svn_mergeinfo_t
> is just a typedef for a apr_hash_t. The keys are described like this:
>
>  * (c) @c svn_mergeinfo_t, called "mergeinfo".  A hash mapping merge
>  *     source paths (@c const char *, starting with slashes) to
>  *     non-empty rangelist arrays.  A @c NULL hash is used to represent
>  *     no mergeinfo and an empty hash is used to represent empty
>  *     mergeinfo.
>
> I'm assuming that means a dirent.

It's a path fragment. In this case, the path is relative to the
repository root. While you may be able to use it as a local file
system path in some situations, it's really a URL fragment.

> If the keys in svn_mergeinfo_t is a dirent then in
> svn_client_mergeinfo_get_merged() we create an url from a dirent.
>
> svn_dirent_uri.h says:
>
>  * When translating between local paths (dirents) and uris code should
>  * always go via the relative path format.
>  * E.g.
>  *  - by truncating a parent portion from a path with svn_*_skip_ancestor(),
>  *  - or by converting portions to basenames and then joining to existing paths.
>
> But in svn_wc__node_get_base_rev() we just skip the leading '/' in the
> join. It's shorter and easier to understand than using
> svn_dirent_skip_ancestor() and svn_dirent_basename. So I let it stay
> that way.
>
> /Daniel
>

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2415274

[Daniel Näslund]

On Fri, Nov 06, 2009 at 01:57:51PM -0800, Daniel Rall wrote:
> On Fri, Nov 6, 2009 at 10:39 AM, Daniel Näslund <daniel@...> wrote:

New log:

[[[
Remove use of deprecated svn_path_join().

* subversion/libsvn_client/mergeinfo.c
  (svn_client__adjust_mergeinfo_source_paths): Replace svn_path_join()
    with svn_uri_join(). An svn_mergeinfo_t key is the "base" parameter.
  (svn_client__get_wc_mergeinfo): Replace svn_path_join() with
    svn_relpath_join(). The "base" parameter is created with a relative
    path from svn_dirent_dirname().
  (svn_client_mergeinfo_get_merged): Replace svn_path_join() with
    svn_uri_join(). target_repos_root is the "base" parameter.
 
Patch by: Daniel Näslund <daniel@...>
Suggested by: dlr
]]]

> > Some background: In subversion/include/svn_mergeinfo.h svn_mergeinfo_t
> > is just a typedef for a apr_hash_t. The keys are described like this:
> >
> >  * (c) @c svn_mergeinfo_t, called "mergeinfo".  A hash mapping merge
> >  *     source paths (@c const char *, starting with slashes) to
> >  *     non-empty rangelist arrays.  A @c NULL hash is used to represent
> >  *     no mergeinfo and an empty hash is used to represent empty
> >  *     mergeinfo.
> >
> > I'm assuming that means a dirent.
>
> It's a path fragment. In this case, the path is relative to the
> repository root. While you may be able to use it as a local file
> system path in some situations, it's really a URL fragment.

After some reading I agree. But, they keys in svn_mergeinfo_t is not
uri_encoded! Or atleast they don't say anything about beeing
uri-encoded and when svn_client_mergeinfo_get_merged wants a hash of
full urls, the keys are being uri-encoded before the join! And in
logs_for_mergeinfo_rangelist() the following lines says that keys are
required to be repository-absolute but does not uri-encode the string:

[[[
  /* FILTER_LOG_ENTRY_BATON_T->TARGET_MERGEINFO_CATALOG's keys are required
     to be repository-absolute. */

  if (apr_hash_count(target_mergeinfo_catalog))
    {
      apr_hash_index_t *hi;
      svn_mergeinfo_catalog_t rekeyed_catalog = apr_hash_make(scratch_pool);

      for (hi = apr_hash_first(scratch_pool, target_mergeinfo_catalog);
           hi;
           hi = apr_hash_next(hi))
        {
          const char *path = svn_apr_hash_index_key(hi);

          if (!svn_dirent_is_absolute(path))
            apr_hash_set(rekeyed_catalog,
                         svn_dirent_join("/", path, scratch_pool),
                         APR_HASH_KEY_STRING,
                         svn_apr_hash_index_val(hi));
        }
      target_mergeinfo_catalog = rekeyed_catalog;
]]]

What I'm saying is that if repository-absolute means that it's an uri
(generalization of url) then it should be uri-encoded, right?

/Daniel

[mergeinfo_path_join.diff]

---

Index: subversion/libsvn_client/mergeinfo.c
===================================================================
--- subversion/libsvn_client/mergeinfo.c (revision 40421)
+++ subversion/libsvn_client/mergeinfo.c (arbetskopia)
@@ -145,7 +145,7 @@
 
       /* Copy inherited mergeinfo into our output hash, adjusting the
          merge source as appropriate. */
-      path = svn_path_join(merge_source, rel_path, pool);
+      path = svn_uri_join(merge_source, rel_path, pool);
       copied_rangelist = svn_rangelist_dup(rangelist, pool);
       apr_hash_set(adjusted_mergeinfo, path, APR_HASH_KEY_STRING,
                    copied_rangelist);
@@ -222,9 +222,9 @@
 
           /* No explicit mergeinfo on this path.  Look higher up the
              directory tree while keeping track of what we've walked. */
-          walk_relpath = svn_path_join(svn_dirent_basename(local_abspath,
-                                                           iterpool),
-                                       walk_relpath, result_pool);
+          walk_relpath = svn_relpath_join(svn_dirent_basename(local_abspath,
+                                                              iterpool),
+                                          walk_relpath, result_pool);
           local_abspath = svn_dirent_dirname(local_abspath, scratch_pool);
 
           SVN_ERR(svn_wc__node_get_base_rev(&parent_base_rev,
@@ -1587,7 +1587,7 @@
           const char *source_url;
 
           source_url = svn_path_uri_encode(key, pool);
-          source_url = svn_path_join(repos_root, source_url + 1, pool);
+          source_url = svn_uri_join(repos_root, source_url + 1, pool);
           apr_hash_set(full_path_mergeinfo, source_url,
                        APR_HASH_KEY_STRING, val);
         }

---

[Julian Foad]

On Sat, 2009-11-07, Daniel Näslund wrote:

> On Fri, Nov 06, 2009 at 01:57:51PM -0800, Daniel Rall wrote:
> > On Fri, Nov 6, 2009 at 10:39 AM, Daniel Näslund <daniel@...> wrote:
>
> New log:
>
> [[[
> Remove use of deprecated svn_path_join().
>
> * subversion/libsvn_client/mergeinfo.c
>   (svn_client__adjust_mergeinfo_source_paths): Replace svn_path_join()
>     with svn_uri_join(). An svn_mergeinfo_t key is the "base" parameter.
>   (svn_client__get_wc_mergeinfo): Replace svn_path_join() with
>     svn_relpath_join(). The "base" parameter is created with a relative
>     path from svn_dirent_dirname().
>   (svn_client_mergeinfo_get_merged): Replace svn_path_join() with
>     svn_uri_join(). target_repos_root is the "base" parameter.
>  
> Patch by: Daniel Näslund <daniel@...>
> Suggested by: dlr
> ]]]
>
> > > Some background: In subversion/include/svn_mergeinfo.h svn_mergeinfo_t
> > > is just a typedef for a apr_hash_t. The keys are described like this:
> > >
> > >  * (c) @c svn_mergeinfo_t, called "mergeinfo".  A hash mapping merge
> > >  *     source paths (@c const char *, starting with slashes) to
> > >  *     non-empty rangelist arrays.  A @c NULL hash is used to represent
> > >  *     no mergeinfo and an empty hash is used to represent empty
> > >  *     mergeinfo.
> > >
> > > I'm assuming that means a dirent.
> >
> > It's a path fragment. In this case, the path is relative to the
> > repository root. While you may be able to use it as a local file
> > system path in some situations, it's really a URL fragment.

It is not _really_ a URL fragment, it is a path fragment that
_represents_ a URL fragment.

> After some reading I agree. But, they keys in svn_mergeinfo_t is not
> uri_encoded! Or atleast they don't say anything about beeing
> uri-encoded and when svn_client_mergeinfo_get_merged wants a hash of
> full urls, the keys are being uri-encoded before the join! And in
> logs_for_mergeinfo_rangelist() the following lines says that keys are
> required to be repository-absolute but does not uri-encode the string:
>
> [[[
>   /* FILTER_LOG_ENTRY_BATON_T->TARGET_MERGEINFO_CATALOG's keys are required
>      to be repository-absolute. */
>
>   if (apr_hash_count(target_mergeinfo_catalog))
>     {
>       apr_hash_index_t *hi;
>       svn_mergeinfo_catalog_t rekeyed_catalog = apr_hash_make(scratch_pool);
>
>       for (hi = apr_hash_first(scratch_pool, target_mergeinfo_catalog);
>            hi;
>            hi = apr_hash_next(hi))
>         {
>           const char *path = svn_apr_hash_index_key(hi);
>
>           if (!svn_dirent_is_absolute(path))
>             apr_hash_set(rekeyed_catalog,
>                          svn_dirent_join("/", path, scratch_pool),
>                          APR_HASH_KEY_STRING,
>                          svn_apr_hash_index_val(hi));
>         }
>       target_mergeinfo_catalog = rekeyed_catalog;
> ]]]
>
> What I'm saying is that if repository-absolute means that it's an uri
> (generalization of url) then it should be uri-encoded, right?

No. When we call it a "repository-relative path" or a
"repository-absolute path" (both meaning the same thing) we mean it is a
plain path with no URI encoding.

- Julian

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2416127

[Daniel Näslund]

On Tue, Nov 10, 2009 at 12:37:10PM +0000, Julian Foad wrote:

> On Sat, 2009-11-07, Daniel Näslund wrote:
> > On Fri, Nov 06, 2009 at 01:57:51PM -0800, Daniel Rall wrote:
> > > On Fri, Nov 6, 2009 at 10:39 AM, Daniel Näslund <daniel@...> wrote:
> >
> > New log:
> >
> > [[[
> > Remove use of deprecated svn_path_join().
> >
> > * subversion/libsvn_client/mergeinfo.c
> >   (svn_client__adjust_mergeinfo_source_paths): Replace svn_path_join()
> >     with svn_uri_join(). An svn_mergeinfo_t key is the "base" parameter.
> >   (svn_client__get_wc_mergeinfo): Replace svn_path_join() with
> >     svn_relpath_join(). The "base" parameter is created with a relative
> >     path from svn_dirent_dirname().
> >   (svn_client_mergeinfo_get_merged): Replace svn_path_join() with
> >     svn_uri_join(). target_repos_root is the "base" parameter.
> >  
> > Patch by: Daniel Näslund <daniel@...>
> > Suggested by: dlr
> > ]]]
> >
> > > > Some background: In subversion/include/svn_mergeinfo.h svn_mergeinfo_t
> > > > is just a typedef for a apr_hash_t. The keys are described like this:
> > > >
> > > >  * (c) @c svn_mergeinfo_t, called "mergeinfo".  A hash mapping merge
> > > >  *     source paths (@c const char *, starting with slashes) to
> > > >  *     non-empty rangelist arrays.  A @c NULL hash is used to represent
> > > >  *     no mergeinfo and an empty hash is used to represent empty
> > > >  *     mergeinfo.
> > > >
> > > > I'm assuming that means a dirent.
> > >
> > > It's a path fragment. In this case, the path is relative to the
> > > repository root. While you may be able to use it as a local file
> > > system path in some situations, it's really a URL fragment.
>
> It is not _really_ a URL fragment, it is a path fragment that
> _represents_ a URL fragment.

> > What I'm saying is that if repository-absolute means that it's an uri
> > (generalization of url) then it should be uri-encoded, right?
>
> No. When we call it a "repository-relative path" or a
> "repository-absolute path" (both meaning the same thing) we mean it is a
> plain path with no URI encoding.

Ok. It's not uri encoded. No need since it's only used internally. But
we still call it an uri since it's part of an url. If that's the case
then this patch is ready.

/Daniel

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2416129

[Julian Foad]

Daniel Näslund wrote:
> [...] If that's the case then this patch is ready.

Thanks. Committed revision 40441.

- Julian

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2416168