[PATCH 1/2] config: accept value-less keys in configuration files

[Jacob Keller]

If stg is run with a configuration file which has a key defined without
a value, the following exception is produced:

Traceback (most recent call last):
File "./stg", line 24, in <module>
from stgit.main import main
File "stgit/main.py", line 86, in <module>
cmd_list = stgit.commands.get_commands()
File "stgit/commands/__init__.py", line 61, in get_commands
for mod, m in _find_commands())
File "stgit/commands/__init__.py", line 60, in <genexpr>
return dict((text(getattr(m, 'name', mod)), (mod, _kinds[m.kind], m.help))
File "stgit/commands/__init__.py", line 45, in _find_commands
m = get_command(mod)
File "stgit/commands/__init__.py", line 29, in get_command
return __import__(__name__ + '.' + mod, globals(), locals(), ['*'])
File "stgit/commands/diff.py", line 48, in <module>
] + argparse.diff_opts_option()
File "stgit/argparse.py", line 208, in diff_opts_option
default = (config.get('stgit.diff-opts') or '').split(),
File "stgit/config.py", line 67, in get
self.load()
File "stgit/config.py", line 63, in load
key, value = line.split('\n', 1)
ValueError: need more than 1 value to unpack

Git considers this valid (as when parsing booleans, a key without
a value at all is considered as true), so we should not be producing an
exception.

To fix this, catch the ValueError, and use None as the value. This
should be fine, since looking up such a key produce None, which is the
expected behavior for get(), getall(), and getint().

We choose this route instead of simply leaving the key undefined, (which
would thus produce a KeyError exception upon lookup), as we will need to
distinguish this case separately in a following patch which adds
getbool() support.

Add a new test-case which highlights the issue.

Signed-off-by: Jacob Keller <jacob.e...@intel.com>
---
stgit/config.py | 6 +++++-
t/t6000-config.sh | 19 +++++++++++++++++++
2 files changed, 24 insertions(+), 1 deletion(-)
create mode 100755 t/t6000-config.sh

diff --git a/stgit/config.py b/stgit/config.py
index 4cb609407aec..b9df20a8c635 100644
--- a/stgit/config.py
+++ b/stgit/config.py
@@ -60,7 +60,11 @@ class GitConfig(object):
lines = Run('git', 'config', '--null', '--list'
).discard_exitcode().output_lines('\0')
for line in lines:
- key, value = line.split('\n', 1)
+ try:
+ key, value = line.split('\n', 1)
+ except ValueError:
+ key = line
+ value = None
self.__cache.setdefault(key, []).append(value)

def get(self, name):
diff --git a/t/t6000-config.sh b/t/t6000-config.sh
new file mode 100755
index 000000000000..425991a6ee58
--- /dev/null
+++ b/t/t6000-config.sh
@@ -0,0 +1,19 @@
+#!/bin/sh
+#
+# Copyright (c) 2017 Intel Corporation
+#
+
+test_description='Simple configuration tests'
+
+. ./test-lib.sh
+
+# Note that it is not possible to use git-config to write such a value, but
+# they are expected to be interpret-able by git commands
+test_expect_success 'stg accepts keys without values' '
+ echo "[stgit]" >> .git/config &&
+ echo " aboolean" >> .git/config &&
+ stg init &&
+ stg status
+'
+
+test_done
--
2.15.0

[Jacob Keller]

This series provides a couple of fixes for boolean configuration values.
First, we fixup the ValueError exception that occurs if you have a
configuration file which has a key that has no defined value, such as

[stgit]
someVariable

In git code, this is valid (though not writable by git-config) and it
wants to treat such a variable as "true" when read via "--bool".

The second patch adds support for getbool() which works in the same was
as git config --get --bool, and canonicalizes variable values into True
or False. This ensures that we allow various "True" literals such as
"on", "yes", and "true".

Without this patch, the only valid "true" value for some of our
configurations is "yes".

The intention of this patch is to ensure that we treat configuration
options as close to git as possible, so that a user is not confused. It
allows for a more lax set of "true" values, and matches expectations of
other configuration values in git.

Jacob Keller (2):
config: accept value-less keys in configuration files
config: parse booleans similar to git-config

stgit/commands/pull.py | 2 +-
stgit/commands/refresh.py | 2 +-
stgit/config.py | 30 +++++++++++++++++++++++++++++-
stgit/git.py | 2 +-
stgit/lib/transaction.py | 2 +-
t/t6000-config.sh | 19 +++++++++++++++++++
6 files changed, 52 insertions(+), 5 deletions(-)
create mode 100755 t/t6000-config.sh

--
2.15.0

[Jacob Keller]

Currently, stgit has three "boolean" configuration values which we
support, "autoimerge", "keepoptimized" and "refreshsubmodules". However,
we only support "yes" and non-"yes" as valid values. (i.e. either the
value is "yes" and it is deemed true, or it is not "yes").

