[PATCH V4] histedit: add support for exec/x command to histedit (issue4036)
Augie Fackler
raf at durin42.com
Thu Nov 5 15:03:11 UTC 2015
(+ some people that understand locking/transactions better than I do)
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)
Thanks for sticking with this! I've got only minor complains with the
histedit-specific parts (basically it wants to do smarter things when
obsolete markers are enabled), but I'm really not sure what to say
about the lock business. See below.
>
> Point of this command is to be able to do some automatic munging
> on one or several commits in a series.
>
> The basic contract is that it receives a clean working copy and is
> expected to leave a clean working copy if it exits 0. If either
> the command leaves the working copy dirty, or it exits non-zero,
> the histedit stops to give opportunity for user intervention.
>
> Exec command functionality is implemented in execute class. It is
> overriding fromrule class method to deal with initializing
> object in different way than other histeditactions. It takes
> state and cmd instead of state and node id of next changeset
> action to perform on.
>
> Run method of exec_ class at first updates to parent of current
> changeset,apply it and commit to obtain copy of current changeset
> in current directory. It is done to let cmd amend to this changeset
> and correctly perform changing history containing this changeset.
This has, historically, been the sticking point on exec. It bums me
out to need to force a rewrite of the node before the 'exec' even if
it's a noop change.
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?
>
> In run method locks are released right before running command to
> allow running hg commands inside it. Acquiring proper number of
> locks is done in finally block in histedit.py:544-545 line, by
> default repo.lock and repo.wlock wait forever for acquiring
> them.
>
> Caches are totally cleared while acquiring the lock again
> in following instructions(histedit.py:544-546):
>
> finally:
> self.state.wlock = repo.wlock(times=wlockheld)
> self.state.lock = repo.lock(times=lockheld)
> repo.invalidateall()
>
> repo.wlock invokes lock constructor with acquirefn callback set to
> localrepo.invalidatedirstate.
>
> repo.lock invokes lock constructor with acquirefn callback set to
> localrepo.invalidate.
>
> invalidateall function in localrepository has invocation only to
> invalidate and invalidatedirstate, so acquiring the locks should
> clear caches. Additional invocation to repo.invalidateall()
> doesnt introduce additional complexity, but protect from situation
> where extension overrides invalidateall() to clear additional caches
> used by it.
>
> 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]
> class histeditstate(object):
> @@ -502,6 +511,80 @@
> editor=editor)
> return repo.commitctx(new)
>
> +def _isdirtywc(repo):
> + s = repo.status()
> + return s.modified or s.added or s.removed or s.deleted
> +
> +class execute(histeditaction):
> + def __init__(self, state, cmd):
> + self.state = state
> + self.cmd = cmd
> +
> + @classmethod
> + def fromrule(cls, state, cmd):
> + return cls(state, cmd)
> +
> + 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()
> +
> + 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.
[snip correct-looking code]
> diff -r 58a309e9cf80 -r 90176aa09b56 mercurial/localrepo.py
> --- a/mercurial/localrepo.py Mon Oct 19 16:49:54 2015 +0200
> +++ b/mercurial/localrepo.py Sun Sep 13 17:11:46 2015 +0200
I'm not sure what this times parameter to the locking layer is all
about. Hopefully someone else can comment.
My suspicion is that all these localrepo changes that add some
features to wlock et al should be their own change with their own
commit message explaning what's going on.
[snip edits to localrepo]
>
> diff -r 58a309e9cf80 -r 90176aa09b56 mercurial/lock.py
> --- a/mercurial/lock.py Mon Oct 19 16:49:54 2015 +0200
> +++ b/mercurial/lock.py Sun Sep 13 17:11:46 2015 +0200
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.
> @@ -40,7 +40,7 @@
> _host = None
>
> def __init__(self, vfs, file, timeout=-1, releasefn=None, acquirefn=None,
> - desc=None, inheritchecker=None, parentlock=None):
> + desc=None, inheritchecker=None, parentlock=None, times=1):
> self.vfs = vfs
> self.f = file
> self.held = 0
> @@ -54,7 +54,7 @@
> self._inherited = False
> self.postrelease = []
> self.pid = self._getpid()
> - self.delay = self.lock()
> + self.delay = self.lock(times)
> if self.acquirefn:
> self.acquirefn()
>
> @@ -74,11 +74,11 @@
> # wrapper around os.getpid() to make testing easier
> return os.getpid()
>
> - def lock(self):
> + def lock(self, times=1):
> timeout = self.timeout
> while True:
> try:
> - self._trylock()
> + self._trylock(times)
> return self.timeout - timeout
> except error.LockHeld as inst:
> if timeout != 0:
> @@ -89,9 +89,9 @@
> raise error.LockHeld(errno.ETIMEDOUT, inst.filename, self.desc,
> inst.locker)
>
> - def _trylock(self):
> + def _trylock(self, times=1):
> if self.held:
> - self.held += 1
> + self.held += times
> return
> if lock._host is None:
> lock._host = socket.gethostname()
> @@ -101,7 +101,7 @@
> retry -= 1
> try:
> self.vfs.makelock(lockname, self.f)
> - self.held = 1
> + self.held = times
> except (OSError, IOError) as why:
> if why.errno == errno.EEXIST:
> locker = self._readlock()
> @@ -111,7 +111,7 @@
> # but the lockfile to not be removed.
> if locker == self.parentlock:
> self._parentheld = True
> - self.held = 1
> + self.held = times
> return
> locker = self._testlock(locker)
> if locker is not None:
> @@ -203,6 +203,12 @@
> self.acquirefn()
> self._inherited = False
>
> + def releaseall(self):
> + origheld = self.held
> + while self.held > 0:
> + self.release()
> + return origheld
> +
> def release(self):
> """release the lock and execute callback function if any
>
> diff -r 58a309e9cf80 -r 90176aa09b56 tests/test-histedit-arguments.t
[snip several obviously correct test changes]
> diff -r 58a309e9cf80 -r 90176aa09b56 tests/test-histedit-exec.t
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/tests/test-histedit-exec.t Sun Sep 13 17:11:46 2015 +0200
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.
> @@ -0,0 +1,246 @@
> + $ . "$TESTDIR/histedit-helpers.sh"
> +
> + $ cat >> $HGRCPATH <<EOF
> + > [extensions]
> + > histedit=
> + > EOF
> +
> + $ initrepo ()
> + > {
> + > hg init r
> + > cd r
> + > for x in a b c d e f g; do
> + > echo $x > $x
> + > hg add $x
> + > hg ci -m $x
> + > done
> + > }
> +
> + $ initrepo
> +
> +log before exec
> + $ hg log --graph
> + @ changeset: 6:3c6a8ed2ebe8
> + | tag: tip
> + | user: test
> + | date: Thu Jan 01 00:00:00 1970 +0000
> + | summary: g
> + |
> + o changeset: 5:652413bf663e
> + | user: test
> + | date: Thu Jan 01 00:00:00 1970 +0000
> + | summary: f
> + |
> + o changeset: 4:e860deea161a
> + | user: test
> + | date: Thu Jan 01 00:00:00 1970 +0000
> + | summary: e
> + |
> + o changeset: 3:055a42cdd887
> + | user: test
> + | date: Thu Jan 01 00:00:00 1970 +0000
> + | summary: d
> + |
> + o changeset: 2:177f92b77385
> + | user: test
> + | date: Thu Jan 01 00:00:00 1970 +0000
> + | summary: c
> + |
> + o changeset: 1:d2ae7f538514
> + | user: test
> + | date: Thu Jan 01 00:00:00 1970 +0000
> + | summary: b
> + |
> + o changeset: 0:cb9a9f314b8b
> + user: test
> + date: Thu Jan 01 00:00:00 1970 +0000
> + summary: a
> +
> +
> +exec echo "Hello World" on directory after picking last changeset
> + $ hg histedit 6 --commands - 2>&1 << EOF
> + > pick 6
> + > exec echo "Hello World"
> + > EOF
> + 0 files updated, 0 files merged, 1 files removed, 0 files unresolved
> + adding g
> + Hello World
> + 0 files updated, 0 files merged, 0 files removed, 0 files unresolved
> + saved backup bundle to $TESTTMP/r/.hg/strip-backup/3c6a8ed2ebe8-f7abffd4-backup.hg (glob)
> +
> +exec ls after revision 1
> + $ hg histedit 1 --commands - 2>&1 << EOF
> + > pick 1
> + > exec echo "ls output start:" && ls && echo "ls output ends"
> + > pick 2
> + > pick 3
> + > pick 4
> + > pick 5
> + > pick 6
> + > EOF
> + 0 files updated, 0 files merged, 6 files removed, 0 files unresolved
> + adding b
> + ls output start:
> + a
> + b
> + ls output ends
> + 0 files updated, 0 files merged, 0 files removed, 0 files unresolved
> + 0 files updated, 0 files merged, 0 files removed, 0 files unresolved
> + 0 files updated, 0 files merged, 0 files removed, 0 files unresolved
> + 0 files updated, 0 files merged, 0 files removed, 0 files unresolved
> + 0 files updated, 0 files merged, 0 files removed, 0 files unresolved
> + 0 files updated, 0 files merged, 0 files removed, 0 files unresolved
> + saved backup bundle to $TESTTMP/r/.hg/strip-backup/d2ae7f538514-1c1cf14d-backup.hg (glob)
> +
> +
> +exec adding b1 file in revision 1
> + $ hg histedit 1 --commands - 2>&1 << EOF
> + > pick 1
> + > exec echo b1 >> b1 && hg add b1 && hg commit --amend -m "b1"
> + > pick 2
> + > pick 3
> + > pick 4
> + > pick 5
> + > pick 6
> + > EOF
> + 0 files updated, 0 files merged, 6 files removed, 0 files unresolved
> + adding b
> + saved backup bundle to $TESTTMP/r/.hg/strip-backup/18a73c9c72cf-c8c1f3b6-amend-backup.hg (glob)
> + 0 files updated, 0 files merged, 0 files removed, 0 files unresolved
> + 0 files updated, 0 files merged, 0 files removed, 0 files unresolved
> + 0 files updated, 0 files merged, 0 files removed, 0 files unresolved
> + 0 files updated, 0 files merged, 0 files removed, 0 files unresolved
> + 0 files updated, 0 files merged, 0 files removed, 0 files unresolved
> + 0 files updated, 0 files merged, 0 files removed, 0 files unresolved
> + saved backup bundle to $TESTTMP/r/.hg/strip-backup/0808241031aa-aa7f6930-backup.hg (glob)
> +
> + $ hg log --graph
> + @ changeset: 6:4696a3362537
> + | tag: tip
> + | user: test
> + | date: Thu Jan 01 00:00:00 1970 +0000
> + | summary: g
> + |
> + o changeset: 5:401045adec09
> + | user: test
> + | date: Thu Jan 01 00:00:00 1970 +0000
> + | summary: f
> + |
> + o changeset: 4:e450e0e843f7
> + | user: test
> + | date: Thu Jan 01 00:00:00 1970 +0000
> + | summary: e
> + |
> + o changeset: 3:dcf604978dad
> + | user: test
> + | date: Thu Jan 01 00:00:00 1970 +0000
> + | summary: d
> + |
> + o changeset: 2:e6d5272deb83
> + | user: test
> + | date: Thu Jan 01 00:00:00 1970 +0000
> + | summary: c
> + |
> + o changeset: 1:887fd832d217
> + | user: test
> + | date: Thu Jan 01 00:00:00 1970 +0000
> + | summary: b1
> + |
> + o changeset: 0:cb9a9f314b8b
> + user: test
> + date: Thu Jan 01 00:00:00 1970 +0000
> + summary: a
> +
> +
> + $ hg log -f b -f b1
> + changeset: 1:887fd832d217
> + user: test
> + date: Thu Jan 01 00:00:00 1970 +0000
> + summary: b1
> +
> +
> +exec creating new file f1 after changeset 5 leaving working dir dirty
> + $ hg histedit 5 --commands - 2>&1 << EOF
> + > pick 5
> + > exec echo "f1" >> f
> + > pick 6
> + > EOF
> + 0 files updated, 0 files merged, 2 files removed, 0 files unresolved
> + adding f
> + Working copy dirty after exec "echo "f1" >> f".
> + Make changes as needed, you may commit or record as needed now.
> + When you are finished, run hg histedit --continue to resume.
> + [1]
> +
> +try to continue while working directory is dirty
> + $ hg histedit --continue
> + Working copy dirty after exec "echo "f1" >> f".
> + Make changes as needed, you may commit or record as needed now.
> + When you are finished, run hg histedit --continue to resume.
> + [1]
> +
> + $ hg commit -m "f1"
> + $ hg histedit --continue
> + 0 files updated, 0 files merged, 0 files removed, 0 files unresolved
> + 0 files updated, 0 files merged, 0 files removed, 0 files unresolved
> + saved backup bundle to $TESTTMP/r/.hg/strip-backup/401045adec09-e5efc200-backup.hg (glob)
> +
> + $ hg log --graph -r 5:
> + @ changeset: 7:037517e744e6
> + | tag: tip
> + | user: test
> + | date: Thu Jan 01 00:00:00 1970 +0000
> + | summary: g
> + |
> + o changeset: 6:bd6d5e3cbcf3
> + | user: test
> + | date: Thu Jan 01 00:00:00 1970 +0000
> + | summary: f1
> + |
> + o changeset: 5:06ff77bf2665
> + | user: test
> + | date: Thu Jan 01 00:00:00 1970 +0000
> + | summary: f
> + |
> +
> + $ hg log -f f
> + changeset: 6:bd6d5e3cbcf3
> + user: test
> + date: Thu Jan 01 00:00:00 1970 +0000
> + summary: f1
> +
> + changeset: 5:06ff77bf2665
> + user: test
> + date: Thu Jan 01 00:00:00 1970 +0000
> + summary: f
> +
> +
> +exec when command returns non zero value
> + $ hg histedit 7 --commands - 2>&1 << EOF
> + > exec exit 1
> + > pick 7
> + > EOF
> + 1 files updated, 0 files merged, 1 files removed, 0 files unresolved
> + reverting f
> + Return code of "exit 1" is 1 (expected zero).
> + Make changes as needed, you may commit or record as needed now.
> + When you are finished, run hg histedit --continue to resume.
> + [1]
> +
> + $ hg histedit --continue
> + 0 files updated, 0 files merged, 0 files removed, 0 files unresolved
> + 0 files updated, 0 files merged, 0 files removed, 0 files unresolved
> + saved backup bundle to $TESTTMP/r/.hg/strip-backup/bd6d5e3cbcf3-8113adfc-backup.hg (glob)
> +
> + $ hg log --graph -r 6:
> + @ changeset: 7:d994db4c657b
> + | tag: tip
> + | user: test
> + | date: Thu Jan 01 00:00:00 1970 +0000
> + | summary: g
> + |
> + o changeset: 6:41b4840c21f7
> + | user: test
> + | date: Thu Jan 01 00:00:00 1970 +0000
> + | summary: f1
> + |
> diff -r 58a309e9cf80 -r 90176aa09b56 tests/test-histedit-obsolete.t
> --- a/tests/test-histedit-obsolete.t Mon Oct 19 16:49:54 2015 +0200
> +++ b/tests/test-histedit-obsolete.t Sun Sep 13 17:11:46 2015 +0200
> @@ -56,6 +56,9 @@
> # d, drop = remove commit from history
> # m, mess = edit commit message without changing commit content
> #
> + # Between changes you can add this:
> + # x, exec = run command (the rest of the line) using shell
> + #
> 0 files updated, 0 files merged, 0 files removed, 0 files unresolved
> $ hg histedit 1 --commands - --verbose <<EOF | grep histedit
> > pick 177f92b77385 2 c
> diff -r 58a309e9cf80 -r 90176aa09b56 tests/test-histedit-outgoing.t
> --- a/tests/test-histedit-outgoing.t Mon Oct 19 16:49:54 2015 +0200
> +++ b/tests/test-histedit-outgoing.t Sun Sep 13 17:11:46 2015 +0200
> @@ -53,6 +53,9 @@
> # d, drop = remove commit from history
> # m, mess = edit commit message without changing commit content
> #
> + # Between changes you can add this:
> + # x, exec = run command (the rest of the line) using shell
> + #
> 0 files updated, 0 files merged, 0 files removed, 0 files unresolved
> $ cd ..
>
> @@ -85,6 +88,9 @@
> # d, drop = remove commit from history
> # m, mess = edit commit message without changing commit content
> #
> + # Between changes you can add this:
> + # x, exec = run command (the rest of the line) using shell
> + #
> 0 files updated, 0 files merged, 0 files removed, 0 files unresolved
> $ cd ..
>
> @@ -109,6 +115,9 @@
> # d, drop = remove commit from history
> # m, mess = edit commit message without changing commit content
> #
> + # Between changes you can add this:
> + # x, exec = run command (the rest of the line) using shell
> + #
> 0 files updated, 0 files merged, 0 files removed, 0 files unresolved
>
> test to check number of roots in outgoing revisions
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
More information about the Mercurial-devel
mailing list