Patchwork [1,of,3] histedit: switch state to store node instead of ctx

login
register
mail settings
Submitter Mateusz Kwapich
Date Feb. 5, 2015, 9:59 p.m.
Message ID <d68972b5e29e920571d9.1423173554@dev1429.prn1.facebook.com>
Download mbox | patch
Permalink /patch/7691/
State Accepted
Headers show

Comments

Mateusz Kwapich - Feb. 5, 2015, 9:59 p.m.
# HG changeset patch
# User Mateusz Kwapich <mitrandir@fb.com>
# Date 1423170607 28800
#      Thu Feb 05 13:10:07 2015 -0800
# Node ID d68972b5e29e920571d976fee41a385d4f6baa95
# Parent  c53bc2e52514825cbd40d29dd08693c8a640256e
histedit: switch state to store node instead of ctx

Currently, if the node no longer exists, the state object fails to load
and pukes with an exception. Changing the state object to only store the
node allows callers to handle these cases.  For instance, in
bootstrapcontinue we can now detect that the node doesn't exist and exit
gracefully.

The alternative is to have the state object store something like None
when the node doesn't exist, but then outside callers won't be able to
access the old node for recovery (unless we store both the node and the
ctx, but why bother).

More importantly it allows us to detect this case when doing hg histedit
--abort.  Currently this situation results in both --continue and
--abort being broken and the user has to rm .hg/histedit-state to unfuck
their repo.
(description by Durham Goode)
Augie Fackler - Feb. 10, 2015, 6:34 p.m.
On Thu, Feb 05, 2015 at 01:59:14PM -0800, Mateusz Kwapich wrote:
> # HG changeset patch
> # User Mateusz Kwapich <mitrandir@fb.com>
> # Date 1423170607 28800
> #      Thu Feb 05 13:10:07 2015 -0800
> # Node ID d68972b5e29e920571d976fee41a385d4f6baa95
> # Parent  c53bc2e52514825cbd40d29dd08693c8a640256e
> histedit: switch state to store node instead of ctx

Queued this, thanks.

