| Submitter | Pierre-Yves David |
|---|---|
| Date | April 15, 2013, 3:15 p.m. |
| Message ID | <715a39973a7af426651c.1366038921@crater1.logilab.fr> |
| Download | mbox | patch |
| Permalink | /patch/1316/ |
| State | Accepted |
| Commit | 31bcc5112191ecb72a964cb270363b9b68da2973 |
| Headers | show |
Comments
On Mon, Apr 15, 2013 at 6:15 PM, <pierre-yves.david@logilab.fr> wrote: > > # HG changeset patch > # User Pierre-Yves David <pierre-yves.david@logilab.fr> > # Date 1366038658 -7200 > # Mon Apr 15 17:10:58 2013 +0200 > # Node ID 715a39973a7af426651cc2522cff04067d8d44ae > # Parent 6891e361bec6831ada94b7c2b49950769aba7be9 > destroyed: invalidate phraserevs cache in all case (issue3858) The real to test code ratio for this fix is disturbing. Can't this be tested more trivially?
On Mon, Apr 15, 2013 at 06:47:53PM +0300, Idan Kamara wrote: > On Mon, Apr 15, 2013 at 6:15 PM, <pierre-yves.david@logilab.fr> wrote: > > > > # HG changeset patch > > # User Pierre-Yves David <pierre-yves.david@logilab.fr> > > # Date 1366038658 -7200 > > # Mon Apr 15 17:10:58 2013 +0200 > > # Node ID 715a39973a7af426651cc2522cff04067d8d44ae > > # Parent 6891e361bec6831ada94b7c2b49950769aba7be9 > > destroyed: invalidate phraserevs cache in all case (issue3858) > > The real to test code ratio for this fix is disturbing. Can't this be > tested more trivially? Not much. it need a branchy history with specify phase setup. We could a tree a bit more compact but I think a wider testcase is useful given the lack of test in this area.
Den 2013-04-15 19:47:53 skrev Idan Kamara <idankk86@gmail.com>: > On Mon, Apr 15, 2013 at 6:15 PM, <pierre-yves.david@logilab.fr> wrote: >> >> # HG changeset patch >> # User Pierre-Yves David <pierre-yves.david@logilab.fr> >> # Date 1366038658 -7200 >> # Mon Apr 15 17:10:58 2013 +0200 >> # Node ID 715a39973a7af426651cc2522cff04067d8d44ae >> # Parent 6891e361bec6831ada94b7c2b49950769aba7be9 >> destroyed: invalidate phraserevs cache in all case (issue3858) > > The real to test code ratio for this fix is disturbing. Can't this be > tested more trivially? If it could, I suppose it wouldn't have taken a month of Matt's, Bryan's and Pierre-Yves' combined efforts to merely reproduce the issue (see bugzilla comments like "I've seen this happen at Facebook, too" and "It might be fixed, but I'm not sure" :)
On Mon, Apr 15, 2013 at 11:04 AM, Nikolaj Sjujskij <sterkrig@myopera.com>wrote: > If it could, I suppose it wouldn't have taken a month of Matt's, Bryan's > and Pierre-Yves' combined efforts to merely reproduce the issue (see > bugzilla comments like "I've seen this happen at Facebook, too" and "It > might be fixed, but I'm not sure" :) > Well. I'm not sure that "did nothing about this for a month" qualifies as "effort"? Although if it does, I have a long list of things I'd like to take credit for.
On 15 avr. 2013, at 20:04, Nikolaj Sjujskij wrote: > Den 2013-04-15 19:47:53 skrev Idan Kamara <idankk86@gmail.com>: > >> On Mon, Apr 15, 2013 at 6:15 PM, <pierre-yves.david@logilab.fr> wrote: >>> >>> # HG changeset patch >>> # User Pierre-Yves David <pierre-yves.david@logilab.fr> >>> # Date 1366038658 -7200 >>> # Mon Apr 15 17:10:58 2013 +0200 >>> # Node ID 715a39973a7af426651cc2522cff04067d8d44ae >>> # Parent 6891e361bec6831ada94b7c2b49950769aba7be9 >>> destroyed: invalidate phraserevs cache in all case (issue3858) >> >> The real to test code ratio for this fix is disturbing. Can't this be >> tested more trivially? > If it could, I suppose it wouldn't have taken a month of Matt's, Bryan's and Pierre-Yves' combined efforts to merely reproduce the issue It took me two month to get a day for looking at it ^^ that is all. The precise bug report with example and data saved the day.
On Mon, 2013-04-15 at 17:15 +0200, pierre-yves.david@logilab.fr wrote: > # HG changeset patch > # User Pierre-Yves David <pierre-yves.david@logilab.fr> > # Date 1366038658 -7200 > # Mon Apr 15 17:10:58 2013 +0200 > # Node ID 715a39973a7af426651cc2522cff04067d8d44ae > # Parent 6891e361bec6831ada94b7c2b49950769aba7be9 > destroyed: invalidate phraserevs cache in all case (issue3858) Queued for default, thanks.
Patch
diff --git a/mercurial/phases.py b/mercurial/phases.py --- a/mercurial/phases.py +++ b/mercurial/phases.py @@ -264,11 +264,19 @@ class phasecache(object): % (short(mnode), phase)) nodes.symmetric_difference_update(missing) filtered = True if filtered: self.dirty = True - self._phaserevs = None + # filterunknown is called by repo.destroyed, we may have no changes in + # root but phaserevs contents is certainly invalide (or at least we + # have not proper way to check that. related to issue 3858. + # + # The other caller is __init__ that have no _phaserevs initialized + # anyway. If this change we should consider adding a dedicated + # "destroyed" function to phasecache or a proper cache key mechanisme + # (see branchmap one) + self._phaserevs = None def advanceboundary(repo, targetphase, nodes): """Add nodes to a phase changing other nodes phases if necessary. This function move boundary *forward* this means that all nodes diff --git a/tests/test-rebase-cache.t b/tests/test-rebase-cache.t --- a/tests/test-rebase-cache.t +++ b/tests/test-rebase-cache.t @@ -383,5 +383,114 @@ Try both orders. 0 files updated, 0 files merged, 2 files removed, 0 files unresolved saved backup bundle to $TESTTMP/a3/c/.hg/strip-backup/*-backup.hg (glob) $ hg theads 0: 'A' + +Make sure rebase does not break for phase/filter related reason +---------------------------------------------------------------- +(issue3858) + + $ cd .. + + $ cat >> $HGRCPATH << EOF + > [ui] + > logtemplate={rev} {desc} {phase}\n + > EOF + $ cat $HGRCPATH + [ui] + slash = True + interactive = False + [defaults] + backout = -d "0 0" + commit = -d "0 0" + tag = -d "0 0" + [extensions] + graphlog= + rebase= + mq= + + [phases] + publish=False + + [alias] + tglog = log -G --template "{rev}: '{desc}' {branches}\n" + theads = heads --template "{rev}: '{desc}' {branches}\n" + [ui] + logtemplate={rev} {desc} {phase}\n + + + $ hg init c4 + $ cd c4 + + $ echo a > a + $ hg ci -Am A + adding a + $ echo b > b + $ hg ci -Am B + adding b + $ hg up 0 + 0 files updated, 0 files merged, 1 files removed, 0 files unresolved + $ echo c > c + $ hg ci -Am C + adding c + created new head + $ hg up 1 + 1 files updated, 0 files merged, 1 files removed, 0 files unresolved + $ hg merge + 1 files updated, 0 files merged, 0 files removed, 0 files unresolved + (branch merge, don't forget to commit) + $ hg ci -m d + $ hg up 2 + 0 files updated, 0 files merged, 1 files removed, 0 files unresolved + $ echo e > e + $ hg ci -Am E + adding e + created new head + $ hg merge 3 + 1 files updated, 0 files merged, 0 files removed, 0 files unresolved + (branch merge, don't forget to commit) + $ hg ci -m F + $ hg up 3 + 0 files updated, 0 files merged, 1 files removed, 0 files unresolved + $ echo g > g + $ hg ci -Am G + adding g + created new head + $ echo h > h + $ hg ci -Am H + adding h + $ hg up 5 + 1 files updated, 0 files merged, 2 files removed, 0 files unresolved + $ echo i > i + $ hg ci -Am I + adding i + +Turn most changeset public + + $ hg ph -p 7 + + $ hg heads + 8 I draft + 7 H public + $ hg log -G + @ 8 I draft + | + | o 7 H public + | | + | o 6 G public + | | + o | 5 F draft + |\| + o | 4 E draft + | | + | o 3 d public + |/| + o | 2 C public + | | + | o 1 B public + |/ + o 0 A public + + + $ hg rebase --dest 7 --source 5 + saved backup bundle to $TESTTMP/a3/c4/.hg/strip-backup/*-backup.hg (glob)
# HG changeset patch # User Pierre-Yves David <pierre-yves.david@logilab.fr> # Date 1366038658 -7200 # Mon Apr 15 17:10:58 2013 +0200 # Node ID 715a39973a7af426651cc2522cff04067d8d44ae # Parent 6891e361bec6831ada94b7c2b49950769aba7be9 destroyed: invalidate phraserevs cache in all case (issue3858) When revisions are destroyed, the `phaserevs` cache becomes invalid in most case. This cache hold a `{rev => phase}` mapping and revision number most likely changed. Since 1c8e0d6ac3b0, we filter unknown phases' roots after changesets destruction. When some roots are filtered the `phaserevs` cache is invalidated. But not if none root where destroyed. We now invalidate the cache in all case filtered root or not. This bug was a bit tricky to reproduce as in most case we either: * rebase a set a draft changeset including root (phaserev invalidated) * strip tip-most changesets (no re-numbering of revision) Note that the invalidation of `phaserevs` are not strictly needed when only tip-most part of the history have been destroyed. But I do not expect the overhead to be significant.