Patchwork D3718: narrow: mark the critical chunks of narrowing/widening as unsafe

login
register
mail settings
Submitter phabricator
Date June 12, 2018, 3:31 p.m.
Message ID <differential-rev-PHID-DREV-sczx2kboalhsnfytekkc-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/32087/
State New
Headers show

Comments

phabricator - June 12, 2018, 3:31 p.m.
durin42 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  I'm _mostly_ sure these are the only unsafe chunks here.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3718

AFFECTED FILES
  hgext/narrow/narrowcommands.py

CHANGE DETAILS




To: durin42, #hg-reviewers
Cc: mercurial-devel
phabricator - June 13, 2018, 5:03 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> narrowcommands.py:248
> +                    repo.wvfs.unlinkpath(f)
> +            repo.setnarrowpats(newincludes, newexcludes)
>  

Perhaps for another patch, but we could probably move this to before the transaction that starts on line 238 and then move the transaction outside of the unsafeoperation(). The repo won't be more broken if the user interrupts while we delete or add files from the working copy or dirstate here than it could be if they interrupt while doing the same things because of an `hg update` (right?).

I guess it depends on what we consider unsafe. Is it just things that would result in lost commits or errors from `hg verify` that we consider unsafe? That seems like a reasonable definition to me.

> narrowcommands.py:250
>  
> -    repo.destroyed()
> +        repo.destroyed()
>  

nit: the previous patch left this line outside of the block. make consistent? doesn't really matter, though...

