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

# HG changeset patch
# User Kostia Balytskyi <ikostia@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.

In light of Mercurial's reluctancy to use Python's json module, this tries
to provide a reasonable alternative for a non-nested named data.
Comparing to current approach of storing state in plain text files, where
semantic meaning of lines of text is only determined by their oreder,
simple key-value file allows for reordering lines and thus helps handle
optional values.

Initial use-case I see for this is obs-shelve's shelve files. Later we
can possibly migrate state files to this approach.

The test is in a new file beause I did not figure out where to put it
within existing test suite. If you give me a better idea, I will gladly
follow it.

On Mon, 5 Dec 2016 15:03:35 -0800, Kostia Balytskyi wrote:
> # HG changeset patch
> # User Kostia Balytskyi <ikostia@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.

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

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@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"?
>

>> +    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).
> _______________________________________________

> Mercurial-devel mailing list

> Mercurial-devel@mercurial-scm.org

> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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@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?)

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

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

Excerpts from Kostia Balytskyi's message of 2016-12-06 13:39:31 +0000:
> Hm, I don't see how upper/lower-case instead of explicit boolean flag 
> handles that...

I didn't realize the issue either, before Yuya's reply.

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

It seems that the user of simplekeyvaluefile have to deal with "required" /
"optional" etc.

How about just removing the "required" check in simplekeyvaluefile?
Leave them to the user to handle. That seems cleaner to me.

On 12/6/16 1:48 PM, Jun Wu wrote:
> Excerpts from Kostia Balytskyi's message of 2016-12-06 13:39:31 +0000:

>> Hm, I don't see how upper/lower-case instead of explicit boolean flag

>> handles that...

> I didn't realize the issue either, before Yuya's reply.

>

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

> It seems that the user of simplekeyvaluefile have to deal with "required" /

> "optional" etc.

>

> How about just removing the "required" check in simplekeyvaluefile?

> Leave them to the user to handle. That seems cleaner to me.

This is what happens if you don't inherit the class or don't override 
the empty KEYS list in the inherited class.

Excerpts from Kostia Balytskyi's message of 2016-12-06 13:54:02 +0000:
> This is what happens if you don't inherit the class or don't override 
> the empty KEYS list in the inherited class.

If KEYS is used to verify existence, how about just making it a list of
strings, instead of tuples? The "required" boolean flag seems to be
unnecessary.

On 12/6/16 2:08 PM, Jun Wu wrote:
> Excerpts from Kostia Balytskyi's message of 2016-12-06 13:54:02 +0000:

>> This is what happens if you don't inherit the class or don't override

>> the empty KEYS list in the inherited class.

> If KEYS is used to verify existence, how about just making it a list of

> strings, instead of tuples? The "required" boolean flag seems to be

> unnecessary.

That is true and I've thought about it: just store the list of required 
keys in KEYS.
The reason I want optional keys to be in this list is verbosity. I 
someone else wrote
the subclass for some key-value file, I as a maintainer have a single 
place to see all
the fields "desired" (required and optional) by current version of code. 
So the purpose
of optional keys there is documentation and readability.

Excerpts from Kostia Balytskyi's message of 2016-12-06 14:30:18 +0000:
> 
> On 12/6/16 2:08 PM, Jun Wu wrote:
> > Excerpts from Kostia Balytskyi's message of 2016-12-06 13:54:02 +0000:
> >> This is what happens if you don't inherit the class or don't override
> >> the empty KEYS list in the inherited class.
> > If KEYS is used to verify existence, how about just making it a list of
> > strings, instead of tuples? The "required" boolean flag seems to be
> > unnecessary.
> That is true and I've thought about it: just store the list of required 
> keys in KEYS.
> The reason I want optional keys to be in this list is verbosity. I 
> someone else wrote
> the subclass for some key-value file, I as a maintainer have a single 
> place to see all
> the fields "desired" (required and optional) by current version of code. 
> So the purpose
> of optional keys there is documentation and readability.

Then how about explicit "REQUIREDKEYS" and "OPTIONALKEYS"?

It seems sub-optimal to me that the "verify" method has to go through keys
unnecessarily.

On 12/6/16 2:45 PM, Jun Wu wrote:
> Excerpts from Kostia Balytskyi's message of 2016-12-06 14:30:18 +0000:

>> On 12/6/16 2:08 PM, Jun Wu wrote:

>>> Excerpts from Kostia Balytskyi's message of 2016-12-06 13:54:02 +0000:

>>>> This is what happens if you don't inherit the class or don't override

>>>> the empty KEYS list in the inherited class.

>>> If KEYS is used to verify existence, how about just making it a list of

>>> strings, instead of tuples? The "required" boolean flag seems to be

>>> unnecessary.

>> That is true and I've thought about it: just store the list of required

>> keys in KEYS.

>> The reason I want optional keys to be in this list is verbosity. I

>> someone else wrote

>> the subclass for some key-value file, I as a maintainer have a single

>> place to see all

>> the fields "desired" (required and optional) by current version of code.

>> So the purpose

>> of optional keys there is documentation and readability.

> Then how about explicit "REQUIREDKEYS" and "OPTIONALKEYS"?

>

> It seems sub-optimal to me that the "verify" method has to go through keys

> unnecessarily.

Well, I don't think performance is an issue here, these classes will 
rarely store more
than 20 fields if written by people and for people. If however someone 
uses it store
some large number of values - like some actual data - I don't think that 
KEYS is going
to be of any value in that case. I don't have a strong opinion about 
this, can change
if it matters.