[PATCH] workingctx: unlink paths while holding the wlock

[Matt Mackall]

On Tue, 2011-05-24 at 20:48 +0200, Adrian Buehlmann wrote:
> On 2011-05-24 19:37, Matt Mackall wrote:
> > On Tue, 2011-05-24 at 15:27 +0200, Adrian Buehlmann wrote:
> >> 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?
> > 
> > This is what we've got version control for:
> > 
> > http://www.selenic.com/hg/diff/c6e6ca96a033/mercurial/localrepo.py
> > 
> > The message predates the introduction of unlinking in this function, so
> > it warns when we remove something without unlinking it.
> > 
> > Then someone came along and decided the warning only made sense for
> > unlinked files:
> > 
> > http://www.selenic.com/hg/diff/9770d260a405/mercurial/localrepo.py
> 
> Thanks.
> 
> But I still can't find a reason for why we should
> 
> (1) delete a set of files
> (2) check that the same files now don't exist any more
> 
> If we've unlinked them, then the unlink raised an exception or they are
> gone. What am I missing?
> 
> And the code raises OSError for anything else than ENOENT. So it can't
> be permission errors...
> 
> I'll send my patch below. I think I'm correct with my analysis.

I think you're right, and Brendan agrees.

-- 
Mathematics is the supreme nostalgia of our time.