Patchwork [2,of,2] rebase: use unfiltered repo and remove complex unhiding code (issue5219)

login
register
mail settings
Submitter via Mercurial-devel
Date March 11, 2017, 1:15 a.m.
Message ID <0c89a4fb256390123fd5.1489194952@martinvonz.mtv.corp.google.com>
Download mbox | patch
Permalink /patch/19093/
State Superseded
Headers show

Comments

via Mercurial-devel - March 11, 2017, 1:15 a.m.
# HG changeset patch
# User Martin von Zweigbergk <martinvonz@google.com>
# Date 1485968584 28800
#      Wed Feb 01 09:03:04 2017 -0800
# Node ID 0c89a4fb256390123fd543d73cbd5f01450e6da7
# Parent  460068415dc97223cce156c70f7f8a6b659c4dd2
rebase: use unfiltered repo and remove complex unhiding code (issue5219)

Instead of dynamically unhiding commits, just work with the unfiltered
repo. The deleted _setrebasesetvisibility() code seems very
intentional, but I can't figure out what I'm breaking with this
patch. Hopefully reviewers can tell me.
Jun Wu - March 11, 2017, 2:06 a.m.
I like this change. It removes a dynamic blocker.

I also did some manual tests and the logic seems good to me.

Excerpts from Martin von Zweigbergk's message of 2017-03-10 17:15:52 -0800:
> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@google.com>
> # Date 1485968584 28800
> #      Wed Feb 01 09:03:04 2017 -0800
> # Node ID 0c89a4fb256390123fd543d73cbd5f01450e6da7
> # Parent  460068415dc97223cce156c70f7f8a6b659c4dd2
> rebase: use unfiltered repo and remove complex unhiding code (issue5219)
> 
> Instead of dynamically unhiding commits, just work with the unfiltered
> repo. The deleted _setrebasesetvisibility() code seems very
> intentional, but I can't figure out what I'm breaking with this
> patch. Hopefully reviewers can tell me.
> 
> diff -r 460068415dc9 -r 0c89a4fb2563 hgext/rebase.py
> --- a/hgext/rebase.py    Wed Feb 01 09:18:44 2017 -0800
> +++ b/hgext/rebase.py    Wed Feb 01 09:03:04 2017 -0800
> @@ -44,7 +44,6 @@
>      phases,
>      registrar,
>      repair,
> -    repoview,
>      revset,
>      scmutil,
>      smartset,
> @@ -128,7 +127,7 @@
>          if opts is None:
>              opts = {}
>  
> -        self.repo = repo
> +        self.repo = repo.unfiltered()
>          self.ui = ui
>          self.opts = opts
>          self.originalwd = None
> @@ -250,7 +249,6 @@
>          repo.ui.debug('computed skipped revs: %s\n' %
>                          (' '.join(str(r) for r in sorted(skipped)) or None))
>          repo.ui.debug('rebase status resumed\n')
> -        _setrebasesetvisibility(repo, state.keys())
>  
>          self.originalwd = originalwd
>          self.target = target
> @@ -1119,7 +1117,6 @@
>  
>  def clearstatus(repo):
>      'Remove the status files'
> -    _clearrebasesetvisibiliy(repo)
>      util.unlinkpath(repo.join("rebasestate"), ignoremissing=True)
>  
>  def needupdate(repo, state):
> @@ -1203,7 +1200,6 @@
>      dest: context
>      rebaseset: set of rev
>      '''
> -    _setrebasesetvisibility(repo, rebaseset)
>  
>      # This check isn't strictly necessary, since mq detects commits over an
>      # applied patch. But it prevents messing up the working directory when
> @@ -1383,31 +1379,6 @@
>  
>      return ret
>  
> -def _setrebasesetvisibility(repo, revs):
> -    """store the currently rebased set on the repo object
> -
> -    This is used by another function to prevent rebased revision to because
> -    hidden (see issue4504)"""
> -    repo = repo.unfiltered()
> -    revs = set(revs)
> -    repo._rebaseset = revs
> -    # invalidate cache if visibility changes
> -    hiddens = repo.filteredrevcache.get('visible', set())
> -    if revs & hiddens:
> -        repo.invalidatevolatilesets()
> -
> -def _clearrebasesetvisibiliy(repo):
> -    """remove rebaseset data from the repo"""
> -    repo = repo.unfiltered()
> -    if '_rebaseset' in vars(repo):
> -        del repo._rebaseset
> -
> -def _rebasedvisible(orig, repo):
> -    """ensure rebased revs stay visible (see issue4504)"""
> -    blockers = orig(repo)
> -    blockers.update(getattr(repo, '_rebaseset', ()))
> -    return blockers
> -
>  def _filterobsoleterevs(repo, revs):
>      """returns a set of the obsolete revisions in revs"""
>      return set(r for r in revs if repo[r].obsolete())
> @@ -1479,5 +1450,3 @@
>           _("use 'hg rebase --continue' or 'hg rebase --abort'")])
>      cmdutil.afterresolvedstates.append(
>          ['rebasestate', _('hg rebase --continue')])
> -    # ensure rebased rev are not hidden
> -    extensions.wrapfunction(repoview, '_getdynamicblockers', _rebasedvisible)
> diff -r 460068415dc9 -r 0c89a4fb2563 tests/test-rebase-obsolete.t
> --- a/tests/test-rebase-obsolete.t    Wed Feb 01 09:18:44 2017 -0800
> +++ b/tests/test-rebase-obsolete.t    Wed Feb 01 09:03:04 2017 -0800
> @@ -279,9 +279,27 @@
>    $ hg --hidden up -qr 'first(hidden())'
>    $ hg rebase --rev 13 --dest 15
>    rebasing 13:98f6af4ee953 "C"
> -  abort: hidden revision '1'!
> -  (use --hidden to access hidden revisions)
> -  [255]
> +  $ hg log -G
> +  o  16:294a2b93eb4d C
> +  |
> +  o  15:627d46148090 D
> +  |
> +  | o  12:462a34d07e59 B
> +  | |
> +  | o  11:4596109a6a43 D
> +  | |
> +  | o  7:02de42196ebe H
> +  | |
> +  +---o  6:eea13746799a G
> +  | |/
> +  | o  5:24b6387c8c8c F
> +  | |
> +  o |  4:9520eea781bc E
> +  |/
> +  | @  1:42ccdea3bb16 B
> +  |/
> +  o  0:cd010b8cd998 A
> +  
>  
>    $ cd ..
>
via Mercurial-devel - March 11, 2017, 6:17 a.m.
On Fri, Mar 10, 2017 at 6:06 PM, Jun Wu <quark@fb.com> wrote:
> I like this change. It removes a dynamic blocker.
>
> I also did some manual tests and the logic seems good to me.
>
> Excerpts from Martin von Zweigbergk's message of 2017-03-10 17:15:52 -0800:
>> # HG changeset patch
>> # User Martin von Zweigbergk <martinvonz@google.com>
>> # Date 1485968584 28800
>> #      Wed Feb 01 09:03:04 2017 -0800
>> # Node ID 0c89a4fb256390123fd543d73cbd5f01450e6da7
>> # Parent  460068415dc97223cce156c70f7f8a6b659c4dd2
>> rebase: use unfiltered repo and remove complex unhiding code (issue5219)
>>
>> Instead of dynamically unhiding commits, just work with the unfiltered
>> repo. The deleted _setrebasesetvisibility() code seems very
>> intentional, but I can't figure out what I'm breaking with this
>> patch. Hopefully reviewers can tell me.

Pierre-Yves expressed unspecified concern that this patch might make
some operations use the unfiltered repo that shouldn't be. So I looked
a little closer and I think I found a case that confirms his
suspicion. When allowunstable is not set, I think the revset
"first(children(<rebaseset>) - rebaseset)" would report an obsolete
child as unstable. I'll try to resend this series with a more narrowly
scoped use of repo.unfiltered(), or maybe with an updated set of
"dynamic blockers".

>>
>> diff -r 460068415dc9 -r 0c89a4fb2563 hgext/rebase.py
>> --- a/hgext/rebase.py    Wed Feb 01 09:18:44 2017 -0800
>> +++ b/hgext/rebase.py    Wed Feb 01 09:03:04 2017 -0800
>> @@ -44,7 +44,6 @@
>>      phases,
>>      registrar,
>>      repair,
>> -    repoview,
>>      revset,
>>      scmutil,
>>      smartset,
>> @@ -128,7 +127,7 @@
>>          if opts is None:
>>              opts = {}
>>
>> -        self.repo = repo
>> +        self.repo = repo.unfiltered()
>>          self.ui = ui
>>          self.opts = opts
>>          self.originalwd = None
>> @@ -250,7 +249,6 @@
>>          repo.ui.debug('computed skipped revs: %s\n' %
>>                          (' '.join(str(r) for r in sorted(skipped)) or None))
>>          repo.ui.debug('rebase status resumed\n')
>> -        _setrebasesetvisibility(repo, state.keys())
>>
>>          self.originalwd = originalwd
>>          self.target = target
>> @@ -1119,7 +1117,6 @@
>>
>>  def clearstatus(repo):
>>      'Remove the status files'
>> -    _clearrebasesetvisibiliy(repo)
>>      util.unlinkpath(repo.join("rebasestate"), ignoremissing=True)
>>
>>  def needupdate(repo, state):
>> @@ -1203,7 +1200,6 @@
>>      dest: context
>>      rebaseset: set of rev
>>      '''
>> -    _setrebasesetvisibility(repo, rebaseset)
>>
>>      # This check isn't strictly necessary, since mq detects commits over an
>>      # applied patch. But it prevents messing up the working directory when
>> @@ -1383,31 +1379,6 @@
>>
>>      return ret
>>
>> -def _setrebasesetvisibility(repo, revs):
>> -    """store the currently rebased set on the repo object
>> -
>> -    This is used by another function to prevent rebased revision to because
>> -    hidden (see issue4504)"""
>> -    repo = repo.unfiltered()
>> -    revs = set(revs)
>> -    repo._rebaseset = revs
>> -    # invalidate cache if visibility changes
>> -    hiddens = repo.filteredrevcache.get('visible', set())
>> -    if revs & hiddens:
>> -        repo.invalidatevolatilesets()
>> -
>> -def _clearrebasesetvisibiliy(repo):
>> -    """remove rebaseset data from the repo"""
>> -    repo = repo.unfiltered()
>> -    if '_rebaseset' in vars(repo):
>> -        del repo._rebaseset
>> -
>> -def _rebasedvisible(orig, repo):
>> -    """ensure rebased revs stay visible (see issue4504)"""
>> -    blockers = orig(repo)
>> -    blockers.update(getattr(repo, '_rebaseset', ()))
>> -    return blockers
>> -
>>  def _filterobsoleterevs(repo, revs):
>>      """returns a set of the obsolete revisions in revs"""
>>      return set(r for r in revs if repo[r].obsolete())
>> @@ -1479,5 +1450,3 @@
>>           _("use 'hg rebase --continue' or 'hg rebase --abort'")])
>>      cmdutil.afterresolvedstates.append(
>>          ['rebasestate', _('hg rebase --continue')])
>> -    # ensure rebased rev are not hidden
>> -    extensions.wrapfunction(repoview, '_getdynamicblockers', _rebasedvisible)
>> diff -r 460068415dc9 -r 0c89a4fb2563 tests/test-rebase-obsolete.t
>> --- a/tests/test-rebase-obsolete.t    Wed Feb 01 09:18:44 2017 -0800
>> +++ b/tests/test-rebase-obsolete.t    Wed Feb 01 09:03:04 2017 -0800
>> @@ -279,9 +279,27 @@
>>    $ hg --hidden up -qr 'first(hidden())'
>>    $ hg rebase --rev 13 --dest 15
>>    rebasing 13:98f6af4ee953 "C"
>> -  abort: hidden revision '1'!
>> -  (use --hidden to access hidden revisions)
>> -  [255]
>> +  $ hg log -G
>> +  o  16:294a2b93eb4d C
>> +  |
>> +  o  15:627d46148090 D
>> +  |
>> +  | o  12:462a34d07e59 B
>> +  | |
>> +  | o  11:4596109a6a43 D
>> +  | |
>> +  | o  7:02de42196ebe H
>> +  | |
>> +  +---o  6:eea13746799a G
>> +  | |/
>> +  | o  5:24b6387c8c8c F
>> +  | |
>> +  o |  4:9520eea781bc E
>> +  |/
>> +  | @  1:42ccdea3bb16 B
>> +  |/
>> +  o  0:cd010b8cd998 A
>> +
>>
>>    $ cd ..
>>
Jun Wu - March 11, 2017, 6:57 p.m.
Excerpts from Martin von Zweigbergk's message of 2017-03-10 22:17:10 -0800:
> On Fri, Mar 10, 2017 at 6:06 PM, Jun Wu <quark@fb.com> wrote:
> > I like this change. It removes a dynamic blocker.
> >
> > I also did some manual tests and the logic seems good to me.
> >
> > Excerpts from Martin von Zweigbergk's message of 2017-03-10 17:15:52 -0800:
> >> # HG changeset patch
> >> # User Martin von Zweigbergk <martinvonz@google.com>
> >> # Date 1485968584 28800
> >> #      Wed Feb 01 09:03:04 2017 -0800
> >> # Node ID 0c89a4fb256390123fd543d73cbd5f01450e6da7
> >> # Parent  460068415dc97223cce156c70f7f8a6b659c4dd2
> >> rebase: use unfiltered repo and remove complex unhiding code (issue5219)
> >>
> >> Instead of dynamically unhiding commits, just work with the unfiltered
> >> repo. The deleted _setrebasesetvisibility() code seems very
> >> intentional, but I can't figure out what I'm breaking with this
> >> patch. Hopefully reviewers can tell me.
> 
> Pierre-Yves expressed unspecified concern that this patch might make
> some operations use the unfiltered repo that shouldn't be. So I looked
> a little closer and I think I found a case that confirms his
> suspicion. When allowunstable is not set, I think the revset
> "first(children(<rebaseset>) - rebaseset)" would report an obsolete
> child as unstable. I'll try to resend this series with a more narrowly
> scoped use of repo.unfiltered(), or maybe with an updated set of
> "dynamic blockers".

