Patchwork [1,of,4] obsolete: gather all contents related to format version 0 in a single place

login
register
mail settings
Submitter Pierre-Yves David
Date Sept. 17, 2014, 2:28 a.m.
Message ID <d5f9d5cb72e220cc8966.1410920911@marginatus.alto.octopoid.net>
Download mbox | patch
Permalink /patch/5840/
State Superseded
Headers show

Comments

Pierre-Yves David - Sept. 17, 2014, 2:28 a.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@fb.com>
# Date 1410525703 -7200
#      Fri Sep 12 14:41:43 2014 +0200
# Node ID d5f9d5cb72e220cc89661dbe08afd9b13436c1df
# Parent  48791c2bea1ceda4e4f28bc11651e281d636ce1a
obsolete: gather all contents related to format version 0 in a single place

All contents embrances documentation, constants and functions. The diff is confusing
because it cannot understand what code is semantically moved around in this
grand migration.
Augie Fackler - Sept. 24, 2014, 3:21 p.m.
On Tue, Sep 16, 2014 at 07:28:31PM -0700, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1410525703 -7200
> #      Fri Sep 12 14:41:43 2014 +0200
> # Node ID d5f9d5cb72e220cc89661dbe08afd9b13436c1df
> # Parent  48791c2bea1ceda4e4f28bc11651e281d636ce1a
> obsolete: gather all contents related to format version 0 in a single place
>
> All contents embrances documentation, constants and functions. The diff is confusing

Should "embrances" have been "enhances"? I'm not quite sure what word you meant.

> because it cannot understand what code is semantically moved around in this
> grand migration.

I assume "no functionality change, just code movement" is true?

This might be a case where a class-as-namespace would make some amount
of sense, as much as I hate myself for the proposal.

