[PATCH 3 of 3 STABLE?] cmdutil: delay examination of dirstate parents until editor invocation

[Pierre-Yves David]

On 11/15/2015 01:14 AM, Yuya Nishihara wrote:
> On Sat, 14 Nov 2015 03:46:25 +0900, FUJIWARA Katsunori wrote:
>> # HG changeset patch
>> # User FUJIWARA Katsunori <foozy at lares.dti.ne.jp>
>> # Date 1447437921 -32400
>> #      Sat Nov 14 03:05:21 2015 +0900
>> # Branch stable
>> # Node ID f90cd44b9860347b58e04e2bf4228bb9566de3f5
>> # Parent  24e29f6c10ed52935786e394954e01af8b0e258b
>> cmdutil: delay examination of dirstate parents until editor invocation
>>
>> Before this patch, 'mergeeditform()' immediately examines dirstate
>> parents, even though it may be invoked outside wlock scope (normal "hg
>> commit" is typical case).
>>
>> This may show "ignoring unknown working parent" message occasionally,
>> because current dirstate parents may not be contained in changelog,
>> which is cached outside wlock scope (see issue4378 for detail).
>>
>> In such case, "editform" may have ".normal" suffix, even if the
>> working directory has actually two parents.
>>
>> This patch delays examination of dirstate parents until editor
>> invocation by returning callable object if examination of dirstate
>> parents is needed.
>>
>> This callable object is evaluated at external editor invocation, and
>> wlock should be already acquired at that point.
>>
>> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
>> --- a/mercurial/cmdutil.py
>> +++ b/mercurial/cmdutil.py
>> @@ -333,19 +333,26 @@ def logmessage(ui, opts):
>>       return message
>>
>>   def mergeeditform(ctxorbool, baseformname):
>> -    """return appropriate editform name (referencing a committemplate)
>> +    """return appropriate editform name or the callable object to get it
>>
>>       'ctxorbool' is either a ctx to be committed, or a bool indicating whether
>>       merging is committed.
>>
>> -    This returns baseformname with '.merge' appended if it is a merge,
>> -    otherwise '.normal' is appended.
>> +    This (or the returned callable object) returns baseformname with
>> +    '.merge' appended if it is a merge, otherwise '.normal' is appended.
>>       """
>>       if isinstance(ctxorbool, bool):
>>           if ctxorbool:
>>               return baseformname + ".merge"
>> -    elif 1 < len(ctxorbool.parents()):
>> -        return baseformname + ".merge"
>> +    else:
>> +        # delay accessing to changelog via localrepository.dirstate
>> +        # (see issue4378 for detail)
>> +        def geteditform():
>> +            if 1 < len(ctxorbool.parents()):
>> +                return baseformname + ".merge"
>> +            else:
>> +                return baseformname + ".normal"
>> +        return geteditform
>
> These patches make me think that the problem is the locking scope is too
> narrow to do commit consistently. Is it wrong to take both lock and wlock
> earlier, at cmdutil.commit() for example?

I agree with Yuya that we have a wider locking scope issue here. We 
should prepare the commit on a locked repository, otherwise we will end 
up with a strange state.

(also the branchheads() things is a bug we just uncovered)

Patch 1 seems correct in itself, Foozy, can you confirm we should still 
take it ?

-- 
Pierre-Yves David