I wish we could kill the "allowunstable" feature eventually. That'll likely
simplify a lot of things. It's not that unrealistic since the "archived"
phase could be a thing, and will help people who want safe history rewriting
without using the obsstore.

Since some caches (branch, tags) depend on "dynamic blockers", making it
more complex may invalidate those caches. I think just checking
"allowunstable" and using "unfiltered" is better if we want better
performance and less complex code.

> >>
> >> diff -r 460068415dc9 -r 0c89a4fb2563 hgext/rebase.py
> >> --- a/hgext/rebase.py    Wed Feb 01 09:18:44 2017 -0800
> >> +++ b/hgext/rebase.py    Wed Feb 01 09:03:04 2017 -0800
> >> @@ -44,7 +44,6 @@
> >>      phases,
> >>      registrar,
> >>      repair,
> >> -    repoview,
> >>      revset,
> >>      scmutil,
> >>      smartset,
> >> @@ -128,7 +127,7 @@
> >>          if opts is None:
> >>              opts = {}
> >>
> >> -        self.repo = repo
> >> +        self.repo = repo.unfiltered()
> >>          self.ui = ui
> >>          self.opts = opts
> >>          self.originalwd = None
> >> @@ -250,7 +249,6 @@
> >>          repo.ui.debug('computed skipped revs: %s\n' %
> >>                          (' '.join(str(r) for r in sorted(skipped)) or None))
> >>          repo.ui.debug('rebase status resumed\n')
> >> -        _setrebasesetvisibility(repo, state.keys())
> >>
> >>          self.originalwd = originalwd
> >>          self.target = target
> >> @@ -1119,7 +1117,6 @@
> >>
> >>  def clearstatus(repo):
> >>      'Remove the status files'
> >> -    _clearrebasesetvisibiliy(repo)
> >>      util.unlinkpath(repo.join("rebasestate"), ignoremissing=True)
> >>
> >>  def needupdate(repo, state):
> >> @@ -1203,7 +1200,6 @@
> >>      dest: context
> >>      rebaseset: set of rev
> >>      '''
> >> -    _setrebasesetvisibility(repo, rebaseset)
> >>
> >>      # This check isn't strictly necessary, since mq detects commits over an
> >>      # applied patch. But it prevents messing up the working directory when
> >> @@ -1383,31 +1379,6 @@
> >>
> >>      return ret
> >>
> >> -def _setrebasesetvisibility(repo, revs):
> >> -    """store the currently rebased set on the repo object
> >> -
> >> -    This is used by another function to prevent rebased revision to because
> >> -    hidden (see issue4504)"""
> >> -    repo = repo.unfiltered()
> >> -    revs = set(revs)
> >> -    repo._rebaseset = revs
> >> -    # invalidate cache if visibility changes
> >> -    hiddens = repo.filteredrevcache.get('visible', set())
> >> -    if revs & hiddens:
> >> -        repo.invalidatevolatilesets()
> >> -
> >> -def _clearrebasesetvisibiliy(repo):
> >> -    """remove rebaseset data from the repo"""
> >> -    repo = repo.unfiltered()
> >> -    if '_rebaseset' in vars(repo):
> >> -        del repo._rebaseset
> >> -
> >> -def _rebasedvisible(orig, repo):
> >> -    """ensure rebased revs stay visible (see issue4504)"""
> >> -    blockers = orig(repo)
> >> -    blockers.update(getattr(repo, '_rebaseset', ()))
> >> -    return blockers
> >> -
> >>  def _filterobsoleterevs(repo, revs):
> >>      """returns a set of the obsolete revisions in revs"""
> >>      return set(r for r in revs if repo[r].obsolete())
> >> @@ -1479,5 +1450,3 @@
> >>           _("use 'hg rebase --continue' or 'hg rebase --abort'")])
> >>      cmdutil.afterresolvedstates.append(
> >>          ['rebasestate', _('hg rebase --continue')])
> >> -    # ensure rebased rev are not hidden
> >> -    extensions.wrapfunction(repoview, '_getdynamicblockers', _rebasedvisible)
> >> diff -r 460068415dc9 -r 0c89a4fb2563 tests/test-rebase-obsolete.t
> >> --- a/tests/test-rebase-obsolete.t    Wed Feb 01 09:18:44 2017 -0800
> >> +++ b/tests/test-rebase-obsolete.t    Wed Feb 01 09:03:04 2017 -0800
> >> @@ -279,9 +279,27 @@
> >>    $ hg --hidden up -qr 'first(hidden())'
> >>    $ hg rebase --rev 13 --dest 15
> >>    rebasing 13:98f6af4ee953 "C"
> >> -  abort: hidden revision '1'!
> >> -  (use --hidden to access hidden revisions)
> >> -  [255]
> >> +  $ hg log -G
> >> +  o  16:294a2b93eb4d C
> >> +  |
> >> +  o  15:627d46148090 D
> >> +  |
> >> +  | o  12:462a34d07e59 B
> >> +  | |
> >> +  | o  11:4596109a6a43 D
> >> +  | |
> >> +  | o  7:02de42196ebe H
> >> +  | |
> >> +  +---o  6:eea13746799a G
> >> +  | |/
> >> +  | o  5:24b6387c8c8c F
> >> +  | |
> >> +  o |  4:9520eea781bc E
> >> +  |/
> >> +  | @  1:42ccdea3bb16 B
> >> +  |/
> >> +  o  0:cd010b8cd998 A
> >> +
> >>
> >>    $ cd ..
> >>

Patch

diff -r 460068415dc9 -r 0c89a4fb2563 hgext/rebase.py
--- a/hgext/rebase.py	Wed Feb 01 09:18:44 2017 -0800
+++ b/hgext/rebase.py	Wed Feb 01 09:03:04 2017 -0800
@@ -44,7 +44,6 @@ 
     phases,
     registrar,
     repair,
-    repoview,
     revset,
     scmutil,
     smartset,
@@ -128,7 +127,7 @@ 
         if opts is None:
             opts = {}
 
-        self.repo = repo
+        self.repo = repo.unfiltered()
         self.ui = ui
         self.opts = opts
         self.originalwd = None
@@ -250,7 +249,6 @@ 
         repo.ui.debug('computed skipped revs: %s\n' %
                         (' '.join(str(r) for r in sorted(skipped)) or None))
         repo.ui.debug('rebase status resumed\n')
-        _setrebasesetvisibility(repo, state.keys())
 
         self.originalwd = originalwd
         self.target = target
@@ -1119,7 +1117,6 @@ 
 
 def clearstatus(repo):
     'Remove the status files'
-    _clearrebasesetvisibiliy(repo)
     util.unlinkpath(repo.join("rebasestate"), ignoremissing=True)
 
 def needupdate(repo, state):
@@ -1203,7 +1200,6 @@ 
     dest: context
     rebaseset: set of rev
     '''
