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

[FUJIWARA Katsunori]

At Tue, 17 Nov 2015 22:29:32 +0900,
Yuya Nishihara wrote:
> 
> 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)

Year, after posting this series, I noticed that the only way to fix
issue4378 completely is sorround whole 'commands.commit()' with wlock
and lock scope for 'repo.branchheads()' at the beginning of
'commands.commit()' to show commit status ("new heads" or so) at the
end of it as you said, too.

    https://selenic.com/repo/hg/file/8a256cee72c8/mercurial/commands.py#l1523

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

After surrounding whole 'commands.commit()' with wlock and (s)lock
scope, invalidating changelog and so on at 'repo.wlock()' (like patch
#1 of this series) seems redundant, from point of view of Mercurial
source tree.

AFAIK, 'repo.branchheads()', 'cmdutil.mergeeditform()' and so on are
invoked inside wlock/(s)lock scope, except for current "hg commit"
without --amend code path.

On the other hand, patch #1 is still useful for 3rd party extensions,
which invoke 'repo.commit()' directly without acquisition of
wlock/(s)lock and expect it to refer recent 'dirstate.parents()'
correctly.

Of course, ignoring such case is also reasonable, because Python API
is non-official and no-warranty :-)

What should we do ?

----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy at lares.dti.ne.jp