Patchwork D3873: rebase: refactor logic to read rebasestate in a separate function

login
register
mail settings
Submitter phabricator
Date July 1, 2018, 8:32 p.m.
Message ID <differential-rev-PHID-DREV-o4zgfnokv25okkdvqszp-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/32553/
State Superseded
Headers show

Comments

phabricator - July 1, 2018, 8:32 p.m.
pulkit created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  This will help us in plugging the use of state.cmdstate() to read rebasestate.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3873

AFFECTED FILES
  hgext/rebase.py

CHANGE DETAILS




To: pulkit, #hg-reviewers
Cc: mercurial-devel
Yuya Nishihara - July 2, 2018, 12:42 p.m.
Queued, thanks.

>              f = repo.vfs("rebasestate")
>              for i, l in enumerate(f.read().splitlines()):
>                  if i == 0:
> -                    originalwd = repo[l].rev()
> +                    data['originalwd'] = repo[l].rev()

FWIW, I don't have any better idea to keep old hg not crash with new state
file, other than:

 a. use separate file (e.g. "rebasestate2") and leave "rebasestate" in old
    format (or make it an empty file to trigger error.)
 b. abuse `repo[l].rev()` to abort with lookup error with somewhat descriptive
    message.
phabricator - July 2, 2018, 12:45 p.m.
yuja added a comment.


  Queued, thanks.
  
  >   f = repo.vfs("rebasestate")
  >   for i, l in enumerate(f.read().splitlines()):
  >       if i == 0:
  > 
  > - originalwd = repo[l].rev() +                    data['originalwd'] = repo[l].rev()
  
  FWIW, I don't have any better idea to keep old hg not crash with new state
  file, other than:
  
  a. use separate file (e.g. "rebasestate2") and leave "rebasestate" in old
  
    format (or make it an empty file to trigger error.)
  
  b. abuse `repo[l].rev()` to abort with lookup error with somewhat descriptive
  
    message.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3873

To: pulkit, #hg-reviewers
Cc: yuja, mercurial-devel
phabricator - July 2, 2018, 6:18 p.m.
pulkit added a comment.


  In https://phab.mercurial-scm.org/D3873#60501, @yuja wrote:
  
  > Queued, thanks.
  >
  > >   f = repo.vfs("rebasestate")
  > >   for i, l in enumerate(f.read().splitlines()):
  > >       if i == 0:
  > > 
  > > - originalwd = repo[l].rev() +                    data['originalwd'] = repo[l].rev()
  >
  > FWIW, I don't have any better idea to keep old hg not crash with new state
  >  file, other than:
  >
  >   a. use separate file (e.g. "rebasestate2") and leave "rebasestate" in old
  >      format (or make it an empty file to trigger error.)
  
  
  Yeah, I was preventing this for a long time but feels like I can't anymore. How about having a `.hg/state/` where we will have our new histedit and rebase state files?
  
  >   b. abuse `repo[l].rev()` to abort with lookup error with somewhat descriptive
  >      message.
  
  This is more bad. Let's not do that.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3873

To: pulkit, #hg-reviewers
Cc: yuja, mercurial-devel
Yuya Nishihara - July 3, 2018, 12:01 p.m.
>   > FWIW, I don't have any better idea to keep old hg not crash with new state
>   >  file, other than:
>   >
>   >   a. use separate file (e.g. "rebasestate2") and leave "rebasestate" in old
>   >      format (or make it an empty file to trigger error.)
>   
>   
>   Yeah, I was preventing this for a long time but feels like I can't anymore.
>   How about having a `.hg/state/` where we will have our new histedit and
>   rebase state files?

