Patchwork [4,of,7] util.copyfiles: do not try to create destination folders which already exist

login
register
mail settings
Submitter Angel Ezquerra
Date Nov. 25, 2015, 12:22 a.m.
Message ID <f275c1ff2148e806e45e.1448410948@waste.org>
Download mbox | patch
Permalink /patch/11634/
State Changes Requested
Headers show

Comments

Angel Ezquerra - Nov. 25, 2015, 12:22 a.m.
# HG changeset patch
# User Angel Ezquerra <angel.ezquerra@gmail.com>
# Date 1448320493 -3600
#      Tue Nov 24 00:14:53 2015 +0100
# Node ID f275c1ff2148e806e45ea308139b0ff28b74b270
# Parent  fd4449f9e1239bc269e7a842168ace286ee4dd9e
util.copyfiles: do not try to create destination folders which already exist

Before this the behavior of util.copyfiles was inconsistent. It would silently
overwrite existing files, but it would balk at copying over an existing folder.
It seems to make more sense to not try to create existing directories (but copy
their contents anyway). We will use this new behavior to unshare full repository
shares.

This change is potentially dangerous since it could change the behavior of users
of util.copyfiles. Before this change trying to copy into an existing directory
would result in an error, whereas now it will not. This seems to be a better
behavior, and the test suite works fine so it does not seem to be a problem in
practice.
Gregory Szorc - Nov. 28, 2015, 10:02 p.m.
On Tue, Nov 24, 2015 at 4:22 PM, Angel Ezquerra <angel.ezquerra@gmail.com>
wrote:

> # HG changeset patch
> # User Angel Ezquerra <angel.ezquerra@gmail.com>
> # Date 1448320493 -3600
> #      Tue Nov 24 00:14:53 2015 +0100
> # Node ID f275c1ff2148e806e45ea308139b0ff28b74b270
> # Parent  fd4449f9e1239bc269e7a842168ace286ee4dd9e
> util.copyfiles: do not try to create destination folders which already
> exist
>
> Before this the behavior of util.copyfiles was inconsistent. It would
> silently
> overwrite existing files, but it would balk at copying over an existing
> folder.
> It seems to make more sense to not try to create existing directories (but
> copy
> their contents anyway). We will use this new behavior to unshare full
> repository
> shares.
>
> This change is potentially dangerous since it could change the behavior of
> users
> of util.copyfiles. Before this change trying to copy into an existing
> directory
> would result in an error, whereas now it will not. This seems to be a
> better
> behavior, and the test suite works fine so it does not seem to be a
> problem in
> practice.
>
> diff --git a/mercurial/util.py b/mercurial/util.py
> --- a/mercurial/util.py
> +++ b/mercurial/util.py
> @@ -812,7 +812,8 @@
>          topic = _('copying')
>
>      if os.path.isdir(src):
> -        os.mkdir(dst)
> +        if not os.path.exists(dst):
> +            os.mkdir(dst)
>

This is an anti-pattern because it performs 2 filesystem calls. The proper
way to do this is to attempt to mkdir() and catch failures due to
error.EEXIST:

try:
    os.mkdir(p)
except OSError as e:
    if e.errno != errno.EEXIST:
        raise

If this doesn't exist as a utility function in util.py or scmutil.py, IMO
it should be made to exist.


>          for name, kind in osutil.listdir(src):
>              srcname = os.path.join(src, name)
>              dstname = os.path.join(dst, name)
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
>

Patch

diff --git a/mercurial/util.py b/mercurial/util.py
--- a/mercurial/util.py
+++ b/mercurial/util.py
@@ -812,7 +812,8 @@ 
         topic = _('copying')
 
     if os.path.isdir(src):
-        os.mkdir(dst)
+        if not os.path.exists(dst):
+            os.mkdir(dst)
         for name, kind in osutil.listdir(src):
             srcname = os.path.join(src, name)
             dstname = os.path.join(dst, name)