[PATCH V4] histedit: add support for exec/x command to histedit (issue4036)
Piotr Listkiewicz
piotr.listkiewicz at gmail.com
Thu Nov 12 07:21:49 CST 2015
>
> 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.
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 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
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20151112/d867bee1/attachment-0001.html>
More information about the Mercurial-devel
mailing list