[PATCH 1 of 2] strip: add a delayedstrip method that works in a transaction
Martin von Zweigbergk
martinvonz at google.com
Mon Jun 26 12:26:56 EDT 2017
On Sun, Jun 25, 2017 at 1:32 PM, Jun Wu <quark at fb.com> wrote:
> # HG changeset patch
> # User Jun Wu <quark at fb.com>
> # Date 1498412325 25200
> # Sun Jun 25 10:38:45 2017 -0700
> # Node ID b70ebbb5b3ffb417d8fe13548b39c9196fadda1f
> # Parent 87db210d4a5ade8d5d8b288a0c7fa65b79eedc08
> # Available At https://bitbucket.org/quark-zju/hg-draft
> # hg pull https://bitbucket.org/quark-zju/hg-draft -r b70ebbb5b3ff
> strip: add a delayedstrip method that works in a transaction
>
> For long, the fact that strip does not work inside a transaction and some
> code has to work with both obsstore and fallback to strip lead to duplicated
> code like:
>
> with repo.transaction():
> ....
> if obsstore:
> obsstore.createmarkers(...)
> if not obsstore:
> repair.strip(...)
>
> Things get more complex when you want to call something which may call strip
> under the hood. Like you cannot simply write:
>
> with repo.transaction():
> ....
> rebasemod.rebase(...) # may call "strip", so this doesn't work
>
> But you do want rebase to run inside a same transaction if possible, so the
> code may look like:
>
> with repo.transaction():
> ....
> if obsstore:
> rebasemod.rebase(...)
> obsstore.createmarkers(...)
> if not obsstore:
> rebasemod.rebase(...)
> repair.strip(...)
>
> That's ugly and error-prone. Ideally it's possible to just write:
>
> with repo.transaction():
> rebasemod.rebase(...)
> saferemovenodes(...)
>
> This patch is the first step towards that. It adds a "delayedstrip" method
> to repair.py which maintains a postclose callback in the transaction object.
Great idea!
>
> diff --git a/mercurial/repair.py b/mercurial/repair.py
> --- a/mercurial/repair.py
> +++ b/mercurial/repair.py
> @@ -254,4 +254,61 @@ def strip(ui, repo, nodelist, backup=Tru
> return backupfile
>
> +def safestriproots(ui, repo, nodes):
> + """return list of roots of nodes where descendants are covered by nodes"""
> + torev = repo.unfiltered().changelog.rev
> + revs = set(torev(n) for n in nodes)
> + # tostrip = wanted - unsafe = wanted - ancestors(orphaned)
> + # orphaned = affected - wanted
> + # affected = descendants(roots(wanted))
> + # wanted = revs
> + tostrip = set(repo.revs('%ld-(::((roots(%ld)::)-%ld))', revs, revs, revs))
> + notstrip = revs - tostrip
> + if notstrip:
> + nodestr = ', '.join(sorted(short(repo[n].node()) for n in notstrip))
> + ui.warn(_('warning: orphaned descendants detected, '
> + 'not stripping %s\n') % nodestr)
> + return [c.node() for c in repo.set('roots(%ld)', tostrip)]
> +
> +class stripcallback(object):
> + """used as a transaction postclose callback"""
> +
> + def __init__(self, ui, repo, backup, topic):
> + self.ui = ui
> + self.repo = repo
> + self.backup = backup
> + self.topic = topic or 'backup'
> + self.nodelist = []
> +
> + def addnodes(self, nodes):
> + self.nodelist.extend(nodes)
> +
> + def __call__(self, tr):
> + roots = safestriproots(self.ui, self.repo, self.nodelist)
> + if roots:
> + strip(self.ui, self.repo, roots, True, self.topic)
> +
> +def delayedstrip(ui, repo, nodelist, topic=None):
> + """like strip, but works inside transaction and won't strip irreverent revs
I think you meant s/irreverent/irrelevant/ :-) But to make it clearer,
maybe "descendants" is even better (assuming that's what it is about)?
> +
> + nodelist must explicitly contain all descendants.
That seems like a safer API in general. Would it be a lot of work to
move this into the normal strip() method?
> Otherwise a warning will
> + be printed that some nodes are not stripped.
Maybe this should be a ProgrammingError?
> +
> + Always do a backup.
Why? Is that more necessary for posthook strips than other strips?
>The last non-None "topic" will be used as the backup
> + topic name. The default backup topic name is "backup".
Why not use a separate backup for each call to delayedstrip()?
> + """
> + tr = repo.currenttransaction()
> + if not tr:
> + nodes = safestriproots(ui, repo, nodelist)
> + return strip(ui, repo, nodes, True, topic)
> + # transaction postclose callbacks are called in alphabet order.
> + # use '\xff' as prefix so we are likely to be called last.
Why do we want them to run a late as possible? Do we think other hooks
would want the to-be-stripped changesets to still be there when they
run?
> + callback = tr.getpostclose('\xffstrip')
> + if callback is None:
> + callback = stripcallback(ui, repo, True, topic)
> + tr.addpostclose('\xffstrip', callback)
> + if topic:
> + callback.topic = topic
> + callback.addnodes(nodelist)
> +
> def striptrees(repo, tr, striprev, files):
> if 'treemanifest' in repo.requirements: # safe but unnecessary
> diff --git a/mercurial/transaction.py b/mercurial/transaction.py
> --- a/mercurial/transaction.py
> +++ b/mercurial/transaction.py
> @@ -407,5 +407,5 @@ class transaction(object):
> @active
> def addpostclose(self, category, callback):
> - """add a callback to be called after the transaction is closed
> + """add or replace a callback to be called after the transaction closed
>
> The transaction will be given as callback's first argument.
> @@ -417,4 +417,9 @@ class transaction(object):
>
> @active
> + def getpostclose(self, category):
> + """return a postclose callback added before, or None"""
> + return self._postclosecallback.get(category, None)
> +
> + @active
> def addabort(self, category, callback):
> """add a callback to be called when the transaction is aborted.
> diff --git a/tests/test-strip.t b/tests/test-strip.t
> --- a/tests/test-strip.t
> +++ b/tests/test-strip.t
> @@ -3,4 +3,5 @@
> $ echo "[extensions]" >> $HGRCPATH
> $ echo "strip=" >> $HGRCPATH
> + $ echo "drawdag=$TESTDIR/drawdag.py" >> $HGRCPATH
>
> $ restore() {
> @@ -941,3 +942,51 @@ Error during post-close callback of the
> [255]
>
> +Use delayedstrip to strip inside a transaction
>
> + $ cd $TESTTMP
> + $ hg init delayedstrip
> + $ cd delayedstrip
> + $ hg debugdrawdag <<'EOS'
> + > D
> + > |
> + > C F H # Commit on top of "I",
> + > | |/| # Strip B+D+I+E+G+H+Z
> + > I B E G
> + > \|/
> + > A Z
> + > EOS
> +
> + $ hg up -C I
> + 2 files updated, 0 files merged, 0 files removed, 0 files unresolved
> + $ echo 3 >> I
> + $ cat > $TESTTMP/delayedstrip.py <<EOF
> + > from mercurial import repair, commands
> + > def reposetup(ui, repo):
> + > def getnodes(expr):
> + > return [repo.changelog.node(r) for r in repo.revs(expr)]
> + > with repo.wlock():
> + > with repo.lock():
> + > with repo.transaction('delayedstrip'):
> + > repair.delayedstrip(ui, repo, getnodes('B+I+Z+D+E'), 'J')
> + > repair.delayedstrip(ui, repo, getnodes('G+H+Z'), 'I')
> + > commands.commit(ui, repo, message='J', date='0 0')
> + > EOF
> + $ hg log -r . -T '\n' --config extensions.t=$TESTTMP/delayedstrip.py
> + warning: orphaned descendants detected, not stripping 08ebfeb61bac, 112478962961, 7fb047a69f22
> + saved backup bundle to $TESTTMP/delayedstrip/.hg/strip-backup/f585351a92f8-81fa23b0-I.hg (glob)
> +
> + $ hg log -G -T '{rev}:{node|short} {desc}' -r 'sort(all(), topo)'
> + @ 6:2f2d51af6205 J
> + |
> + o 3:08ebfeb61bac I
> + |
> + | o 5:64a8289d2492 F
> + | |
> + | o 2:7fb047a69f22 E
> + |/
> + | o 4:26805aba1e60 C
> + | |
> + | o 1:112478962961 B
> + |/
> + o 0:426bada5c675 A
> +
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
More information about the Mercurial-devel
mailing list