Patchwork D3866: histedit: use cbor to write histedit-state file

login
register
mail settings
Submitter phabricator
Date June 30, 2018, 3:10 p.m.
Message ID <differential-rev-PHID-DREV-we2ofvxvos6rttmcfuyw-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/32530/
State New
Headers show

Comments

phabricator - June 30, 2018, 3:10 p.m.
pulkit created this revision.
Herald added a reviewer: durin42.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  This patch starts using state.cmdstate.write() which uses cbor as serialization
  format to write histedit-state files.
  
  Compatibility for reading old state files using new code is maintained.
  
  Some changes in test which depended on the format of histedit state file.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  hgext/histedit.py
  tests/test-histedit-arguments.t
  tests/test-histedit-edit.t

CHANGE DETAILS




To: pulkit, durin42, #hg-reviewers
Cc: mercurial-devel
Yuya Nishihara - July 1, 2018, 12:35 p.m.
Queued the first 4 patches, thanks.

>    $ HGEDITOR="sh $TESTTMP/editplan.sh" hg histedit --edit-plan
>    $ cat .hg/histedit-state
> -  v1
> -  055a42cdd88768532f9cf79daa407fc8d138de9b
> -  3c6a8ed2ebe862cc949d2caa30775dd6f16fb799
> -  False
> -  3
> -  drop
> -  e860deea161a2f77de56603b340ebbb4536308ae
> -  drop
> -  652413bf663ef2a641cab26574e46d5f5a64a55a
> -  drop
> -  3c6a8ed2ebe862cc949d2caa30775dd6f16fb799
> -  0
> -  strip-backup/177f92b77385-0ebe6a8f-histedit.hg
> +  2

I know anything other than "v1" will make old hg crash, but I think the
version syntax should stay "v%d" (i.e. "v2" in this case.)

>      def _write(self, fp):
> -        fp.write('v1\n')
> -        fp.write('%s\n' % node.hex(self.parentctxnode))
> -        fp.write('%s\n' % node.hex(self.topmost))
> -        fp.write('%s\n' % ('True' if self.keep else 'False'))
> -        fp.write('%d\n' % len(self.actions))
> +        data = {'parentctxnode': self.parentctxnode,
> +                'topmost': self.topmost,
> +                'keep': self.keep,
> +                'backupfile': self.backupfile,
> +                'replacements': self.replacements}
> +        rules = []
>          for action in self.actions:
> -            fp.write('%s\n' % action.tostate())
> -        fp.write('%d\n' % len(self.replacements))
> -        for replacement in self.replacements:
> -            fp.write('%s%s\n' % (node.hex(replacement[0]), ''.join(node.hex(r)
> -                for r in replacement[1])))
> -        backupfile = self.backupfile
> -        if not backupfile:
> -            backupfile = ''
> -        fp.write('%s\n' % backupfile)
> +            rules.append(action.tostate().split('\n'))
> +        rules = "\n".join(["%s %s" % (verb, rest) for [verb, rest] in rules])

Isn't it better to serialize `[(verb, rest)]` as is since CBOR can store that?
phabricator - July 1, 2018, 12:35 p.m.
yuja added a comment.


  Queued the first 4 patches, thanks.
  
  >   $ HGEDITOR="sh $TESTTMP/editplan.sh" hg histedit --edit-plan
  >   $ cat .hg/histedit-state
  > 
  > - v1
  > - 055a42cdd88768532f9cf79daa407fc8d138de9b
  > - 3c6a8ed2ebe862cc949d2caa30775dd6f16fb799
  > - False
  > - 3
  > - drop
  > - e860deea161a2f77de56603b340ebbb4536308ae
  > - drop
  > - 652413bf663ef2a641cab26574e46d5f5a64a55a
  > - drop
  > - 3c6a8ed2ebe862cc949d2caa30775dd6f16fb799
  > - 0
  > - strip-backup/177f92b77385-0ebe6a8f-histedit.hg +  2
  
  I know anything other than "v1" will make old hg crash, but I think the
  version syntax should stay "v%d" (i.e. "v2" in this case.)
  
  >   def _write(self, fp):
  > 
  > - fp.write('v1\n')
  > - fp.write('%s\n' % node.hex(self.parentctxnode))
  > - fp.write('%s\n' % node.hex(self.topmost))
  > - fp.write('%s\n' % ('True' if self.keep else 'False'))
  > - fp.write('%d\n' % len(self.actions)) +        data = {'parentctxnode': self.parentctxnode, +                'topmost': self.topmost, +                'keep': self.keep, +                'backupfile': self.backupfile, +                'replacements': self.replacements} +        rules = [] for action in self.actions:
  > - fp.write('%s\n' % action.tostate())
  > - fp.write('%d\n' % len(self.replacements))
  > - for replacement in self.replacements:
  > - fp.write('%s%s\n' % (node.hex(replacement[0]), ''.join(node.hex(r)
  > - for r in replacement[1])))
  > - backupfile = self.backupfile
  > - if not backupfile:
  > - backupfile = ''
  > - fp.write('%s\n' % backupfile) +            rules.append(action.tostate().split('\n')) +        rules = "\n".join(["%s %s" % (verb, rest) for [verb, rest] in rules])
  
  Isn't it better to serialize `[(verb, rest)]` as is since CBOR can store that?

