Patchwork [2,of,3] dirstate: wrap setparent calls with begin/endparentchange (issue4353)

login
register
mail settings
Submitter Durham Goode
Date Sept. 5, 2014, 7:36 p.m.
Message ID <ca5a5cd5a9840dd3d391.1409945780@dev2000.prn2.facebook.com>
Download mbox | patch
Permalink /patch/5706/
State Superseded
Commit 6f63c47cbb861169d700830ad70cf7297fdb46fb
Headers show

Comments

Durham Goode - Sept. 5, 2014, 7:36 p.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1409942180 25200
#      Fri Sep 05 11:36:20 2014 -0700
# Node ID ca5a5cd5a9840dd3d391a6204f5e3edb8b097b44
# Parent  8c6d502c92c8ed8dfa79b8fac4c8a2cc9e6c4153
dirstate: wrap setparent calls with begin/endparentchange (issue4353)

This wraps all the locations of dirstate.setparent with the appropriate
begin/endparentchange calls. This will prevent exceptions during those calls
from causing incoherent dirstates (issue4353).

I'm not sure how to test this, since it should only ever occur if there's an
exception, like ctrl+c.
Siddharth Agarwal - Sept. 5, 2014, 8:52 p.m.
On 09/05/2014 12:36 PM, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1409942180 25200
> #      Fri Sep 05 11:36:20 2014 -0700
> # Node ID ca5a5cd5a9840dd3d391a6204f5e3edb8b097b44
> # Parent  8c6d502c92c8ed8dfa79b8fac4c8a2cc9e6c4153
> dirstate: wrap setparent calls with begin/endparentchange (issue4353)
>
> This wraps all the locations of dirstate.setparent with the appropriate
> begin/endparentchange calls. This will prevent exceptions during those calls
> from causing incoherent dirstates (issue4353).
>
> I'm not sure how to test this, since it should only ever occur if there's an
> exception, like ctrl+c.

The easiest way to do that is to write an extension that monkeypatches a 
function to raise KeyboardInterrupt. See the patch in 
http://bz.selenic.com/show_bug.cgi?id=4344#c0 for an example (it raises 
util.Abort instead, which is just as good).

