Patchwork rebase: turn rebase revs into set before filtering obsolete

login
register
mail settings
Submitter Simon Farnsworth
Date July 19, 2016, 10:39 a.m.
Message ID <39ca6ff7183e4a42fea8.1468924783@devvm631.lla1.facebook.com>
Download mbox | patch
Permalink /patch/15954/
State Accepted
Headers show

Comments

Simon Farnsworth - July 19, 2016, 10:39 a.m.
# HG changeset patch
# User Simon Farnsworth <simonfar@fb.com>
# Date 1468924193 25200
#      Tue Jul 19 03:29:53 2016 -0700
# Branch stable
# Node ID 39ca6ff7183e4a42fea832e0fedfe65de9b70ffa
# Parent  02a8fea4289b51992b2495a06d4b12cbda876cf1
rebase: turn rebase revs into set before filtering obsolete

When the inhibit extension from mutable-history is enabled, it attempts to
iterate over the rebaseset to prevent the nodes being rebased from being
marked obsolete. This happens at the same time as rebase's
_filterobsoleterevs function trying to iterate over the rebaseset to figure
out which ones are obsolete. The two of these iterating over the same
revset generatorset cause a 'generator already executing' exception. This is
probably a flaw in the revset implementation, since iterating over the same
set twice should be supported.

This regression was introduced in 5d16ebe7b14, since it changed
_filterobsoleterevs to be called before the rebaseset was turned into a
set(). For now let’s just make the rebaseset an actual set again before
calling that function. This was caught by the inhibit tests.

The relevant call stack from test-inhibit.t:

   File "/tmp/hgtests.jgjrN5/install/lib/python/hgext/rebase.py", line 285, in _preparenewrebase
     obsrevs = _filterobsoleterevs(self.repo, rebaseset)
   File "/data/hgbuild/facebook-hg-rpms/mutable-history/hgext/inhibit.py", line 197, in _filterobsoleterevswrap
     r = orig(repo, rebasesetrevs, *args, **kwargs)
   File "/tmp/hgtests.jgjrN5/install/lib/python/hgext/rebase.py", line 1380, in _filterobsoleterevs
     return set(r for r in revs if repo[r].obsolete())
   File "/tmp/hgtests.jgjrN5/install/lib/python/hgext/rebase.py", line 1380, in <genexpr>
     return set(r for r in revs if repo[r].obsolete())
   File "/tmp/hgtests.jgjrN5/install/lib/python/mercurial/revset.py", line 3079, in _iterordered
     val2 = next(iter2)
   File "/tmp/hgtests.jgjrN5/install/lib/python/mercurial/revset.py", line 3417, in gen
     yield nextrev()
   File "/tmp/hgtests.jgjrN5/install/lib/python/mercurial/revset.py", line 3424, in _consumegen
     for item in self._gen:
   File "/tmp/hgtests.jgjrN5/install/lib/python/mercurial/revset.py", line 71, in iterate
     cl = repo.changelog
   File "/tmp/hgtests.jgjrN5/install/lib/python/mercurial/repoview.py", line 319, in changelog
     revs = filterrevs(unfi, self.filtername)
   File "/tmp/hgtests.jgjrN5/install/lib/python/mercurial/repoview.py", line 261, in filterrevs
     repo.filteredrevcache[filtername] = func(repo.unfiltered())
   File "/data/hgbuild/facebook-hg-rpms/mutable-history/hgext/directaccess.py", line 65, in _computehidden
     hidden = repoview.filterrevs(repo, 'visible')
   File "/tmp/hgtests.jgjrN5/install/lib/python/mercurial/repoview.py", line 261, in filterrevs
     repo.filteredrevcache[filtername] = func(repo.unfiltered())
   File "/tmp/hgtests.jgjrN5/install/lib/python/mercurial/repoview.py", line 175, in computehidden
     hideable = hideablerevs(repo)
   File "/tmp/hgtests.jgjrN5/install/lib/python/mercurial/repoview.py", line 33, in hideablerevs
     return obsolete.getrevs(repo, 'obsolete')
   File "/tmp/hgtests.jgjrN5/install/lib/python/mercurial/obsolete.py", line 1097, in getrevs
     repo.obsstore.caches[name] = cachefuncs[name](repo)
   File "/data/hgbuild/facebook-hg-rpms/mutable-history/hgext/inhibit.py", line 255, in _computeobsoleteset
     if getrev(n) not in blacklist:
   File "/tmp/hgtests.jgjrN5/install/lib/python/mercurial/revset.py", line 3264, in __contains__
     return x in self._r1 or x in self._r2
   File "/tmp/hgtests.jgjrN5/install/lib/python/mercurial/revset.py", line 3348, in __contains__
     for l in self._consumegen():
   File "/tmp/hgtests.jgjrN5/install/lib/python/mercurial/revset.py", line 3424, in _consumegen
     for item in self._gen:
 ValueError: generator already executing
