[ftputil] Problem using FTPHost.makedirs()

[Roger Demetrescu]

Hi Stefan

On Thu, Nov 6, 2014 at 10:46 AM, Roger Demetrescu
<roger.demetrescu at gmail.com> wrote:
> Hi Stefan,
>
> On Wed, Nov 5, 2014 at 5:27 AM, Stefan Schwarzer
> <sschwarzer at sschwarzer.net> wrote:
>> Hi Roger :-)
>>
>> On 2014-11-04 18:04, Roger Demetrescu wrote:
>>> Ok... I did some more testes and found out that I am not able to see
>>> any files/directories inside:
>>>
>>> /aaa
>>> /aaa/bbb
>>
>> I'm always surprised about the anomalies of FTP servers,
>> although having seen a bunch in bug reports. ;-)
>
> So do I...  :)
>
>
>>
>>> I had to change FTPHost's makedirs implementation to this code:
>>
>>>     def makedirs(self, path, mode=None):
>>>         ...
>>>         path = ftputil.tool.as_unicode(path)
>>>         path = self.path.abspath(path)
>>>         directories = path.split(self.sep)
>>>         old_dir = self.getcwd()
>>>
>>>         try:
>>>             # Try to build the directory chain from the "uppermost" to
>>>             # the "lowermost" directory.
>>>             for index in range(1, len(directories)):
>>>                 # Re-insert the separator which got lost by using `path.split`.
>>>                 next_directory = self.sep + self.path.join(*directories[:index+1])
>>>                 try:
>>>                     self.chdir(next_directory)
>>>                 except ftputil.error.PermanentError:
>>>                     self.mkdir(next_directory)
>>>         finally:
>>>             self.chdir(old_dir)
>>>
>>> It works like a charm...   :)
>>
>> I'm glad you could make it work - and saved me some brooding
>> over the problem. ;-)
>>
>> When seeing your solution I was thinking about whether it
>> was a complete substitute (and hence could go into ftputil)
>> or if it was failing for some other cases the current
>> `makedirs` implementation covers. Particularly, could there
>> be reasons for `chdir` to fail, apart from the non-existence
>> of the directory? What about other problems?
>>
>> I can think of these issues:
>>
>> - The directory `next_directory` exists, but you can't
>>   change to it because you don't have execute permission (or
>>   some equivalent on the server).
>>
>>   In this case `mkdir` would try to create it and fail and
>>   thus raise an exception (as in the old implementation). If
>>   the directory existed and was the last in the chain of
>>   directories to create, `makedirs` should have succeeded
>>   though. So this is a case that the old implementation
>>   covers but not the new one.
>>
>>   If the directory is not the last in the chain, `chdir`
>>   would fail and `mkdir` as well and raise a
>>   `PermanentError`. The same would have happened with the
>>   old implementation and is in my opinion the expected
>>   behavior.
>>
>> - The "directory" `next_directory` exists, but is a file.
>>
>>   Here, `chdir` would fail and then the `mkdir` as well,
>>   which would be the expected behavior.
>>
>> - You aren't allowed to change to `/aaa` though you might be
>>   allowed to change to `/aaa/bbb`.
>>
>>   On Unix, you can't change to `/aaa/bbb` if you can't
>>   change to `/aaa` if I'm not mistaken. I don't know if the
>>   same is true for FTP servers. It might be but due to some
>>   anomaly it might not be. If this anomaly applies, the old
>>   implementation might work but not the new one.
>>
>> - What cases might I have forgotten? ...
>>
>> Can you think of a way to counter the above problems with
>> the new approach but still keeping the advantages of the old
>> and new one? Ok, the last bullet item seems obscure to me,
>> but the problem from the first one not so much. But even if
>> we were able to solve the problem from the first item, it
>> would mean we make `makedirs` work for one obscure case, but
>> fail for another obscure case. ;-/
>
>
> We could  mix the two approachs:
>
>
>
>         try:
>             self.chdir(next_directory)
>         except ftputil.error.PermanentError:
>             try:
>                 self.mkdir(next_directory)
>             except ftputil.error.PermanentError:
>                 if not self.path.isdir(next_directory):
>                     raise
>
>
> Does it solve all (existing for now) concerns ?   :)


Better put the full code:    :)



    # Ignore unused argument `mode`
    # pylint: disable=unused-argument
    def makedirs(self, path, mode=None):
        """
        Make the directory `path`, but also make not yet existing
        intermediate directories, like `os.makedirs`. The value
        of `mode` is only accepted for compatibility with
        `os.makedirs` but otherwise ignored.
        """
        path = ftputil.tool.as_unicode(path)
        path = self.path.abspath(path)
        directories = path.split(self.sep)
        old_dir = self.getcwd()
        try:
            # Try to build the directory chain from the "uppermost" to
            # the "lowermost" directory.
            for index in range(1, len(directories)):
                # Re-insert the separator which got lost by using `path.split`.
                next_directory = self.sep +
self.path.join(*directories[:index+1])
                try:
                    self.chdir(next_directory)
                except ftputil.error.PermanentError:
                    try:
                        self.mkdir(next_directory)
                    except ftputil.error.PermanentError:
                        # Find out the cause of the error. Re-raise the
                        # exception only if the directory didn't exist already,
                        # else something went _really_ wrong, e. g. there's a
                        # regular file with the name of the directory.
                        if not self.path.isdir(next_directory):
                            raise
        except:
            self.chdir(old_dir)









Best regards,

Roger