This is an archive of the discontinued Mercurial Phabricator instance.

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

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

Download Raw Diff

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

Herald added a reviewer: hg-reviewers. · View Herald TranscriptMar 3 2018, 2:19 PM

Herald added a subscriber: mercurial-devel. · View Herald Transcript

pulkit added a child revision: D2592: state: add registrar to register function to read old state files.Mar 3 2018, 2:19 PM

pulkit mentioned this in D2409: graft: add no-commit mode (issue5631).Mar 3 2018, 2:41 PM

pulkit edited parent revisions, added: D2752: cbor: add a __init__.py to top level cbor module; removed: D2590: cbor: add a __init__.py to top level cbor module.Mar 9 2018, 6:02 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

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

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 child revision: D3572: state: don't have a dict like interface for cmdstate class.May 17 2018, 8:06 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.

pulkit removed a child revision: D2592: state: add registrar to register function to read old state files.May 19 2018, 10:52 AM

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

			Path	Packages
A	M		mercurial/state.py (93 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	6442		Mar 3 2018, 2:19 PM	★	★
Diff 2	7297		Mar 26 2018, 1:08 PM	★	★
Diff 3	8850	rHGa2f83661f7210bfc1c105404d0726bd7deeba465	Feb 21 2018, 6:50 AM	★	★

Status	Author	Revision
Closed	pulkit	D3579 state: write the version number in plain text on top of state files
Closed	pulkit	D3578 state: set canonical=True to write deterministically
Closed	pulkit	D3572 state: don't have a dict like interface for cmdstate class
Closed	pulkit	D2591 state: import the file to write state files from evolve extension
Closed	pulkit	D2752 cbor: add a __init__.py to top level cbor module
Closed	pulkit	D2751 cbor: remove unrequired files and fix core test-check*
Closed	pulkit	D2750 thirdparty: start vendoring cbor python library

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)