[PATCH 3 of 3] util.makedirs: cleanup of termination of recursion

Adrian Buehlmann adrian at cadifra.com
Mon Aug 22 07:55:27 CDT 2011


On 2011-08-22 12:22, Mads Kiilerich wrote:
> On 08/22/2011 10:25 AM, Adrian Buehlmann wrote:
>> On 2011-08-22 10:07, Adrian Buehlmann wrote:
>>> If we want to postpone (and reorder) the parent calculation, we could do:
>>>
>>> diff --git a/mercurial/util.py b/mercurial/util.py
>>> --- a/mercurial/util.py
>>> +++ b/mercurial/util.py
>>> @@ -780,12 +780,12 @@
>>>
>>>   def makedirs(name, mode=None):
>>>       """recursive directory creation with parent mode inheritance"""
>>> -    parent = os.path.abspath(os.path.dirname(name))
>>>       try:
>>>           os.mkdir(name)
>>>       except OSError, err:
>>>           if err.errno == errno.EEXIST:
>>>               return
>>> +        parent = os.path.dirname(os.path.abspath(name))
>>>           if not name or parent == name or err.errno != errno.ENOENT:
>>>               raise
>>>           makedirs(parent, mode)
>>>
>>
>> or perhaps:
>>
>> # HG changeset patch
>> # User Adrian Buehlmann<adrian at cadifra.com>
>> # Date 1314001316 -7200
>> # Node ID 906f611f9c32ec362d7c4e9d67397ad05f232445
>> # Parent  5a50f36ec65f3eb2246407f46fdf9bf53e38aca7
>> util: postpone and reorder parent calculation in makedirs
>>
>> diff --git a/mercurial/util.py b/mercurial/util.py
>> --- a/mercurial/util.py
>> +++ b/mercurial/util.py
>> @@ -780,13 +780,15 @@
>>
>>   def makedirs(name, mode=None):
>>       """recursive directory creation with parent mode inheritance"""
>> -    parent = os.path.abspath(os.path.dirname(name))
>>       try:
>>           os.mkdir(name)
>>       except OSError, err:
>>           if err.errno == errno.EEXIST:
>>               return
>> -        if not name or parent == name or err.errno != errno.ENOENT:
>> +        if err.errno != errno.ENOENT or not name:
>> +            raise
>> +        parent = os.path.dirname(os.path.abspath(name))
>> +        if parent == name:
>>               raise
>>           makedirs(parent, mode)
>>           os.mkdir(name)
> 
> Agreed. I realized this morning while waking up that the assumption that 
> dirname 'reduces' the problem was wrong.
> 
> The path operations are however relatively cheap and most of the time we 
> won't get that far, so now I prefer:
> 
>          parent = os.path.dirname(os.path.abspath(name))
>          if err.errno != errno.ENOENT or not name or parent == name:
>              raise
> 
> I will ignore that we perhaps in principle should add some normpath and 
> realpath to this ...

I have a slight preference for my latest version, on the grounds that
it's best to check and raise for as much as we can before reaching for
more library calls (os.*).

That is, I prefer

def makedirs(name, mode=None):
    """recursive directory creation with parent mode inheritance"""
    try:
        os.mkdir(name)
    except OSError, err:
        if err.errno == errno.EEXIST:
            return
        if err.errno != errno.ENOENT or not name:
            raise
        parent = os.path.dirname(os.path.abspath(name))
        if parent == name:
            raise
        makedirs(parent, mode)
        os.mkdir(name)
    if mode is not None:
        os.chmod(name, mode)

because it's better to have the

        if err.errno != errno.ENOENT or not name:
            raise

check and raise done before reaching for more troubles by calling
into the platform again with

        parent = os.path.dirname(os.path.abspath(name))

It's not about speed, but more about correctness. Consider what happens
if the abspath call would throw.


More information about the Mercurial-devel mailing list