> narrowcommands.py:284-295
> +        actions = {k: [] for k in 'a am f g cd dc r dm dg m e k p pr'.split()}
> +        addgaction = actions['g'].append
>  
> -    mf = repo['.'].manifest().matches(newmatch)
> -    for f, fn in mf.iteritems():
> -        if f not in repo.dirstate:
> -            addgaction((f, (mf.flags(f), False),
> -                        "add from widened narrow clone"))
> +        mf = repo['.'].manifest().matches(newmatch)
> +        for f, fn in mf.iteritems():
> +            if f not in repo.dirstate:
> +                addgaction((f, (mf.flags(f), False),

similar here: i think this could be left outside the unsafeoperation()

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3718

To: durin42, #hg-reviewers
Cc: martinvonz, mercurial-devel

Patch

diff --git a/hgext/narrow/narrowcommands.py b/hgext/narrow/narrowcommands.py
--- a/hgext/narrow/narrowcommands.py
+++ b/hgext/narrow/narrowcommands.py
@@ -203,50 +203,51 @@ 
                               hint=_('use --force-delete-local-changes to '
                                      'ignore'))
 
-    if revstostrip:
-        tostrip = [unfi.changelog.node(r) for r in revstostrip]
-        if repo['.'].node() in tostrip:
-            # stripping working copy, so move to a different commit first
-            urev = max(repo.revs('(::%n) - %ln + null',
-                                 repo['.'].node(), visibletostrip))
-            hg.clean(repo, urev)
-        repair.strip(ui, unfi, tostrip, topic='narrow')
+    with ui.unsafeoperation():
+        if revstostrip:
+            tostrip = [unfi.changelog.node(r) for r in revstostrip]
+            if repo['.'].node() in tostrip:
+                # stripping working copy, so move to a different commit first
+                urev = max(repo.revs('(::%n) - %ln + null',
+                                     repo['.'].node(), visibletostrip))
+                hg.clean(repo, urev)
+            repair.strip(ui, unfi, tostrip, topic='narrow')
 
-    todelete = []
-    for f, f2, size in repo.store.datafiles():
-        if f.startswith('data/'):
-            file = f[5:-2]
-            if not newmatch(file):
-                todelete.append(f)
-        elif f.startswith('meta/'):
-            dir = f[5:-13]
-            dirs = ['.'] + sorted(util.dirs({dir})) + [dir]
-            include = True
-            for d in dirs:
-                visit = newmatch.visitdir(d)
-                if not visit:
-                    include = False
-                    break
-                if visit == 'all':
-                    break
-            if not include:
-                todelete.append(f)
+        todelete = []
+        for f, f2, size in repo.store.datafiles():
+            if f.startswith('data/'):
+                file = f[5:-2]
+                if not newmatch(file):
+                    todelete.append(f)
+            elif f.startswith('meta/'):
+                dir = f[5:-13]
+                dirs = ['.'] + sorted(util.dirs({dir})) + [dir]
+                include = True
+                for d in dirs:
+                    visit = newmatch.visitdir(d)
+                    if not visit:
+                        include = False
+                        break
+                    if visit == 'all':
+                        break
+                if not include:
+                    todelete.append(f)
 
-    repo.destroying()
+        repo.destroying()
 
-    with repo.transaction("narrowing"):
-        for f in todelete:
-            ui.status(_('deleting %s\n') % f)
-            util.unlinkpath(repo.svfs.join(f))
-            repo.store.markremoved(f)
+        with repo.transaction("narrowing"):
+            for f in todelete:
+                ui.status(_('deleting %s\n') % f)
+                util.unlinkpath(repo.svfs.join(f))
+                repo.store.markremoved(f)
 
-        for f in repo.dirstate:
-            if not newmatch(f):
-                repo.dirstate.drop(f)
-                repo.wvfs.unlinkpath(f)
-        repo.setnarrowpats(newincludes, newexcludes)
+            for f in repo.dirstate:
+                if not newmatch(f):
+                    repo.dirstate.drop(f)
+                    repo.wvfs.unlinkpath(f)
+            repo.setnarrowpats(newincludes, newexcludes)
 
-    repo.destroyed()
+        repo.destroyed()
 
 def _widen(ui, repo, remote, commoninc, newincludes, newexcludes):
     newmatch = narrowspec.match(repo.root, newincludes, newexcludes)
@@ -269,28 +270,29 @@ 
         repo.setnarrowpats(newincludes, newexcludes)
     repo.setnewnarrowpats = setnewnarrowpats
 
-    ds = repo.dirstate
-    p1, p2 = ds.p1(), ds.p2()
-    with ds.parentchange():
-        ds.setparents(node.nullid, node.nullid)
-    common = commoninc[0]
-    with wrappedextraprepare:
-        exchange.pull(repo, remote, heads=common)
-    with ds.parentchange():
-        ds.setparents(p1, p2)
+    with ui.unsafeoperation():
+        ds = repo.dirstate
+        p1, p2 = ds.p1(), ds.p2()
+        with ds.parentchange():
+            ds.setparents(node.nullid, node.nullid)
+        common = commoninc[0]
+        with wrappedextraprepare:
+            exchange.pull(repo, remote, heads=common)
+        with ds.parentchange():
+            ds.setparents(p1, p2)
 
-    actions = {k: [] for k in 'a am f g cd dc r dm dg m e k p pr'.split()}
-    addgaction = actions['g'].append
+        actions = {k: [] for k in 'a am f g cd dc r dm dg m e k p pr'.split()}
+        addgaction = actions['g'].append
 
-    mf = repo['.'].manifest().matches(newmatch)
-    for f, fn in mf.iteritems():
-        if f not in repo.dirstate:
-            addgaction((f, (mf.flags(f), False),
-                        "add from widened narrow clone"))
+        mf = repo['.'].manifest().matches(newmatch)
+        for f, fn in mf.iteritems():
+            if f not in repo.dirstate:
+                addgaction((f, (mf.flags(f), False),
+                            "add from widened narrow clone"))
 
-    merge.applyupdates(repo, actions, wctx=repo[None],
-                       mctx=repo['.'], overwrite=False)
-    merge.recordupdates(repo, actions, branchmerge=False)
+        merge.applyupdates(repo, actions, wctx=repo[None],
+                           mctx=repo['.'], overwrite=False)
+        merge.recordupdates(repo, actions, branchmerge=False)
 
 # TODO(rdamazio): Make new matcher format and update description
 @command('tracked',