This is an archive of the discontinued Mercurial Phabricator instance.

Differential D3718

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

Authored by durin42 on Jun 12 2018, 11:31 AM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHGa1d5951efce7: narrow: mark the critical chunks of narrowing/widening as unsafe
Summary

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

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

durin42 created this revision.Jun 12 2018, 11:31 AM
martinvonz added a subscriber: martinvonz.Jun 13 2018, 1:03 PM
martinvonz added inline comments.
hgext/narrow/narrowcommands.py
248

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.

250

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

284–296

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

durin42 updated this revision to Diff 9328.Jun 27 2018, 11:47 AM
Closed by commit rHGa1d5951efce7: narrow: mark the critical chunks of narrowing/widening as unsafe (authored by durin42). · Explain WhyJul 3 2018, 1:40 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mhgext/narrow/narrowcommands.py (118 lines)

Diff 9328

hgext/narrow/narrowcommands.py

ui.status('%s\n' % node.short(n)) ui.status('%s\n' % node.short(n))
ui.status(_('...and %d more, use --verbose to list all\n') % ui.status(_('...and %d more, use --verbose to list all\n') %
(len(visibletostrip) - maxnodes)) (len(visibletostrip) - maxnodes))
if not force: if not force:
raise error.Abort(_('local changes found'), raise error.Abort(_('local changes found'),
hint=_('use --force-delete-local-changes to ' hint=_('use --force-delete-local-changes to '
'ignore')) 'ignore'))
with ui.uninterruptable():
if revstostrip: if revstostrip:
tostrip = [unfi.changelog.node(r) for r in revstostrip] tostrip = [unfi.changelog.node(r) for r in revstostrip]
if repo['.'].node() in tostrip: if repo['.'].node() in tostrip:
# stripping working copy, so move to a different commit first # stripping working copy, so move to a different commit first
urev = max(repo.revs('(::%n) - %ln + null', urev = max(repo.revs('(::%n) - %ln + null',
repo['.'].node(), visibletostrip)) repo['.'].node(), visibletostrip))
hg.clean(repo, urev) hg.clean(repo, urev)
repair.strip(ui, unfi, tostrip, topic='narrow') repair.strip(ui, unfi, tostrip, topic='narrow')
todelete = [] todelete = []
for f, f2, size in repo.store.datafiles(): for f, f2, size in repo.store.datafiles():
if f.startswith('data/'): if f.startswith('data/'):
file = f[5:-2] file = f[5:-2]
if not newmatch(file): if not newmatch(file):
todelete.append(f) todelete.append(f)
elif f.startswith('meta/'): elif f.startswith('meta/'):
dir = f[5:-13] dir = f[5:-13]
dirs = ['.'] + sorted(util.dirs({dir})) + [dir] dirs = ['.'] + sorted(util.dirs({dir})) + [dir]
include = True include = True
for d in dirs: for d in dirs:
visit = newmatch.visitdir(d) visit = newmatch.visitdir(d)
if not visit: if not visit:
include = False include = False
break break
if visit == 'all': if visit == 'all':
break break
if not include: if not include:
todelete.append(f) todelete.append(f)
repo.destroying() repo.destroying()
with repo.transaction("narrowing"): with repo.transaction("narrowing"):
for f in todelete: for f in todelete:
ui.status(_('deleting %s\n') % f) ui.status(_('deleting %s\n') % f)
util.unlinkpath(repo.svfs.join(f)) util.unlinkpath(repo.svfs.join(f))
repo.store.markremoved(f) repo.store.markremoved(f)
for f in repo.dirstate: for f in repo.dirstate:
if not newmatch(f): if not newmatch(f):
repo.dirstate.drop(f) repo.dirstate.drop(f)
repo.wvfs.unlinkpath(f) repo.wvfs.unlinkpath(f)
repo.setnarrowpats(newincludes, newexcludes) repo.setnarrowpats(newincludes, newexcludes)
martinvonzUnsubmitted
Not Done

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.

martinvonz: Perhaps for another patch, but we could probably move this to before the transaction that…
repo.destroyed() repo.destroyed()
martinvonzUnsubmitted
Not Done

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