>
> Currently, if the node no longer exists, the state object fails to load
> and pukes with an exception. Changing the state object to only store the
> node allows callers to handle these cases.  For instance, in
> bootstrapcontinue we can now detect that the node doesn't exist and exit
> gracefully.
>
> The alternative is to have the state object store something like None
> when the node doesn't exist, but then outside callers won't be able to
> access the old node for recovery (unless we store both the node and the
> ctx, but why bother).
>
> More importantly it allows us to detect this case when doing hg histedit
> --abort.  Currently this situation results in both --continue and
> --abort being broken and the user has to rm .hg/histedit-state to unfuck
> their repo.
> (description by Durham Goode)
>
> diff --git a/hgext/histedit.py b/hgext/histedit.py
> --- a/hgext/histedit.py
> +++ b/hgext/histedit.py
> @@ -189,13 +189,13 @@
>  """)
>
>  class histeditstate(object):
> -    def __init__(self, repo, parentctx=None, rules=None, keep=None,
> +    def __init__(self, repo, parentctxnode=None, rules=None, keep=None,
>              topmost=None, replacements=None, lock=None, wlock=None):
>          self.repo = repo
>          self.rules = rules
>          self.keep = keep
>          self.topmost = topmost
> -        self.parentctx = parentctx
> +        self.parentctxnode = parentctxnode
>          self.lock = lock
>          self.wlock = wlock
>          if replacements is None:
> @@ -214,7 +214,7 @@
>
>          parentctxnode, rules, keep, topmost, replacements = pickle.load(fp)
>
> -        self.parentctx = self.repo[parentctxnode]
> +        self.parentctxnode = parentctxnode
>          self.rules = rules
>          self.keep = keep
>          self.topmost = topmost
> @@ -222,7 +222,7 @@
>
>      def write(self):
>          fp = self.repo.vfs('histedit-state', 'w')
> -        pickle.dump((self.parentctx.node(), self.rules, self.keep,
> +        pickle.dump((self.parentctxnode, self.rules, self.keep,
>                       self.topmost, self.replacements), fp)
>          fp.close()
>
> @@ -346,7 +346,8 @@
>      return repo.commitctx(new)
>
>  def pick(ui, state, ha, opts):
> -    repo, ctx = state.repo, state.parentctx
> +    repo, ctxnode = state.repo, state.parentctxnode
> +    ctx = repo[ctxnode]
>      oldctx = repo[ha]
>      if oldctx.parents()[0] == ctx:
>          ui.debug('node %s unchanged\n' % ha[:12])
> @@ -368,7 +369,8 @@
>
>
>  def edit(ui, state, ha, opts):
> -    repo, ctx = state.repo, state.parentctx
> +    repo, ctxnode = state.repo, state.parentctxnode
> +    ctx = repo[ctxnode]
>      oldctx = repo[ha]
>      hg.update(repo, ctx.node())
>      applychanges(ui, repo, oldctx, opts)
> @@ -382,7 +384,8 @@
>      return fold(ui, state, ha, rollupopts)
>
>  def fold(ui, state, ha, opts):
> -    repo, ctx = state.repo, state.parentctx
> +    repo, ctxnode = state.repo, state.parentctxnode
> +    ctx = repo[ctxnode]
>      oldctx = repo[ha]
>      hg.update(repo, ctx.node())
>      stats = applychanges(ui, repo, oldctx, opts)
> @@ -438,12 +441,14 @@
>      return repo[n], replacements
>
>  def drop(ui, state, ha, opts):
> -    repo, ctx = state.repo, state.parentctx
> +    repo, ctxnode = state.repo, state.parentctxnode
> +    ctx = repo[ctxnode]
>      return ctx, [(repo[ha].node(), ())]
>
>
>  def message(ui, state, ha, opts):
> -    repo, ctx = state.repo, state.parentctx
> +    repo, ctxnode = state.repo, state.parentctxnode
> +    ctx = repo[ctxnode]
>      oldctx = repo[ha]
>      hg.update(repo, ctx.node())
>      stats = applychanges(ui, repo, oldctx, opts)
> @@ -603,7 +608,7 @@
>          ui.debug('restore wc to old parent %s\n' % node.short(state.topmost))
>          # check whether we should update away
>          parentnodes = [c.node() for c in repo[None].parents()]
> -        for n in leafs | set([state.parentctx.node()]):
> +        for n in leafs | set([state.parentctxnode]):
>              if n in parentnodes:
>                  hg.clean(repo, state.topmost)
>                  break
> @@ -659,9 +664,9 @@
>                   if l and not l.startswith('#')]
>          rules = verifyrules(rules, repo, ctxs)
>
> -        parentctx = repo[root].parents()[0]
> +        parentctxnode = repo[root].parents()[0].node()
>
> -        state.parentctx = parentctx
> +        state.parentctxnode = parentctxnode
>          state.rules = rules
>          state.keep = keep
>          state.topmost = topmost
> @@ -672,10 +677,11 @@
>          action, ha = state.rules.pop(0)
>          ui.debug('histedit: processing %s %s\n' % (action, ha[:12]))
>          actfunc = actiontable[action]
> -        state.parentctx, replacement_ = actfunc(ui, state, ha, opts)
> +        parentctx, replacement_ = actfunc(ui, state, ha, opts)
> +        state.parentctxnode = parentctx.node()
>          state.replacements.extend(replacement_)
>
> -    hg.update(repo, state.parentctx.node())
> +    hg.update(repo, state.parentctxnode)
>
>      mapping, tmpnodes, created, ntm = processreplacement(state)
>      if mapping:
> @@ -728,7 +734,8 @@
>      return newchildren
>
>  def bootstrapcontinue(ui, state, opts):
> -    repo, parentctx = state.repo, state.parentctx
> +    repo, parentctxnode = state.repo, state.parentctxnode
> +    parentctx = repo[parentctxnode]
>      action, currentnode = state.rules.pop(0)
>      ctx = repo[currentnode]
>
> @@ -784,7 +791,7 @@
>          # otherwise update "parentctx" before proceeding to further operation
>          parentctx = repo[newchildren[-1]]
>
> -    state.parentctx = parentctx
> +    state.parentctxnode = parentctx.node()
>      state.replacements.extend(replacements)
>
>      return state
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel

Patch

diff --git a/hgext/histedit.py b/hgext/histedit.py
--- a/hgext/histedit.py
+++ b/hgext/histedit.py
@@ -189,13 +189,13 @@ 
 """)
 
 class histeditstate(object):
