FileReference / and parent

Hi All,

I previously suggested a change to Path>>/ which actually covered two
issues:

1. The handling of the parent directory notation, i.e. ".."
2. The construction of path segments when appending a string.

As Damien pointed out, the first issue needs a bit more consideration.

I think the second point is still problematic and can be addressed
separately.  In particular:

('/a/b/c' asFileReference / 'd/e/f') parent  "File @ /a/b/c"

I would expect the result to be "File @ /a/b/c/d/e"

The fix is straightforward (although someone may be able to propose a
more elgant solution):

--
/ aString
        | path additionalPath index |

        aString isEmptyOrNil
                ifTrue: [ Error signal: 'Path element cannot be empty or nil'].

        additionalPath := Path from: aString.
        path := self class new: self size + additionalPath size.
        path copyFrom: self.
        index := self size + 1.
        additionalPath do: [ :each |
                path at: index put: each.
                index := index + 1.
                ].
        ^ path
--

1. Do you agree with the proposed change?
2. (Assuming you agree): Should we target Pharo 6.0 or 7.0?
   On one side, this is clearly a bug, on the other, no one has reported
   it to date, so it isn't having a big impact.

Thanks,
Alistair

Hi Stef,

On Tue, Apr 04, 2017 at 04:33:56PM +0200, Stephane Ducasse wrote:
> Hi alistair
>
> We should target Pharo 70.

Yep, I saw Esteban's message saying he hopes 6.0 is only a week away, so
this can definitely wait.


> Now it is great that you help improving the file frameworks.
> Could you open a bug entry?

Yep, I was just waiting to see if there was lots of resistance to these
changes (which there doesn't seem to be).


> Do you have tests?

I was working on them when this email arrived. :-)

I expect it will take me a week to get them to the point I'm happy (this
is a part time hobby), but they'll definitely be part of the slice /
pull request when I submit it.

Cheers,
Alistair

>
> Stef
>
> On Mon, Apr 3, 2017 at 2:04 PM, Alistair Grant <[hidden email]>
> wrote:
>
> > Hi All,
> >
> > I previously suggested a change to Path>>/ which actually covered two
> > issues:
> >
> > 1. The handling of the parent directory notation, i.e. ".."
> > 2. The construction of path segments when appending a string.
> >
> > As Damien pointed out, the first issue needs a bit more consideration.
> >
> > I think the second point is still problematic and can be addressed
> > separately.  In particular:
> >
> > ('/a/b/c' asFileReference / 'd/e/f') parent  "File @ /a/b/c"
> >
> > I would expect the result to be "File @ /a/b/c/d/e"
> >
> > The fix is straightforward (although someone may be able to propose a
> > more elgant solution):
> >
> > --
> > / aString
> >         | path additionalPath index |
> >
> >         aString isEmptyOrNil
> >                 ifTrue: [ Error signal: 'Path element cannot be empty or
> > nil'].
> >
> >         additionalPath := Path from: aString.
> >         path := self class new: self size + additionalPath size.
> >         path copyFrom: self.
> >         index := self size + 1.
> >         additionalPath do: [ :each |
> >                 path at: index put: each.
> >                 index := index + 1.
> >                 ].
> >         ^ path
> > --
> >
> > 1. Do you agree with the proposed change?
> > 2. (Assuming you agree): Should we target Pharo 6.0 or 7.0?
> >    On one side, this is clearly a bug, on the other, no one has reported
> >    it to date, so it isn't having a big impact.
> >
> > Thanks,
> > Alistair
> >
> >

On Fri, Apr 07, 2017 at 08:43:50PM +0200, Esteban Lorenzano wrote:
>
> > On 7 Apr 2017, at 20:30, Alistair Grant <[hidden email]> wrote:
> >
> > Hi Dennis,
> >
> > On Fri, Apr 07, 2017 at 10:58:57AM +0200, Denis Kudriashov wrote:
> >> Hi Alistair.
> >>
> >> Do you open the issue?
> >> I found that it was already exist. Look at 13217
> >> <https://pharo.fogbugz.com/f/cases/13217/FS-basename-with-compound-path-string>
> >
> > I haven't opened it yet - as you suggested in the issue, given how busy
> > Esteban and everyone is with getting Pharo 6 out the door, I'm waiting
> > for Pharo 7 dev to start.  I've got the code and automated tests done,
> > although reading the issue has made me think that I should also review
> > the class comments.
> >
> > I'll add a comment to the issue and then submit the patch once the Pharo
> > 7 inbox is open.
>
> that's very good... thank you very much!
>
> Esteban

And of course, just after posting the proposed patch in the issue, I
found a bug with it.

After re-reading the Path class comment I've also realised that my patch
breaks the explicit behaviour in the comment.  But...

The class comment in Path (I belive) is wrong.

The class comment states that a slash (/) can be included in the file
name with "parent\/child\/", however it didn't work when I tried it, and
there are multiple answers in stackoverflow and stackexchange stating
that / and \0 (null character) are the two forbidden (ascii) characters
on linux.  Windows includes / (slash), \ (backslash) and several others.

Wikipedia has an article on it:
https://en.wikipedia.org/wiki/Filename#Reserved_characters_and_words

From a purely practical point of view, I think trying to be completely
general (meaning allowing the directory separator in a filename) in this
case causes many more problems with unexpected behaviour than it solves,
so I still would like to see this changed in Pharo 7 (once I fix my
patch and extend the automated tests :-)).

Cheers,
Alistair