Patchwork Purge crash on junction point

login
register
mail settings
Submitter Mathieu Cantin
Date Feb. 18, 2014, 11:08 p.m.
Message ID <e6277671f69e4f3e9e6be90a1a1a9308@VSVR-MX-STORE04.corp.coveo.com>
Download mbox | patch
Permalink /patch/3701/
State Deferred
Headers show

Comments

Mathieu Cantin - Feb. 18, 2014, 11:08 p.m.
# HG changeset patch
# User Mathieu Cantin <mcantin@coveo.com>
# Date 1392764497 18000
#      Tue Feb 18 18:01:37 2014 -0500
# Node ID bf485635a586a487a732ba27e122531235062da7
# Parent  0e2877f8605dcaf4fdf2ab7e0046f1f6f80161dd
purge: delete invalid junction point

When you have 2 folders linked to each other by a junction point. For exemple
"A" is a folder and "B" is a junction point to "A". "A" is deleted, then
os.listdir throws an error on "B" because the target of "B" doesn't exist
anymore.

\ No newline at end of file
Matt Mackall - Feb. 19, 2014, 10:16 p.m.
On Tue, 2014-02-18 at 23:08 +0000, Mathieu Cantin wrote:
> # HG changeset patch
> # User Mathieu Cantin <mcantin@coveo.com>
> # Date 1392764497 18000
> #      Tue Feb 18 18:01:37 2014 -0500
> # Node ID bf485635a586a487a732ba27e122531235062da7
> # Parent  0e2877f8605dcaf4fdf2ab7e0046f1f6f80161dd
> purge: delete invalid junction point
> 
> When you have 2 folders linked to each other by a junction point. For exemple
> "A" is a folder and "B" is a junction point to "A". "A" is deleted, then
> os.listdir throws an error on "B" because the target of "B" doesn't exist
> anymore.

Does this issue only exist with purge?

> +    def hasfilesindir(path):
> +        try:
> +            return len(os.listdir(path)) > 0
> +        except OSError:
> +            # It's a junction point with an invalid target

Is this really the only way os.listdir can get an OSError?
Mathieu Cantin - Feb. 19, 2014, 10:43 p.m.
>Does this issue only exist with purge?
Yes, I think. Our company has been using the fix for 1 or 2 weeks and I don't have others problems with junction points, but I always purge all repositories before pulling or updating (in the context of continuous integration).

>Is this really the only way os.listdir can get an OSError?
Yes, the only one other solution would be to call the win32api in order to know if the directory is a junction.

Mathieu
Matt Mackall - Feb. 23, 2014, 6:34 p.m.
On Wed, 2014-02-19 at 22:43 +0000, Mathieu Cantin wrote:
> >Does this issue only exist with purge?
> Yes, I think. Our company has been using the fix for 1 or 2 weeks and
> I don't have others problems with junction points, but I always purge
> all repositories before pulling or updating (in the context of
> continuous integration).

My point is purge isn't doing anything that doesn't happen elsewhere in
Mercurial (deleting files and directories), so this bug is almost
certainly more generic and thus wants a less specific fix.

> >Is this really the only way os.listdir can get an OSError?
> Yes, the only one other solution would be to call the win32api in order to know if the directory is a junction.

There are actually lots of ways listdir can get an OSError that have
nothing to do with junctions.. and should probably continue to stop
Mercurial with an error. Your patch breaks all those cases because it
doesn't try to distinguish between types of error.

Patch

diff -r 0e2877f8605d -r bf485635a586 hgext/purge.py
--- a/hgext/purge.py Sat Feb 15 22:09:32 2014 -0600
+++ b/hgext/purge.py Tue Feb 18 18:01:37 2014 -0500
@@ -95,6 +95,13 @@ 
             os.chmod(path, stat.S_IMODE(s.st_mode) | stat.S_IWRITE)
             os.remove(path)

+    def hasfilesindir(path):
+        try:
+            return len(os.listdir(path)) > 0
+        except OSError:
+            # It's a junction point with an invalid target
+            return False
+
     directories = []
     match = scmutil.match(repo[None], dirs, opts)
     match.explicitdir = match.traversedir = directories.append
@@ -105,6 +112,6 @@ 
         remove(removefile, f)

     for f in sorted(directories, reverse=True):
-        if match(f) and not os.listdir(repo.wjoin(f)):
+        if match(f) and not hasfilesindir(repo.wjoin(f)):
             ui.note(_('removing directory %s\n') % f)
-            remove(os.rmdir, f)
+            remove(os.rmdir, f)