-    def __init__(self, repo, parentctx=None, rules=None, keep=None,
+    def __init__(self, repo, parentctxnode=None, rules=None, keep=None,
             topmost=None, replacements=None, lock=None, wlock=None):
         self.repo = repo
         self.rules = rules
         self.keep = keep
         self.topmost = topmost
-        self.parentctx = parentctx
+        self.parentctxnode = parentctxnode
         self.lock = lock
         self.wlock = wlock
         if replacements is None:
@@ -214,7 +214,7 @@ 
 
         parentctxnode, rules, keep, topmost, replacements = pickle.load(fp)
 
-        self.parentctx = self.repo[parentctxnode]
+        self.parentctxnode = parentctxnode
         self.rules = rules
         self.keep = keep
         self.topmost = topmost
@@ -222,7 +222,7 @@ 
 
     def write(self):
         fp = self.repo.vfs('histedit-state', 'w')
-        pickle.dump((self.parentctx.node(), self.rules, self.keep,
+        pickle.dump((self.parentctxnode, self.rules, self.keep,
                      self.topmost, self.replacements), fp)
         fp.close()
 
@@ -346,7 +346,8 @@ 
     return repo.commitctx(new)
 
 def pick(ui, state, ha, opts):
-    repo, ctx = state.repo, state.parentctx
+    repo, ctxnode = state.repo, state.parentctxnode
+    ctx = repo[ctxnode]
     oldctx = repo[ha]
     if oldctx.parents()[0] == ctx:
         ui.debug('node %s unchanged\n' % ha[:12])
@@ -368,7 +369,8 @@ 
 
 
 def edit(ui, state, ha, opts):
-    repo, ctx = state.repo, state.parentctx
+    repo, ctxnode = state.repo, state.parentctxnode
+    ctx = repo[ctxnode]
     oldctx = repo[ha]
     hg.update(repo, ctx.node())
     applychanges(ui, repo, oldctx, opts)
@@ -382,7 +384,8 @@ 
     return fold(ui, state, ha, rollupopts)
 
 def fold(ui, state, ha, opts):
-    repo, ctx = state.repo, state.parentctx
+    repo, ctxnode = state.repo, state.parentctxnode
+    ctx = repo[ctxnode]
     oldctx = repo[ha]
     hg.update(repo, ctx.node())
     stats = applychanges(ui, repo, oldctx, opts)
@@ -438,12 +441,14 @@ 
     return repo[n], replacements
 
 def drop(ui, state, ha, opts):
-    repo, ctx = state.repo, state.parentctx
+    repo, ctxnode = state.repo, state.parentctxnode
+    ctx = repo[ctxnode]
     return ctx, [(repo[ha].node(), ())]
 
 
 def message(ui, state, ha, opts):
-    repo, ctx = state.repo, state.parentctx
+    repo, ctxnode = state.repo, state.parentctxnode
+    ctx = repo[ctxnode]
     oldctx = repo[ha]
     hg.update(repo, ctx.node())
     stats = applychanges(ui, repo, oldctx, opts)
@@ -603,7 +608,7 @@ 
         ui.debug('restore wc to old parent %s\n' % node.short(state.topmost))
         # check whether we should update away
         parentnodes = [c.node() for c in repo[None].parents()]
-        for n in leafs | set([state.parentctx.node()]):
+        for n in leafs | set([state.parentctxnode]):
             if n in parentnodes:
                 hg.clean(repo, state.topmost)
                 break
@@ -659,9 +664,9 @@ 
                  if l and not l.startswith('#')]
         rules = verifyrules(rules, repo, ctxs)
 
-        parentctx = repo[root].parents()[0]
+        parentctxnode = repo[root].parents()[0].node()
 
-        state.parentctx = parentctx
+        state.parentctxnode = parentctxnode
         state.rules = rules
         state.keep = keep
         state.topmost = topmost
@@ -672,10 +677,11 @@ 
         action, ha = state.rules.pop(0)
         ui.debug('histedit: processing %s %s\n' % (action, ha[:12]))
         actfunc = actiontable[action]
-        state.parentctx, replacement_ = actfunc(ui, state, ha, opts)
+        parentctx, replacement_ = actfunc(ui, state, ha, opts)
+        state.parentctxnode = parentctx.node()
         state.replacements.extend(replacement_)
 
-    hg.update(repo, state.parentctx.node())
+    hg.update(repo, state.parentctxnode)
 
     mapping, tmpnodes, created, ntm = processreplacement(state)
     if mapping:
@@ -728,7 +734,8 @@ 
     return newchildren
 
 def bootstrapcontinue(ui, state, opts):
-    repo, parentctx = state.repo, state.parentctx
+    repo, parentctxnode = state.repo, state.parentctxnode
+    parentctx = repo[parentctxnode]
     action, currentnode = state.rules.pop(0)
     ctx = repo[currentnode]
 
@@ -784,7 +791,7 @@ 
         # otherwise update "parentctx" before proceeding to further operation
         parentctx = repo[newchildren[-1]]
 
-    state.parentctx = parentctx
+    state.parentctxnode = parentctx.node()
     state.replacements.extend(replacements)
 
     return state