[PATCH RFC v3] scmutil: add a simple key-value file helper

Kostia Balytskyi kobalyts at outlook.com
Tue Dec 6 08:39:31 EST 2016

On 12/6/16 1:28 PM, Jun Wu wrote:
> Excerpts from Kostia Balytskyi's message of 2016-12-06 13:22:40 +0000:
>> On 12/6/16 1:04 PM, Yuya Nishihara wrote:
>>> On Mon, 5 Dec 2016 15:03:35 -0800, Kostia Balytskyi wrote:
>>>> # HG changeset patch
>>>> # User Kostia Balytskyi <ikostia at fb.com>
>>>> # Date 1480978778 28800
>>>> #      Mon Dec 05 14:59:38 2016 -0800
>>>> # Node ID bdc49209b0b926214e865f9a98ea987866f64d68
>>>> # Parent  243ecbd4f5c9f452275d4435866359cf84dc03ff
>>>> scmutil: add a simple key-value file helper
>>>> The purpose of the added class is to serve purposes like save files of shelve
>>>> or state files of shelve, rebase and histedit. Keys of these files can be
>>>> alphanumeric and start with letters, while values must not contain newlines.
>>>> Keys which start with an uppercase letter are required, while other keys
>>>> are optional.
>>>> +class simplekeyvaluefile(object):
>>>> +    """A simple file with key=value lines
>>>> +
>>>> +    Keys must be alphanumerics and start with a letter, values might not
>>>> +    contain '\n' characters
>>>> +    """
>>>> +    class InvalidKeyValueFile(Exception): pass
>>>> +    class InvalidKeyInFileException(Exception): pass
>>>> +    class InvalidValueInFileException(Exception): pass
>>>> +    class MissingRequiredKeyInFileException(Exception): pass
>>>> +
>>>> +    # if KEYS is non-empty, read values are validated against it:
>>>> +    # each key is a tuple (keyname, required)
>>>> +    KEYS = []
>>> If we want to define a required key back to older Mercurial versions (like
>>> mergestate), we'll need a static rule such as "uppercase is required" seen in
>>> your V1 patch.
>> I am not sure I understand this one. If you meant that
>> simplekeyvaluefile should be able
>> to read state files created by older mercurail versions, I don't think
>> it's possible. I don't think
>> simplekeyvaluefile itself should provide a backwards-compatible layer.
>> Rather, when we
>> migrate a state file, we should first try to parse it into a
>> simlpekeyvaluefile and if this fails,
>> fallback to a traditional file format. New fields should only be added
>> to a simplekeyvaluefile
>> code path, while traditional parsing should replace them with some sort
>> of empty/default values.
>> Can you clarify what you meant by "define a required key back to older
>> Mercurial versions"?
> It means, for example, in hg v5.0, we have keys "a" and "b" as "required".
> And we add another key "c" as required in v5.1. What will v5.1 do with the
> simplekeyvaluefile written by v5.0 ? (and what will we do if we want to
> remove required keys?)
Hm, I don't see how upper/lower-case instead of explicit boolean flag 
handles that...
But to answer your question:
1. When you add a new field and you mark it as required, you really have 
only two options:
   a. Breaking backwards compatibility. I guess this is wanted rarely.
   b. Adding a logic to replace the missing required field with some 
default value. My opinion is that such logic should exist outside of 
simplekeyvaluefile implementation, so from this class' perspective that 
field is still optional
2. When you remove a required field or make field which was required 
optional. Because we don't limit the optional
fields in any way, this should just work - unexpected field in the file 
is just read and stored in the dictionary.
>>>> +    def __init__(self, vfs, path):
>>>> +        self.vfs = vfs
>>>> +        self.path = path
>>>> +
>>>> +    def validate(self, d):
>>>> +        for key, req in self.KEYS:
>>>> +            if req and key not in d:
>>>> +                e = _("missing a required key: '%s'") % key
>>>> +                raise self.MissingRequiredKeyInFileException(e)
>>> _() is necessary only for user-facing errors.
>>> I don't think nested classes are useful unless we want to give a room for
>>> sub classes to override them.
>>>> +            lines.append("%s=%s\n" % (k, v))
>>>> +        with self.vfs(self.path, mode='wb', atomictemp=True) as fp:
>>>> +            fp.write(''.join(lines))
>>> BTW, the syntax of this file seems somewhat similar to config.config.
>> I wanted something explicitly much simpler than config.config (also with
>> possibility to further limit
>> what can be values if needed).
>>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

More information about the Mercurial-devel mailing list