Patchwork D2591: state: import the file to write state files from evolve extension

login
register
mail settings
Submitter phabricator
Date March 3, 2018, 7:19 p.m.
Message ID <differential-rev-PHID-DREV-w7c53acyyzpxh7i3yph2-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/28799/
State Superseded
Headers show

Comments

phabricator - March 3, 2018, 7:19 p.m.
pulkit created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  This patch moves the file which is used to write state files easily in a good
  way using the cbor format.
  
  The file is moved from changeset 1baf32675ec60d622bd2c3cdce9dbf5a1020c06d of the
  evolve extension. The following changes are made to the file while moving to
  core:
  
  - import util from current directory as this file in mercurial/ now
  - make cmdstate class extend object
  - removed mutable default value for opts in cmdstate.__init__
  - some doc changes to replace out of core things with in-core ones
  
  evolve extension can be found at https://bitbucket.org/marmoute/mutable-history

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/state.py

CHANGE DETAILS




To: pulkit, #hg-reviewers
Cc: mercurial-devel
phabricator - March 25, 2018, 6:20 a.m.
martinvonz added a comment.


  > This patch moves the file which is used to write state files easily in a good way using the cbor format.
  
  Is "moves the file" referring to "import from the evolve extension"? It would also be good to be more concrete about what "easily in a good way" means. I get the feeling that there's a not-so-good way you have in mind that you want to prevent.

INLINE COMMENTS

> state.py:61
> +    def load(self):
> +        """load the existing evolvestate file into the class object"""
> +        op = self._read()

drop "evolve" here and a few other places

REPOSITORY
  rHG Mercurial

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

To: pulkit, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - March 26, 2018, 3:44 p.m.
indygreg requested changes to this revision.
indygreg added a comment.
This revision now requires changes to proceed.


  Pulkit apparently has changes for this series pending. Marking accordingly.

REPOSITORY
  rHG Mercurial

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

To: pulkit, #hg-reviewers, indygreg
Cc: indygreg, martinvonz, mercurial-devel
phabricator - March 27, 2018, 1:16 p.m.
yuja added inline comments.

INLINE COMMENTS

> state.py:55
> +    def __nonzero__(self):
> +        return self.exists()
> +

Nit: this seems too clever. I wouldn't expect `if state` issues a system call.

REPOSITORY
  rHG Mercurial

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

To: pulkit, #hg-reviewers, indygreg
Cc: yuja, indygreg, martinvonz, mercurial-devel
phabricator - March 28, 2018, 6:22 p.m.
indygreg requested changes to this revision.
indygreg added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> yuja wrote in state.py:55
> Nit: this seems too clever. I wouldn't expect `if state` issues a system call.

I agree. Let's not implement `__nonzero__`.

> state.py:79
> +        with self._repo.vfs(self.fname, 'wb', atomictemp=True) as fp:
> +            cbor.dump(self.opts, fp)
> +

We probably want `canonical=True` here so behavior is deterministic. All that does in reality is sort dicts and sets. I doubt we have any data structures large enough for this to be a problem.

REPOSITORY
  rHG Mercurial

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

To: pulkit, #hg-reviewers, indygreg
Cc: yuja, indygreg, martinvonz, mercurial-devel
phabricator - April 17, 2018, 8:33 a.m.
pulkit added a comment.


  I was unable to followup on this. Let's leave this series here for the freeze and I will send new versions once freeze ends.

REPOSITORY
  rHG Mercurial

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

To: pulkit, #hg-reviewers, indygreg
Cc: yuja, indygreg, martinvonz, mercurial-devel
phabricator - May 17, 2018, 12:09 p.m.
pulkit added a comment.


  @martinvonz @yuja I am trying to get this series reviewed in pieces. This one and it's child https://phab.mercurial-scm.org/D3572 is ready for review.

REPOSITORY
  rHG Mercurial

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

To: pulkit, #hg-reviewers, indygreg
Cc: yuja, indygreg, martinvonz, mercurial-devel
phabricator - May 17, 2018, 4:48 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> indygreg wrote in state.py:79
> We probably want `canonical=True` here so behavior is deterministic. All that does in reality is sort dicts and sets. I doubt we have any data structures large enough for this to be a problem.

Looks like you missed this. (If you didn't and you simply disagree, please explain why you disagree.)

REPOSITORY
  rHG Mercurial

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

To: pulkit, #hg-reviewers, indygreg
Cc: yuja, indygreg, martinvonz, mercurial-devel
phabricator - May 17, 2018, 6:22 p.m.
pulkit added inline comments.

INLINE COMMENTS

> martinvonz wrote in state.py:79
> Looks like you missed this. (If you didn't and you simply disagree, please explain why you disagree.)

I just simply missed it.

REPOSITORY
  rHG Mercurial

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

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

Patch

diff --git a/mercurial/state.py b/mercurial/state.py
new file mode 100644
--- /dev/null
+++ b/mercurial/state.py
@@ -0,0 +1,89 @@ 
+# state.py - writing and reading state files in Mercurial
+#
+# Copyright 2018 Pulkit Goyal <pulkitmgoyal@gmail.com>
+#
+# This software may be used and distributed according to the terms of the
+# GNU General Public License version 2 or any later version.
+
+"""
+This file contains class to wrap the state for commands and other
+related logic.
+
+All the data related to the command state is stored as dictionary in the object.
+The class has methods using which the data can be stored to disk in a file under
+.hg/ directory.
+
+We store the data on disk in cbor, for which we use the third party cbor library
+to serialize and deserialize data.
+"""
+
+from __future__ import absolute_import
+
+from .thirdparty import cbor
+
+from . import (
+    util,
+)
+
+class cmdstate(object):
+    """a wrapper class to store the state of commands like `rebase`, `graft`,
+    `histedit`, `shelve` etc. Extensions can also use this to write state files.
+
+    All the data for the state is stored in the form of key-value pairs in a
+    dictionary.
+
+    The class object can write all the data to a file in .hg/ directory and
+    can populate the object data reading that file.
+
+    Uses cbor to serialize and deserialize data while writing and reading from
+    disk.
+    """
+
+    def __init__(self, repo, fname, opts=None):
+        """ repo is the repo object
+        fname is the file name in which data should be stored in .hg directory
+        opts is a dictionary of data of the statefile
+        """
+        self._repo = repo
+        self.fname = fname
+        if not opts:
+            self.opts = {}
+        else:
+            self.opts = opts
+
+    def __nonzero__(self):
+        return self.exists()
+
+    def __getitem__(self, key):
+        return self.opts[key]
+
+    def load(self):
+        """load the existing evolvestate file into the class object"""
+        op = self._read()
+        self.opts.update(op)
+
+    def addopts(self, opts):
+        """add more key-value pairs to the data stored by the object"""
+        self.opts.update(opts)
+
+    def save(self):
+        """write all the evolvestate data stored in .hg/evolvestate file
+
+        we use third-party library cbor to serialize data to write in the file.
+        """
+        with self._repo.vfs(self.fname, 'wb', atomictemp=True) as fp:
+            cbor.dump(self.opts, fp)
+
+    def _read(self):
+        """reads the evolvestate file and returns a dictionary which contain
+        data in the same format as it was before storing"""
+        with self._repo.vfs(self.fname, 'rb') as fp:
+            return cbor.load(fp)
+
+    def delete(self):
+        """drop the evolvestate file if exists"""
+        util.unlinkpath(self._repo.vfs.join(self.fname), ignoremissing=True)
+
+    def exists(self):
+        """check whether the evolvestate file exists or not"""
+        return self._repo.vfs.exists(self.fname)