[PATCH] apr_filepath_merge

[Sander Striker <st...@apache.org>]

Hi,


This patch is trying to solve the following problems:

1)  apr_filepath_merge("", ".") returns "" instead of "."

This is a one line fix, but needs to be reviewed in the win32
case, since a check for "/. /" is missed due to this patch.


2)  apr_filepath_merge(".", "") returns "./"

Is a bit harder.  I wanted the function to exit as early as
possible, so after the basic checks we just copy basepath and
exit if addpath == "".

I wasn't sure how to incorporate this change in the win32
function, since that is really a nasty piece of code.  If
someone familiar with that code(wrowe?) could take a look at it,
I would appreciate it.


Sander


Index: file_io/unix/filepath.c
===================================================================
RCS file: /home/cvs/apr/file_io/unix/filepath.c,v
retrieving revision 1.16
diff -u -r1.16 filepath.c
--- file_io/unix/filepath.c     26 Jul 2002 07:55:57 -0000      1.16
+++ file_io/unix/filepath.c     28 Jul 2002 09:41:43 -0000
@@ -183,6 +183,19 @@
          */
     }

+    if (addpath[0] == '\0') {
+        /* If there is nothing being merged in, simply return a dup
+         * of the rootpath.  If APR_FILEPATH_NOTRELATIVE is specified,
+         * the caller requires an absolute result.  The rootpath
+         * has to meet this condition.
+         */
+        if (rootpath[0] != '/' && (flags & APR_FILEPATH_NOTRELATIVE))
+            return APR_ERELATIVE;
+
+        *newpath = apr_pstrdup(p, rootpath);
+        return APR_SUCCESS;
+    }
+
     rootlen = strlen(rootpath);
     maxlen = rootlen + strlen(addpath) + 4; /* 4 for slashes at start, after
                                              * root, and at end, plus trailing
@@ -231,7 +244,7 @@
         }
         seglen = next - addpath;

-        if (seglen == 0 || (seglen == 1 && addpath[0] == '.')) {
+        if (seglen == 0 || (seglen == 1 && addpath[0] == '.' && pathlen > 0)) {
             /* noop segment (/ or ./) so skip it
              */
         }
Index: file_io/win32/filepath.c
===================================================================
RCS file: /home/cvs/apr/file_io/win32/filepath.c,v
retrieving revision 1.25
diff -u -r1.25 filepath.c
--- file_io/win32/filepath.c    10 Jul 2002 06:01:12 -0000      1.25
+++ file_io/win32/filepath.c    28 Jul 2002 09:41:45 -0000
@@ -667,7 +667,7 @@
                 break;
         }

-        if (seglen == 0 || (seglen == 1 && addpath[0] == '.'))
+        if (seglen == 0 || (seglen == 1 && addpath[0] == '.' && pathlen > 0))
         {
             /* NOTE: win32 _hates_ '/ /' and '/. /' (yes, with spaces in there)
              * so eliminate all preconceptions that it is valid.

Re: [PATCH] apr_filepath_merge

["William A. Rowe, Jr." <wr...@rowe-clan.net>]

At 05:09 AM 7/28/2002, Sander Striker wrote:
>Hi,
>
>This patch is trying to solve the following problems:
>
>1)  apr_filepath_merge("", ".") returns "" instead of "."
>
>This is a one line fix, but needs to be reviewed in the win32
>case, since a check for "/. /" is missed due to this patch.

Strangely enough, the existing behavior is correct.  apr_filepath_merge
should always eliminate all identity and parent-identity objects.  It's a
canonicalization function, so it cannot allow ambiguity like that.

Once you've apr_filepath_merge'd a path, you EXPECT to be able to
use it as a basepath.  But the basepath is never parsed to collapse these
constructs, since it's presumed to be canonical.

Please don't change the existing behavior in that respect... if you want to
test for and treat the result "" as ".", or "./" or whatever... feel free, 
but the
existing behavior needs to remain.

>2)  apr_filepath_merge(".", "") returns "./"
>
>Is a bit harder.  I wanted the function to exit as early as
>possible, so after the basic checks we just copy basepath and
>exit if addpath == "".
>
>I wasn't sure how to incorporate this change in the win32
>function, since that is really a nasty piece of code.  If
>someone familiar with that code(wrowe?) could take a look at it,
>I would appreciate it.

Well, if the left hand side is "." then it remains dot.  that's why you
get this behavior.

Remember the LHS MUST be canonical or the results are a bit
unpredictable, as you noticed.

I would accept a patch that if the RHS is an empty string, the LHS
is returned.  That's about all of this patch that I could accept.

Bill

RE: [PATCH] apr_filepath_merge

[Sander Striker <st...@apache.org>]

> From: Sander Striker [mailto:striker@apache.org]
> Sent: 28 July 2002 12:09

> Hi,
> 
> 
> This patch is trying to solve the following problems:
> 
> 1)  apr_filepath_merge("", ".") returns "" instead of "."
> 
> This is a one line fix, but needs to be reviewed in the win32
> case, since a check for "/. /" is missed due to this patch.
> 
> 
> 2)  apr_filepath_merge(".", "") returns "./"

I meant to say 'instead of "."'  in case it wasn't clear.
 
> Is a bit harder.  I wanted the function to exit as early as
> possible, so after the basic checks we just copy basepath and
> exit if addpath == "".
> 
> I wasn't sure how to incorporate this change in the win32
> function, since that is really a nasty piece of code.  If
> someone familiar with that code(wrowe?) could take a look at it,
> I would appreciate it.
> 
> 
> Sander