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

login
register
mail settings
Submitter Kostia Balytskyi
Date Dec. 3, 2016, 11:37 p.m.
Message ID <c4312906ef7d06dc3be1.1480808275@dev1902.lla1.facebook.com>
Download mbox | patch
Permalink /patch/17815/
State Superseded
Headers show

Comments

Kostia Balytskyi - Dec. 3, 2016, 11:37 p.m.
# HG changeset patch
# User Kostia Balytskyi <ikostia@fb.com>
# Date 1480808065 28800
#      Sat Dec 03 15:34:25 2016 -0800
# Node ID c4312906ef7d06dc3be1575ee5cde574b915c9c4
# Parent  715232cf9182f904d5153862a1ec4905faaeee6e
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.
Augie Fackler - Dec. 5, 2016, 4:46 p.m.
> On Dec 3, 2016, at 18:37, Kostia Balytskyi <ikostia@fb.com> wrote:
> 
> # HG changeset patch
> # User Kostia Balytskyi <ikostia@fb.com>
> # Date 1480808065 28800
> #      Sat Dec 03 15:34:25 2016 -0800
> # Node ID c4312906ef7d06dc3be1575ee5cde574b915c9c4
> # Parent  715232cf9182f904d5153862a1ec4905faaeee6e
> 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.

Overall I guess I'm not opposed to this in principle, though I find myself skeptical that we'll ever migrate the older formats like histedit...

> 
> diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
> --- a/mercurial/scmutil.py
> +++ b/mercurial/scmutil.py
> @@ -1571,3 +1571,51 @@ class checkambigatclosing(closewrapbase)
>     def close(self):
>         self._origfh.close()
>         self._checkambig()
> +
> +class simplekeyvaluefile(object):
> +    """A simple file with key=value lines
> +
> +    Kyes must be alphanumerics and start with a letter, values might not
> +    contain '\n' characters
> +    """
> +    class InvalidKeyInFileException(Exception): pass
> +    class InvalidValueInFileException(Exception): pass
> +    class MissingRequiredKeyInFileException(Exception): pass
> +
> +    # if KEYS is non-empty, read values are validated against it:
> +    # - if field name starts with an uppercase letter, this FIELDS
> +    #   is considered required and its absense is an Exception
> +    # - if field name starts with a lowercase letter, it is optional
> +    #   and serves mainly as a reference for code reader
> +    KEYS = []
> +
> +    def __init__(self, repo, path):

I'd be hesitant to have this take the repo reference, and might instead want to hand it the vfs explicitly, to avoid potential cycles between the repo and this object.

