This is an archive of the discontinued Mercurial Phabricator instance.

Differential D2591

state: import the file to write state files from evolve extension
ClosedPublic

Authored by pulkit on Mar 3 2018, 2:19 PM.

Details

Reviewers
indygreg
martinvonz
Group Reviewers
hg-reviewers
Commits
rHGa2f83661f721: state: import the file to write state files from evolve extension
Summary

The current way of writing state files is very obscure with each state file
having it's own format to store state files. There is no centralized way to
write state files in a good format. Moreover the current state files are not
extensible, you cannot add more data to store in state files in reliable ways.

To solve the problem, I wrote my own serialization and deserialization format,
looked into existing formats like Protobuf, MessagePack, JSON but CBOR looks
very promising and is suggested by people in the community.

The current interface to store state files is to directly write data in files
when things abort. Using the class imported by this commit, we can create
objects which has a dict like interface and can store data on the object and
store it on the file when things abort.

The evolve extension is using the state file for evolve, grab commands and
using it for resolution of orphaness, phase-divergence and content-divergence.

The file is moved from changeset e4ac2e2c2086f977afa35e23a62f849e9305a225 of the
evolve extension which is also tagged as 7.3.0.
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

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

pulkit created this revision.Mar 3 2018, 2:19 PM
martinvonz added a subscriber: martinvonz.Mar 25 2018, 2:20 AM

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.

mercurial/state.py
62

drop "evolve" here and a few other places

indygreg requested changes to this revision.Mar 26 2018, 11:44 AM
indygreg added a subscriber: indygreg.

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

This revision now requires changes to proceed.Mar 26 2018, 11:44 AM
pulkit edited the summary of this revision. (Show Details)Mar 26 2018, 1:08 PM
pulkit updated this revision to Diff 7297.
pulkit marked an inline comment as done.Mar 26 2018, 1:15 PM
yuja added a subscriber: yuja.Mar 27 2018, 9:16 AM
yuja added inline comments.
mercurial/state.py
56

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

indygreg requested changes to this revision.Mar 28 2018, 2:22 PM
indygreg added inline comments.
mercurial/state.py
56

I agree. Let's not implement __nonzero__.

80

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.

This revision now requires changes to proceed.Mar 28 2018, 2:22 PM
pulkit added a comment.Apr 17 2018, 4:33 AM

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.

pulkit added a comment.May 17 2018, 8:09 AM

@martinvonz @yuja I am trying to get this series reviewed in pieces. This one and it's child D3572 is ready for review.

martinvonz added inline comments.May 17 2018, 12:48 PM
mercurial/state.py
80

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

pulkit added inline comments.May 17 2018, 2:22 PM
mercurial/state.py
80

I just simply missed it.

martinvonz accepted this revision.May 21 2018, 11:59 AM
Closed by commit rHGa2f83661f721: state: import the file to write state files from evolve extension (authored by pulkit). · Explain WhyMay 21 2018, 1:40 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
AMmercurial/state.py (93 lines)

Diff 8850

mercurial/state.py

  • This file was added.
# 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()
yujaUnsubmitted
Not Done

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

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

I agree. Let's not implement __nonzero__.

indygreg: I agree. Let's not implement `__nonzero__`.
def __getitem__(self, key):
return self.opts[key]
def __setitem__(self, key, value):
updates = {key: value}
self.opts.update(updates)
martinvonzUnsubmitted
Done

drop "evolve" here and a few other places

martinvonz: drop "evolve" here and a few other places
def load(self):
"""load the existing state 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 state data stored to .hg/<filename> 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)
indygregUnsubmitted
Not Done

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.

indygreg: We probably want `canonical=True` here so behavior is deterministic. All that does in reality…
martinvonzUnsubmitted
Not Done

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

martinvonz: Looks like you missed this. (If you didn't and you simply disagree, please explain why you…
pulkitAuthorUnsubmitted
Not Done

I just simply missed it.

pulkit: I just simply missed it.
def _read(self):
"""reads the state 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 state file if exists"""
util.unlinkpath(self._repo.vfs.join(self.fname), ignoremissing=True)
def exists(self):
"""check whether the state file exists or not"""
return self._repo.vfs.exists(self.fname)