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

[Augie Fackler]

On Wed, Aug 1, 2012 at 9:35 AM, Mads Kiilerich <mads at kiilerich.com> wrote:
> 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.

Okay, then I'll fix the patch to only grab the repo lock.

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

That's probably true, but OTOH there are cases in histedit where it
aborts and lets the user fiddle around, so getting this right is
probably a little subtle. I'll make sure we've got the locks around
the store right, since those are (IMO) the most worrying to have
wrong.

Can you share your lock analysis code so I can use it?

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