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

FUJIWARA Katsunori foozy at lares.dti.ne.jp
Fri Nov 13 12:58:40 CST 2015


At Thu, 12 Nov 2015 14:21:49 +0100,
Piotr Listkiewicz wrote:
> 
> [1  <text/plain; UTF-8 (7bit)>]
> >
> > 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.
> 
> 
> You are totally right, i just don't know what should i do with it.

If 'exec' action mainly focuses on "amend"-ing (as the first step),
below solutions seem reasonable.

  - make 'verifyrules()' prevent rules from having 'exec' as the 1st
    action of it (for the former problem), and

  - use 'histedit_source' extra data of amending target explicitly
    (for the latter problem)

    for example:
    ====================
    diff --git a/hgext/histedit.py b/hgext/histedit.py
    --- a/hgext/histedit.py
    +++ b/hgext/histedit.py
    @@ -391,7 +391,7 @@ class histeditaction(object):
                 return ctx, []
             return ctx, [(self.node, (ctx.node(),))]
    
    -def commitfuncfor(repo, src):
    +def commitfuncfor(repo, src, histeditsource=None):
         """Build a commit function for the replacement of <src>
    
         This function ensure we apply the same treatment to all changesets.
    @@ -408,7 +408,10 @@ def commitfuncfor(repo, src):
                 repo.ui.setconfig('phases', 'new-commit', phasemin,
                                   'histedit')
                 extra = kwargs.get('extra', {}).copy()
    -            extra['histedit_source'] = src.hex()
    +            if histeditsource:
    +                extra['histedit_source'] = histeditsource
    +            else:
    +                extra['histedit_source'] = src.hex()
                 kwargs['extra'] = extra
                 return repo.commit(**kwargs)
             finally:
    @@ -574,7 +577,8 @@ class execute(histeditaction):
             rulectx = repo[node]
    
             editor = self.commiteditor()
    -        commit = commitfuncfor(repo, rulectx)
    +        histeditsource = rulectx.extra().get('histedit_source', None)
    +        commit = commitfuncfor(repo, rulectx, histeditsource)
    
             commit(text=rulectx.description(), user=rulectx.user(),
                    date=rulectx.date(), extra=rulectx.extra(), editor=editor)
    ====================

IMHO, in addition to it, code path below should have comment
explanation like below, because this requirement is difficult to
understand quickly at glance.

        # amending by external process requires this (duplicated)
        # revision, because (1) 'pick' before 'exec' may does nothing
        # for target revision (= keep it as it is) and (2) amending
        # original target revision is inhibited by stripwrapper
        hg.update(repo, repo[node].parents()[0])
        applychanges(repo.ui, repo, repo[node], {})
        self._commit()


BTW, 'execute.continueclean()' seems to have examine whether parent
revision is amended or not, like below:

    ====================
         def continueclean(self):
             ctx = self.state.repo['.']
             node = self.state.parentctxnode
    +        if ctx.node() == node:
    +            # nothing amend
    +            return ctx, []
             return ctx, [(node, (ctx.node(),))]
    ====================

Without this, the rule containing 'exec' actions both for amending and
NOP like below causes infinite loop in 'processreplacement()' at the
end of histedit:

    ====================
    pick 1
    exec hg commit --amend ....
    exec echo hello world
    ====================


> 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

IMHO, steps below make lock inheriting mechanism enable for 'exec'
action of histedit. But please ask Siddharth about detail of it :-)

  1. apply changes like below for 'repo.lock()':

     - https://selenic.com/repo/hg/rev/efd57cd6fd1d
     - https://selenic.com/repo/hg/rev/2a3fc0272e3f
     - https://selenic.com/repo/hg/rev/e72b62b154b0

  2. add "scmutil.wlocksub()" + "scmutil._locksub()" like function,
     which inherits both (store) lock and wlock


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

Sorry, this change is redundant, because I overlooked that
'lock.inherit()' implies below. Please forget paragraph above :-)

  - 'dirstate.write()' via 'lock.releasefn()', and
  - 'repo.invalidatedirstate()'via 'lock.acquirefn()'


> I definetely need more guidance with those, i am too nooby to understand
> this:P
>
> 2015-11-12 14:12 GMT+01:00 Piotr Listkiewicz <piotr.listkiewicz at gmail.com>:
> 
> > We might want to use repo[None].dirty(...) (= workingctx.dirty()).
> >
> >
> > I will changed it in Patch V5
> >
> > Perhaps this isn't an InterventionRequired error (code=1) because the
> >> specified
> >> command is wrong?
> >
> >
> > Could be, i just don't know what other error should i use
> >
> > If I understand it, new wlock (and lock) should be the same object to old
> >> one.
> >> I think that's the reason why localrepo keeps weakref to the active lock.
> >> (but I might be wrong.)
> >> But here, wlock (and lock) seem to be recreated. (tested by the following
> >> assertion.)
> >
> >
> > I dont understand if i should do sth about it
> >
> > It should be  errormsg + "\n" + _(...).
> >
> >
> > Changed in V5
> >
> > 2015-11-12 14:06 GMT+01:00 Piotr Listkiewicz <piotr.listkiewicz at gmail.com>
> > :
> >
> >> Thanks for sticking with this!
> >>
> >>
> >> My pleasure :P
> >>
> >> That said, there's enough demand for this I'm coming around to a
> >>> middle ground: can we conditionalize the copy of the change and only
> >>> do it if obsolete markers aren't enabled?
> >>
> >>
> >> I need more guidance with this. I don't even know what "obsolete markers"
> >> are, how should i get to know this etc.
> >>
> >> Nit: I *think* this 'hg.update()' call is always going to be a noop.
> >>
> >>
> >> I need more explanation, this update is (as You noticed) done to recreate
> >> current commit in order to always change old "current commit" to current
> >> commit after executing command.
> >>
> >> Reading this reinforces my thought earlier: this change is probably
> >>> three changes:
> >>> 1) introduce times feature to lock, explain what it does
> >>> 2) Add forwarding params to localrepo to expose the times feature
> >>> 3) histedit changes
> >>> Doing that change might help get good feedback from locking experts,
> >>> as right now the major complexity in this change is actually the
> >>> locking stuff, but that lede is kind of buried behind the user-visible
> >>> feature of histedit exec.
> >>
> >>
> >> Totally right, in patch V5 i will divide it
> >>
> >> Hmm. Can you work this into one of the existing histedit tests at the
> >>> end? It's probably not necessary to add a whole extra test file for this
> >>> feature.
> >>
> >>
> >> I thought it is a matter of convention - every histedit cmd has own test
> >> suite in test-histedit-*.t. If it is ok to break this convention for exec -
> >> sure i will , so let me know what you think.
> >>
> >>
> >> 2015-11-11 20:04 GMT+01:00 FUJIWARA Katsunori <foozy at lares.dti.ne.jp>:
> >>
> >>>
> >>> 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
> >>>
> >>
> >>
> >
> [2  <text/html; UTF-8 (quoted-printable)>]
> 

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


More information about the Mercurial-devel mailing list