> +        self.vfs = repo.vfs
> +        self.path = path
> +
> +    def validate(self, d):
> +        for k in self.KEYS:
> +            if k[0].isupper() and k not in d:
> +                e = _("Missing a required key: '%s'" % k)
> +                raise self.MissingRequiredKeyInFileException(e)
> +
> +    def read(self):
> +        lines = self.vfs.readlines(self.path)
> +        d = dict(line[:-1].split('=', 1) for line in lines)
> +        self.validate(d)
> +        return d
> +
> +    def write(self, data):
> +        lines = []
> +        for k, v in data.items():
> +            if not k[0].isalpha():
> +                e = _("Keys must start with a letter in a key-value file")
> +                raise self.InvalidKeyInFileException(e)
> +            if not k.isalnum():
> +                e = _("Invalid key name in a simple key-value file")
> +                raise self.InvalidKeyInFileException(e)
> +            if '\n' in v:
> +                e = _("Invalid value in a simple key-value file")
> +                raise self.InvalidValueInFileException(e)
> +            lines.append("%s=%s\n" % (k, v))
> +        self.vfs.writelines(self.path, lines)
> diff --git a/tests/test-other.t b/tests/test-other.t
> new file mode 100644
> --- /dev/null
> +++ b/tests/test-other.t
> @@ -0,0 +1,95 @@
> +Test simple key-value files
> +  $ cd $TESTTMP
> +  $ hg init repo
> +  $ cd $TESTTMP/repo
> +
> +Test simple key-value file creation
> +  $ cat <<EOF > keyvalwriter.py
> +  > from mercurial import ui, hg
> +  > from mercurial.scmutil import simplekeyvaluefile
> +  > ui = ui.ui()
> +  > repo = hg.repository(ui, '$TESTTMP/repo')
> +  > d = {'key1': 'value1', 'Key2': 'value2'}
> +  > kvf = simplekeyvaluefile(repo, 'kvfile').write(d)
> +  > EOF
> +  $ python keyvalwriter.py
> +  $ cat .hg/kvfile | sort
> +  Key2=value2
> +  key1=value1
> +
> +Test simple key-value file reading with invalid keys or values
> +  $ cat <<EOF > keyvalwriter.py
> +  > from mercurial import ui, hg
> +  > from mercurial.scmutil import simplekeyvaluefile
> +  > ui = ui.ui()
> +  > repo = hg.repository(ui, '$TESTTMP/repo')
> +  > d = {'0key1': 'value1', 'Key2': 'value2'}
> +  > kvf = simplekeyvaluefile(repo, 'kvfile').write(d)
> +  > EOF
> +  $ python keyvalwriter.py 2>&1 | tail -1
> +  mercurial.scmutil.InvalidKeyInFileException: Keys must start with a letter in a key-value file
> +  $ cat <<EOF > keyvalwriter.py
> +  > from mercurial import ui, hg
> +  > from mercurial.scmutil import simplekeyvaluefile
> +  > ui = ui.ui()
> +  > repo = hg.repository(ui, '$TESTTMP/repo')
> +  > d = {'key@1': 'value1'}
> +  > kvf = simplekeyvaluefile(repo, 'kvfile').write(d)
> +  > EOF
> +  $ python keyvalwriter.py 2>&1 | tail -1
> +  mercurial.scmutil.InvalidKeyInFileException: Invalid key name in a simple key-value file
> +  $ cat <<EOF > keyvalwriter.py
> +  > from mercurial import ui, hg
> +  > from mercurial.scmutil import simplekeyvaluefile
> +  > ui = ui.ui()
> +  > repo = hg.repository(ui, '$TESTTMP/repo')
> +  > d = {'key1': 'value\n1'}
> +  > kvf = simplekeyvaluefile(repo, 'kvfile').write(d)
> +  > EOF
> +  $ python keyvalwriter.py 2>&1 | tail -1
> +  mercurial.scmutil.InvalidValueInFileException: Invalid value in a simple key-value file
> +
> +Test simple key-value file reading without field list
> +  $ cat <<EOF > keyvalreader.py
> +  > from mercurial import ui, hg
> +  > from mercurial.scmutil import simplekeyvaluefile
> +  > ui = ui.ui()
> +  > repo = hg.repository(ui, '$TESTTMP/repo')
> +  > d = simplekeyvaluefile(repo, 'kvfile').read()
> +  > for k, v in sorted(d.items()):
> +  >     print "%s => %s" % (k, v)
> +  > EOF
> +  $ python keyvalreader.py
> +  Key2 => value2
> +  key1 => value1
> +
> +Test simeple key-value file when necessary files are present
> +  $ cat <<EOF > keyvalreader.py
> +  > from mercurial import ui, hg
> +  > from mercurial.scmutil import simplekeyvaluefile
> +  > ui = ui.ui()
> +  > repo = hg.repository(ui, '$TESTTMP/repo')
> +  > class kvf(simplekeyvaluefile):
> +  >     KEYS = ['key3', 'Key2']
> +  > d = kvf(repo, 'kvfile').read()
> +  > for k, v in sorted(d.items()):
> +  >     print "%s => %s" % (k, v)
> +  > EOF
> +  $ python keyvalreader.py
> +  Key2 => value2
> +  key1 => value1
> +
> +Test simeple key-value file when necessary files are absent
> +  $ cat <<EOF > keyvalreader.py
> +  > from mercurial import ui, hg
> +  > from mercurial.scmutil import simplekeyvaluefile
> +  > ui = ui.ui()
> +  > repo = hg.repository(ui, '$TESTTMP/repo')
> +  > class kvf(simplekeyvaluefile):
> +  >     KEYS = ['Key4', 'Key2']
> +  > d = kvf(repo, 'kvfile').read()
> +  > for k, v in sorted(d.items()):
> +  >     print "%s => %s" % (k, v)
> +  > EOF
> +  $ python keyvalreader.py 2>&1 | tail -1
> +  mercurial.scmutil.MissingRequiredKeyInFileException: Missing a required key: 'Key4'
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Jun Wu - Dec. 5, 2016, 9:54 p.m.
Excerpts from Kostia Balytskyi's message of 2016-12-03 15:37:55 -0800:
> # HG changeset patch
> # User Kostia Balytskyi <ikostia@fb.com>
> # Date 1480808065 28800
> #      Sat Dec 03 15:34:25 2016 -0800
> # Node ID c4312906ef7d06dc3be1575ee5cde574b915c9c4
> # Parent  715232cf9182f904d5153862a1ec4905faaeee6e
> 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.
> 
> diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
> --- a/mercurial/scmutil.py
> +++ b/mercurial/scmutil.py
> @@ -1571,3 +1571,51 @@ class checkambigatclosing(closewrapbase)
>      def close(self):
>          self._origfh.close()
>          self._checkambig()
> +
> +class simplekeyvaluefile(object):
> +    """A simple file with key=value lines
> +
> +    Kyes must be alphanumerics and start with a letter, values might not

Keys.

> +    contain '\n' characters
> +    """
> +    class InvalidKeyInFileException(Exception): pass
> +    class InvalidValueInFileException(Exception): pass
> +    class MissingRequiredKeyInFileException(Exception): pass