martinvonz: nit: the previous patch left this line outside of the block. make consistent? doesn't really…
def _widen(ui, repo, remote, commoninc, newincludes, newexcludes): def _widen(ui, repo, remote, commoninc, newincludes, newexcludes):
newmatch = narrowspec.match(repo.root, newincludes, newexcludes) newmatch = narrowspec.match(repo.root, newincludes, newexcludes)
# TODO(martinvonz): Get expansion working with widening/narrowing. # TODO(martinvonz): Get expansion working with widening/narrowing.
if narrowspec.needsexpansion(newincludes): if narrowspec.needsexpansion(newincludes):
raise error.Abort('Expansion not yet supported on pull') raise error.Abort('Expansion not yet supported on pull')
def pullbundle2extraprepare_widen(orig, pullop, kwargs): def pullbundle2extraprepare_widen(orig, pullop, kwargs):
orig(pullop, kwargs) orig(pullop, kwargs)
# The old{in,ex}cludepats have already been set by orig() # The old{in,ex}cludepats have already been set by orig()
kwargs['includepats'] = newincludes kwargs['includepats'] = newincludes
kwargs['excludepats'] = newexcludes kwargs['excludepats'] = newexcludes
wrappedextraprepare = extensions.wrappedfunction(exchange, wrappedextraprepare = extensions.wrappedfunction(exchange,
'_pullbundle2extraprepare', pullbundle2extraprepare_widen) '_pullbundle2extraprepare', pullbundle2extraprepare_widen)
# define a function that narrowbundle2 can call after creating the # define a function that narrowbundle2 can call after creating the
# backup bundle, but before applying the bundle from the server # backup bundle, but before applying the bundle from the server
def setnewnarrowpats(): def setnewnarrowpats():
repo.setnarrowpats(newincludes, newexcludes) repo.setnarrowpats(newincludes, newexcludes)
repo.setnewnarrowpats = setnewnarrowpats repo.setnewnarrowpats = setnewnarrowpats
with ui.uninterruptable():
ds = repo.dirstate ds = repo.dirstate
p1, p2 = ds.p1(), ds.p2() p1, p2 = ds.p1(), ds.p2()
with ds.parentchange(): with ds.parentchange():
ds.setparents(node.nullid, node.nullid) ds.setparents(node.nullid, node.nullid)
common = commoninc[0] common = commoninc[0]
with wrappedextraprepare: with wrappedextraprepare:
exchange.pull(repo, remote, heads=common) exchange.pull(repo, remote, heads=common)
with ds.parentchange(): with ds.parentchange():
ds.setparents(p1, p2) ds.setparents(p1, p2)
actions = {k: [] for k in 'a am f g cd dc r dm dg m e k p pr'.split()} actions = {k: [] for k in 'a am f g cd dc r dm dg m e k p pr'.split()}
addgaction = actions['g'].append addgaction = actions['g'].append
mf = repo['.'].manifest().matches(newmatch) mf = repo['.'].manifest().matches(newmatch)
for f, fn in mf.iteritems(): for f, fn in mf.iteritems():
if f not in repo.dirstate: if f not in repo.dirstate:
addgaction((f, (mf.flags(f), False), addgaction((f, (mf.flags(f), False),
"add from widened narrow clone")) "add from widened narrow clone"))
merge.applyupdates(repo, actions, wctx=repo[None], merge.applyupdates(repo, actions, wctx=repo[None],
mctx=repo['.'], overwrite=False) mctx=repo['.'], overwrite=False)
merge.recordupdates(repo, actions, branchmerge=False) merge.recordupdates(repo, actions, branchmerge=False)
martinvonzUnsubmitted
Not Done

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

martinvonz: similar here: i think this could be left outside the unsafeoperation()
# TODO(rdamazio): Make new matcher format and update description # TODO(rdamazio): Make new matcher format and update description
@command('tracked', @command('tracked',
[('', 'addinclude', [], _('new paths to include')), [('', 'addinclude', [], _('new paths to include')),
('', 'removeinclude', [], _('old paths to no longer include')), ('', 'removeinclude', [], _('old paths to no longer include')),
('', 'addexclude', [], _('new paths to exclude')), ('', 'addexclude', [], _('new paths to exclude')),
('', 'removeexclude', [], _('old paths to no longer exclude')), ('', 'removeexclude', [], _('old paths to no longer exclude')),
('', 'clear', False, _('whether to replace the existing narrowspec')), ('', 'clear', False, _('whether to replace the existing narrowspec')),
('', 'force-delete-local-changes', False, ('', 'force-delete-local-changes', False,