-    _setrebasesetvisibility(repo, rebaseset)
 
     # This check isn't strictly necessary, since mq detects commits over an
     # applied patch. But it prevents messing up the working directory when
@@ -1383,31 +1379,6 @@ 
 
     return ret
 
-def _setrebasesetvisibility(repo, revs):
-    """store the currently rebased set on the repo object
-
-    This is used by another function to prevent rebased revision to because
-    hidden (see issue4504)"""
-    repo = repo.unfiltered()
-    revs = set(revs)
-    repo._rebaseset = revs
-    # invalidate cache if visibility changes
-    hiddens = repo.filteredrevcache.get('visible', set())
-    if revs & hiddens:
-        repo.invalidatevolatilesets()
-
-def _clearrebasesetvisibiliy(repo):
-    """remove rebaseset data from the repo"""
-    repo = repo.unfiltered()
-    if '_rebaseset' in vars(repo):
-        del repo._rebaseset
-
-def _rebasedvisible(orig, repo):
-    """ensure rebased revs stay visible (see issue4504)"""
-    blockers = orig(repo)
-    blockers.update(getattr(repo, '_rebaseset', ()))
-    return blockers
-
 def _filterobsoleterevs(repo, revs):
     """returns a set of the obsolete revisions in revs"""
     return set(r for r in revs if repo[r].obsolete())
