[PATCH stable] histedit: add proper locking around repair.strip() calls

Mads Kiilerich mads at kiilerich.com
Wed Aug 1 09:35:16 CDT 2012


On 01/08/12 15:51, Augie Fackler wrote:
> On Aug 1, 2012, at 8:27 AM, Idan Kamara wrote:
>
>> On Wed, Aug 1, 2012 at 4:22 PM, Augie Fackler <raf at durin42.com> wrote:
>>> # HG changeset patch
>>> # User Augie Fackler <raf at durin42.com>
>>> # Date 1343772411 18000
>>> # Branch stable
>>> # Node ID 07af978f9019634f1cdcdac3c3d04de992038fe4
>>> # Parent  98166640b356b4c44bc87ce9137c7647eb728332
>>> histedit: add proper locking around repair.strip() calls
>> Why do you need a wlock?
> Hm, I probably don't. I'll look at the report from Mads again and correct that if necessary. I was copying other code that does locking around strip() calls. (Then again, I think strip reserves the right to modify the working copy, so maybe that's why it's required.)

( http://bz.selenic.com/show_bug.cgi?id=3566 )

I find it plausible that histedit can cause changes to the working 
directory and thus needs the wlock. But I agree and doubt they are 
necessary when adding tight locks around strip like done in this patch. 
repair.strip will AFAIK not update the working directory but assume it 
already has been done if it is necessary.

But I would expect that the locks should be added around bigger parts of 
the code than done here. I assume bad things could happen if another 
process modified the repository between the point where histedit figure 
out what to do and the point where it starts modifying the repo. I 
assume it ends up taking the wlock somewhere in the critical region, so 
wlock should be taken before taken lock.

/Mads
- who thinks locking needs bigger changes than just adding a read lock




More information about the Mercurial-devel mailing list