[PATCH] workingctx: unlink paths while holding the wlock

[Adrian Buehlmann]

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.

>> 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)