This enforces the caller to do

  excpet simplekeyvaluefile.InvalidKeyInFileException

I guess we put every exception classes inside error.py?
(See below, I think the exceptions used in "write" should be just
 "RuntimeError"s)

> +
> +    # if KEYS is non-empty, read values are validated against it:
> +    # - if field name starts with an uppercase letter, this FIELDS
> +    #   is considered required and its absense is an Exception
> +    # - if field name starts with a lowercase letter, it is optional
> +    #   and serves mainly as a reference for code reader

This may make the file look a bit strange when editing directly (mixed of
upper case and lower cases). As the coding style is basically "lowercase
everywhere". I'd suggest underscore or an explicit boolean flag instead.

> +    KEYS = []
> +
> +    def __init__(self, repo, path):
> +        self.vfs = repo.vfs
> +        self.path = path
> +
> +    def validate(self, d):
> +        for k in self.KEYS:
> +            if k[0].isupper() and k not in d:
> +                e = _("Missing a required key: '%s'" % k)

I think we use lower-case in exception message ("missing" vs "Missing").

> +                raise self.MissingRequiredKeyInFileException(e)
> +
> +    def read(self):
> +        lines = self.vfs.readlines(self.path)
> +        d = dict(line[:-1].split('=', 1) for line in lines)
> +        self.validate(d)
> +        return d
> +
> +    def write(self, data):

data deserves a docstring about its type.

> +        lines = []
> +        for k, v in data.items():
> +            if not k[0].isalpha():
> +                e = _("Keys must start with a letter in a key-value file")
> +                raise self.InvalidKeyInFileException(e)
> +            if not k.isalnum():
> +                e = _("Invalid key name in a simple key-value file")
> +                raise self.InvalidKeyInFileException(e)
> +            if '\n' in v:
> +                e = _("Invalid value in a simple key-value file")
> +                raise self.InvalidValueInFileException(e)

These sound like a programmer error. Maybe just raise RuntimeError.

> +            lines.append("%s=%s\n" % (k, v))
> +        self.vfs.writelines(self.path, lines)

Probably use atomictemp to avoid race conditions.