>
> diff --git a/mercurial/obsolete.py b/mercurial/obsolete.py
> --- a/mercurial/obsolete.py
> +++ b/mercurial/obsolete.py
> @@ -61,29 +61,12 @@ Markers are stored in an append-only fil
>
>  The file starts with a version header:
>
>  - 1 unsigned byte: version number, starting at zero.
>
> -
> -The header is followed by the markers. Each marker is made of:
> -
> -- 1 unsigned byte: number of new changesets "N", can be zero.
> -
> -- 1 unsigned 32-bits integer: metadata size "M" in bytes.
> -
> -- 1 byte: a bit field. It is reserved for flags used in common
> -  obsolete marker operations, to avoid repeated decoding of metadata
> -  entries.
> -
> -- 20 bytes: obsoleted changeset identifier.
> -
> -- N*20 bytes: new changesets identifiers.
> -
> -- M bytes: metadata as a sequence of nul-terminated strings. Each
> -  string contains a key and a value, separated by a colon ':', without
> -  additional encoding. Keys cannot contain '\0' or ':' and values
> -  cannot contain '\0'.
> +The header is followed by the markers. Marker format depend of the version. See
> +comment associated with each format for details.
>
>  """
>  import struct
>  import util, base85, node
>  import phases
> @@ -96,17 +79,10 @@ from i18n import _
>
>  # the obsolete feature is not mature enough to be enabled by default.
>  # you have to rely on third party extension extension to enable this.
>  _enabled = False
>
> -# data used for parsing and writing
> -_fm0version = 0
> -_fm0fixed   = '>BIB20s'
> -_fm0node = '20s'
> -_fm0fsize = struct.calcsize(_fm0fixed)
> -_fm0fnodesize = struct.calcsize(_fm0node)
> -
>  ### obsolescence marker flag
>
>  ## bumpedfix flag
>  #
>  # When a changeset A' succeed to a changeset A which became public, we call A'
> @@ -135,28 +111,35 @@ from i18n import _
>  # This flag mean that the successors express the changes between the public and
>  # bumped version and fix the situation, breaking the transitivity of
>  # "bumped" here.
>  bumpedfix = 1
>
> -def _readmarkers(data):
> -    """Read and enumerate markers from raw data"""
> -    off = 0
> -    diskversion = _unpack('>B', data[off:off + 1])[0]
> -    off += 1
> -    if diskversion not in formats:
> -        raise util.Abort(_('parsing obsolete marker: unknown version %r')
> -                         % diskversion)
> -    return diskversion, formats[diskversion][0](data, off)
> -
> -def encodemarkers(markers, addheader=False, version=_fm0version):
> -    # Kept separate from flushmarkers(), it will be reused for
> -    # markers exchange.
> -    encodeone = formats[version][1]
> -    if addheader:
> -        yield _pack('>B', _fm0version)
> -    for marker in markers:
> -        yield encodeone(marker)
> +## Parsing and writing of version "0"
> +#
> +# The header is followed by the markers. Each marker is made of:
> +#
> +# - 1 unsigned byte: number of new changesets "N", can be zero.
> +#
> +# - 1 unsigned 32-bits integer: metadata size "M" in bytes.
> +#
> +# - 1 byte: a bit field. It is reserved for flags used in common
> +#   obsolete marker operations, to avoid repeated decoding of metadata
> +#   entries.
> +#
> +# - 20 bytes: obsoleted changeset identifier.
> +#
> +# - N*20 bytes: new changesets identifiers.
> +#
> +# - M bytes: metadata as a sequence of nul-terminated strings. Each
> +#   string contains a key and a value, separated by a colon ':', without
> +#   additional encoding. Keys cannot contain '\0' or ':' and values
> +#   cannot contain '\0'.
> +_fm0version = 0
> +_fm0fixed   = '>BIB20s'
> +_fm0node = '20s'
> +_fm0fsize = struct.calcsize(_fm0fixed)
> +_fm0fnodesize = struct.calcsize(_fm0node)
>
>  def _fm0readmarkers(data, off=0):
>      # Loop on markers
>      l = len(data)
>      while off + _fm0fsize <= l:
> @@ -227,10 +210,30 @@ def _fm0encodeonemarker(marker):
>
>  # mapping to read/write various marker formats
>  # <version> -> (decoder, encoder)
>  formats = {0: (_fm0readmarkers, _fm0encodeonemarker)}
>
> +def _readmarkers(data):
> +    """Read and enumerate markers from raw data"""
> +    off = 0
> +    diskversion = _unpack('>B', data[off:off + 1])[0]
> +    off += 1
> +    if diskversion not in formats:
> +        raise util.Abort(_('parsing obsolete marker: unknown version %r')
> +                         % diskversion)
> +    return diskversion, formats[diskversion][0](data, off)
> +
> +def encodemarkers(markers, addheader=False, version=_fm0version):
> +    # Kept separate from flushmarkers(), it will be reused for
> +    # markers exchange.
> +    encodeone = formats[version][1]
> +    if addheader:
> +        yield _pack('>B', _fm0version)
> +    for marker in markers:
> +        yield encodeone(marker)
> +
> +
>  def encodemeta(meta):
>      """Return encoded metadata string to string mapping.
>
>      Assume no ':' in key and no '\0' in both key and value."""
>      for key, value in meta.iteritems():
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Pierre-Yves David - Sept. 24, 2014, 4:40 p.m.
On 09/24/2014 08:21 AM, Augie Fackler wrote:
> On Tue, Sep 16, 2014 at 07:28:31PM -0700, Pierre-Yves David wrote:
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@fb.com>
>> # Date 1410525703 -7200
>> #      Fri Sep 12 14:41:43 2014 +0200
>> # Node ID d5f9d5cb72e220cc89661dbe08afd9b13436c1df
>> # Parent  48791c2bea1ceda4e4f28bc11651e281d636ce1a
>> obsolete: gather all contents related to format version 0 in a single place
>>
>> All contents embrances documentation, constants and functions. The diff is confusing
>
> Should "embrances" have been "enhances"? I'm not quite sure what word you meant.

Nope.

I mean "all content" ==  documentation + constants + functions.

Not sure what the word should be here.

>> because it cannot understand what code is semantically moved around in this
>> grand migration.
>
> I assume "no functionality change, just code movement" is true?

Yop

> This might be a case where a class-as-namespace would make some amount
> of sense, as much as I hate myself for the proposal.

Not sure. It is tempting but there is actually just a couple of function 
and an handful of constant. Not sure what the class would bring but the 
namespace. However the proposal makes sense and I tempted to do it. My 
plan it to discuss it with mpm in person next week.
Augie Fackler - Sept. 24, 2014, 5:02 p.m.
On Wed, Sep 24, 2014 at 12:40 PM, Pierre-Yves David
<pierre-yves.david@ens-lyon.org> wrote:
>>> All contents embrances documentation, constants and functions. The diff
>>> is confusing
>>
>>
>> Should "embrances" have been "enhances"? I'm not quite sure what word you
>> meant.
>
>
> Nope.
>
> I mean "all content" ==  documentation + constants + functions.
>
> Not sure what the word should be here.


All contents includes documentation, constants, and functions, so we
gather all of those things into a single location.

How's that? Is that what you meant?
Pierre-Yves David - Sept. 24, 2014, 5:03 p.m.
On 09/24/2014 10:02 AM, Augie Fackler wrote:
> On Wed, Sep 24, 2014 at 12:40 PM, Pierre-Yves David
> <pierre-yves.david@ens-lyon.org> wrote:
>>>> All contents embrances documentation, constants and functions. The diff
>>>> is confusing
>>>
>>>
>>> Should "embrances" have been "enhances"? I'm not quite sure what word you
>>> meant.
>>
>>
>> Nope.
>>
>> I mean "all content" ==  documentation + constants + functions.
>>
>> Not sure what the word should be here.
>
>
> All contents includes documentation, constants, and functions, so we
> gather all of those things into a single location.
>
> How's that? Is that what you meant?

yop

Patch

diff --git a/mercurial/obsolete.py b/mercurial/obsolete.py
--- a/mercurial/obsolete.py
+++ b/mercurial/obsolete.py
@@ -61,29 +61,12 @@  Markers are stored in an append-only fil
 
 The file starts with a version header:
 
 - 1 unsigned byte: version number, starting at zero.
 
-
-The header is followed by the markers. Each marker is made of:
-
-- 1 unsigned byte: number of new changesets "N", can be zero.
-
-- 1 unsigned 32-bits integer: metadata size "M" in bytes.
-
-- 1 byte: a bit field. It is reserved for flags used in common
-  obsolete marker operations, to avoid repeated decoding of metadata
-  entries.
-
-- 20 bytes: obsoleted changeset identifier.
-
-- N*20 bytes: new changesets identifiers.
-
-- M bytes: metadata as a sequence of nul-terminated strings. Each
-  string contains a key and a value, separated by a colon ':', without
-  additional encoding. Keys cannot contain '\0' or ':' and values
-  cannot contain '\0'.
+The header is followed by the markers. Marker format depend of the version. See
+comment associated with each format for details.
 
 """
 import struct
 import util, base85, node
 import phases
