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

[durin42 (Augie Fackler)]

This revision was automatically updated to reflect the committed changes.
Closed by commit rHGa1d5951efce7: narrow: mark the critical chunks of 
narrowing/widening as unsafe (authored by durin42, committed by ).

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D3718?vs=9328=9413

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

AFFECTED FILES
  hgext/narrow/narrowcommands.py

CHANGE DETAILS

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.uninterruptable():
+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.uninterruptable():
+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()}
-

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

[durin42 (Augie Fackler)]

durin42 updated this revision to Diff 9328.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D3718?vs=9025=9328

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

AFFECTED FILES
  hgext/narrow/narrowcommands.py

CHANGE DETAILS

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.uninterruptable():
+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.uninterruptable():
+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 = 

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

[martinvonz (Martin von Zweigbergk)]

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
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

[durin42 (Augie Fackler)]

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

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