> diff --git a/tests/test-other.t b/tests/test-other.t
> new file mode 100644
> --- /dev/null
> +++ b/tests/test-other.t
> @@ -0,0 +1,95 @@
> +Test simple key-value files
> +  $ cd $TESTTMP
> +  $ hg init repo
> +  $ cd $TESTTMP/repo
> +
> +Test simple key-value file creation
> +  $ cat <<EOF > keyvalwriter.py
> +  > from mercurial import ui, hg
> +  > from mercurial.scmutil import simplekeyvaluefile
> +  > ui = ui.ui()
> +  > repo = hg.repository(ui, '$TESTTMP/repo')
> +  > d = {'key1': 'value1', 'Key2': 'value2'}
> +  > kvf = simplekeyvaluefile(repo, 'kvfile').write(d)
> +  > EOF
> +  $ python keyvalwriter.py
> +  $ cat .hg/kvfile | sort
> +  Key2=value2
> +  key1=value1
> +
> +Test simple key-value file reading with invalid keys or values
> +  $ cat <<EOF > keyvalwriter.py
> +  > from mercurial import ui, hg
> +  > from mercurial.scmutil import simplekeyvaluefile
> +  > ui = ui.ui()
> +  > repo = hg.repository(ui, '$TESTTMP/repo')
> +  > d = {'0key1': 'value1', 'Key2': 'value2'}
> +  > kvf = simplekeyvaluefile(repo, 'kvfile').write(d)
> +  > EOF
> +  $ python keyvalwriter.py 2>&1 | tail -1
> +  mercurial.scmutil.InvalidKeyInFileException: Keys must start with a letter in a key-value file
> +  $ cat <<EOF > keyvalwriter.py
> +  > from mercurial import ui, hg
> +  > from mercurial.scmutil import simplekeyvaluefile
> +  > ui = ui.ui()
> +  > repo = hg.repository(ui, '$TESTTMP/repo')
> +  > d = {'key@1': 'value1'}
> +  > kvf = simplekeyvaluefile(repo, 'kvfile').write(d)
> +  > EOF
> +  $ python keyvalwriter.py 2>&1 | tail -1
> +  mercurial.scmutil.InvalidKeyInFileException: Invalid key name in a simple key-value file
> +  $ cat <<EOF > keyvalwriter.py
> +  > from mercurial import ui, hg
> +  > from mercurial.scmutil import simplekeyvaluefile
> +  > ui = ui.ui()
> +  > repo = hg.repository(ui, '$TESTTMP/repo')
> +  > d = {'key1': 'value\n1'}
> +  > kvf = simplekeyvaluefile(repo, 'kvfile').write(d)
> +  > EOF
> +  $ python keyvalwriter.py 2>&1 | tail -1
> +  mercurial.scmutil.InvalidValueInFileException: Invalid value in a simple key-value file
> +
> +Test simple key-value file reading without field list
> +  $ cat <<EOF > keyvalreader.py
> +  > from mercurial import ui, hg
> +  > from mercurial.scmutil import simplekeyvaluefile
> +  > ui = ui.ui()
> +  > repo = hg.repository(ui, '$TESTTMP/repo')
> +  > d = simplekeyvaluefile(repo, 'kvfile').read()
> +  > for k, v in sorted(d.items()):
> +  >     print "%s => %s" % (k, v)
> +  > EOF
> +  $ python keyvalreader.py
> +  Key2 => value2
> +  key1 => value1
> +
> +Test simeple key-value file when necessary files are present
> +  $ cat <<EOF > keyvalreader.py
> +  > from mercurial import ui, hg
> +  > from mercurial.scmutil import simplekeyvaluefile
> +  > ui = ui.ui()
> +  > repo = hg.repository(ui, '$TESTTMP/repo')
> +  > class kvf(simplekeyvaluefile):
> +  >     KEYS = ['key3', 'Key2']
> +  > d = kvf(repo, 'kvfile').read()
> +  > for k, v in sorted(d.items()):
> +  >     print "%s => %s" % (k, v)
> +  > EOF
> +  $ python keyvalreader.py
> +  Key2 => value2
> +  key1 => value1
> +
> +Test simeple key-value file when necessary files are absent
> +  $ cat <<EOF > keyvalreader.py
> +  > from mercurial import ui, hg
> +  > from mercurial.scmutil import simplekeyvaluefile
> +  > ui = ui.ui()
> +  > repo = hg.repository(ui, '$TESTTMP/repo')
> +  > class kvf(simplekeyvaluefile):
> +  >     KEYS = ['Key4', 'Key2']
> +  > d = kvf(repo, 'kvfile').read()
> +  > for k, v in sorted(d.items()):
> +  >     print "%s => %s" % (k, v)
> +  > EOF
> +  $ python keyvalreader.py 2>&1 | tail -1
> +  mercurial.scmutil.MissingRequiredKeyInFileException: Missing a required key: 'Key4'
Augie Fackler - Dec. 5, 2016, 10:07 p.m.
> On Dec 5, 2016, at 16:54, Jun Wu <quark@fb.com> wrote:
> 
>> +                e = _("Invalid value in a simple key-value file")
>> +                raise self.InvalidValueInFileException(e)
> 
> These sound like a programmer error. Maybe just raise RuntimeError.

I'm not comfortable with that, FYI. RuntimeError IMO should be reserved for errors in the Python runtime itself, not run-time errors caused by programmer error.
Kostia Balytskyi - Dec. 5, 2016, 10:49 p.m.
On 12/5/16 9:54 PM, Jun Wu wrote:
> Excerpts from Kostia Balytskyi's message of 2016-12-03 15:37:55 -0800:

>> # HG changeset patch

>> # User Kostia Balytskyi <ikostia@fb.com>

>> # Date 1480808065 28800

>> #      Sat Dec 03 15:34:25 2016 -0800

>> # Node ID c4312906ef7d06dc3be1575ee5cde574b915c9c4

>> # Parent  715232cf9182f904d5153862a1ec4905faaeee6e

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

>>

>> diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py

>> --- a/mercurial/scmutil.py

>> +++ b/mercurial/scmutil.py

>> @@ -1571,3 +1571,51 @@ class checkambigatclosing(closewrapbase)

>>       def close(self):

>>           self._origfh.close()

>>           self._checkambig()

>> +

>> +class simplekeyvaluefile(object):

>> +    """A simple file with key=value lines

>> +

>> +    Kyes must be alphanumerics and start with a letter, values might not

> Keys.

Good catch.
>

>> +    contain '\n' characters

>> +    """

>> +    class InvalidKeyInFileException(Exception): pass

>> +    class InvalidValueInFileException(Exception): pass

>> +    class MissingRequiredKeyInFileException(Exception): pass

> This enforces the caller to do

>

>    excpet simplekeyvaluefile.InvalidKeyInFileException

>

> I guess we put every exception classes inside error.py?

> (See below, I think the exceptions used in "write" should be just

>   "RuntimeError"s)