@@ -1479,5 +1450,3 @@ 
          _("use 'hg rebase --continue' or 'hg rebase --abort'")])
     cmdutil.afterresolvedstates.append(
         ['rebasestate', _('hg rebase --continue')])
-    # ensure rebased rev are not hidden
-    extensions.wrapfunction(repoview, '_getdynamicblockers', _rebasedvisible)
diff -r 460068415dc9 -r 0c89a4fb2563 tests/test-rebase-obsolete.t
--- a/tests/test-rebase-obsolete.t	Wed Feb 01 09:18:44 2017 -0800
+++ b/tests/test-rebase-obsolete.t	Wed Feb 01 09:03:04 2017 -0800
@@ -279,9 +279,27 @@ 
   $ hg --hidden up -qr 'first(hidden())'
   $ hg rebase --rev 13 --dest 15
   rebasing 13:98f6af4ee953 "C"
-  abort: hidden revision '1'!
-  (use --hidden to access hidden revisions)
-  [255]
+  $ hg log -G
+  o  16:294a2b93eb4d C
+  |
+  o  15:627d46148090 D
+  |
+  | o  12:462a34d07e59 B
+  | |
+  | o  11:4596109a6a43 D
+  | |
+  | o  7:02de42196ebe H
+  | |
+  +---o  6:eea13746799a G
+  | |/
+  | o  5:24b6387c8c8c F
+  | |
+  o |  4:9520eea781bc E
+  |/
+  | @  1:42ccdea3bb16 B
+  |/
+  o  0:cd010b8cd998 A
+  
 
   $ cd ..