A separate directory wouldn't make the situation better. We can't stop writing
a v1 state file anyway since missing v1 state file means "no rebase in progress"
in old hg versions.
phabricator - July 3, 2018, 12:02 p.m.
yuja added a comment.


  >   > FWIW, I don't have any better idea to keep old hg not crash with new state
  >   >  file, other than:
  >   >
  >   >   a. use separate file (e.g. "rebasestate2") and leave "rebasestate" in old
  >   >      format (or make it an empty file to trigger error.)
  >   
  >   
  >   Yeah, I was preventing this for a long time but feels like I can't anymore.
  >   How about having a `.hg/state/` where we will have our new histedit and
  >   rebase state files?
  
  A separate directory wouldn't make the situation better. We can't stop writing
  a v1 state file anyway since missing v1 state file means "no rebase in progress"
  in old hg versions.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3873

To: pulkit, #hg-reviewers
Cc: yuja, mercurial-devel

Patch

diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -225,40 +225,52 @@ 
 
     def restorestatus(self):
         """Restore a previously stored status"""
+        data = self._read()
+        self.repo.ui.debug('rebase status resumed\n')
+
+        self.originalwd = data['originalwd']
+        self.destmap = data['destmap']
+        self.state = data['state']
+        self.skipped = data['skipped']
+        self.collapsef = data['collapse']
+        self.keepf = data['keep']
+        self.keepbranchesf = data['keepbranches']
+        self.external = data['external']
+        self.activebookmark = data['activebookmark']
+
+    def _read(self):
         self.prepared = True
         repo = self.repo
         assert repo.filtername is None
-        keepbranches = None
+        data = {'keepbranches': None, 'collapse': None, 'activebookmark': None,
+                'external': nullrev, 'keep': None, 'originalwd': None}
         legacydest = None
-        collapse = False
-        external = nullrev
-        activebookmark = None
         state = {}
         destmap = {}
 
         try:
             f = repo.vfs("rebasestate")
             for i, l in enumerate(f.read().splitlines()):
                 if i == 0:
-                    originalwd = repo[l].rev()
+                    data['originalwd'] = repo[l].rev()
                 elif i == 1:
                     # this line should be empty in newer version. but legacy
                     # clients may still use it
                     if l:
                         legacydest = repo[l].rev()
                 elif i == 2:
-                    external = repo[l].rev()
+                    data['external'] = repo[l].rev()
                 elif i == 3:
-                    collapse = bool(int(l))
+                    data['collapse'] = bool(int(l))
                 elif i == 4:
-                    keep = bool(int(l))
+                    data['keep'] = bool(int(l))
                 elif i == 5:
-                    keepbranches = bool(int(l))
+                    data['keepbranches'] = bool(int(l))
                 elif i == 6 and not (len(l) == 81 and ':' in l):
                     # line 6 is a recent addition, so for backwards
                     # compatibility check that the line doesn't look like the
                     # oldrev:newrev lines
-                    activebookmark = l
+                    data['activebookmark'] = l
                 else:
                     args = l.split(':')
                     oldrev = repo[args[0]].rev()
@@ -281,30 +293,24 @@ 
                 raise
             cmdutil.wrongtooltocontinue(repo, _('rebase'))
 
-        if keepbranches is None:
+        if data['keepbranches'] is None:
             raise error.Abort(_('.hg/rebasestate is incomplete'))
 
+        data['destmap'] = destmap
+        data['state'] = state
         skipped = set()
         # recompute the set of skipped revs
-        if not collapse:
+        if not data['collapse']:
             seen = set(destmap.values())
             for old, new in sorted(state.items()):
                 if new != revtodo and new in seen:
                     skipped.add(old)
                 seen.add(new)
+        data['skipped'] = skipped
         repo.ui.debug('computed skipped revs: %s\n' %
                         (' '.join('%d' % r for r in sorted(skipped)) or ''))
-        repo.ui.debug('rebase status resumed\n')
 
-        self.originalwd = originalwd
-        self.destmap = destmap
-        self.state = state
-        self.skipped = skipped
-        self.collapsef = collapse
-        self.keepf = keep
-        self.keepbranchesf = keepbranches
-        self.external = external
-        self.activebookmark = activebookmark
+        return data
 
     def _handleskippingobsolete(self, obsoleterevs, destmap):
         """Compute structures necessary for skipping obsolete revisions