How strong is our habit of storing exceptions in error.py? I feel like
having exceptions namespaced in simplekeyvaluefile reduces the need
to have really verbose names. In short, I am for keeping exceptions in
the class, but I don't feel too strongly about it.
>

>> +

>> +    # if KEYS is non-empty, read values are validated against it:

>> +    # - if field name starts with an uppercase letter, this FIELDS

>> +    #   is considered required and its absense is an Exception

>> +    # - if field name starts with a lowercase letter, it is optional

>> +    #   and serves mainly as a reference for code reader

> This may make the file look a bit strange when editing directly (mixed of

> upper case and lower cases). As the coding style is basically "lowercase

> everywhere". I'd suggest underscore or an explicit boolean flag instead.

Will do.
>

>> +    KEYS = []

>> +

>> +    def __init__(self, repo, path):

>> +        self.vfs = repo.vfs

>> +        self.path = path

>> +

>> +    def validate(self, d):

>> +        for k in self.KEYS:

>> +            if k[0].isupper() and k not in d:

>> +                e = _("Missing a required key: '%s'" % k)

> I think we use lower-case in exception message ("missing" vs "Missing").

Will fix.
>

>> +                raise self.MissingRequiredKeyInFileException(e)

>> +

>> +    def read(self):

>> +        lines = self.vfs.readlines(self.path)

>> +        d = dict(line[:-1].split('=', 1) for line in lines)

>> +        self.validate(d)

>> +        return d

>> +

>> +    def write(self, data):

> data deserves a docstring about its type.

Good idea, will fix.
>

>> +        lines = []

>> +        for k, v in data.items():

>> +            if not k[0].isalpha():

>> +                e = _("Keys must start with a letter in a key-value file")

>> +                raise self.InvalidKeyInFileException(e)

>> +            if not k.isalnum():

>> +                e = _("Invalid key name in a simple key-value file")

>> +                raise self.InvalidKeyInFileException(e)

>> +            if '\n' in v:

>> +                e = _("Invalid value in a simple key-value file")

>> +                raise self.InvalidValueInFileException(e)

> These sound like a programmer error. Maybe just raise RuntimeError.

I agree with Augie and I see no harm in having an extra exception that 
will basically only fire during development phases.
>

>> +            lines.append("%s=%s\n" % (k, v))

>> +        self.vfs.writelines(self.path, lines)

> Probably use atomictemp to avoid race conditions.

>

>> diff --git a/tests/test-other.t b/tests/test-other.t

>> new file mode 100644

>> --- /dev/null

>> +++ b/tests/test-other.t

>> @@ -0,0 +1,95 @@

>> +Test simple key-value files

>> +  $ cd $TESTTMP

>> +  $ hg init repo

>> +  $ cd $TESTTMP/repo

>> +

>> +Test simple key-value file creation

>> +  $ cat <<EOF > keyvalwriter.py

>> +  > from mercurial import ui, hg

>> +  > from mercurial.scmutil import simplekeyvaluefile

>> +  > ui = ui.ui()

>> +  > repo = hg.repository(ui, '$TESTTMP/repo')

>> +  > d = {'key1': 'value1', 'Key2': 'value2'}

>> +  > kvf = simplekeyvaluefile(repo, 'kvfile').write(d)

>> +  > EOF

>> +  $ python keyvalwriter.py

>> +  $ cat .hg/kvfile | sort

>> +  Key2=value2

>> +  key1=value1

>> +

>> +Test simple key-value file reading with invalid keys or values

>> +  $ cat <<EOF > keyvalwriter.py

>> +  > from mercurial import ui, hg

>> +  > from mercurial.scmutil import simplekeyvaluefile

>> +  > ui = ui.ui()

>> +  > repo = hg.repository(ui, '$TESTTMP/repo')

>> +  > d = {'0key1': 'value1', 'Key2': 'value2'}

>> +  > kvf = simplekeyvaluefile(repo, 'kvfile').write(d)

>> +  > EOF

>> +  $ python keyvalwriter.py 2>&1 | tail -1

>> +  mercurial.scmutil.InvalidKeyInFileException: Keys must start with a letter in a key-value file

>> +  $ cat <<EOF > keyvalwriter.py

>> +  > from mercurial import ui, hg

>> +  > from mercurial.scmutil import simplekeyvaluefile

>> +  > ui = ui.ui()

>> +  > repo = hg.repository(ui, '$TESTTMP/repo')

>> +  > d = {'key@1': 'value1'}

>> +  > kvf = simplekeyvaluefile(repo, 'kvfile').write(d)

>> +  > EOF

>> +  $ python keyvalwriter.py 2>&1 | tail -1

>> +  mercurial.scmutil.InvalidKeyInFileException: Invalid key name in a simple key-value file

