Re: Comment/code inconsistency: svn_path_get_absolute

On Mon, Jul 30, 2007 at 09:21:18AM -0400, C. Michael Pilato wrote:
> Malcolm Rowe wrote:
> > I'd go for the option of removing the canonicalisation from the
> > implementation: if the contract always required canonicalised paths, we
> > don't need to change it now just because we've been canonicalising by
> > mistake.
>
> +1.
>
> (Just don't let Stefan know.) ;-)
>

Okay. Could someone cast an eye over this patch? I'm fairly sure we
don't need to convert from UTF-8 paths to APR paths for URLs (and what
would that even mean?). I also assume that we should re-canonicalise
the result of apr_filepath_merge().

This will produce slightly different results for non-canonicalised URLs,
but that's okay: they're invalid for this function anyway.

[[[
* subversion/libsvn_subr/path.c
  (svn_path_get_absolute): Don't canonicalise incoming paths, since the
    API already requires canonical paths. Also stop roundtripping URL
    paths from UTF-8 to APR encoding and back.
]]]

Regards,
Malcolm

Index: subversion/libsvn_subr/path.c
===================================================================
--- subversion/libsvn_subr/path.c (revision 25886)
+++ subversion/libsvn_subr/path.c (working copy)
@@ -1088,29 +1088,26 @@ svn_path_get_absolute(const char **pabso
   char *buffer;
   apr_status_t apr_err;
   const char *path_apr;
 
- SVN_ERR(svn_path_cstring_from_utf8
- (&path_apr, svn_path_canonicalize(relative, pool), pool));
-
- if (svn_path_is_url(path_apr))
- {
- buffer = apr_pstrdup(pool, path_apr);
- }
- else
+ if (svn_path_is_url(relative))
     {
- apr_err = apr_filepath_merge(&buffer, NULL,
- path_apr,
- (APR_FILEPATH_NOTRELATIVE
- | APR_FILEPATH_TRUENAME),
- pool);
-
- if (apr_err)
- return svn_error_createf(SVN_ERR_BAD_FILENAME, NULL,
- _("Couldn't determine absolute path of '%s'"),
- svn_path_local_style(relative, pool));
+ *pabsolute = apr_pstrdup(pool, relative);
+ return SVN_NO_ERROR;
     }
 
+ SVN_ERR(svn_path_cstring_from_utf8(&path_apr, relative, pool));
+
+ apr_err = apr_filepath_merge(&buffer, NULL,
+ path_apr,
+ APR_FILEPATH_NOTRELATIVE
+ | APR_FILEPATH_TRUENAME,
+ pool);
+ if (apr_err)
+ return svn_error_createf(SVN_ERR_BAD_FILENAME, NULL,
+ _("Couldn't determine absolute path of '%s'"),
+ svn_path_local_style(relative, pool));
+
   SVN_ERR(svn_path_cstring_to_utf8(pabsolute, buffer, pool));
   *pabsolute = svn_path_canonicalize(*pabsolute, pool);
   return SVN_NO_ERROR;
 }