[PATCH 1 of 7] obsolete: introduction of obsolete markers

[Matt Mackall]

On Mon, 2012-05-14 at 18:10 +0200, Pierre-Yves David wrote:
> +    @storecache('obsmarkers')
> +    def obsstore(self):
> +        return obsolete.readmarkers(self)

We probably ought to call the file obsstore too.

> +            if 'obsstore' in vars(self) and self.obsstore._new:
> +                obsolete.flushmarkers(self)

See below.

>              for k, ce in self._filecache.items():
>                  if k == 'dirstate':
>                      continue
> diff -r ddd4996740c7 -r 76b72def92ce mercurial/obsolete.py
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/mercurial/obsolete.py	Mon May 14 17:05:06 2012 +0200
> @@ -0,0 +1,209 @@
> +"""Code related to Obsolete marker

Random capitalization -> overexposure to German?

> The file starts with a version header
> +* 'I' size of version String
> +* '-s' Version string

Version _string_?? How about a byte or a u32.

> +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

This description would be a lot easier to read in English rather than
Pythonese:

 * 1 byte: number of replacements

> +        ">B>I>B20s(20s)*(-s\0-s\0)*

..because stuff like this less helpful than reading the source.

> +"""
> +import struct
> +_pack = struct.pack
> +_unpack = struct.unpack

These are private..

> +fmversion = 'Alpha'
> +fmfixed   = '>BIB20s'
> +fmnode = '20s'
> +fmfsize = struct.calcsize(fmfixed)
> +fnodesize = struct.calcsize(fmnode)

..but these aren't?

> +
> +
> +def readmarkers(repo):
> +    """Read obsolete markers from disk"""
> +    store = obsstore()
> +    try:
> +        f = repo.sopener('obsmarkers', 'rb')
> +        try:
> +            data = f.read()
> +        finally:
> +            f.close()
> +        store.loadmarkers(data)

Patrick already told you to use opener.read()?

> +    except IOError:
> +        pass

So it's ok to silently ignore EIO or EPERM?

Use opener.tryread() and this function becomes a _correct_ one-liner.

> +def flushmarkers(repo):

writemarkers, please. This should probably internalize the _new -> write
needed logic so that random external code doesn't need to know about it.

> +    @staticmethod
> +    def _readmarkers(data):

@staticmethod -> mostly a way to add extra indentation to your code.

> +    def _writemarkers(self, stream):
> +        """Write all obsolete markers to a stream
> +        """

Not sure why there are separate write and flush methods.

-- 
Mathematics is the supreme nostalgia of our time.