The standard for git config is to use the "--bool" parameter, which
interprets configuration values as boolean literals:

"yes", "on", "true" and any positive number are true
"no", "off", "false", zero, and the empty string are false

Additionally, a key defined without "= <value>" is taken as true.

Since this is the standard method for git, add a new getbool function
for the config module, which will parse values in this way.

We'll now use this getbool() method for each of these values, ensuring
that these boolean configurations are treated similar to proper git
configuration.

Note that we do not use get() directly in implementing getbool(). This
is because we need to distinguish the case of a defined key with an
undefined value to be True. Since get() converts KeyError exceptions
into None, we would not be able to distinguish the cases of an undefined
key (which should be False) with a key that has an undefined value
(which should be True).

I thought to add new tests for these, but since we don't directly expose
the config.py module, and I don't see an easy way to add tests for
python code explicitly

Signed-off-by: Jacob Keller <jacob.e...@intel.com>
---

stgit/commands/pull.py | 2 +-
stgit/commands/refresh.py | 2 +-

stgit/config.py | 24 ++++++++++++++++++++++++

stgit/git.py | 2 +-
stgit/lib/transaction.py | 2 +-

5 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/stgit/commands/pull.py b/stgit/commands/pull.py
index 75a883444577..ed268eeb998b 100644
--- a/stgit/commands/pull.py
+++ b/stgit/commands/pull.py
@@ -114,7 +114,7 @@ def func(parser, options, args):
post_rebase(crt_series, applied, options.nopush, options.merged)

# maybe tidy up
- if config.get('stgit.keepoptimized') == 'yes':
+ if config.getbool('stgit.keepoptimized'):
git.repack()

print_crt_patch(crt_series)
diff --git a/stgit/commands/refresh.py b/stgit/commands/refresh.py
index 631167cf5445..b780ba397460 100644
--- a/stgit/commands/refresh.py
+++ b/stgit/commands/refresh.py
@@ -270,7 +270,7 @@ def func(parser, options, args):
# If submodules was not specified on the command line, infer a default
# from configuration.
if options.submodules is None:
- options.submodules = (config.get('stgit.refreshsubmodules') == 'yes')
+ options.submodules = (config.getbool('stgit.refreshsubmodules'))

stack = directory.repository.current_stack
patch_name = get_patch(stack, options.patch)
diff --git a/stgit/config.py b/stgit/config.py
index b9df20a8c635..8ac53d64558b 100644
--- a/stgit/config.py
+++ b/stgit/config.py
@@ -91,6 +91,30 @@ class GitConfig(object):
raise GitConfigException('Value for "%s" is not an integer: "%s"' %
(name, value))

+ def getbool(self, name):
+ """Report the canonicalized boolean value for a given key."""
+ # We cannot directly call get() because we need to use the KeyError in
+ # order to distinguish between the case of a key with an undefined
+ # value, and a completely undefined key. Git expects the former to be
+ # reported as "true".
+ self.load()
+ try:
+ value = self.__cache[name][-1]
+ except KeyError:
+ return None
+ if value is None:
+ # The key is defined, but the value is not, so treat it as true.
+ return True
+ elif value in ['yes', 'on', 'true']:
+ return True
+ elif value in ['no', 'off', 'false', '']:
+ return False
+ elif value.isdigit():
+ return bool(value)
+ else:
+ raise GitConfigException('Value for "%s" is not a booleain: "%s"' %
+ (name, value))
+
def getstartswith(self, name):
self.load()
return ((n, v[-1]) for (n, v) in self.__cache.items()
diff --git a/stgit/git.py b/stgit/git.py
index 826e81f399fb..92a26fc08409 100644
--- a/stgit/git.py
+++ b/stgit/git.py
@@ -607,7 +607,7 @@ def merge_recursive(base, head1, head2):
output = p.output_lines()
if p.exitcode:
# There were conflicts
- if config.get('stgit.autoimerge') == 'yes':
+ if config.getbool('stgit.autoimerge'):
mergetool()
else:
conflicts = [l for l in output if l.startswith('CONFLICT')]
diff --git a/stgit/lib/transaction.py b/stgit/lib/transaction.py
index a19796cb5859..1afa31782071 100644
--- a/stgit/lib/transaction.py
+++ b/stgit/lib/transaction.py
@@ -362,7 +362,7 @@ class StackTransaction(object):
self.__halt('Index/worktree dirty')
try:
interactive = (allow_interactive and
- config.get('stgit.autoimerge') == 'yes')
+ config.getbool('stgit.autoimerge'))
iw.merge(base, ours, theirs, interactive = interactive)
tree = iw.index.write_tree()
self.__current_tree = tree
--
2.15.0