Submitter | Mateusz Kwapich |
---|---|
Date | Feb. 3, 2015, 12:32 a.m. |
Message ID | <5868827d6d1a2a75d962.1422923547@dev1429.prn1.facebook.com> |
Download | mbox | patch |
Permalink | /patch/7614/ |
State | Superseded |
Commit | e15c5498d588801f57f2a8c40b48205ce643a48e |
Headers | show |
Comments
On 02/03/2015 12:32 AM, Mateusz Kwapich wrote: > # HG changeset patch > # User Mateusz Kwapich <mitrandir@fb.com> > # Date 1422314748 28800 > # Mon Jan 26 15:25:48 2015 -0800 > # Node ID 5868827d6d1a2a75d9628d502281d0d7fad2b115 > # Parent 0c8cf9835334d76f52e7cebf9d70b5b0df646871 > histedit: switch state to store node instead of ctx (I commented on V1 before seing V2, copy pasting comment) why is it necessary to change it on the state object itself? It is not obvious to me how it helps. (If you want to clean stuff in this area, feel free to also nuke the last on disk usage of pickle in the Mercurial codebase (beside convert madness).
On Mon, Feb 02, 2015 at 04:32:27PM -0800, Mateusz Kwapich wrote: > # HG changeset patch > # User Mateusz Kwapich <mitrandir@fb.com> > # Date 1422314748 28800 > # Mon Jan 26 15:25:48 2015 -0800 > # Node ID 5868827d6d1a2a75d9628d502281d0d7fad2b115 > # Parent 0c8cf9835334d76f52e7cebf9d70b5b0df646871 > histedit: switch state to store node instead of ctx > > As part of making histedit more robust to odd changes between actions, we need > to store the parentctxnode on the state instead of the parentctx. This will > allow the parentctx to be deleted in the middle of a histedit (like if someone > pauses the histedit and amends a file away). > > This change just changes what is stored, it doesn't actually fix histedit to > work if the parentctx gets deleted. That will come later. > > 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, By changing the format of this file, you win the lottery! By which I mean "it's time to stop using pickle, so fix that first." > 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) > @@ -599,7 +604,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 > @@ -655,9 +660,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 > @@ -668,10 +673,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: > @@ -724,7 +730,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] > > @@ -780,7 +787,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
It’s just a first step in direction of not relying on parentctx during hist edit. Until now if the parentctx was missing the histedit failed during loading the state. In this commit I’m moving the ctx resolution down the stack so I can then gradually remove the dependence on the ctx from all places where it’s used. On 2/3/15, 3:17 AM, "Pierre-Yves David" <pierre-yves.david@ens-lyon.org> wrote: > > >On 02/03/2015 12:32 AM, Mateusz Kwapich wrote: >> # HG changeset patch >> # User Mateusz Kwapich <mitrandir@fb.com> >> # Date 1422314748 28800 >> # Mon Jan 26 15:25:48 2015 -0800 >> # Node ID 5868827d6d1a2a75d9628d502281d0d7fad2b115 >> # Parent 0c8cf9835334d76f52e7cebf9d70b5b0df646871 >> histedit: switch state to store node instead of ctx > >(I commented on V1 before seing V2, copy pasting comment) > >why is it necessary to change it on the state object itself? It is not >obvious to me how it helps. > >(If you want to clean stuff in this area, feel free to also nuke the >last on disk usage of pickle in the Mercurial codebase (beside convert >madness). > > > >-- >Pierre-Yves David
I’m not changing the state file format here. I know that state file format should be changed, but I think that shouldn’t be the first step of histedit refactoring because I’m not yet sure how histedit state file should contain in the future. But I definitely want to change it in really near future. On 2/3/15, 7:39 AM, "Augie Fackler" <raf@durin42.com> wrote: >On Mon, Feb 02, 2015 at 04:32:27PM -0800, Mateusz Kwapich wrote: >> # HG changeset patch >> # User Mateusz Kwapich <mitrandir@fb.com> >> # Date 1422314748 28800 >> # Mon Jan 26 15:25:48 2015 -0800 >> # Node ID 5868827d6d1a2a75d9628d502281d0d7fad2b115 >> # Parent 0c8cf9835334d76f52e7cebf9d70b5b0df646871 >> histedit: switch state to store node instead of ctx >> >> As part of making histedit more robust to odd changes between actions, >>we need >> to store the parentctxnode on the state instead of the parentctx. This >>will >> allow the parentctx to be deleted in the middle of a histedit (like if >>someone >> pauses the histedit and amends a file away). >> >> This change just changes what is stored, it doesn't actually fix >>histedit to >> work if the parentctx gets deleted. That will come later. >> >> 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, > >By changing the format of this file, you win the lottery! By which I >mean "it's time to stop using pickle, so fix that first." > >> 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) >> @@ -599,7 +604,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 >> @@ -655,9 +660,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 >> @@ -668,10 +673,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: >> @@ -724,7 +730,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] >> >> @@ -780,7 +787,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
On 2/3/15 3:17 AM, Pierre-Yves David wrote: > > > On 02/03/2015 12:32 AM, Mateusz Kwapich wrote: >> # HG changeset patch >> # User Mateusz Kwapich <mitrandir@fb.com> >> # Date 1422314748 28800 >> # Mon Jan 26 15:25:48 2015 -0800 >> # Node ID 5868827d6d1a2a75d9628d502281d0d7fad2b115 >> # Parent 0c8cf9835334d76f52e7cebf9d70b5b0df646871 >> histedit: switch state to store node instead of ctx > > (I commented on V1 before seing V2, copy pasting comment) > > why is it necessary to change it on the state object itself? It is not > obvious to me how it helps. > > (If you want to clean stuff in this area, feel free to also nuke the > last on disk usage of pickle in the Mercurial codebase (beside convert > madness) 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.
On Wed, Feb 4, 2015 at 3:45 PM, Durham Goode <durham@fb.com> wrote: > > On 2/3/15 3:17 AM, Pierre-Yves David wrote: >> >> >> >> On 02/03/2015 12:32 AM, Mateusz Kwapich wrote: >>> >>> # HG changeset patch >>> # User Mateusz Kwapich <mitrandir@fb.com> >>> # Date 1422314748 28800 >>> # Mon Jan 26 15:25:48 2015 -0800 >>> # Node ID 5868827d6d1a2a75d9628d502281d0d7fad2b115 >>> # Parent 0c8cf9835334d76f52e7cebf9d70b5b0df646871 >>> histedit: switch state to store node instead of ctx >> >> >> (I commented on V1 before seing V2, copy pasting comment) >> >> why is it necessary to change it on the state object itself? It is not >> obvious to me how it helps. >> >> (If you want to clean stuff in this area, feel free to also nuke the last >> on disk usage of pickle in the Mercurial codebase (beside convert madness) > > 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. I look forward to a resend that includes this excellent documentation in the log message. Thanks! Augie
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) @@ -599,7 +604,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 @@ -655,9 +660,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 @@ -668,10 +673,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: @@ -724,7 +730,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] @@ -780,7 +787,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