>> +  $ cat <<EOF > keyvalwriter.py

>> +  > from mercurial import ui, hg

>> +  > from mercurial.scmutil import simplekeyvaluefile

>> +  > ui = ui.ui()

>> +  > repo = hg.repository(ui, '$TESTTMP/repo')

>> +  > d = {'key1': 'value\n1'}

>> +  > kvf = simplekeyvaluefile(repo, 'kvfile').write(d)

>> +  > EOF

>> +  $ python keyvalwriter.py 2>&1 | tail -1

>> +  mercurial.scmutil.InvalidValueInFileException: Invalid value in a simple key-value file

>> +

>> +Test simple key-value file reading without field list

>> +  $ cat <<EOF > keyvalreader.py

>> +  > from mercurial import ui, hg

>> +  > from mercurial.scmutil import simplekeyvaluefile

>> +  > ui = ui.ui()

>> +  > repo = hg.repository(ui, '$TESTTMP/repo')

>> +  > d = simplekeyvaluefile(repo, 'kvfile').read()

>> +  > for k, v in sorted(d.items()):

>> +  >     print "%s => %s" % (k, v)

>> +  > EOF

>> +  $ python keyvalreader.py

>> +  Key2 => value2

>> +  key1 => value1

>> +

>> +Test simeple key-value file when necessary files are present

>> +  $ cat <<EOF > keyvalreader.py

>> +  > from mercurial import ui, hg

>> +  > from mercurial.scmutil import simplekeyvaluefile

>> +  > ui = ui.ui()

>> +  > repo = hg.repository(ui, '$TESTTMP/repo')

>> +  > class kvf(simplekeyvaluefile):

>> +  >     KEYS = ['key3', 'Key2']

>> +  > d = kvf(repo, 'kvfile').read()

>> +  > for k, v in sorted(d.items()):

>> +  >     print "%s => %s" % (k, v)

>> +  > EOF

>> +  $ python keyvalreader.py

>> +  Key2 => value2

>> +  key1 => value1

>> +

>> +Test simeple key-value file when necessary files are absent

>> +  $ cat <<EOF > keyvalreader.py

>> +  > from mercurial import ui, hg

>> +  > from mercurial.scmutil import simplekeyvaluefile

>> +  > ui = ui.ui()

>> +  > repo = hg.repository(ui, '$TESTTMP/repo')

>> +  > class kvf(simplekeyvaluefile):

>> +  >     KEYS = ['Key4', 'Key2']

>> +  > d = kvf(repo, 'kvfile').read()

>> +  > for k, v in sorted(d.items()):

>> +  >     print "%s => %s" % (k, v)

>> +  > EOF

>> +  $ python keyvalreader.py 2>&1 | tail -1

>> +  mercurial.scmutil.MissingRequiredKeyInFileException: Missing a required key: 'Key4'

> _______________________________________________

> Mercurial-devel mailing list

> Mercurial-devel@mercurial-scm.org

> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Jun Wu - Dec. 5, 2016, 10:51 p.m.
Excerpts from Augie Fackler's message of 2016-12-05 17:07:47 -0500:
> 
> > On Dec 5, 2016, at 16:54, Jun Wu <quark@fb.com> wrote:
> > 
> >> +                e = _("Invalid value in a simple key-value file")
> >> +                raise self.InvalidValueInFileException(e)
> > 
> > These sound like a programmer error. Maybe just raise RuntimeError.
> 
> I'm not comfortable with that, FYI. RuntimeError IMO should be reserved for errors in the Python runtime itself, not run-time errors caused by programmer error.

