[PATCH 0/6] add support for repository-local .patman config file

[PATCH 0/6] add support for repository-local .patman config file

[Maxim Cournoyer]

This series is based on top of series #333345.  It adds support for a
.patman config file local to the git repository.  A new test
'test_settings.py' test module is added; it is automatically
discovered and run via pytest, e.g.: 'cd tools/patman && pytest'.


Maxim Cournoyer (6):
  patman: fix pep8 warnings in settings module
  patman: replace deprecated SafeConfigParser with ConfigParser
  patman: import gitutil module where it is needed
  patman: set the default config_fname argument value to None
  patman: fail early in Setup when provided config file does not exist
  patman: additionally honor a local .patman config file

 tools/patman/__main__.py      |   3 +-
 tools/patman/patman.rst       |   8 ++-
 tools/patman/settings.py      | 101 +++++++++++++++++++++-------------
 tools/patman/test_settings.py |  43 +++++++++++++++
 4 files changed, 115 insertions(+), 40 deletions(-)
 create mode 100644 tools/patman/test_settings.py


base-commit: 9bd3d354a1a0712ac27c717df9ad60566b0406ee
prerequisite-patch-id: 10ced6811f0468ea30ab793f3d33a43416dc148c
prerequisite-patch-id: d841542388f09c77a7b22bc7b94d72628408fd7f
prerequisite-patch-id: 414fe8a358404113a0926217fcd75150d1aabaf6
prerequisite-patch-id: 59f0b30f075c78657a17cbbb75af471e37580bdb
prerequisite-patch-id: c7a9c4f2bd34df9da0f173c61fa40bf2f89b6929
prerequisite-patch-id: bd190cf8b6da6e8b20125f957c44cc7f7ea5dd5b
prerequisite-patch-id: 26558d3671f2a854303a959f738dfb67b25f7109
prerequisite-patch-id: 19f2332545f2c338004b1728e5cb15cce32656c0
-- 
2.38.1

[PATCH 1/6] patman: fix pep8 warnings in settings module

[Maxim Cournoyer]

Remove extraneous imports, variables and comply to PEP 8 maximum line
width, among other PEP 8 changes suggested by Pyflake.

Signed-off-by: Maxim Cournoyer <maxim.cournoyer@savoirfairelinux.com>
---

 tools/patman/settings.py | 38 +++++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/tools/patman/settings.py b/tools/patman/settings.py
index 903d6fcb0b..b6884a073e 100644
--- a/tools/patman/settings.py
+++ b/tools/patman/settings.py
@@ -4,16 +4,13 @@
 
 try:
     import configparser as ConfigParser
-except:
+except Exception:
     import ConfigParser
 
 import argparse
 import os
 import re
 
-from patman import command
-from patman import tools
-
 """Default settings per-project.
 
 These are used by _ProjectConfigParser.  Settings names should match
@@ -32,6 +29,7 @@ _default_settings = {
     },
 }
 
+
 class _ProjectConfigParser(ConfigParser.SafeConfigParser):
     """ConfigParser that handles projects.
 
@@ -83,8 +81,8 @@ class _ProjectConfigParser(ConfigParser.SafeConfigParser):
     def __init__(self, project_name):
         """Construct _ProjectConfigParser.
 
-        In addition to standard SafeConfigParser initialization, this also loads
-        project defaults.
+        In addition to standard SafeConfigParser initialization, this
+        also loads project defaults.
 
         Args:
             project_name: The name of the project.
@@ -155,6 +153,7 @@ class _ProjectConfigParser(ConfigParser.SafeConfigParser):
         item_dict.update(project_items)
         return {(item, val) for item, val in item_dict.items()}
 
+
 def ReadGitAliases(fname):
     """Read a git alias file. This is in the form used by git:
 
@@ -170,7 +169,7 @@ def ReadGitAliases(fname):
         print("Warning: Cannot find alias file '%s'" % fname)
         return
 
-    re_line = re.compile('alias\s+(\S+)\s+(.*)')
+    re_line = re.compile(r'alias\s+(\S+)\s+(.*)')
     for line in fd.readlines():
         line = line.strip()
         if not line or line[0] == '#':
@@ -190,6 +189,7 @@ def ReadGitAliases(fname):
 
     fd.close()
 
