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

[Yuya Nishihara]

On Mon, 16 Nov 2015 18:54:00 -0800, Pierre-Yves David wrote:
> 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 ?

I consider the patch 1 is also a locking scope issue. If repo.lock is taken,
dirstate.parents() can see the fresh changelog after repo.invalidate().