[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