@@ -96,17 +79,10 @@  from i18n import _
 
 # the obsolete feature is not mature enough to be enabled by default.
 # you have to rely on third party extension extension to enable this.
 _enabled = False
 
-# data used for parsing and writing
-_fm0version = 0
-_fm0fixed   = '>BIB20s'
-_fm0node = '20s'
-_fm0fsize = struct.calcsize(_fm0fixed)
-_fm0fnodesize = struct.calcsize(_fm0node)
-
 ### obsolescence marker flag
 
 ## bumpedfix flag
 #
 # When a changeset A' succeed to a changeset A which became public, we call A'
@@ -135,28 +111,35 @@  from i18n import _
 # This flag mean that the successors express the changes between the public and
 # bumped version and fix the situation, breaking the transitivity of
 # "bumped" here.
 bumpedfix = 1
 
-def _readmarkers(data):
-    """Read and enumerate markers from raw data"""
-    off = 0
-    diskversion = _unpack('>B', data[off:off + 1])[0]
-    off += 1
-    if diskversion not in formats:
-        raise util.Abort(_('parsing obsolete marker: unknown version %r')
-                         % diskversion)
-    return diskversion, formats[diskversion][0](data, off)
-
-def encodemarkers(markers, addheader=False, version=_fm0version):
-    # Kept separate from flushmarkers(), it will be reused for
-    # markers exchange.
-    encodeone = formats[version][1]
-    if addheader:
-        yield _pack('>B', _fm0version)
-    for marker in markers:
-        yield encodeone(marker)
+## Parsing and writing of version "0"
+#
+# The header is followed by the markers. Each marker is made of:
+#
+# - 1 unsigned byte: number of new changesets "N", can be zero.
+#
+# - 1 unsigned 32-bits integer: metadata size "M" in bytes.
+#
+# - 1 byte: a bit field. It is reserved for flags used in common
+#   obsolete marker operations, to avoid repeated decoding of metadata
+#   entries.
+#
+# - 20 bytes: obsoleted changeset identifier.
+#
+# - N*20 bytes: new changesets identifiers.
+#
+# - M bytes: metadata as a sequence of nul-terminated strings. Each
+#   string contains a key and a value, separated by a colon ':', without
+#   additional encoding. Keys cannot contain '\0' or ':' and values
+#   cannot contain '\0'.
+_fm0version = 0
+_fm0fixed   = '>BIB20s'
+_fm0node = '20s'
+_fm0fsize = struct.calcsize(_fm0fixed)
+_fm0fnodesize = struct.calcsize(_fm0node)
 
 def _fm0readmarkers(data, off=0):
     # Loop on markers
     l = len(data)
     while off + _fm0fsize <= l:
@@ -227,10 +210,30 @@  def _fm0encodeonemarker(marker):
 
 # mapping to read/write various marker formats
 # <version> -> (decoder, encoder)
 formats = {0: (_fm0readmarkers, _fm0encodeonemarker)}
 
+def _readmarkers(data):
+    """Read and enumerate markers from raw data"""
+    off = 0
+    diskversion = _unpack('>B', data[off:off + 1])[0]
+    off += 1
+    if diskversion not in formats:
+        raise util.Abort(_('parsing obsolete marker: unknown version %r')
+                         % diskversion)
+    return diskversion, formats[diskversion][0](data, off)
+
+def encodemarkers(markers, addheader=False, version=_fm0version):
+    # Kept separate from flushmarkers(), it will be reused for
+    # markers exchange.
+    encodeone = formats[version][1]
+    if addheader:
+        yield _pack('>B', _fm0version)
+    for marker in markers:
+        yield encodeone(marker)
+
+
 def encodemeta(meta):
     """Return encoded metadata string to string mapping.
 
     Assume no ':' in key and no '\0' in both key and value."""
     for key, value in meta.iteritems():