>
> diff --git a/hgext/histedit.py b/hgext/histedit.py
> --- a/hgext/histedit.py
> +++ b/hgext/histedit.py
> @@ -220,6 +220,7 @@
>           cmdutil.revert(ui, repo, ctx, (wcpar, node.nullid), all=True)
>           stats = None
>       else:
> +        repo.dirstate.beginparentchange()
>           try:
>               # ui.forcemerge is an internal variable, do not document
>               repo.ui.setconfig('ui', 'forcemerge', opts.get('tool', ''),
> @@ -229,6 +230,7 @@
>           finally:
>               repo.ui.setconfig('ui', 'forcemerge', '', 'histedit')
>           repo.setparents(wcpar, node.nullid)
> +        repo.dirstate.endparentchange()
>           repo.dirstate.write()
>           # fix up dirstate for copies and renames
>       cmdutil.duplicatecopies(repo, ctx.rev(), ctx.p1().rev())
> diff --git a/hgext/mq.py b/hgext/mq.py
> --- a/hgext/mq.py
> +++ b/hgext/mq.py
> @@ -816,12 +816,14 @@
>                           merged.append(f)
>                       else:
>                           removed.append(f)
> +                repo.dirstate.beginparentchange()
>                   for f in removed:
>                       repo.dirstate.remove(f)
>                   for f in merged:
>                       repo.dirstate.merge(f)
>                   p1, p2 = repo.dirstate.parents()
>                   repo.setparents(p1, merge)
> +                repo.dirstate.endparentchange()
>   
>               if all_files and '.hgsubstate' in all_files:
>                   wctx = repo[None]
> @@ -1451,7 +1453,7 @@
>                   if keepchanges and tobackup:
>                       raise util.Abort(_("local changes found, refresh first"))
>                   self.backup(repo, tobackup)
> -
> +                repo.dirstate.beginparentchange()
>                   for f in a:
>                       util.unlinkpath(repo.wjoin(f), ignoremissing=True)
>                       repo.dirstate.drop(f)
> @@ -1460,6 +1462,7 @@
>                       repo.wwrite(f, fctx.data(), fctx.flags())
>                       repo.dirstate.normal(f)
>                   repo.setparents(qp, nullid)
> +                repo.dirstate.endparentchange()
>               for patch in reversed(self.applied[start:end]):
>                   self.ui.status(_("popping %s\n") % patch.name)
>               del self.applied[start:end]
> @@ -1599,6 +1602,7 @@
>               bmlist = repo[top].bookmarks()
>   
>               try:
> +                repo.dirstate.beginparentchange()
>                   if diffopts.git or diffopts.upgrade:
>                       copies = {}
>                       for dst in a:
> @@ -1651,6 +1655,7 @@
>   
>                   # assumes strip can roll itself back if interrupted
>                   repo.setparents(*cparents)
> +                repo.dirstate.endparentchange()
>                   self.applied.pop()
>                   self.applieddirty = True
>                   strip(self.ui, repo, [top], update=False, backup=False)
> diff --git a/hgext/rebase.py b/hgext/rebase.py
> --- a/hgext/rebase.py
> +++ b/hgext/rebase.py
> @@ -360,7 +360,9 @@
>                                             editor=editor)
>                   else:
>                       # Skip commit if we are collapsing
> +                    repo.dirstate.beginparentchange()
>                       repo.setparents(repo[p1].node())
> +                    repo.dirstate.endparentchange()
>                       newrev = None
>                   # Update the state
>                   if newrev is not None:
> @@ -466,7 +468,9 @@
>   def concludenode(repo, rev, p1, p2, commitmsg=None, editor=None, extrafn=None):
>       'Commit the changes and store useful information in extra'
>       try:
> +        repo.dirstate.beginparentchange()
>           repo.setparents(repo[p1].node(), repo[p2].node())
> +        repo.dirstate.endparentchange()
>           ctx = repo[rev]
>           if commitmsg is None:
>               commitmsg = ctx.description()
> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
> --- a/mercurial/cmdutil.py
> +++ b/mercurial/cmdutil.py
> @@ -659,6 +659,7 @@
>   
>           n = None
>           if update:
> +            repo.dirstate.beginparentchange()
>               if p1 != parents[0]:
>                   updatefunc(repo, p1.node())
>               if p2 != parents[1]:
> @@ -698,6 +699,7 @@
>                   n = repo.commit(message, opts.get('user') or user,
>                                   opts.get('date') or date, match=m,
>                                   editor=editor, force=partial)
> +            repo.dirstate.endparentchange()
>           else:
>               if opts.get('exact') or opts.get('import_branch'):
>                   branch = branch or 'default'
> diff --git a/mercurial/commands.py b/mercurial/commands.py
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -483,9 +483,11 @@
>               try:
>                   ui.setconfig('ui', 'forcemerge', opts.get('tool', ''),
>                                'backout')
> +                repo.dirstate.beginparentchange()
>                   stats = mergemod.update(repo, parent, True, True, False,
>                                           node, False)
>                   repo.setparents(op1, op2)
> +                repo.dirstate.endparentchange()
>                   hg._showstats(repo, stats)
>                   if stats[3]:
>                       repo.ui.status(_("use 'hg resolve' to retry unresolved "
> @@ -2746,7 +2748,9 @@
>   
>       wlock = repo.wlock()
>       try:
> +        repo.dirstate.beginparentchange()
>           repo.setparents(r1, r2)
> +        repo.dirstate.endparentchange()
>       finally:
>           wlock.release()
>   
> @@ -3306,10 +3310,12 @@
>                   cont = False
>   
>               # drop the second merge parent
> +            repo.dirstate.beginparentchange()
>               repo.setparents(current.node(), nullid)
>               repo.dirstate.write()
>               # fix up dirstate for copies and renames
>               cmdutil.duplicatecopies(repo, ctx.rev(), ctx.p1().rev())
> +            repo.dirstate.endparentchange()
>   
>               # commit
>               node = repo.commit(text=message, user=user,
> @@ -3918,6 +3924,7 @@
>       try:
>           try:
>               wlock = repo.wlock()
> +            repo.dirstate.beginparentchange()
>               if not opts.get('no_commit'):
>                   lock = repo.lock()
>                   tr = repo.transaction('import')
> @@ -3958,6 +3965,7 @@
>                   tr.close()
>               if msgs:
>                   repo.savecommitmessage('\n* * *\n'.join(msgs))
> +            repo.dirstate.endparentchange()
>               return ret
>           except: # re-raises
>               # wlock.release() indirectly calls dirstate.write(): since
> diff --git a/mercurial/context.py b/mercurial/context.py
> --- a/mercurial/context.py
> +++ b/mercurial/context.py
> @@ -1171,11 +1171,13 @@
>   
>           """
>   
> +        self._repo.dirstate.beginparentchange()
>           for f in self.modified() + self.added():
>               self._repo.dirstate.normal(f)
>           for f in self.removed():
>               self._repo.dirstate.drop(f)
>           self._repo.dirstate.setparents(node)
> +        self._repo.dirstate.endparentchange()
>   
>       def dirs(self):
>           return self._repo.dirstate.dirs()
> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> --- a/mercurial/localrepo.py
> +++ b/mercurial/localrepo.py
> @@ -764,6 +764,7 @@
>           return self[changeid].parents()
>   
>       def setparents(self, p1, p2=nullid):
> +        self.dirstate.beginparentchange()
>           copies = self.dirstate.setparents(p1, p2)
>           pctx = self[p1]
>           if copies:
> @@ -777,6 +778,7 @@
>               for f, s in sorted(self.dirstate.copies().items()):
>                   if f not in pctx and s not in pctx:
>                       self.dirstate.copy(None, f)
> +        self.dirstate.endparentchange()
>   
>       def filectx(self, path, changeid=None, fileid=None):
>           """changeid can be a changeset revision, node, or tag.
> diff --git a/mercurial/merge.py b/mercurial/merge.py
> --- a/mercurial/merge.py
> +++ b/mercurial/merge.py
> @@ -1134,6 +1134,7 @@
>           stats = applyupdates(repo, actions, wc, p2, overwrite, labels=labels)
>   
>           if not partial:
> +            repo.dirstate.beginparentchange()
>               repo.setparents(fp1, fp2)
>               recordupdates(repo, actions, branchmerge)
>               # update completed, clear state
> @@ -1141,6 +1142,7 @@
>   
>               if not branchmerge:
>                   repo.dirstate.setbranch(p2.branch())
> +            repo.dirstate.endparentchange()
>       finally:
>           wlock.release()
>   
> _______________________________________________
> 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
@@ -220,6 +220,7 @@ 
         cmdutil.revert(ui, repo, ctx, (wcpar, node.nullid), all=True)
         stats = None
     else:
+        repo.dirstate.beginparentchange()
         try:
             # ui.forcemerge is an internal variable, do not document
             repo.ui.setconfig('ui', 'forcemerge', opts.get('tool', ''),
@@ -229,6 +230,7 @@ 
         finally:
             repo.ui.setconfig('ui', 'forcemerge', '', 'histedit')
         repo.setparents(wcpar, node.nullid)
+        repo.dirstate.endparentchange()
         repo.dirstate.write()
         # fix up dirstate for copies and renames
     cmdutil.duplicatecopies(repo, ctx.rev(), ctx.p1().rev())
diff --git a/hgext/mq.py b/hgext/mq.py
--- a/hgext/mq.py
+++ b/hgext/mq.py
@@ -816,12 +816,14 @@ 
                         merged.append(f)
                     else:
                         removed.append(f)
+                repo.dirstate.beginparentchange()
                 for f in removed:
                     repo.dirstate.remove(f)
                 for f in merged:
                     repo.dirstate.merge(f)
                 p1, p2 = repo.dirstate.parents()
                 repo.setparents(p1, merge)
+                repo.dirstate.endparentchange()
 
             if all_files and '.hgsubstate' in all_files:
                 wctx = repo[None]
@@ -1451,7 +1453,7 @@ 
                 if keepchanges and tobackup:
                     raise util.Abort(_("local changes found, refresh first"))
                 self.backup(repo, tobackup)
-
+                repo.dirstate.beginparentchange()
                 for f in a:
                     util.unlinkpath(repo.wjoin(f), ignoremissing=True)
                     repo.dirstate.drop(f)
@@ -1460,6 +1462,7 @@ 
                     repo.wwrite(f, fctx.data(), fctx.flags())
                     repo.dirstate.normal(f)
                 repo.setparents(qp, nullid)
+                repo.dirstate.endparentchange()
             for patch in reversed(self.applied[start:end]):
                 self.ui.status(_("popping %s\n") % patch.name)
             del self.applied[start:end]
@@ -1599,6 +1602,7 @@ 
             bmlist = repo[top].bookmarks()
 
             try:
+                repo.dirstate.beginparentchange()
                 if diffopts.git or diffopts.upgrade:
                     copies = {}
                     for dst in a:
@@ -1651,6 +1655,7 @@ 
 
                 # assumes strip can roll itself back if interrupted
                 repo.setparents(*cparents)
+                repo.dirstate.endparentchange()
                 self.applied.pop()
                 self.applieddirty = True
                 strip(self.ui, repo, [top], update=False, backup=False)
diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -360,7 +360,9 @@ 
                                           editor=editor)
                 else:
                     # Skip commit if we are collapsing
+                    repo.dirstate.beginparentchange()
                     repo.setparents(repo[p1].node())
+                    repo.dirstate.endparentchange()
                     newrev = None
                 # Update the state
                 if newrev is not None:
@@ -466,7 +468,9 @@ 
 def concludenode(repo, rev, p1, p2, commitmsg=None, editor=None, extrafn=None):
     'Commit the changes and store useful information in extra'
     try:
+        repo.dirstate.beginparentchange()
         repo.setparents(repo[p1].node(), repo[p2].node())
+        repo.dirstate.endparentchange()
         ctx = repo[rev]
         if commitmsg is None:
             commitmsg = ctx.description()
diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -659,6 +659,7 @@ 
 
         n = None
         if update:
+            repo.dirstate.beginparentchange()
             if p1 != parents[0]:
                 updatefunc(repo, p1.node())
             if p2 != parents[1]:
@@ -698,6 +699,7 @@ 
                 n = repo.commit(message, opts.get('user') or user,
                                 opts.get('date') or date, match=m,
                                 editor=editor, force=partial)
+            repo.dirstate.endparentchange()
         else:
             if opts.get('exact') or opts.get('import_branch'):
                 branch = branch or 'default'
diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -483,9 +483,11 @@ 
             try:
                 ui.setconfig('ui', 'forcemerge', opts.get('tool', ''),
                              'backout')
+                repo.dirstate.beginparentchange()
                 stats = mergemod.update(repo, parent, True, True, False,
                                         node, False)
                 repo.setparents(op1, op2)
+                repo.dirstate.endparentchange()
                 hg._showstats(repo, stats)
                 if stats[3]:
                     repo.ui.status(_("use 'hg resolve' to retry unresolved "
@@ -2746,7 +2748,9 @@ 
 
     wlock = repo.wlock()
     try:
+        repo.dirstate.beginparentchange()
         repo.setparents(r1, r2)
+        repo.dirstate.endparentchange()
     finally:
         wlock.release()
 
@@ -3306,10 +3310,12 @@ 
                 cont = False
 
             # drop the second merge parent
+            repo.dirstate.beginparentchange()
             repo.setparents(current.node(), nullid)
             repo.dirstate.write()
             # fix up dirstate for copies and renames
             cmdutil.duplicatecopies(repo, ctx.rev(), ctx.p1().rev())
+            repo.dirstate.endparentchange()
 
             # commit
             node = repo.commit(text=message, user=user,
@@ -3918,6 +3924,7 @@ 
     try:
         try:
             wlock = repo.wlock()
+            repo.dirstate.beginparentchange()
             if not opts.get('no_commit'):
                 lock = repo.lock()
                 tr = repo.transaction('import')
@@ -3958,6 +3965,7 @@ 
                 tr.close()
             if msgs:
                 repo.savecommitmessage('\n* * *\n'.join(msgs))
+            repo.dirstate.endparentchange()
             return ret
         except: # re-raises
             # wlock.release() indirectly calls dirstate.write(): since
diff --git a/mercurial/context.py b/mercurial/context.py
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -1171,11 +1171,13 @@ 
 
         """
 
+        self._repo.dirstate.beginparentchange()
         for f in self.modified() + self.added():
             self._repo.dirstate.normal(f)
         for f in self.removed():
             self._repo.dirstate.drop(f)
         self._repo.dirstate.setparents(node)
+        self._repo.dirstate.endparentchange()
 
     def dirs(self):
         return self._repo.dirstate.dirs()
diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -764,6 +764,7 @@ 
         return self[changeid].parents()
 
     def setparents(self, p1, p2=nullid):
+        self.dirstate.beginparentchange()
         copies = self.dirstate.setparents(p1, p2)
         pctx = self[p1]
         if copies:
@@ -777,6 +778,7 @@ 
             for f, s in sorted(self.dirstate.copies().items()):
                 if f not in pctx and s not in pctx:
                     self.dirstate.copy(None, f)
+        self.dirstate.endparentchange()
 
     def filectx(self, path, changeid=None, fileid=None):
         """changeid can be a changeset revision, node, or tag.
diff --git a/mercurial/merge.py b/mercurial/merge.py
--- a/mercurial/merge.py
+++ b/mercurial/merge.py
@@ -1134,6 +1134,7 @@ 
         stats = applyupdates(repo, actions, wc, p2, overwrite, labels=labels)
 
         if not partial:
+            repo.dirstate.beginparentchange()
             repo.setparents(fp1, fp2)
             recordupdates(repo, actions, branchmerge)
             # update completed, clear state
@@ -1141,6 +1142,7 @@ 
 
             if not branchmerge:
                 repo.dirstate.setbranch(p2.branch())
+            repo.dirstate.endparentchange()
     finally:
         wlock.release()