+
 def CreatePatmanConfigFile(gitutil, config_fname):
     """Creates a config file under $(HOME)/.patman if it can't find one.
 
@@ -200,12 +200,12 @@ def CreatePatmanConfigFile(gitutil, config_fname):
         None
     """
     name = gitutil.get_default_user_name()
-    if name == None:
+    if name is None:
         name = input("Enter name: ")
 
     email = gitutil.get_default_user_email()
 
-    if email == None:
+    if email is None:
         email = input("Enter email: ")
 
     try:
@@ -220,7 +220,8 @@ me: %s <%s>
 [bounces]
 nxp = Zhikang Zhang <zhikang.zhang@nxp.com>
 ''' % (name, email), file=f)
-    f.close();
+    f.close()
+
 
 def _UpdateDefaults(main_parser, config):
     """Update the given OptionParser defaults based on config.
@@ -242,8 +243,8 @@ def _UpdateDefaults(main_parser, config):
     # Find all the parsers and subparsers
     parsers = [main_parser]
     parsers += [subparser for action in main_parser._actions
-                  if isinstance(action, argparse._SubParsersAction)
-                  for _, subparser in action.choices.items()]
+                if isinstance(action, argparse._SubParsersAction)
+                for _, subparser in action.choices.items()]
 
     # Collect the defaults from each parser
     defaults = {}
@@ -270,8 +271,9 @@ def _UpdateDefaults(main_parser, config):
     # Set all the defaults and manually propagate them to subparsers
     main_parser.set_defaults(**defaults)
     for parser, pdefs in zip(parsers, parser_defaults):
-        parser.set_defaults(**{ k: v for k, v in defaults.items()
-                                    if k in pdefs })
+        parser.set_defaults(**{k: v for k, v in defaults.items()
+                               if k in pdefs})
+
 
 def _ReadAliasFile(fname):
     """Read in the U-Boot git alias file if it exists.
@@ -298,6 +300,7 @@ def _ReadAliasFile(fname):
         if bad_line:
             print(bad_line)
 
+
 def _ReadBouncesFile(fname):
     """Read in the bounces file if it exists
 
@@ -311,6 +314,7 @@ def _ReadBouncesFile(fname):
                     continue
                 bounces.add(line.strip())
 
+
 def GetItems(config, section):
     """Get the items from a section of the config.
 
@@ -323,10 +327,9 @@ def GetItems(config, section):
     """
     try:
         return config.items(section)
-    except ConfigParser.NoSectionError as e:
+    except ConfigParser.NoSectionError:
         return []
-    except:
-        raise
+
 
 def Setup(gitutil, parser, project_name, config_fname=''):
     """Set up the settings module by reading config files.
@@ -358,6 +361,7 @@ def Setup(gitutil, parser, project_name, config_fname=''):
 
     _UpdateDefaults(parser, config)
 
+
 # These are the aliases we understand, indexed by alias. Each member is a list.
 alias = {}
 bounces = set()
-- 
2.38.1

[PATCH 2/6] patman: replace deprecated SafeConfigParser with ConfigParser

[Maxim Cournoyer]

The SafeConfigParser class has been renamed in Python 3.2 to
ConfigParser, and the old alias has been deprecated since.

Signed-off-by: Maxim Cournoyer <maxim.cournoyer@savoirfairelinux.com>
---

 tools/patman/settings.py | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/tools/patman/settings.py b/tools/patman/settings.py
index b6884a073e..7fb9d6d5a0 100644
--- a/tools/patman/settings.py
+++ b/tools/patman/settings.py
@@ -30,7 +30,7 @@ _default_settings = {
 }
 
 
-class _ProjectConfigParser(ConfigParser.SafeConfigParser):
+class _ProjectConfigParser(ConfigParser.ConfigParser):
     """ConfigParser that handles projects.
 
     There are two main goals of this class:
@@ -81,14 +81,14 @@ class _ProjectConfigParser(ConfigParser.SafeConfigParser):
     def __init__(self, project_name):
         """Construct _ProjectConfigParser.
 
-        In addition to standard SafeConfigParser initialization, this
-        also loads project defaults.
+        In addition to standard ConfigParser initialization, this also
+        loads project defaults.
 
         Args:
             project_name: The name of the project.
         """
         self._project_name = project_name
-        ConfigParser.SafeConfigParser.__init__(self)
+        ConfigParser.ConfigParser.__init__(self)
 
         # Update the project settings in the config based on
         # the _default_settings global.
@@ -100,31 +100,31 @@ class _ProjectConfigParser(ConfigParser.SafeConfigParser):
             self.set(project_settings, setting_name, setting_value)
 
     def get(self, section, option, *args, **kwargs):
-        """Extend SafeConfigParser to try project_section before section.
+        """Extend ConfigParser to try project_section before section.
 
         Args:
-            See SafeConfigParser.
+            See ConfigParser.
         Returns:
-            See SafeConfigParser.
+            See ConfigParser.
         """
         try:
-            val = ConfigParser.SafeConfigParser.get(
+            val = ConfigParser.ConfigParser.get(
                 self, "%s_%s" % (self._project_name, section), option,
                 *args, **kwargs
             )
         except (ConfigParser.NoSectionError, ConfigParser.NoOptionError):
-            val = ConfigParser.SafeConfigParser.get(
+            val = ConfigParser.ConfigParser.get(
                 self, section, option, *args, **kwargs
             )
         return val
 
     def items(self, section, *args, **kwargs):
-        """Extend SafeConfigParser to add project_section to section.
+        """Extend ConfigParser to add project_section to section.
 
         Args:
-            See SafeConfigParser.
+            See ConfigParser.
         Returns:
-            See SafeConfigParser.
+            See ConfigParser.
         """
         project_items = []
         has_project_section = False
@@ -132,7 +132,7 @@ class _ProjectConfigParser(ConfigParser.SafeConfigParser):
 
         # Get items from the project section
         try:
-            project_items = ConfigParser.SafeConfigParser.items(
+            project_items = ConfigParser.ConfigParser.items(
                 self, "%s_%s" % (self._project_name, section), *args, **kwargs
             )
             has_project_section = True
@@ -141,7 +141,7 @@ class _ProjectConfigParser(ConfigParser.SafeConfigParser):
 
         # Get top-level items
         try:
-            top_items = ConfigParser.SafeConfigParser.items(
+            top_items = ConfigParser.ConfigParser.items(
                 self, section, *args, **kwargs
             )
         except ConfigParser.NoSectionError:
-- 
2.38.1

[PATCH 3/6] patman: import gitutil module where it is needed

[Maxim Cournoyer]

Instead of propagating it from the module entry point (main script).

Signed-off-by: Maxim Cournoyer <maxim.cournoyer@savoirfairelinux.com>
---

 tools/patman/__main__.py | 3 +--
 tools/patman/settings.py | 8 +++++---
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/tools/patman/__main__.py b/tools/patman/__main__.py
index 11f19281fb..3f092367ec 100755
--- a/tools/patman/__main__.py
+++ b/tools/patman/__main__.py
@@ -22,7 +22,6 @@ if __name__ == "__main__":
 from patman import command
 from patman import control
 from patman import func_test
-from patman import gitutil
 from patman import project
 from patman import settings
 from patman import terminal
@@ -119,7 +118,7 @@ status.add_argument('-f', '--force', action='store_true',
 argv = sys.argv[1:]
 args, rest = parser.parse_known_args(argv)
 if hasattr(args, 'project'):
-    settings.Setup(gitutil, parser, args.project, '')
+    settings.Setup(parser, args.project, '')
     args, rest = parser.parse_known_args(argv)
 
 # If we have a command, it is safe to parse all arguments
diff --git a/tools/patman/settings.py b/tools/patman/settings.py
index 7fb9d6d5a0..5efad5ed78 100644
--- a/tools/patman/settings.py
+++ b/tools/patman/settings.py
@@ -11,6 +11,8 @@ import argparse
 import os
 import re
 
+from patman import gitutil
+
 """Default settings per-project.
 
 These are used by _ProjectConfigParser.  Settings names should match
@@ -190,7 +192,7 @@ def ReadGitAliases(fname):
     fd.close()
 
 
-def CreatePatmanConfigFile(gitutil, config_fname):
+def CreatePatmanConfigFile(config_fname):
     """Creates a config file under $(HOME)/.patman if it can't find one.
 
     Args:
@@ -331,7 +333,7 @@ def GetItems(config, section):
         return []
 
 
-def Setup(gitutil, parser, project_name, config_fname=''):
+def Setup(parser, project_name, config_fname=''):
     """Set up the settings module by reading config files.
 
     Args:
@@ -348,7 +350,7 @@ def Setup(gitutil, parser, project_name, config_fname=''):
 
     if not os.path.exists(config_fname):
         print("No config file found ~/.patman\nCreating one...\n")
-        CreatePatmanConfigFile(gitutil, config_fname)
+        CreatePatmanConfigFile(config_fname)
 
     config.read(config_fname)
 
-- 
2.38.1

[PATCH 4/6] patman: set the default config_fname argument value to None

[Maxim Cournoyer]

This better matches Python conventions, allowing to easily test
whether the optional argument is provided.

Signed-off-by: Maxim Cournoyer <maxim.cournoyer@savoirfairelinux.com>
---

 tools/patman/__main__.py | 2 +-
 tools/patman/settings.py | 9 +++++----
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/tools/patman/__main__.py b/tools/patman/__main__.py
index 3f092367ec..15fd7603d7 100755
--- a/tools/patman/__main__.py
+++ b/tools/patman/__main__.py
@@ -118,7 +118,7 @@ status.add_argument('-f', '--force', action='store_true',
 argv = sys.argv[1:]
 args, rest = parser.parse_known_args(argv)
 if hasattr(args, 'project'):
-    settings.Setup(parser, args.project, '')
+    settings.Setup(parser, args.project)
     args, rest = parser.parse_known_args(argv)
 
 # If we have a command, it is safe to parse all arguments
diff --git a/tools/patman/settings.py b/tools/patman/settings.py
index 5efad5ed78..8b846799df 100644
--- a/tools/patman/settings.py
+++ b/tools/patman/settings.py
@@ -333,19 +333,20 @@ def GetItems(config, section):
         return []
 
 
-def Setup(parser, project_name, config_fname=''):
+def Setup(parser, project_name, config_fname=None):
     """Set up the settings module by reading config files.
 
     Args:
-        parser:         The parser to update
+        parser:         The parser to update.
         project_name:   Name of project that we're working on; we'll look
             for sections named "project_section" as well.
-        config_fname:   Config filename to read ('' for default)
+        config_fname:   Config filename to read.
     """
     # First read the git alias file if available
     _ReadAliasFile('doc/git-mailrc')
     config = _ProjectConfigParser(project_name)
-    if config_fname == '':
+
+    if not config_fname:
         config_fname = '%s/.patman' % os.getenv('HOME')
 
     if not os.path.exists(config_fname):
-- 
2.38.1

[PATCH 5/6] patman: fail early in Setup when provided config file does not exist

[Maxim Cournoyer]

Rationale: if the user explicitly provide this argument, they probably
intend for it to be used.

Signed-off-by: Maxim Cournoyer <maxim.cournoyer@savoirfairelinux.com>
---

 tools/patman/settings.py | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tools/patman/settings.py b/tools/patman/settings.py
index 8b846799df..c05efd2475 100644
--- a/tools/patman/settings.py
+++ b/tools/patman/settings.py
@@ -340,12 +340,16 @@ def Setup(parser, project_name, config_fname=None):
         parser:         The parser to update.
         project_name:   Name of project that we're working on; we'll look
             for sections named "project_section" as well.
-        config_fname:   Config filename to read.
+        config_fname:   Config filename to read.  An error is raised if it
+            does not exist.
     """
     # First read the git alias file if available
     _ReadAliasFile('doc/git-mailrc')
     config = _ProjectConfigParser(project_name)
 
+    if config_fname and not os.path.exists(config_fname):
+        raise Exception(f'provided {config_fname} does not exist')
+
     if not config_fname:
         config_fname = '%s/.patman' % os.getenv('HOME')
 
-- 
2.38.1

[PATCH 6/6] patman: additionally honor a local .patman config file

[Maxim Cournoyer]

This enables versioning a project specific patman configuration file.
It also makes it possible to declare the the project name is,
which is not a useful thing to do in $HOME/.patman.  A new test is
added, along updated documentation.

Signed-off-by: Maxim Cournoyer <maxim.cournoyer@savoirfairelinux.com>
---

 tools/patman/patman.rst       |  8 ++++++-
 tools/patman/settings.py      | 24 +++++++++++++++----
 tools/patman/test_settings.py | 43 +++++++++++++++++++++++++++++++++++
 3 files changed, 70 insertions(+), 5 deletions(-)
 create mode 100644 tools/patman/test_settings.py

diff --git a/tools/patman/patman.rst b/tools/patman/patman.rst
index b06399b459..02c08179af 100644
--- a/tools/patman/patman.rst
+++ b/tools/patman/patman.rst
@@ -74,7 +74,7 @@ out where to send patches pretty well.
 During the first run patman creates a config file for you by taking the default
 user name and email address from the global .gitconfig file.
 
-To add your own, create a file ~/.patman like this::
+To add your own, create a file `~/.patman` like this::
 
     # patman alias file
 
@@ -85,6 +85,12 @@ To add your own, create a file ~/.patman like this::
     wolfgang: Wolfgang Denk <wd@denx.de>
     others: Mike Frysinger <vapier@gentoo.org>, Fred Bloggs <f.bloggs@napier.net>
 
+Patman will also look for a `.patman` configuration file at the root
+of the current project git repository, which makes it possible to
+override the `project` settings variable or anything else in a
+project-specific way. The values of this "local" configuration file
+take precedence over those of the "global" one.
+
 Aliases are recursive.
 
 The checkpatch.pl in the U-Boot tools/ subdirectory will be located and
diff --git a/tools/patman/settings.py b/tools/patman/settings.py
index c05efd2475..636983e32d 100644
--- a/tools/patman/settings.py
+++ b/tools/patman/settings.py
@@ -1,5 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0+
 # Copyright (c) 2011 The Chromium OS Authors.
+# Copyright (c) 2022 Maxim Cournoyer <maxim.cournoyer@savoirfairelinux.com>
 #
 
 try:
@@ -336,6 +337,12 @@ def GetItems(config, section):
 def Setup(parser, project_name, config_fname=None):
     """Set up the settings module by reading config files.
 
+    Unless `config_fname` is specified, a `.patman` config file local
+    to the git repository is consulted, followed by the global
+    `$HOME/.patman`. If none exists, the later is created. Values
+    defined in the local config file take precedence over those
+    defined in the global one.
+
     Args:
         parser:         The parser to update.
         project_name:   Name of project that we're working on; we'll look
@@ -352,12 +359,21 @@ def Setup(parser, project_name, config_fname=None):
 
     if not config_fname:
         config_fname = '%s/.patman' % os.getenv('HOME')
+    has_config = os.path.exists(config_fname)
 
-    if not os.path.exists(config_fname):
-        print("No config file found ~/.patman\nCreating one...\n")
-        CreatePatmanConfigFile(config_fname)
+    git_local_config_fname = os.path.join(gitutil.get_top_level(), '.patman')
+    has_git_local_config = os.path.exists(git_local_config_fname)
 
-    config.read(config_fname)
+    # Read the git local config last, so that its values override
+    # those of the global config, if any.
+    if has_config:
+        config.read(config_fname)
+    if has_git_local_config:
+        config.read(git_local_config_fname)
+
+    if not (has_config or has_git_local_config):
+        print("No config file found.\nCreating ~/.patman...\n")
+        CreatePatmanConfigFile(config_fname)
 
     for name, value in GetItems(config, 'alias'):
         alias[name] = value.split(',')
diff --git a/tools/patman/test_settings.py b/tools/patman/test_settings.py
new file mode 100644
index 0000000000..9c14b4aaa3
--- /dev/null
+++ b/tools/patman/test_settings.py
@@ -0,0 +1,43 @@
+# SPDX-License-Identifier: GPL-2.0+
+#
+# Copyright (c) 2022 Maxim Cournoyer <maxim.cournoyer@savoirfairelinux.com>
+#
+
+import argparse
+import contextlib
+import os
+import subprocess
+import tempfile
+
+from patman import settings
+
+
+@contextlib.contextmanager
+def empty_git_repository():
+    with tempfile.TemporaryDirectory() as tmpdir:
+        os.chdir(tmpdir)
+        subprocess.check_call(['git', 'init'])
+        yield tmpdir
+
+
+def test_git_local_config():
+    with empty_git_repository():
+        with tempfile.NamedTemporaryFile() as global_config:
+            global_config.write(b'[settings]\n'
+                                b'project=u-boot\n')
+            global_config.flush()
+            parser = argparse.ArgumentParser()
+            parser.add_argument('-p', '--project', default='unknown')
+
+            # Test "global" config is used.
+            settings.Setup(parser, 'unknown', global_config.name)
+            args, _ = parser.parse_known_args()
+            assert args.project == 'u-boot'
+
+            # Test local config can shadow it.
+            with open('.patman', 'w', buffering=1) as f:
+                f.write('[settings]\n'
+                        'project=guix-patches\n')
+            settings.Setup(parser, 'unknown', global_config.name)
+            args, _ = parser.parse_known_args([])
+            assert args.project == 'guix-patches'
-- 
2.38.1

Re: [PATCH 1/6] patman: fix pep8 warnings in settings module

[Simon Glass]

On Mon, 19 Dec 2022 at 08:50, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:
>
> Remove extraneous imports, variables and comply to PEP 8 maximum line
> width, among other PEP 8 changes suggested by Pyflake.
>
> Signed-off-by: Maxim Cournoyer <maxim.cournoyer@savoirfairelinux.com>
> ---
>
>  tools/patman/settings.py | 38 +++++++++++++++++++++-----------------
>  1 file changed, 21 insertions(+), 17 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 2/6] patman: replace deprecated SafeConfigParser with ConfigParser

[Simon Glass]

On Mon, 19 Dec 2022 at 08:50, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:
>
> The SafeConfigParser class has been renamed in Python 3.2 to
> ConfigParser, and the old alias has been deprecated since.
>
> Signed-off-by: Maxim Cournoyer <maxim.cournoyer@savoirfairelinux.com>
> ---
>
>  tools/patman/settings.py | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 3/6] patman: import gitutil module where it is needed

[Simon Glass]

On Mon, 19 Dec 2022 at 08:50, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:
>
> Instead of propagating it from the module entry point (main script).
>
> Signed-off-by: Maxim Cournoyer <maxim.cournoyer@savoirfairelinux.com>
> ---
>
>  tools/patman/__main__.py | 3 +--
>  tools/patman/settings.py | 8 +++++---
>  2 files changed, 6 insertions(+), 5 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 4/6] patman: set the default config_fname argument value to None

[Simon Glass]

On Mon, 19 Dec 2022 at 08:50, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:
>
> This better matches Python conventions, allowing to easily test
> whether the optional argument is provided.
>
> Signed-off-by: Maxim Cournoyer <maxim.cournoyer@savoirfairelinux.com>
> ---
>
>  tools/patman/__main__.py | 2 +-
>  tools/patman/settings.py | 9 +++++----
>  2 files changed, 6 insertions(+), 5 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 5/6] patman: fail early in Setup when provided config file does not exist

[Simon Glass]

On Mon, 19 Dec 2022 at 08:50, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:
>
> Rationale: if the user explicitly provide this argument, they probably
> intend for it to be used.
>
> Signed-off-by: Maxim Cournoyer <maxim.cournoyer@savoirfairelinux.com>
> ---
>
>  tools/patman/settings.py | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 6/6] patman: additionally honor a local .patman config file

[Simon Glass]

Hi Maxim,

On Mon, 19 Dec 2022 at 08:50, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:
>
> This enables versioning a project specific patman configuration file.
> It also makes it possible to declare the the project name is,
> which is not a useful thing to do in $HOME/.patman.  A new test is
> added, along updated documentation.
>
> Signed-off-by: Maxim Cournoyer <maxim.cournoyer@savoirfairelinux.com>
> ---
>
>  tools/patman/patman.rst       |  8 ++++++-
>  tools/patman/settings.py      | 24 +++++++++++++++----
>  tools/patman/test_settings.py | 43 +++++++++++++++++++++++++++++++++++
>  3 files changed, 70 insertions(+), 5 deletions(-)
>  create mode 100644 tools/patman/test_settings.py
>
> diff --git a/tools/patman/patman.rst b/tools/patman/patman.rst
> index b06399b459..02c08179af 100644
> --- a/tools/patman/patman.rst
> +++ b/tools/patman/patman.rst
> @@ -74,7 +74,7 @@ out where to send patches pretty well.
>  During the first run patman creates a config file for you by taking the default
>  user name and email address from the global .gitconfig file.
>
> -To add your own, create a file ~/.patman like this::
> +To add your own, create a file `~/.patman` like this::
>
>      # patman alias file
>
> @@ -85,6 +85,12 @@ To add your own, create a file ~/.patman like this::
>      wolfgang: Wolfgang Denk <wd@denx.de>
>      others: Mike Frysinger <vapier@gentoo.org>, Fred Bloggs <f.bloggs@napier.net>
>
> +Patman will also look for a `.patman` configuration file at the root
> +of the current project git repository, which makes it possible to
> +override the `project` settings variable or anything else in a
> +project-specific way. The values of this "local" configuration file
> +take precedence over those of the "global" one.
> +
>  Aliases are recursive.
>
>  The checkpatch.pl in the U-Boot tools/ subdirectory will be located and
> diff --git a/tools/patman/settings.py b/tools/patman/settings.py
> index c05efd2475..636983e32d 100644
> --- a/tools/patman/settings.py
> +++ b/tools/patman/settings.py
> @@ -1,5 +1,6 @@
>  # SPDX-License-Identifier: GPL-2.0+
>  # Copyright (c) 2011 The Chromium OS Authors.
> +# Copyright (c) 2022 Maxim Cournoyer <maxim.cournoyer@savoirfairelinux.com>
>  #
>
>  try:
> @@ -336,6 +337,12 @@ def GetItems(config, section):
>  def Setup(parser, project_name, config_fname=None):
>      """Set up the settings module by reading config files.
>
> +    Unless `config_fname` is specified, a `.patman` config file local
> +    to the git repository is consulted, followed by the global
> +    `$HOME/.patman`. If none exists, the later is created. Values
> +    defined in the local config file take precedence over those
> +    defined in the global one.
> +
>      Args:
>          parser:         The parser to update.
>          project_name:   Name of project that we're working on; we'll look
> @@ -352,12 +359,21 @@ def Setup(parser, project_name, config_fname=None):
>
>      if not config_fname:
>          config_fname = '%s/.patman' % os.getenv('HOME')
> +    has_config = os.path.exists(config_fname)
>
> -    if not os.path.exists(config_fname):
> -        print("No config file found ~/.patman\nCreating one...\n")
> -        CreatePatmanConfigFile(config_fname)
> +    git_local_config_fname = os.path.join(gitutil.get_top_level(), '.patman')
> +    has_git_local_config = os.path.exists(git_local_config_fname)
>
> -    config.read(config_fname)
> +    # Read the git local config last, so that its values override
> +    # those of the global config, if any.
> +    if has_config:
> +        config.read(config_fname)
> +    if has_git_local_config:
> +        config.read(git_local_config_fname)
> +
> +    if not (has_config or has_git_local_config):
> +        print("No config file found.\nCreating ~/.patman...\n")
> +        CreatePatmanConfigFile(config_fname)
>
>      for name, value in GetItems(config, 'alias'):
>          alias[name] = value.split(',')
> diff --git a/tools/patman/test_settings.py b/tools/patman/test_settings.py
> new file mode 100644
> index 0000000000..9c14b4aaa3
> --- /dev/null
> +++ b/tools/patman/test_settings.py
> @@ -0,0 +1,43 @@
> +# SPDX-License-Identifier: GPL-2.0+
> +#
> +# Copyright (c) 2022 Maxim Cournoyer <maxim.cournoyer@savoirfairelinux.com>
> +#
> +
> +import argparse
> +import contextlib
> +import os
> +import subprocess
> +import tempfile
> +
> +from patman import settings
> +
> +
> +@contextlib.contextmanager
> +def empty_git_repository():
> +    with tempfile.TemporaryDirectory() as tmpdir:
> +        os.chdir(tmpdir)
> +        subprocess.check_call(['git', 'init'])

We normally use tools.run()

> +        yield tmpdir
> +
> +
> +def test_git_local_config():
> +    with empty_git_repository():
> +        with tempfile.NamedTemporaryFile() as global_config:
> +            global_config.write(b'[settings]\n'
> +                                b'project=u-boot\n')
> +            global_config.flush()
> +            parser = argparse.ArgumentParser()
> +            parser.add_argument('-p', '--project', default='unknown')
> +
> +            # Test "global" config is used.
> +            settings.Setup(parser, 'unknown', global_config.name)
> +            args, _ = parser.parse_known_args()
> +            assert args.project == 'u-boot'
> +
> +            # Test local config can shadow it.
> +            with open('.patman', 'w', buffering=1) as f:

Can this be created in the temporary dir? At present it looks like it
might overwrite a file in the current dir?

> +                f.write('[settings]\n'
> +                        'project=guix-patches\n')
> +            settings.Setup(parser, 'unknown', global_config.name)
> +            args, _ = parser.parse_known_args([])
> +            assert args.project == 'guix-patches'
> --
> 2.38.1
>

Regards,
Simon

Re: [PATCH 6/6] patman: additionally honor a local .patman config file

[Maxim Cournoyer]

Hi Simon,

Simon Glass <sjg@chromium.org> writes:

> Hi Maxim,
>
> On Mon, 19 Dec 2022 at 08:50, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:
>>
>> This enables versioning a project specific patman configuration file.
>> It also makes it possible to declare the the project name is,
>> which is not a useful thing to do in $HOME/.patman.  A new test is
>> added, along updated documentation.
>>
>> Signed-off-by: Maxim Cournoyer <maxim.cournoyer@savoirfairelinux.com>
>> ---
>>
>>  tools/patman/patman.rst       |  8 ++++++-
>>  tools/patman/settings.py      | 24 +++++++++++++++----
>>  tools/patman/test_settings.py | 43 +++++++++++++++++++++++++++++++++++
>>  3 files changed, 70 insertions(+), 5 deletions(-)
>>  create mode 100644 tools/patman/test_settings.py

[...]

>> diff --git a/tools/patman/test_settings.py b/tools/patman/test_settings.py
>> new file mode 100644
>> index 0000000000..9c14b4aaa3
>> --- /dev/null
>> +++ b/tools/patman/test_settings.py
>> @@ -0,0 +1,43 @@
>> +# SPDX-License-Identifier: GPL-2.0+
>> +#
>> +# Copyright (c) 2022 Maxim Cournoyer <maxim.cournoyer@savoirfairelinux.com>
>> +#
>> +
>> +import argparse
>> +import contextlib
>> +import os
>> +import subprocess
>> +import tempfile
>> +
>> +from patman import settings
>> +
>> +
>> +@contextlib.contextmanager
>> +def empty_git_repository():
>> +    with tempfile.TemporaryDirectory() as tmpdir:
>> +        os.chdir(tmpdir)
>> +        subprocess.check_call(['git', 'init'])
>
> We normally use tools.run()

Adjusted like so:

--8<---------------cut here---------------start------------->8---
modified   tools/patman/test_settings.py
@@ -6,18 +6,18 @@
 import argparse
 import contextlib
 import os
-import subprocess
 import sys
 import tempfile
 
 from patman import settings
+from patman import tools
 
 
 @contextlib.contextmanager
 def empty_git_repository():
     with tempfile.TemporaryDirectory() as tmpdir:
         os.chdir(tmpdir)
-        subprocess.check_call(['git', 'init'])
+        tools.run('git', 'init', raise_on_error=True)
         yield tmpdir
--8<---------------cut here---------------end--------------->8---

>> +        yield tmpdir
>> +
>> +
>> +def test_git_local_config():
>> +    with empty_git_repository():
>> +        with tempfile.NamedTemporaryFile() as global_config:
>> +            global_config.write(b'[settings]\n'
>> +                                b'project=u-boot\n')
>> +            global_config.flush()
>> +            parser = argparse.ArgumentParser()
>> +            parser.add_argument('-p', '--project', default='unknown')
>> +
>> +            # Test "global" config is used.
>> +            settings.Setup(parser, 'unknown', global_config.name)
>> +            args, _ = parser.parse_known_args()
>> +            assert args.project == 'u-boot'
>> +
>> +            # Test local config can shadow it.
>> +            with open('.patman', 'w', buffering=1) as f:
>
> Can this be created in the temporary dir? At present it looks like it
> might overwrite a file in the current dir?

The 'empty_git_repository' context manager chdir to a temporary
directory upon entry, so anything that runs in its block such as the
open call above will be executed in that temporary directory.  See the
'os.chdir(tmpdir)' line above.

I sent a v4 with the above change.

-- 
Thanks,
Maxim