[PATCH] workingctx: unlink paths while holding the wlock

[Adrian Buehlmann]

On 2011-05-24 14:46, Adrian Buehlmann wrote:
> # HG changeset patch
> # User Adrian Buehlmann <adrian at cadifra.com>
> # Date 1306238900 -7200
> # Node ID 17e7ee042dc451fae1363803ce10af8543bffd05
> # Parent  25137d99a5ed215f302ffc1793590bdbdb437b55
> workingctx: unlink paths while holding the wlock
> 
> diff --git a/mercurial/context.py b/mercurial/context.py
> --- a/mercurial/context.py
> +++ b/mercurial/context.py
> @@ -852,15 +852,15 @@
>              yield changectx(self._repo, a)
>  
>      def remove(self, list, unlink=False):
> -        if unlink:
> -            for f in list:
> -                try:
> -                    util.unlinkpath(self._repo.wjoin(f))
> -                except OSError, inst:
> -                    if inst.errno != errno.ENOENT:
> -                        raise
>          wlock = self._repo.wlock()
>          try:
> +            if unlink:
> +                for f in list:
> +                    try:
> +                        util.unlinkpath(self._repo.wjoin(f))
> +                    except OSError, inst:
> +                        if inst.errno != errno.ENOENT:
> +                            raise
>              for f in list:
>                  if unlink and os.path.lexists(self._repo.wjoin(f)):
>                      self._repo.ui.warn(_("%s still exists!\n") % f)

I'm wondering here: why do we check for the existence of the files
again, right after having unlinked them?

FWIW, if I apply the following patch on top, the testsuite is ok

# HG changeset patch
# User Adrian Buehlmann <adrian at cadifra.com>
# Date 1306241543 -7200
# Node ID 3641b251b2a34dc22dbd73cd4796c5e87cc9925c
# Parent  17e7ee042dc451fae1363803ce10af8543bffd05
workingctx.remove: don't stat file again after unlinking

we just unlinked it right before

diff --git a/mercurial/context.py b/mercurial/context.py
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -862,9 +862,7 @@
                         if inst.errno != errno.ENOENT:
                             raise
             for f in list:
-                if unlink and os.path.lexists(self._repo.wjoin(f)):
-                    self._repo.ui.warn(_("%s still exists!\n") % f)
-                elif self._repo.dirstate[f] == 'a':
+                if self._repo.dirstate[f] == 'a':
                     self._repo.dirstate.forget(f)
                 elif f not in self._repo.dirstate:
                     self._repo.ui.warn(_("%s not tracked!\n") % f)