REPOSITORY
  rHG Mercurial

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

To: pulkit, durin42, #hg-reviewers
Cc: yuja, mercurial-devel
phabricator - July 1, 2018, 9:11 p.m.
pulkit added a comment.


  In https://phab.mercurial-scm.org/D3866#60421, @yuja wrote:
  
  > Queued the first 4 patches, thanks.
  >
  > >   $ HGEDITOR="sh $TESTTMP/editplan.sh" hg histedit --edit-plan
  > >   $ cat .hg/histedit-state
  > > 
  > > - v1
  > > - 055a42cdd88768532f9cf79daa407fc8d138de9b
  > > - 3c6a8ed2ebe862cc949d2caa30775dd6f16fb799
  > > - False
  > > - 3
  > > - drop
  > > - e860deea161a2f77de56603b340ebbb4536308ae
  > > - drop
  > > - 652413bf663ef2a641cab26574e46d5f5a64a55a
  > > - drop
  > > - 3c6a8ed2ebe862cc949d2caa30775dd6f16fb799
  > > - 0
  > > - strip-backup/177f92b77385-0ebe6a8f-histedit.hg +  2
  >
  > I know anything other than "v1" will make old hg crash, but I think the
  >  version syntax should stay "v%d" (i.e. "v2" in this case.)
  
  
  Yeah, I think we need to pass version number in the state.cmdstate.read(). Supporting rebasestate will also need similar tweak.
  
  > 
  > 
  >>   def _write(self, fp):
  >> 
  >> - fp.write('v1\n')
  >> - fp.write('%s\n' % node.hex(self.parentctxnode))
  >> - fp.write('%s\n' % node.hex(self.topmost))
  >> - fp.write('%s\n' % ('True' if self.keep else 'False'))
  >> - fp.write('%d\n' % len(self.actions)) +        data = {'parentctxnode': self.parentctxnode, +                'topmost': self.topmost, +                'keep': self.keep, +                'backupfile': self.backupfile, +                'replacements': self.replacements} +        rules = [] for action in self.actions:
  >> - fp.write('%s\n' % action.tostate())
  >> - fp.write('%d\n' % len(self.replacements))
  >> - for replacement in self.replacements:
  >> - fp.write('%s%s\n' % (node.hex(replacement[0]), ''.join(node.hex(r)
  >> - for r in replacement[1])))
  >> - backupfile = self.backupfile
  >> - if not backupfile:
  >> - backupfile = ''
  >> - fp.write('%s\n' % backupfile) +            rules.append(action.tostate().split('\n')) +        rules = "\n".join(["%s %s" % (verb, rest) for [verb, rest] in rules])
  > 
  > Isn't it better to serialize `[(verb, rest)]` as is since CBOR can store that?
  
  Yeah, it's better. I will do that.

REPOSITORY
  rHG Mercurial

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

To: pulkit, durin42, #hg-reviewers
Cc: yuja, mercurial-devel
phabricator - July 9, 2018, 6:31 p.m.
pulkit planned changes to this revision.
pulkit added a comment.


  I am not confident to land some state format changes at this point in the cycle. Let's do it early in next cycle.

REPOSITORY
  rHG Mercurial

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

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

Patch

diff --git a/tests/test-histedit-edit.t b/tests/test-histedit-edit.t
--- a/tests/test-histedit-edit.t
+++ b/tests/test-histedit-edit.t
@@ -93,40 +93,22 @@ 
   > EOF
   $ HGEDITOR="sh $TESTTMP/editplan.sh" hg histedit --edit-plan
   $ cat .hg/histedit-state