Jun Wu - July 19, 2016, 12:24 p.m.
LGTM. Thanks for the detailed explanation.

Excerpts from Simon Farnsworth's message of 2016-07-19 03:39:43 -0700:
> # HG changeset patch
> # User Simon Farnsworth <simonfar@fb.com>
> # Date 1468924193 25200
> #      Tue Jul 19 03:29:53 2016 -0700
> # Branch stable
> # Node ID 39ca6ff7183e4a42fea832e0fedfe65de9b70ffa
> # Parent  02a8fea4289b51992b2495a06d4b12cbda876cf1
> rebase: turn rebase revs into set before filtering obsolete
> 
> When the inhibit extension from mutable-history is enabled, it attempts to
> iterate over the rebaseset to prevent the nodes being rebased from being
> marked obsolete. This happens at the same time as rebase's
> _filterobsoleterevs function trying to iterate over the rebaseset to figure
> out which ones are obsolete. The two of these iterating over the same
> revset generatorset cause a 'generator already executing' exception. This is
> probably a flaw in the revset implementation, since iterating over the same
> set twice should be supported.
> 
> This regression was introduced in 5d16ebe7b14, since it changed
> _filterobsoleterevs to be called before the rebaseset was turned into a
> set(). For now let’s just make the rebaseset an actual set again before
> calling that function. This was caught by the inhibit tests.
> 
> The relevant call stack from test-inhibit.t:
> 
>    File "/tmp/hgtests.jgjrN5/install/lib/python/hgext/rebase.py", line 285, in _preparenewrebase
>      obsrevs = _filterobsoleterevs(self.repo, rebaseset)
>    File "/data/hgbuild/facebook-hg-rpms/mutable-history/hgext/inhibit.py", line 197, in _filterobsoleterevswrap
>      r = orig(repo, rebasesetrevs, *args, **kwargs)
>    File "/tmp/hgtests.jgjrN5/install/lib/python/hgext/rebase.py", line 1380, in _filterobsoleterevs
>      return set(r for r in revs if repo[r].obsolete())
>    File "/tmp/hgtests.jgjrN5/install/lib/python/hgext/rebase.py", line 1380, in <genexpr>
>      return set(r for r in revs if repo[r].obsolete())
>    File "/tmp/hgtests.jgjrN5/install/lib/python/mercurial/revset.py", line 3079, in _iterordered
>      val2 = next(iter2)
>    File "/tmp/hgtests.jgjrN5/install/lib/python/mercurial/revset.py", line 3417, in gen
>      yield nextrev()
>    File "/tmp/hgtests.jgjrN5/install/lib/python/mercurial/revset.py", line 3424, in _consumegen
>      for item in self._gen:
>    File "/tmp/hgtests.jgjrN5/install/lib/python/mercurial/revset.py", line 71, in iterate
>      cl = repo.changelog
>    File "/tmp/hgtests.jgjrN5/install/lib/python/mercurial/repoview.py", line 319, in changelog
>      revs = filterrevs(unfi, self.filtername)
>    File "/tmp/hgtests.jgjrN5/install/lib/python/mercurial/repoview.py", line 261, in filterrevs
>      repo.filteredrevcache[filtername] = func(repo.unfiltered())
>    File "/data/hgbuild/facebook-hg-rpms/mutable-history/hgext/directaccess.py", line 65, in _computehidden
>      hidden = repoview.filterrevs(repo, 'visible')
>    File "/tmp/hgtests.jgjrN5/install/lib/python/mercurial/repoview.py", line 261, in filterrevs
>      repo.filteredrevcache[filtername] = func(repo.unfiltered())
>    File "/tmp/hgtests.jgjrN5/install/lib/python/mercurial/repoview.py", line 175, in computehidden
>      hideable = hideablerevs(repo)
>    File "/tmp/hgtests.jgjrN5/install/lib/python/mercurial/repoview.py", line 33, in hideablerevs
>      return obsolete.getrevs(repo, 'obsolete')
>    File "/tmp/hgtests.jgjrN5/install/lib/python/mercurial/obsolete.py", line 1097, in getrevs
>      repo.obsstore.caches[name] = cachefuncs[name](repo)
>    File "/data/hgbuild/facebook-hg-rpms/mutable-history/hgext/inhibit.py", line 255, in _computeobsoleteset
>      if getrev(n) not in blacklist:
>    File "/tmp/hgtests.jgjrN5/install/lib/python/mercurial/revset.py", line 3264, in __contains__
>      return x in self._r1 or x in self._r2
>    File "/tmp/hgtests.jgjrN5/install/lib/python/mercurial/revset.py", line 3348, in __contains__
>      for l in self._consumegen():
>    File "/tmp/hgtests.jgjrN5/install/lib/python/mercurial/revset.py", line 3424, in _consumegen
>      for item in self._gen:
>  ValueError: generator already executing
> 
> diff --git a/hgext/rebase.py b/hgext/rebase.py
> --- a/hgext/rebase.py
> +++ b/hgext/rebase.py
> @@ -281,7 +281,7 @@
>                    " unrebased descendants"),
>                  hint=_('use --keep to keep original changesets'))
>  
> -        obsrevs = _filterobsoleterevs(self.repo, rebaseset)
> +        obsrevs = _filterobsoleterevs(self.repo, set(rebaseset))
>          self._handleskippingobsolete(rebaseset, obsrevs, dest)
>  
>          result = buildstate(self.repo, dest, rebaseset, self.collapsef,
Yuya Nishihara - July 19, 2016, 1:23 p.m.
On Tue, 19 Jul 2016 03:39:43 -0700, Simon Farnsworth wrote:
> # HG changeset patch
> # User Simon Farnsworth <simonfar@fb.com>
> # Date 1468924193 25200
> #      Tue Jul 19 03:29:53 2016 -0700
> # Branch stable
> # Node ID 39ca6ff7183e4a42fea832e0fedfe65de9b70ffa
> # Parent  02a8fea4289b51992b2495a06d4b12cbda876cf1
> rebase: turn rebase revs into set before filtering obsolete
> 
> When the inhibit extension from mutable-history is enabled, it attempts to
> iterate over the rebaseset to prevent the nodes being rebased from being
> marked obsolete. This happens at the same time as rebase's
> _filterobsoleterevs function trying to iterate over the rebaseset to figure
> out which ones are obsolete. The two of these iterating over the same
> revset generatorset cause a 'generator already executing' exception. This is
> probably a flaw in the revset implementation, since iterating over the same
> set twice should be supported.

Perhaps, revset, repoview or obsolete?

The fix is simple enough for stable. Queued, thanks.

Patch

diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -281,7 +281,7 @@ 
                   " unrebased descendants"),
                 hint=_('use --keep to keep original changesets'))
 
-        obsrevs = _filterobsoleterevs(self.repo, rebaseset)
+        obsrevs = _filterobsoleterevs(self.repo, set(rebaseset))
         self._handleskippingobsolete(rebaseset, obsrevs, dest)
 
         result = buildstate(self.repo, dest, rebaseset, self.collapsef,