[PATCH 1 of 6 OBSOLETE-MARKERS] obsolete: introduction obsolete markers
Patrick Mézard
patrick at mezard.eu
Sun May 13 08:15:13 CDT 2012
Le 12/05/12 19:08, Pierre-Yves David a écrit :
> # HG changeset patch
> # User Pierre-Yves.David at ens-lyon.org
> # Date 1336836092 -7200
> # Node ID b2cecf48226020c40d448617fc6d421c4e7f6cfe
> # Parent ddd4996740c785cc187766249977ea3ece8c17ab
> obsolete: introduction obsolete markers
>
> This changeset add a obsstore attribute on localrepository. We are able to Save
> to disk and load to it. Format is describe in the module docstring.
>
> There is still a lot a work to do. In particular using standard transaction to
> manage the write and aborting.
>
> diff -r ddd4996740c7 -r b2cecf482260 mercurial/localrepo.py
> --- a/mercurial/localrepo.py Tue May 08 12:05:45 2012 -0500
> +++ b/mercurial/localrepo.py Sat May 12 17:21:32 2012 +0200
> @@ -7,7 +7,7 @@
>
> from node import bin, hex, nullid, nullrev, short
> from i18n import _
> -import repo, changegroup, subrepo, discovery, pushkey
> +import repo, changegroup, subrepo, discovery, pushkey, obsolete
> import changelog, dirstate, filelog, manifest, context, bookmarks, phases
> import lock, transaction, store, encoding
> import scmutil, util, extensions, hook, error, revset
> @@ -200,6 +200,14 @@
> cache[rev] = phase
> return cache
>
> + @storecache('obsmarkers')
> + def obsstore(self):
> + return obsolete.readmarkers(self)
> +
> + @property
> + def _dirtyobsstore(self):
> + return 'obsstore' in vars(self) and self.obsstore._new
> +
Any reason not to inline it? Also, do we want to write the markers unconditionally somewhere or do we always check the _new (dirty?) flag? If so why not let writemarkers() handle the dirtyness itself (like in phasecache)?
> @storecache('00changelog.i')
> def changelog(self):
> c = changelog.changelog(self.sopener)
> @@ -935,6 +943,8 @@
> if self._dirtyphases:
> phases.writeroots(self)
> self._dirtyphases = False
> + if self._dirtyobsstore:
> + obsolete.writemarkers(self)
> for k, ce in self._filecache.items():
> if k == 'dirstate':
> continue
> diff -r ddd4996740c7 -r b2cecf482260 mercurial/obsolete.py
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/mercurial/obsolete.py Sat May 12 17:21:32 2012 +0200
> @@ -0,0 +1,163 @@
> +"""Code related to Obsolete marker
> +
> +
> +Obsolete marker content
> +------------------------------
> +
> +A marker hold 4 data:
> +* obsoleted changeset (1 mandatory node id)
> +* replacements changeset (0..n node id)
> +* binary flag field (for various use)
> +* various metadata (date, user, maybe more)
Cannot the binary flags be stored as metadata?
Why a non-textual format?
> +
> +Obsolete marker storage format
> +------------------------------
> +
> +The file starts with a version header
> +* 'I' size of version String
> +* '-s' Version string
> +
> +Remaining content are about markers. Each marker consist in:
> +
> + A fixed width header:
> + * 'B' number of remplacements: "nb-R" (in number of nodes)
> + * 'I' metadata size (in Bytes)
> + * 'B' multipurpose Flag Field
> + * '20s' Obsoleted nodeid
> +
> + Then Replacements nodeid:
> + * (20s){nb-R}
> +
> + Then comes metadata:
> + * (key\0value\0)+
> +
> + Final result looks like::
> +
> + ">B>I>B20s(20s)*(-s\0-s\0)*
> +"""
> +import struct
> +_pack = struct.pack
> +_unpack = struct.unpack
> +
> +from mercurial import util
> +from i18n import _
> +
> +
> +# data used for parsing and writing
> +fmversion = 'Alpha'
> +fmfixed = '>BIB20s'
> +fmnode = '20s'
> +fmfsize = struct.calcsize(fmfixed)
> +fnodesize = struct.calcsize(fmnode)
> +
> +
Either it should take an obsstore() instance in parameter or be an obstore method. This kind of indirect dependencies is a pain to follow, and that is exactly what I removed from phase code.
> +def readmarkers(repo):
> + """Read obsolete markers from disk
> +
> + (This need to be splitted to allow parsing content from the network)"""
> + store = obsstore()
> + try:
> + f = repo.sopener('obsmarkers', 'rb')
> + try:
> + data = f.read()
> + finally:
> + f.close()
data = repo.sopener.read()
> + ### read format version
> + # XXX need some security around here if the file is really short.
> + # version size
Yes :-) ?
> + off = 0
> + s = struct.calcsize('>Q')
> + cur = data[off:off + s]
> + off += s
> + s, = _unpack('>Q', cur)
> + # version String itself
> + diskversion = data[off:off + s]
More offset test.
> + off += s
> + if diskversion != fmversion:
> + raise util.Abort(_('parsing obsolete marker: unknown version %r')
> + % diskversion)
Shouldn't that be unpacked before being printed?
> + l = len(data)
> + while off + fmfsize <= l:
We ignore stray bytes?
> + ### read marker
> + # read fixed part
> + cur = data[off:off + fmfsize]
> + off += fmfsize
> + nbrepl, mdsize, flags, obs = _unpack(fmfixed, cur)
> + # read replacement
> + repls = tuple()
> + if nbrepl:
> + s = (fnodesize * nbrepl)
> + cur = data[off:off + s]
Offset checking
> + repls = _unpack(fmnode * nbrepl, cur)
> + off += s
> + # read metadata
> + # (metadata will be decoded on demand)
> + metadata = data[off:off + mdsize]
> + off += mdsize
Offset checking
> + marker = (obs, repls, flags, metadata)
> + store._load(marker)
> + except IOError:
Shouldn't that be handle when opening the file, to reduce the try/except scope?
> + pass
> + return store
> +
> +
Idem readmarkers()
> +def writemarkers(repo):
> + """Write obsolete markers to disk
> +
> + Transaction logic should be used here. But for now rewritting the whole
rewriting
> + file is good enough.
> +
> + (This need to be splitted to allow parsing content from the network)"""
> + f = repo.sopener('obsmarkers', 'wb', atomictemp=True)
> + store = repo.obsstore
> + try:
> + ### write version header
> + vlen = len(fmversion)
> + f.write(_pack('>Q', vlen))
> + f.write(_pack('%is' % vlen, fmversion))
> + for marker in store._all:
> + ### marker
> + # fixed header
> + obs, repls, flags, metadata = marker
> + nbrepl = len(repls)
> + format = fmfixed + (fmnode * nbrepl)
> + data = [nbrepl, len(metadata), flags, obs]
> + data.extend(repls)
> + f.write(_pack(format, *data))
> + f.write(metadata)
You want to .close() here
> + store._new[:] = []
> + finally:
> + f.close()
And discard there. The goal is not to write on error.
> +
> +
> +class obsstore(object):
> + """store all obsolete marker
markers
> +
> + marker are accessible through two mappingi
mappings
> +
> + :store.obsoleted: obsolete -> marker
> + :store.replacements: replacement -> marker
> + """
> +
> + def __init__(self):
> + self.obsoleted = {}
> + self.replacements = {}
> + self._all = []
> + self._new = []
> +
> + def add(self, marker):
> + """Add a new marker to the store
> +
> + This marker still need to be written to disk"""
> + self._new.append(marker)
> + self._load(marker)
> +
> + def _load(self, marker):
> + """load a marker in the store
> +
> + The marker do need to be written to disk"""
> + self._all.append(marker)
> + obs, rpls = marker[:2]
> + self.obsoleted.setdefault(obs, set()).add(marker)
> + for rp in rpls:
> + self.replacements.setdefault(rp, set()).add(marker)
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>
More information about the Mercurial-devel
mailing list