Patchwork destroyed: invalidate phraserevs cache in all case (issue3858)

login
register
mail settings
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

Pierre-Yves David - April 15, 2013, 3:15 p.m.
# 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.
Idan Kamara - April 15, 2013, 3:47 p.m.
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?
Pierre-Yves David - April 15, 2013, 3:52 p.m.
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.
Nikolaj Sjujskij - April 15, 2013, 6:04 p.m.
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" :)
Bryan O'Sullivan - April 15, 2013, 7:03 p.m.
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.
Pierre-Yves David - April 15, 2013, 8:57 p.m.
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.
Matt Mackall - April 16, 2013, 4:48 p.m.
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)