From what I have learn from the code base. We use RuntimeError for
programmer errors. Seems to be the most "popular" solution.

  merge.py
  317:            raise RuntimeError("localctx accessed but self._local isn't set")
  323:            raise RuntimeError("otherctx accessed but self._other isn't set")
  
  context.py
  2011:           raise RuntimeError('can\'t reuse the manifest: '
  2014:           raise RuntimeError('can\'t reuse the manifest: '

  localrepo.py
  1019:           raise RuntimeError('programming error: transaction requires '

There are some subclasses of RuntimeError though:

  bundle2.py
  298:class TransactionUnavailable(RuntimeError):
  856:                raise RuntimeError('duplicated params: %s' % pname)
  909:            raise RuntimeError('part can only be consumed once')
  1078:        raise RuntimeError('no repo access from stream interruption')

  error.py
  149:class LockInheritanceContractViolation(RuntimeError):
  168:class PushRaced(RuntimeError):
  198:class ReadOnlyPartError(RuntimeError):
Augie Fackler - Dec. 5, 2016, 11:13 p.m.
> On Dec 5, 2016, at 17:51, Jun Wu <quark@fb.com> wrote:
> 
> Excerpts from Augie Fackler's message of 2016-12-05 17:07:47 -0500:
>> 
>>> On Dec 5, 2016, at 16:54, Jun Wu <quark@fb.com> wrote:
>>> 
>>>> +                e = _("Invalid value in a simple key-value file")
>>>> +                raise self.InvalidValueInFileException(e)
>>> 
>>> These sound like a programmer error. Maybe just raise RuntimeError.
>> 
>> I'm not comfortable with that, FYI. RuntimeError IMO should be reserved for errors in the Python runtime itself, not run-time errors caused by programmer error.
> 
> From what I have learn from the code base. We use RuntimeError for
> programmer errors. Seems to be the most "popular" solution.

Past mistakes don't convince me that we should continue doing this. :)

> 
>  merge.py
>  317:            raise RuntimeError("localctx accessed but self._local isn't set")
>  323:            raise RuntimeError("otherctx accessed but self._other isn't set")
> 
>  context.py
>  2011:           raise RuntimeError('can\'t reuse the manifest: '
>  2014:           raise RuntimeError('can\'t reuse the manifest: '
> 
>  localrepo.py
>  1019:           raise RuntimeError('programming error: transaction requires '
> 
> There are some subclasses of RuntimeError though:
> 
>  bundle2.py
>  298:class TransactionUnavailable(RuntimeError):
>  856:                raise RuntimeError('duplicated params: %s' % pname)
>  909:            raise RuntimeError('part can only be consumed once')
>  1078:        raise RuntimeError('no repo access from stream interruption')
> 
>  error.py
>  149:class LockInheritanceContractViolation(RuntimeError):
>  168:class PushRaced(RuntimeError):
>  198:class ReadOnlyPartError(RuntimeError):

I'd rather see a new ProgrammingError or something in error.py, and have it inherit from RuntimeError for six months or so and then cut the cord on this silliness.

(I know this opinion of mine is not widely held, but I'd also rather we /at least/ introduce new specific exceptions rather than just blindly reusing something this generic (note that if you catch RuntimeError, you're also transitively catching NotImplementedError, which is probably not what you had in mind...)
Jun Wu - Dec. 5, 2016, 11:50 p.m.
Excerpts from Augie Fackler's message of 2016-12-05 18:13:14 -0500:
> I'd rather see a new ProgrammingError or something in error.py, and have
> it inherit from RuntimeError for six months or so and then cut the cord on
> this silliness.

+1 on ProgrammingError. I was actually a bit surprised that it does not
exist already.

> (I know this opinion of mine is not widely held, but I'd also rather we
> /at least/ introduce new specific exceptions rather than just blindly
> reusing something this generic (note that if you catch RuntimeError,
> you're also transitively catching NotImplementedError, which is probably
> not what you had in mind...)

Patch

diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
--- a/mercurial/scmutil.py
+++ b/mercurial/scmutil.py
@@ -1571,3 +1571,51 @@  class checkambigatclosing(closewrapbase)
     def close(self):
         self._origfh.close()
         self._checkambig()
+
+class simplekeyvaluefile(object):
+    """A simple file with key=value lines
+
+    Kyes must be alphanumerics and start with a letter, values might not
+    contain '\n' characters
+    """
+    class InvalidKeyInFileException(Exception): pass
+    class InvalidValueInFileException(Exception): pass
+    class MissingRequiredKeyInFileException(Exception): pass
+
+    # if KEYS is non-empty, read values are validated against it:
+    # - if field name starts with an uppercase letter, this FIELDS
+    #   is considered required and its absense is an Exception
+    # - if field name starts with a lowercase letter, it is optional
+    #   and serves mainly as a reference for code reader
+    KEYS = []
+
+    def __init__(self, repo, path):
+        self.vfs = repo.vfs
+        self.path = path
+
+    def validate(self, d):
+        for k in self.KEYS:
+            if k[0].isupper() and k not in d:
+                e = _("Missing a required key: '%s'" % k)
+                raise self.MissingRequiredKeyInFileException(e)
+
+    def read(self):
+        lines = self.vfs.readlines(self.path)
+        d = dict(line[:-1].split('=', 1) for line in lines)
+        self.validate(d)
+        return d
+
+    def write(self, data):
+        lines = []
+        for k, v in data.items():
+            if not k[0].isalpha():
+                e = _("Keys must start with a letter in a key-value file")
+                raise self.InvalidKeyInFileException(e)
+            if not k.isalnum():
+                e = _("Invalid key name in a simple key-value file")
+                raise self.InvalidKeyInFileException(e)
+            if '\n' in v:
+                e = _("Invalid value in a simple key-value file")
+                raise self.InvalidValueInFileException(e)
+            lines.append("%s=%s\n" % (k, v))
+        self.vfs.writelines(self.path, lines)
diff --git a/tests/test-other.t b/tests/test-other.t
new file mode 100644
--- /dev/null
+++ b/tests/test-other.t
@@ -0,0 +1,95 @@ 
+Test simple key-value files
+  $ cd $TESTTMP
+  $ hg init repo
+  $ cd $TESTTMP/repo
+
+Test simple key-value file creation
+  $ cat <<EOF > keyvalwriter.py
+  > from mercurial import ui, hg
+  > from mercurial.scmutil import simplekeyvaluefile
+  > ui = ui.ui()
+  > repo = hg.repository(ui, '$TESTTMP/repo')
+  > d = {'key1': 'value1', 'Key2': 'value2'}
+  > kvf = simplekeyvaluefile(repo, 'kvfile').write(d)
+  > EOF
+  $ python keyvalwriter.py
+  $ cat .hg/kvfile | sort
+  Key2=value2
+  key1=value1
+
+Test simple key-value file reading with invalid keys or values
+  $ cat <<EOF > keyvalwriter.py
+  > from mercurial import ui, hg
+  > from mercurial.scmutil import simplekeyvaluefile
+  > ui = ui.ui()
+  > repo = hg.repository(ui, '$TESTTMP/repo')
+  > d = {'0key1': 'value1', 'Key2': 'value2'}
+  > kvf = simplekeyvaluefile(repo, 'kvfile').write(d)
+  > EOF
+  $ python keyvalwriter.py 2>&1 | tail -1
+  mercurial.scmutil.InvalidKeyInFileException: Keys must start with a letter in a key-value file
+  $ cat <<EOF > keyvalwriter.py
+  > from mercurial import ui, hg
+  > from mercurial.scmutil import simplekeyvaluefile
+  > ui = ui.ui()
+  > repo = hg.repository(ui, '$TESTTMP/repo')
+  > d = {'key@1': 'value1'}
+  > kvf = simplekeyvaluefile(repo, 'kvfile').write(d)
+  > EOF
+  $ python keyvalwriter.py 2>&1 | tail -1
+  mercurial.scmutil.InvalidKeyInFileException: Invalid key name in a simple key-value file
+  $ cat <<EOF > keyvalwriter.py
+  > from mercurial import ui, hg
+  > from mercurial.scmutil import simplekeyvaluefile
+  > ui = ui.ui()
+  > repo = hg.repository(ui, '$TESTTMP/repo')
+  > d = {'key1': 'value\n1'}
+  > kvf = simplekeyvaluefile(repo, 'kvfile').write(d)
+  > EOF
+  $ python keyvalwriter.py 2>&1 | tail -1
+  mercurial.scmutil.InvalidValueInFileException: Invalid value in a simple key-value file
+
+Test simple key-value file reading without field list
+  $ cat <<EOF > keyvalreader.py
+  > from mercurial import ui, hg
+  > from mercurial.scmutil import simplekeyvaluefile
+  > ui = ui.ui()
+  > repo = hg.repository(ui, '$TESTTMP/repo')
+  > d = simplekeyvaluefile(repo, 'kvfile').read()
+  > for k, v in sorted(d.items()):
+  >     print "%s => %s" % (k, v)
+  > EOF
+  $ python keyvalreader.py
+  Key2 => value2
+  key1 => value1
+
+Test simeple key-value file when necessary files are present
+  $ cat <<EOF > keyvalreader.py
+  > from mercurial import ui, hg
+  > from mercurial.scmutil import simplekeyvaluefile
+  > ui = ui.ui()
+  > repo = hg.repository(ui, '$TESTTMP/repo')
+  > class kvf(simplekeyvaluefile):
+  >     KEYS = ['key3', 'Key2']
+  > d = kvf(repo, 'kvfile').read()
+  > for k, v in sorted(d.items()):
+  >     print "%s => %s" % (k, v)
+  > EOF
+  $ python keyvalreader.py
+  Key2 => value2
+  key1 => value1
+
+Test simeple key-value file when necessary files are absent
+  $ cat <<EOF > keyvalreader.py
+  > from mercurial import ui, hg
+  > from mercurial.scmutil import simplekeyvaluefile
+  > ui = ui.ui()
+  > repo = hg.repository(ui, '$TESTTMP/repo')
+  > class kvf(simplekeyvaluefile):
+  >     KEYS = ['Key4', 'Key2']
+  > d = kvf(repo, 'kvfile').read()
+  > for k, v in sorted(d.items()):
+  >     print "%s => %s" % (k, v)
+  > EOF
+  $ python keyvalreader.py 2>&1 | tail -1
+  mercurial.scmutil.MissingRequiredKeyInFileException: Missing a required key: 'Key4'