-  v1
-  055a42cdd88768532f9cf79daa407fc8d138de9b
-  3c6a8ed2ebe862cc949d2caa30775dd6f16fb799
-  False
-  3
-  drop
-  e860deea161a2f77de56603b340ebbb4536308ae
-  drop
-  652413bf663ef2a641cab26574e46d5f5a64a55a
-  drop
-  3c6a8ed2ebe862cc949d2caa30775dd6f16fb799
-  0
-  strip-backup/177f92b77385-0ebe6a8f-histedit.hg
+  2
+  \xa6Dkeep\xf4ErulesX\x89drop e860deea161a2f77de56603b340ebbb4536308ae (esc)
+  drop 652413bf663ef2a641cab26574e46d5f5a64a55a
+  drop 3c6a8ed2ebe862cc949d2caa30775dd6f16fb799GtopmostT<j\x8e\xd2\xeb\xe8b\xcc\x94\x9d,\xaa0w]\xd6\xf1o\xb7\x99JbackupfileX.strip-backup/177f92b77385-0ebe6a8f-histedit.hgLreplacements\x80MparentctxnodeT\x05ZB\xcd\xd8\x87hS/\x9c\xf7\x9d\xaa@\x7f\xc8\xd18\xde\x9b (no-eol) (esc)
 
 edit the plan via --commands
   $ hg histedit --edit-plan --commands - 2>&1 << EOF
   > edit e860deea161a e
   > pick 652413bf663e f
   > drop 3c6a8ed2ebe8 g
   > EOF
   $ cat .hg/histedit-state
-  v1
-  055a42cdd88768532f9cf79daa407fc8d138de9b
-  3c6a8ed2ebe862cc949d2caa30775dd6f16fb799
-  False
-  3
-  edit
-  e860deea161a2f77de56603b340ebbb4536308ae
-  pick
-  652413bf663ef2a641cab26574e46d5f5a64a55a
-  drop
-  3c6a8ed2ebe862cc949d2caa30775dd6f16fb799
-  0
-  strip-backup/177f92b77385-0ebe6a8f-histedit.hg
+  2
+  \xa6Dkeep\xf4ErulesX\x89edit e860deea161a2f77de56603b340ebbb4536308ae (esc)
+  pick 652413bf663ef2a641cab26574e46d5f5a64a55a
+  drop 3c6a8ed2ebe862cc949d2caa30775dd6f16fb799GtopmostT<j\x8e\xd2\xeb\xe8b\xcc\x94\x9d,\xaa0w]\xd6\xf1o\xb7\x99JbackupfileX.strip-backup/177f92b77385-0ebe6a8f-histedit.hgLreplacements\x80MparentctxnodeT\x05ZB\xcd\xd8\x87hS/\x9c\xf7\x9d\xaa@\x7f\xc8\xd18\xde\x9b (no-eol) (esc)
 
 Go at a random point and try to continue
 
diff --git a/tests/test-histedit-arguments.t b/tests/test-histedit-arguments.t
--- a/tests/test-histedit-arguments.t
+++ b/tests/test-histedit-arguments.t
@@ -350,7 +350,7 @@ 
   (hg histedit --continue to resume)
   [1]
 Corrupt histedit state file
-  $ sed 's/8fda0c726bf2/123456789012/' .hg/histedit-state > ../corrupt-histedit
+  $ sed 's/8fda0c726bf2/123456789012/g; s/\x8f\xda/\x23\x56/g' .hg/histedit-state > ../corrupt-histedit
   $ mv ../corrupt-histedit .hg/histedit-state
   $ hg histedit --abort
   warning: encountered an exception during histedit --abort; the repository may not have been completely cleaned up
diff --git a/hgext/histedit.py b/hgext/histedit.py
--- a/hgext/histedit.py
+++ b/hgext/histedit.py
@@ -326,6 +326,10 @@ 
         self.backupfile = data['backupfile']
 
     def _read(self):
+        try:
+            return self.stateobj.read()
+        except error.CorruptedState:
+            pass
         fp = self.repo.vfs.read('histedit-state')
         if fp.startswith('v1\n'):
             data = self._load()
@@ -349,21 +353,17 @@ 
                 self._write(f)
 
     def _write(self, fp):
-        fp.write('v1\n')
-        fp.write('%s\n' % node.hex(self.parentctxnode))
-        fp.write('%s\n' % node.hex(self.topmost))
-        fp.write('%s\n' % ('True' if self.keep else 'False'))
-        fp.write('%d\n' % len(self.actions))
+        data = {'parentctxnode': self.parentctxnode,
+                'topmost': self.topmost,
+                'keep': self.keep,
+                'backupfile': self.backupfile,
+                'replacements': self.replacements}
+        rules = []
         for action in self.actions:
-            fp.write('%s\n' % action.tostate())
-        fp.write('%d\n' % len(self.replacements))
-        for replacement in self.replacements:
-            fp.write('%s%s\n' % (node.hex(replacement[0]), ''.join(node.hex(r)
-                for r in replacement[1])))
-        backupfile = self.backupfile
-        if not backupfile:
-            backupfile = ''
-        fp.write('%s\n' % backupfile)
+            rules.append(action.tostate().split('\n'))
+        rules = "\n".join(["%s %s" % (verb, rest) for [verb, rest] in rules])
+        data['rules'] = rules
+        self.stateobj.save(2, data)
 
     def _load(self):
         fp = self.repo.vfs('histedit-state', 'r')