[PATCH V4] histedit: add support for exec/x command to histedit (issue4036)

[FUJIWARA Katsunori]

At Thu, 5 Nov 2015 10:03:11 -0500,
Augie Fackler wrote:
> 
> (+ some people that understand locking/transactions better than I do)

(cc to Siddharth as a expert of lock inheriting)

> On Wed, Oct 28, 2015 at 09:15:58AM +0100, liscju wrote:
> > # HG changeset patch
> > # User liscju <piotr.listkiewicz at gmail.com>
> > # Date 1442157106 -7200
> > #      Sun Sep 13 17:11:46 2015 +0200
> > # Node ID 90176aa09b5619526cd8b25b23f7a3649e27f0df
> > # Parent  58a309e9cf80d74d96e8c56cb95be20a4b130092
> > histedit: add support for exec/x command to histedit (issue4036)

[snip]

> > diff -r 58a309e9cf80 -r 90176aa09b56 hgext/histedit.py
> > --- a/hgext/histedit.py	Mon Oct 19 16:49:54 2015 +0200
> > +++ b/hgext/histedit.py	Sun Sep 13 17:11:46 2015 +0200
> > @@ -40,6 +40,9 @@

[snip]

> > +    def run(self):
> > +        repo = self.state.repo
> > +        node = self.state.parentctxnode
> > +
> > +        hg.update(repo, repo[node].parents()[0])
> 
> Nit: I *think* this 'hg.update()' call is always going to be a noop.

> > +        applychanges(repo.ui, repo, repo[node], {})
> > +        self._commit()

As far as I confirmed, this 'run()' implementation causes problems
below:

  - if 'exec' is the 1st action in rule file:

    The parent revision of the root of histedit targets is duplicated
    with 'histedit_source' extra data, and original one is stripped at
    the end of successful histedit-ing.

    For example, after histedit-ing revision N and its descendant,
    revision N - 1 has 'histedit_source=ORIGINAL_HASH_OF_N-1' extra
    data unintentionally.

  - otherwise:

    'histedit_source' of the current parent, which is embedded by
    previous histedit action, is always overwritten by 'exec' action,
    because 'self._commit()' of 'exec' uses 'commitfuncfor()' with the
    current parent revision.

    This makes finding actual histedit source out difficult.


> > +        applychanges(repo.ui, repo, repo[node], {})
> > +        self._commit()
> > +
> > +        lockheld = self.state.lock.releaseall()
> > +        wlockheld = self.state.wlock.releaseall()
> > +        try:
> > +            return_code = repo.ui.system(self.cmd, cwd=repo.root)
> > +        except OSError:
> > +            raise error.InterventionRequired(
> > +                self._getinterventionmsg(
> > +                    _('Execution of "%s" failed.') % self.cmd))
> > +        finally:
> > +            self.state.wlock = repo.wlock(times=wlockheld)
> > +            self.state.lock = repo.lock(times=lockheld)
> > +            repo.invalidateall()
> > +
> 
> This is really the meat of it, and I'm not sure if the locking is correct.

IMHO, to avoid lock interception at task switching, lock inheriting
mechanism (available since 3.6-rc or so) seems suitable for such
perpose, even though both lock(store lock) and wlock should be
inherited in this situation.

    https://selenic.com/repo/hg/file/db68fc04875c/mercurial/scmutil.py#l1155


BTW, to make in-memory dirstate changes visible to sub-process
certainly, we should execute code path below before spawning
sub-process, like 'cmdutil.commitforceeditor()':

    https://selenic.com/repo/hg/file/db68fc04875c/mercurial/cmdutil.py#l2719

    # for safety, even though this is also checked in
    # localrepo._wlockchecktransaction :-)
    assert not repo.currenttransaction()

    repo.dirstate.write(None)

    # to re-load forcibly from .hg/dirstate at next access
    repo.dirstate.invalidate()

(I'll post this patch)

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