[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