[PATCH 1 of 2 STABLE] hooks: always include HG_PENDING
FUJIWARA Katsunori
foozy at lares.dti.ne.jp
Thu Nov 5 00:26:22 UTC 2015
At Tue, 3 Nov 2015 17:19:37 -0800,
Durham Goode wrote:
>
> # HG changeset patch
> # User Durham Goode <durham at fb.com>
> # Date 1446598693 28800
> # Tue Nov 03 16:58:13 2015 -0800
> # Branch stable
> # Node ID 0a772553285d61aeea3226f74698223b09fae7ac
> # Parent 58b7f3e93bbab749ab16c09df12aae5ba7880708
> hooks: always include HG_PENDING
>
> Previously we would only include HG_PENDING in the hook args if the
> transaction's writepending() actually wrote something. This is a bad criteria,
> since it's possible that a previous call to writepending() wrote stuff and the
> hooks want to still see that.
>
> The solution is to always have hooks execute within the scope of the pending
> changes by always putting HG_PENDING in the environment.
>
> diff --git a/mercurial/hook.py b/mercurial/hook.py
> --- a/mercurial/hook.py
> +++ b/mercurial/hook.py
> @@ -122,7 +122,8 @@ def _exthook(ui, repo, name, cmd, args,
> # make in-memory changes visible to external process
> tr = repo.currenttransaction()
> repo.dirstate.write(tr)
> - if tr and tr.writepending():
> + if tr:
> + tr.writepending()
> env['HG_PENDING'] = repo.root
>
> for k, v in args.iteritems():
'transaction.writepending()' saves whether there is any pending data
or not into 'self._anypending'.
https://selenic.com/repo/hg/file/e5a1df51bb25/mercurial/transaction.py#l350
@active
def writepending(self):
'''write pending file to temporary version
This is used to allow hooks to view a transaction before commit'''
categories = sorted(self._pendingcallback)
for cat in categories:
# remove callback since the data will have been flushed
any = self._pendingcallback.pop(cat)(self)
self._anypending = self._anypending or any
self._anypending |= self._generatefiles(suffix='.pending')
return self._anypending
It should return True as expected, if pending data has been written out
once.
The root cause of this issue seems that 'transaction.writepending()'
removes pending callback invoked once, even if pending callback didn't
actually write changes out yet.
This removal of callbacks is reasonable, if it is assumed that pending
callback may not be "idempotent" (e.g. writing into the file opened
with "append" mode).
But on the other hand, this removal may cause similar issue in other
code paths = other hook combinations (or introducing new hooks).
To avoid such similar issues:
- invoke 'changelog.delayupdate()' explicitly before
'transaction.writepending()' for external hook invocation, like
'dirstate.write(tr)' above
e.g. in _exthook()@hook.py
tr = repo.currenttransaction()
repo.dirstate.write(tr)
if tr:
repo.changelog.delayupdate(tr)
if tr.writepending()
env['HG_PENDING'] = repo.root
- invoke 'changelog.delayupdate()' internally in changelog by
overriding functions below, which open files in 'w'/'a' mode by
'self.opener'
- revlog.addgroup
- revlog.addrevision
- revlog.checkinlinesize
e.g.:
def addrevision(self, text, transaction, link, p1, p2, cachedelta=None,
node=None)
self.delayupdate(transaction)
return super(changelog, self).addrevision(.....)
The former still needs 'changelog.delayupdate()' before starting
changes to setup opener (and other internal status) of changelog, but
the latter doesn't.
I think the latter is reasonable. (yes, the former is just for
comparison :-)) How about these ?
BTW, #1 of this series causes that HG_PENDING is always passed to
external hooks while transaction running, even if there is no pending
data. Is this reasonable ? I think that this should be backed out to
avoid unintentional (trial) accessing to pending files.
> diff --git a/tests/test-hook.t b/tests/test-hook.t
> --- a/tests/test-hook.t
> +++ b/tests/test-hook.t
> @@ -113,7 +113,7 @@ test generic hooks
> $ hg pull ../a
> pulling from ../a
> searching for changes
> - prechangegroup hook: HG_SOURCE=pull HG_TXNID=TXN:* HG_URL=file:$TESTTMP/a (glob)
> + prechangegroup hook: HG_PENDING=$TESTTMP/b HG_SOURCE=pull HG_TXNID=TXN:* HG_URL=file:$TESTTMP/a (glob)
> adding changesets
> adding manifests
> adding file changes
> @@ -272,7 +272,7 @@ test that prepushkey can prevent incomin
> listkeys hook: HG_NAMESPACE=bookmarks HG_VALUES={'bar': '0000000000000000000000000000000000000000', 'foo': '0000000000000000000000000000000000000000'}
> no changes found
> pretxnopen hook: HG_TXNID=TXN:* HG_TXNNAME=push (glob)
> - prepushkey.forbid hook: HG_BUNDLE2=1 HG_KEY=baz HG_NAMESPACE=bookmarks HG_NEW=0000000000000000000000000000000000000000 HG_SOURCE=push HG_TXNID=TXN:* HG_URL=push (glob)
> + prepushkey.forbid hook: HG_BUNDLE2=1 HG_KEY=baz HG_NAMESPACE=bookmarks HG_NEW=0000000000000000000000000000000000000000 HG_PENDING=$TESTTMP/a HG_SOURCE=push HG_TXNID=TXN:* HG_URL=push (glob)
> pushkey-abort: prepushkey hook exited with status 1
> abort: exporting bookmark baz failed!
> [255]
> @@ -306,7 +306,7 @@ prechangegroup hook can prevent incoming
> $ hg pull ../a
> pulling from ../a
> searching for changes
> - prechangegroup.forbid hook: HG_SOURCE=pull HG_TXNID=TXN:* HG_URL=file:$TESTTMP/a (glob)
> + prechangegroup.forbid hook: HG_PENDING=$TESTTMP/b HG_SOURCE=pull HG_TXNID=TXN:* HG_URL=file:$TESTTMP/a (glob)
> abort: prechangegroup.forbid hook exited with status 1
> [255]
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
----------------------------------------------------------------------
[FUJIWARA Katsunori] foozy at lares.dti.ne.jp
More information about the Mercurial-devel
mailing list