[PATCH v2 0/6] patman: Small fixes plus remove --no-tree from checkpatch for linux

[PATCH v2 0/6] patman: Small fixes plus remove --no-tree from checkpatch for linux

[Douglas Anderson]

The whole point of this series is really to make it so that when we're
using patman for sending Linux patches that we don't pass "--no-tree"
to checkpatch. While doing that, though, I found a number of bugs
including an explanation about why recent version of patman have been
yelling about "tags" when used with Linux even though Linux is
supposed to have "process_tags" defaulted to False.

Changes in v2:
- Fix doc string for --ignore-bad-tags
- Make comment about parsing three times less nonsensical.

Douglas Anderson (6):
  patman: Fix updating argument defaults from settings
  patman: Fix implicit command inserting
  patman: Don't look at sys.argv when parsing settings
  patman: Make most bool arguments BooleanOptionalAction
  patman: By default don't pass "--no-tree" to checkpatch for linux
  patman: Take project defaults into account for --help

 tools/patman/checkpatch.py | 11 ++++--
 tools/patman/control.py    |  7 ++--
 tools/patman/main.py       | 77 ++++++++++++++++++++++----------------
 tools/patman/settings.py   | 46 ++++++++++++-----------
 4 files changed, 80 insertions(+), 61 deletions(-)

-- 
2.37.0.rc0.161.g10f37bed90-goog

[PATCH v2 1/6] patman: Fix updating argument defaults from settings

[Douglas Anderson]

Ever since commit 4600767d294d ("patman: Refactor how the default
subcommand works"), when I use patman on the Linux tree I get grumbles
about unknown tags. This is because the Linux default making
process_tags be False wasn't working anymore.

It appears that the comment claiming that the defaults propagates
through all subparsers no longer works for some reason.

We're already looking at all the subparsers anyway. Let's just update
each one.

Fixes: 4600767d294d ("patman: Refactor how the default subcommand works")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>
Reviewed-by: Brian Norris <briannorris@chromium.org>
---

(no changes since v1)

 tools/patman/settings.py | 41 +++++++++++++++++++++-------------------
 1 file changed, 22 insertions(+), 19 deletions(-)

diff --git a/tools/patman/settings.py b/tools/patman/settings.py
index 7c2b5c196c06..5eefe3d1f55e 100644
--- a/tools/patman/settings.py
+++ b/tools/patman/settings.py
@@ -244,28 +244,31 @@ def _UpdateDefaults(main_parser, config):
                   if isinstance(action, argparse._SubParsersAction)
                   for _, subparser in action.choices.items()]
 
+    unknown_settings = set(name for name, val in config.items('settings'))
+
     # Collect the defaults from each parser
-    defaults = {}
     for parser in parsers:
         pdefs = parser.parse_known_args()[0]
-        defaults.update(vars(pdefs))
-
-    # Go through the settings and collect defaults
-    for name, val in config.items('settings'):
-        if name in defaults:
-            default_val = defaults[name]
-            if isinstance(default_val, bool):
-                val = config.getboolean('settings', name)
-            elif isinstance(default_val, int):
-                val = config.getint('settings', name)
-            elif isinstance(default_val, str):
-                val = config.get('settings', name)
-            defaults[name] = val
-        else:
-            print("WARNING: Unknown setting %s" % name)
-
-    # Set all the defaults (this propagates through all subparsers)
-    main_parser.set_defaults(**defaults)
+        defaults = dict(vars(pdefs))
+
+        # Go through the settings and collect defaults
+        for name, val in config.items('settings'):
+            if name in defaults:
+                default_val = defaults[name]
+                if isinstance(default_val, bool):
+                    val = config.getboolean('settings', name)
+                elif isinstance(default_val, int):
+                    val = config.getint('settings', name)
+                elif isinstance(default_val, str):
+                    val = config.get('settings', name)
+                defaults[name] = val
+                unknown_settings.discard(name)
+
+        # Set all the defaults
+        parser.set_defaults(**defaults)
+
+    for name in sorted(unknown_settings):
+        print("WARNING: Unknown setting %s" % name)
 
 def _ReadAliasFile(fname):
     """Read in the U-Boot git alias file if it exists.
-- 
2.37.0.rc0.161.g10f37bed90-goog

[PATCH v2 2/6] patman: Fix implicit command inserting

[Douglas Anderson]

The logic to insert an implicit command has always been a bit broken
but it was masked by another bug fixed in the patch ("patman: Don't
look at sys.argv when parsing settings"). Specifically, imagine that
you're just calling patman like this:

  patman -c1

After the parse_known_args() command then the "-c1" will have been
parsed and we'll have no command. The "rest" variable will be an empty
list. Going into the logic you can see that nargs = 0. The implicit
insertion of send ideally would create an argument list of:
  ['-c1', 'send']
...but it doesn't because argv[:-0] is the same as argv[:0] and that's
an empty list.

Let's fix this little glitch.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>
Reviewed-by: Brian Norris <briannorris@chromium.org>
---

(no changes since v1)

 tools/patman/main.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/patman/main.py b/tools/patman/main.py
index 2a2ac4570931..336f4e439aa9 100755
--- a/tools/patman/main.py
+++ b/tools/patman/main.py
@@ -120,7 +120,9 @@ else:
     # No command, so insert it after the known arguments and before the ones
     # that presumably relate to the 'send' subcommand
     nargs = len(rest)
-    argv = argv[:-nargs] + ['send'] + rest
+    if nargs:
+        argv = argv[:-nargs]
+    argv = argv + ['send'] + rest
     args = parser.parse_args(argv)
 
 if __name__ != "__main__":
-- 
2.37.0.rc0.161.g10f37bed90-goog

[PATCH v2 3/6] patman: Don't look at sys.argv when parsing settings

[Douglas Anderson]

If you call the parser and tell it to parse but don't pass arguments
in then it will default to looking at sys.argv. This isn't really what
was intended and seems to have some side effects. Let's not do it.

NOTE: to see some of the side effects, note that this patch breaks
"patman -c1" if you don't have the patch ("patman: Fix implicit
command inserting") before it.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>
Reviewed-by: Brian Norris <briannorris@chromium.org>
---

(no changes since v1)

 tools/patman/settings.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/patman/settings.py b/tools/patman/settings.py
index 5eefe3d1f55e..92d82d5e8682 100644
--- a/tools/patman/settings.py
+++ b/tools/patman/settings.py
@@ -248,7 +248,7 @@ def _UpdateDefaults(main_parser, config):
 
     # Collect the defaults from each parser
     for parser in parsers:
-        pdefs = parser.parse_known_args()[0]
+        pdefs = parser.parse_known_args([])[0]
         defaults = dict(vars(pdefs))
 
         # Go through the settings and collect defaults
-- 
2.37.0.rc0.161.g10f37bed90-goog

[PATCH v2 4/6] patman: Make most bool arguments BooleanOptionalAction

[Douglas Anderson]

For boolean arguments it's convenient to be able to specify both the
argument and its opposite on the command line. This is especially
convenient because you can change the default via the settings file
and being able express the opposite can be the only way to override
things.

Luckily python handles this well--we just need to specify things with
BooleanOptionalAction. We'll do that for all options except
"full-help" (where it feels silly). This uglifies the help text a
little bit but does give maximum flexibility.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>
Reviewed-by: Brian Norris <briannorris@chromium.org>
---

Changes in v2:
- Fix doc string for --ignore-bad-tags

 tools/patman/main.py | 55 ++++++++++++++++++++++----------------------
 1 file changed, 28 insertions(+), 27 deletions(-)

diff --git a/tools/patman/main.py b/tools/patman/main.py
index 336f4e439aa9..66424f15c204 100755
--- a/tools/patman/main.py
+++ b/tools/patman/main.py
@@ -6,7 +6,7 @@
 
 """See README for more information"""
 
-from argparse import ArgumentParser
+from argparse import ArgumentParser, BooleanOptionalAction
 import os
 import re
 import shutil
@@ -40,7 +40,7 @@ parser.add_argument('-c', '--count', dest='count', type=int,
     default=-1, help='Automatically create patches from top n commits')
 parser.add_argument('-e', '--end', type=int, default=0,
     help='Commits to skip at end of patch list')
-parser.add_argument('-D', '--debug', action='store_true',
+parser.add_argument('-D', '--debug', action=BooleanOptionalAction,
     help='Enabling debugging (provides a full traceback on error)')
 parser.add_argument('-p', '--project', default=project.detect_project(),
                     help="Project name; affects default option values and "
@@ -50,42 +50,43 @@ parser.add_argument('-P', '--patchwork-url',
                     help='URL of patchwork server [default: %(default)s]')
 parser.add_argument('-s', '--start', dest='start', type=int,
     default=0, help='Commit to start creating patches from (0 = HEAD)')
-parser.add_argument('-v', '--verbose', action='store_true', dest='verbose',
-                    default=False, help='Verbose output of errors and warnings')
+parser.add_argument('-v', '--verbose', action=BooleanOptionalAction,
+                    dest='verbose', default=False,
+                    help='Verbose output of errors and warnings')
 parser.add_argument('-H', '--full-help', action='store_true', dest='full_help',
                     default=False, help='Display the README file')
 
 subparsers = parser.add_subparsers(dest='cmd')
 send = subparsers.add_parser('send')
-send.add_argument('-i', '--ignore-errors', action='store_true',
-       dest='ignore_errors', default=False,
-       help='Send patches email even if patch errors are found')
+send.add_argument('-i', '--ignore-errors', action=BooleanOptionalAction,
+                  dest='ignore_errors', default=False,
+                  help='Send patches email even if patch errors are found')
 send.add_argument('-l', '--limit-cc', dest='limit', type=int, default=None,
        help='Limit the cc list to LIMIT entries [default: %(default)s]')
-send.add_argument('-m', '--no-maintainers', action='store_false',
-       dest='add_maintainers', default=True,
-       help="Don't cc the file maintainers automatically")
-send.add_argument('-n', '--dry-run', action='store_true', dest='dry_run',
-       default=False, help="Do a dry run (create but don't email patches)")
+send.add_argument('-m', '--maintainers', action=BooleanOptionalAction,
+                  dest='add_maintainers', default=True,
+                  help="cc the file maintainers automatically")
+send.add_argument('-n', '--dry-run', action=BooleanOptionalAction,
+                  dest='dry_run', default=False,
+                  help="Do a dry run (create but don't email patches)")
 send.add_argument('-r', '--in-reply-to', type=str, action='store',
                   help="Message ID that this series is in reply to")
-send.add_argument('-t', '--ignore-bad-tags', action='store_true',
-                  default=False,
-                  help='Ignore bad tags / aliases (default=warn)')
-send.add_argument('-T', '--thread', action='store_true', dest='thread',
+send.add_argument('-t', '--ignore-bad-tags', action=BooleanOptionalAction,
+                  default=False, help='Ignore bad tags / aliases')
+send.add_argument('-T', '--thread', action=BooleanOptionalAction, dest='thread',
                   default=False, help='Create patches as a single thread')
 send.add_argument('--cc-cmd', dest='cc_cmd', type=str, action='store',
        default=None, help='Output cc list for patch file (used by git)')
-send.add_argument('--no-binary', action='store_true', dest='ignore_binary',
-                  default=False,
-                  help="Do not output contents of changes in binary files")
-send.add_argument('--no-check', action='store_false', dest='check_patch',
+send.add_argument('--binary', action=BooleanOptionalAction,
+                  dest='ignore_binary', default=False,
+                  help="Output contents of changes in binary files")
+send.add_argument('--check', action=BooleanOptionalAction, dest='check_patch',
                   default=True,
-                  help="Don't check for patch compliance")
-send.add_argument('--no-tags', action='store_false', dest='process_tags',
-                  default=True, help="Don't process subject tags as aliases")
-send.add_argument('--no-signoff', action='store_false', dest='add_signoff',
-                  default=True, help="Don't add Signed-off-by to patches")
+                  help="Check for patch compliance")
+send.add_argument('--tags', action=BooleanOptionalAction, dest='process_tags',
+                  default=True, help="Process subject tags as aliases")
+send.add_argument('--signoff', action=BooleanOptionalAction, dest='add_signoff',
+                  default=True, help="Add Signed-off-by to patches")
 send.add_argument('--smtp-server', type=str,
                   help="Specify the SMTP server to 'git send-email'")
 
@@ -97,11 +98,11 @@ test_parser.add_argument('testname', type=str, default=None, nargs='?',
 
 status = subparsers.add_parser('status',
                                help='Check status of patches in patchwork')
-status.add_argument('-C', '--show-comments', action='store_true',
+status.add_argument('-C', '--show-comments', action=BooleanOptionalAction,
                     help='Show comments from each patch')
 status.add_argument('-d', '--dest-branch', type=str,
                     help='Name of branch to create with collected responses')
-status.add_argument('-f', '--force', action='store_true',
+status.add_argument('-f', '--force', action=BooleanOptionalAction,
                     help='Force overwriting an existing branch')
 
 # Parse options twice: first to get the project and second to handle
-- 
2.37.0.rc0.161.g10f37bed90-goog

[PATCH v2 5/6] patman: By default don't pass "--no-tree" to checkpatch for linux

[Douglas Anderson]

When you pass "--no-tree" to checkpatch it disables some extra checks
that are important for Linux. Specifically I want checks like:

  warning: DT compatible string "boogie,woogie" appears un-documented
  check ./Documentation/devicetree/bindings/

Let's make the default for Linux to _not_ pass --no-tree. We'll have a
config option and command line flag to override.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>
Reviewed-by: Brian Norris <briannorris@chromium.org>
---

(no changes since v1)

 tools/patman/checkpatch.py | 11 +++++++----
 tools/patman/control.py    |  7 ++++---
 tools/patman/main.py       |  3 +++
 tools/patman/settings.py   |  3 ++-
 4 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/tools/patman/checkpatch.py b/tools/patman/checkpatch.py
index 70ba561c2686..d1b902dd9627 100644
--- a/tools/patman/checkpatch.py
+++ b/tools/patman/checkpatch.py
@@ -186,7 +186,7 @@ def check_patch_parse(checkpatch_output, verbose=False):
     return result
 
 
-def check_patch(fname, verbose=False, show_types=False):
+def check_patch(fname, verbose=False, show_types=False, use_tree=False):
     """Run checkpatch.pl on a file and parse the results.
 
     Args:
@@ -194,6 +194,7 @@ def check_patch(fname, verbose=False, show_types=False):
         verbose: True to print out every line of the checkpatch output as it is
             parsed
         show_types: Tell checkpatch to show the type (number) of each message
+        use_tree (bool): If False we'll pass '--no-tree' to checkpatch.
 
     Returns:
         namedtuple containing:
@@ -210,7 +211,9 @@ def check_patch(fname, verbose=False, show_types=False):
             stdout: Full output of checkpatch
     """
     chk = find_check_patch()
-    args = [chk, '--no-tree']
+    args = [chk]
+    if not use_tree:
+        args.append('--no-tree')
     if show_types:
         args.append('--show-types')
     output = command.output(*args, fname, raise_on_error=False)
@@ -236,13 +239,13 @@ def get_warning_msg(col, msg_type, fname, line, msg):
     line_str = '' if line is None else '%d' % line
     return '%s:%s: %s: %s\n' % (fname, line_str, msg_type, msg)
 
-def check_patches(verbose, args):
+def check_patches(verbose, args, use_tree):
     '''Run the checkpatch.pl script on each patch'''
     error_count, warning_count, check_count = 0, 0, 0
     col = terminal.Color()
 
     for fname in args:
-        result = check_patch(fname, verbose)
+        result = check_patch(fname, verbose, use_tree=use_tree)
         if not result.ok:
             error_count += result.errors
             warning_count += result.warnings
diff --git a/tools/patman/control.py b/tools/patman/control.py
index b40382388e07..bf426cf7bcf4 100644
--- a/tools/patman/control.py
+++ b/tools/patman/control.py
@@ -64,7 +64,7 @@ def prepare_patches(col, branch, count, start, end, ignore_binary, signoff):
         patchstream.insert_cover_letter(cover_fname, series, to_do)
     return series, cover_fname, patch_files
 
-def check_patches(series, patch_files, run_checkpatch, verbose):
+def check_patches(series, patch_files, run_checkpatch, verbose, use_tree):
     """Run some checks on a set of patches
 
     This santiy-checks the patman tags like Series-version and runs the patches
@@ -77,6 +77,7 @@ def check_patches(series, patch_files, run_checkpatch, verbose):
         run_checkpatch (bool): True to run checkpatch.pl
         verbose (bool): True to print out every line of the checkpatch output as
             it is parsed
+        use_tree (bool): If False we'll pass '--no-tree' to checkpatch.
 
     Returns:
         bool: True if the patches had no errors, False if they did
@@ -86,7 +87,7 @@ def check_patches(series, patch_files, run_checkpatch, verbose):
 
     # Check the patches, and run them through 'git am' just to be sure
     if run_checkpatch:
-        ok = checkpatch.check_patches(verbose, patch_files)
+        ok = checkpatch.check_patches(verbose, patch_files, use_tree)
     else:
         ok = True
     return ok
@@ -165,7 +166,7 @@ def send(args):
         col, args.branch, args.count, args.start, args.end,
         args.ignore_binary, args.add_signoff)
     ok = check_patches(series, patch_files, args.check_patch,
-                       args.verbose)
+                       args.verbose, args.check_patch_use_tree)
 
     ok = ok and gitutil.check_suppress_cc_config()
 
diff --git a/tools/patman/main.py b/tools/patman/main.py
index 66424f15c204..feb26f22e46c 100755
--- a/tools/patman/main.py
+++ b/tools/patman/main.py
@@ -83,6 +83,9 @@ send.add_argument('--binary', action=BooleanOptionalAction,
 send.add_argument('--check', action=BooleanOptionalAction, dest='check_patch',
                   default=True,
                   help="Check for patch compliance")
+send.add_argument('--tree', action=BooleanOptionalAction,
+                  dest='check_patch_use_tree', default=False,
+                  help="If False, pass '--no-tree' to checkpatch")
 send.add_argument('--tags', action=BooleanOptionalAction, dest='process_tags',
                   default=True, help="Process subject tags as aliases")
 send.add_argument('--signoff', action=BooleanOptionalAction, dest='add_signoff',
diff --git a/tools/patman/settings.py b/tools/patman/settings.py
index 92d82d5e8682..bbf271066cec 100644
--- a/tools/patman/settings.py
+++ b/tools/patman/settings.py
@@ -23,6 +23,7 @@ _default_settings = {
     "u-boot": {},
     "linux": {
         "process_tags": "False",
+        "check_patch_use_tree": "True",
     },
     "gcc": {
         "process_tags": "False",
@@ -71,7 +72,7 @@ class _ProjectConfigParser(ConfigParser.SafeConfigParser):
     >>> config = _ProjectConfigParser("linux")
     >>> config.readfp(StringIO(sample_config))
     >>> sorted((str(a), str(b)) for (a, b) in config.items("settings"))
-    [('am_hero', 'True'), ('process_tags', 'False')]
+    [('am_hero', 'True'), ('check_patch_use_tree', 'True'), ('process_tags', 'False')]
 
     # Check to make sure that settings works with unknown project.
     >>> config = _ProjectConfigParser("unknown")
-- 
2.37.0.rc0.161.g10f37bed90-goog

[PATCH v2 6/6] patman: Take project defaults into account for --help

[Douglas Anderson]

I'd like it so that when you do "patman send --help" and you're using
Linux that it show it the proper defaults for Linux.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>
---

Changes in v2:
- Make comment about parsing three times less nonsensical.

 tools/patman/main.py | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/tools/patman/main.py b/tools/patman/main.py
index feb26f22e46c..792124bb7555 100755
--- a/tools/patman/main.py
+++ b/tools/patman/main.py
@@ -108,14 +108,19 @@ status.add_argument('-d', '--dest-branch', type=str,
 status.add_argument('-f', '--force', action=BooleanOptionalAction,
                     help='Force overwriting an existing branch')
 
-# Parse options twice: first to get the project and second to handle
-# defaults properly (which depends on project)
-# Use parse_known_args() in case 'cmd' is omitted
+# Parse options several times:
+# - First to get the project.
+# - Second to handle defaults properly (which depends on project). This
+#   makes help display the right defaults.
+# - Finally after we have added an implicit command if necessary.
+#
+# Use parse_known_args() for the first two in case 'cmd' is omitted
+argv = [arg for arg in sys.argv[1:] if arg not in ('-h', '--help')]
+args, _ = parser.parse_known_args(argv)
 argv = sys.argv[1:]
-args, rest = parser.parse_known_args(argv)
 if hasattr(args, 'project'):
     settings.Setup(gitutil, parser, args.project, '')
-    args, rest = parser.parse_known_args(argv)
+args, rest = parser.parse_known_args(argv)
 
 # If we have a command, it is safe to parse all arguments
 if args.cmd:
-- 
2.37.0.rc0.161.g10f37bed90-goog

Re: [PATCH v2 1/6] patman: Fix updating argument defaults from settings

[Simon Glass]

On Fri, 1 Jul 2022 at 14:24, Douglas Anderson <dianders@chromium.org> wrote:
>
> Ever since commit 4600767d294d ("patman: Refactor how the default
> subcommand works"), when I use patman on the Linux tree I get grumbles
> about unknown tags. This is because the Linux default making
> process_tags be False wasn't working anymore.
>
> It appears that the comment claiming that the defaults propagates
> through all subparsers no longer works for some reason.
>
> We're already looking at all the subparsers anyway. Let's just update
> each one.
>
> Fixes: 4600767d294d ("patman: Refactor how the default subcommand works")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Tested-by: Brian Norris <briannorris@chromium.org>
> Reviewed-by: Brian Norris <briannorris@chromium.org>
> ---
>
> (no changes since v1)
>
>  tools/patman/settings.py | 41 +++++++++++++++++++++-------------------
>  1 file changed, 22 insertions(+), 19 deletions(-)

+Sean Anderson

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

Re: [PATCH v2 2/6] patman: Fix implicit command inserting

[Simon Glass]

On Fri, 1 Jul 2022 at 14:24, Douglas Anderson <dianders@chromium.org> wrote:
>
> The logic to insert an implicit command has always been a bit broken
> but it was masked by another bug fixed in the patch ("patman: Don't
> look at sys.argv when parsing settings"). Specifically, imagine that
> you're just calling patman like this:
>
>   patman -c1
>
> After the parse_known_args() command then the "-c1" will have been
> parsed and we'll have no command. The "rest" variable will be an empty
> list. Going into the logic you can see that nargs = 0. The implicit
> insertion of send ideally would create an argument list of:
>   ['-c1', 'send']
> ...but it doesn't because argv[:-0] is the same as argv[:0] and that's
> an empty list.
>
> Let's fix this little glitch.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Tested-by: Brian Norris <briannorris@chromium.org>
> Reviewed-by: Brian Norris <briannorris@chromium.org>
> ---
>
> (no changes since v1)
>
>  tools/patman/main.py | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

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

Re: [PATCH v2 3/6] patman: Don't look at sys.argv when parsing settings

[Simon Glass]

On Fri, 1 Jul 2022 at 14:24, Douglas Anderson <dianders@chromium.org> wrote:
>
> If you call the parser and tell it to parse but don't pass arguments
> in then it will default to looking at sys.argv. This isn't really what
> was intended and seems to have some side effects. Let's not do it.
>
> NOTE: to see some of the side effects, note that this patch breaks
> "patman -c1" if you don't have the patch ("patman: Fix implicit
> command inserting") before it.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Tested-by: Brian Norris <briannorris@chromium.org>
> Reviewed-by: Brian Norris <briannorris@chromium.org>
> ---
>
> (no changes since v1)
>
>  tools/patman/settings.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

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

Re: [PATCH v2 4/6] patman: Make most bool arguments BooleanOptionalAction

[Simon Glass]

On Fri, 1 Jul 2022 at 14:24, Douglas Anderson <dianders@chromium.org> wrote:
>
> For boolean arguments it's convenient to be able to specify both the
> argument and its opposite on the command line. This is especially
> convenient because you can change the default via the settings file
> and being able express the opposite can be the only way to override
> things.
>
> Luckily python handles this well--we just need to specify things with
> BooleanOptionalAction. We'll do that for all options except
> "full-help" (where it feels silly). This uglifies the help text a
> little bit but does give maximum flexibility.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Tested-by: Brian Norris <briannorris@chromium.org>
> Reviewed-by: Brian Norris <briannorris@chromium.org>
> ---
>
> Changes in v2:
> - Fix doc string for --ignore-bad-tags
>
>  tools/patman/main.py | 55 ++++++++++++++++++++++----------------------
>  1 file changed, 28 insertions(+), 27 deletions(-)

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

Re: [PATCH v2 5/6] patman: By default don't pass "--no-tree" to checkpatch for linux

[Simon Glass]

On Fri, 1 Jul 2022 at 14:24, Douglas Anderson <dianders@chromium.org> wrote:
>
> When you pass "--no-tree" to checkpatch it disables some extra checks
> that are important for Linux. Specifically I want checks like:
>
>   warning: DT compatible string "boogie,woogie" appears un-documented
>   check ./Documentation/devicetree/bindings/
>
> Let's make the default for Linux to _not_ pass --no-tree. We'll have a
> config option and command line flag to override.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Tested-by: Brian Norris <briannorris@chromium.org>
> Reviewed-by: Brian Norris <briannorris@chromium.org>
> ---
>
> (no changes since v1)
>
>  tools/patman/checkpatch.py | 11 +++++++----
>  tools/patman/control.py    |  7 ++++---
>  tools/patman/main.py       |  3 +++
>  tools/patman/settings.py   |  3 ++-
>  4 files changed, 16 insertions(+), 8 deletions(-)

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

Re: [PATCH v2 6/6] patman: Take project defaults into account for --help

[Simon Glass]

On Fri, 1 Jul 2022 at 14:24, Douglas Anderson <dianders@chromium.org> wrote:
>
> I'd like it so that when you do "patman send --help" and you're using
> Linux that it show it the proper defaults for Linux.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Tested-by: Brian Norris <briannorris@chromium.org>
> ---
>
> Changes in v2:
> - Make comment about parsing three times less nonsensical.
>
>  tools/patman/main.py | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)

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

Re: [PATCH v2 1/6] patman: Fix updating argument defaults from settings

[Sean Anderson]

Hi Doug,

On 7/1/22 4:23 PM, Douglas Anderson wrote:
> Ever since commit 4600767d294d ("patman: Refactor how the default
> subcommand works"), when I use patman on the Linux tree I get grumbles
> about unknown tags. This is because the Linux default making
> process_tags be False wasn't working anymore.
> 
> It appears that the comment claiming that the defaults propagates
> through all subparsers no longer works for some reason.
> 
> We're already looking at all the subparsers anyway. Let's just update
> each one.
> 
> Fixes: 4600767d294d ("patman: Refactor how the default subcommand works")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Tested-by: Brian Norris <briannorris@chromium.org>
> Reviewed-by: Brian Norris <briannorris@chromium.org>
> ---
> 
> (no changes since v1)
> 
>  tools/patman/settings.py | 41 +++++++++++++++++++++-------------------
>  1 file changed, 22 insertions(+), 19 deletions(-)
> 
> diff --git a/tools/patman/settings.py b/tools/patman/settings.py
> index 7c2b5c196c06..5eefe3d1f55e 100644
> --- a/tools/patman/settings.py
> +++ b/tools/patman/settings.py
> @@ -244,28 +244,31 @@ def _UpdateDefaults(main_parser, config):
>                    if isinstance(action, argparse._SubParsersAction)
>                    for _, subparser in action.choices.items()]
>  
> +    unknown_settings = set(name for name, val in config.items('settings'))
> +
>      # Collect the defaults from each parser
> -    defaults = {}
>      for parser in parsers:
>          pdefs = parser.parse_known_args()[0]
> -        defaults.update(vars(pdefs))
> -
> -    # Go through the settings and collect defaults
> -    for name, val in config.items('settings'):
> -        if name in defaults:
> -            default_val = defaults[name]
> -            if isinstance(default_val, bool):
> -                val = config.getboolean('settings', name)
> -            elif isinstance(default_val, int):
> -                val = config.getint('settings', name)
> -            elif isinstance(default_val, str):
> -                val = config.get('settings', name)
> -            defaults[name] = val
> -        else:
> -            print("WARNING: Unknown setting %s" % name)
> -
> -    # Set all the defaults (this propagates through all subparsers)
> -    main_parser.set_defaults(**defaults)
> +        defaults = dict(vars(pdefs))
> +
> +        # Go through the settings and collect defaults
> +        for name, val in config.items('settings'):
> +            if name in defaults:
> +                default_val = defaults[name]
> +                if isinstance(default_val, bool):
> +                    val = config.getboolean('settings', name)
> +                elif isinstance(default_val, int):
> +                    val = config.getint('settings', name)
> +                elif isinstance(default_val, str):
> +                    val = config.get('settings', name)
> +                defaults[name] = val
> +                unknown_settings.discard(name)
> +
> +        # Set all the defaults
> +        parser.set_defaults(**defaults)
> +
> +    for name in sorted(unknown_settings):
> +        print("WARNING: Unknown setting %s" % name)

Can you see if 4780f7d8a6b ("patman: Fix defaults not propagating to
subparsers") [1] addresses this problem? The implementation is different,
but I believe these should have the same effect.

--Sean

[1] https://lore.kernel.org/u-boot/20220429145334.2497202-1-sean.anderson@seco.com/

Re: [PATCH v2 1/6] patman: Fix updating argument defaults from settings

[Doug Anderson]

Hi,

On Tue, Jul 5, 2022 at 11:16 AM Sean Anderson <sean.anderson@seco.com> wrote:
>
> Hi Doug,
>
> On 7/1/22 4:23 PM, Douglas Anderson wrote:
> > Ever since commit 4600767d294d ("patman: Refactor how the default
> > subcommand works"), when I use patman on the Linux tree I get grumbles
> > about unknown tags. This is because the Linux default making
> > process_tags be False wasn't working anymore.
> >
> > It appears that the comment claiming that the defaults propagates
> > through all subparsers no longer works for some reason.
> >
> > We're already looking at all the subparsers anyway. Let's just update
> > each one.
> >
> > Fixes: 4600767d294d ("patman: Refactor how the default subcommand works")
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > Tested-by: Brian Norris <briannorris@chromium.org>
> > Reviewed-by: Brian Norris <briannorris@chromium.org>
> > ---
> >
> > (no changes since v1)
> >
> >  tools/patman/settings.py | 41 +++++++++++++++++++++-------------------
> >  1 file changed, 22 insertions(+), 19 deletions(-)
> >
> > diff --git a/tools/patman/settings.py b/tools/patman/settings.py
> > index 7c2b5c196c06..5eefe3d1f55e 100644
> > --- a/tools/patman/settings.py
> > +++ b/tools/patman/settings.py
> > @@ -244,28 +244,31 @@ def _UpdateDefaults(main_parser, config):
> >                    if isinstance(action, argparse._SubParsersAction)
> >                    for _, subparser in action.choices.items()]
> >
> > +    unknown_settings = set(name for name, val in config.items('settings'))
> > +
> >      # Collect the defaults from each parser
> > -    defaults = {}
> >      for parser in parsers:
> >          pdefs = parser.parse_known_args()[0]
> > -        defaults.update(vars(pdefs))
> > -
> > -    # Go through the settings and collect defaults
> > -    for name, val in config.items('settings'):
> > -        if name in defaults:
> > -            default_val = defaults[name]
> > -            if isinstance(default_val, bool):
> > -                val = config.getboolean('settings', name)
> > -            elif isinstance(default_val, int):
> > -                val = config.getint('settings', name)
> > -            elif isinstance(default_val, str):
> > -                val = config.get('settings', name)
> > -            defaults[name] = val
> > -        else:
> > -            print("WARNING: Unknown setting %s" % name)
> > -
> > -    # Set all the defaults (this propagates through all subparsers)
> > -    main_parser.set_defaults(**defaults)
> > +        defaults = dict(vars(pdefs))
> > +
> > +        # Go through the settings and collect defaults
> > +        for name, val in config.items('settings'):
> > +            if name in defaults:
> > +                default_val = defaults[name]
> > +                if isinstance(default_val, bool):
> > +                    val = config.getboolean('settings', name)
> > +                elif isinstance(default_val, int):
> > +                    val = config.getint('settings', name)
> > +                elif isinstance(default_val, str):
> > +                    val = config.get('settings', name)
> > +                defaults[name] = val
> > +                unknown_settings.discard(name)
> > +
> > +        # Set all the defaults
> > +        parser.set_defaults(**defaults)
> > +
> > +    for name in sorted(unknown_settings):
> > +        print("WARNING: Unknown setting %s" % name)
>
> Can you see if 4780f7d8a6b ("patman: Fix defaults not propagating to
> subparsers") [1] addresses this problem? The implementation is different,
> but I believe these should have the same effect.

To my mind the logic of your patch is a bit harder to follow, but I
believe you're correct that it accomplishes the same thing. ...and my
quick test also seems to confirm that yours works fine. Too bad it
wasn't already in "-next" or it would have saved me a bit of time...

I'm curious whether you agree that the logic in my patch is a little
simpler. Should I re-post it as a squashed revert of yours and then
apply mine and call it a "simplify" instead of a bugfix? ...or just
leave yours alone? If we leave yours alone, I guess my patch #2 needs
a trivial rebase to fix a merge conflict.

-Doug

Re: [PATCH v2 1/6] patman: Fix updating argument defaults from settings

[Simon Glass]

Hi Doug,

On Wed, 6 Jul 2022 at 18:08, Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Tue, Jul 5, 2022 at 11:16 AM Sean Anderson <sean.anderson@seco.com> wrote:
> >
> > Hi Doug,
> >
> > On 7/1/22 4:23 PM, Douglas Anderson wrote:
> > > Ever since commit 4600767d294d ("patman: Refactor how the default
> > > subcommand works"), when I use patman on the Linux tree I get grumbles
> > > about unknown tags. This is because the Linux default making
> > > process_tags be False wasn't working anymore.
> > >
> > > It appears that the comment claiming that the defaults propagates
> > > through all subparsers no longer works for some reason.
> > >
> > > We're already looking at all the subparsers anyway. Let's just update
> > > each one.
> > >
> > > Fixes: 4600767d294d ("patman: Refactor how the default subcommand works")
> > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > Tested-by: Brian Norris <briannorris@chromium.org>
> > > Reviewed-by: Brian Norris <briannorris@chromium.org>
> > > ---
> > >
> > > (no changes since v1)
> > >
> > >  tools/patman/settings.py | 41 +++++++++++++++++++++-------------------
> > >  1 file changed, 22 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/tools/patman/settings.py b/tools/patman/settings.py
> > > index 7c2b5c196c06..5eefe3d1f55e 100644
> > > --- a/tools/patman/settings.py
> > > +++ b/tools/patman/settings.py
> > > @@ -244,28 +244,31 @@ def _UpdateDefaults(main_parser, config):
> > >                    if isinstance(action, argparse._SubParsersAction)
> > >                    for _, subparser in action.choices.items()]
> > >
> > > +    unknown_settings = set(name for name, val in config.items('settings'))
> > > +
> > >      # Collect the defaults from each parser
> > > -    defaults = {}
> > >      for parser in parsers:
> > >          pdefs = parser.parse_known_args()[0]
> > > -        defaults.update(vars(pdefs))
> > > -
> > > -    # Go through the settings and collect defaults
> > > -    for name, val in config.items('settings'):
> > > -        if name in defaults:
> > > -            default_val = defaults[name]
> > > -            if isinstance(default_val, bool):
> > > -                val = config.getboolean('settings', name)
> > > -            elif isinstance(default_val, int):
> > > -                val = config.getint('settings', name)
> > > -            elif isinstance(default_val, str):
> > > -                val = config.get('settings', name)
> > > -            defaults[name] = val
> > > -        else:
> > > -            print("WARNING: Unknown setting %s" % name)
> > > -
> > > -    # Set all the defaults (this propagates through all subparsers)
> > > -    main_parser.set_defaults(**defaults)
> > > +        defaults = dict(vars(pdefs))
> > > +
> > > +        # Go through the settings and collect defaults
> > > +        for name, val in config.items('settings'):
> > > +            if name in defaults:
> > > +                default_val = defaults[name]
> > > +                if isinstance(default_val, bool):
> > > +                    val = config.getboolean('settings', name)
> > > +                elif isinstance(default_val, int):
> > > +                    val = config.getint('settings', name)
> > > +                elif isinstance(default_val, str):
> > > +                    val = config.get('settings', name)
> > > +                defaults[name] = val
> > > +                unknown_settings.discard(name)
> > > +
> > > +        # Set all the defaults
> > > +        parser.set_defaults(**defaults)
> > > +
> > > +    for name in sorted(unknown_settings):
> > > +        print("WARNING: Unknown setting %s" % name)
> >
> > Can you see if 4780f7d8a6b ("patman: Fix defaults not propagating to
> > subparsers") [1] addresses this problem? The implementation is different,
> > but I believe these should have the same effect.
>
> To my mind the logic of your patch is a bit harder to follow, but I
> believe you're correct that it accomplishes the same thing. ...and my
> quick test also seems to confirm that yours works fine. Too bad it
> wasn't already in "-next" or it would have saved me a bit of time...

+Tom Rini

It's been languishing for two months due to me taking a break. The PR
itself was sent a week ago but is waiting on one discussion.

>
> I'm curious whether you agree that the logic in my patch is a little
> simpler. Should I re-post it as a squashed revert of yours and then
> apply mine and call it a "simplify" instead of a bugfix? ...or just
> leave yours alone? If we leave yours alone, I guess my patch #2 needs
> a trivial rebase to fix a merge conflict.
>

Regards,
Simon

Re: [PATCH v2 1/6] patman: Fix updating argument defaults from settings

[Sean Anderson]

On 7/6/22 8:07 PM, Doug Anderson wrote:
> Hi,
> 
> On Tue, Jul 5, 2022 at 11:16 AM Sean Anderson <sean.anderson@seco.com> wrote:
>>
>> Hi Doug,
>>
>> On 7/1/22 4:23 PM, Douglas Anderson wrote:
>> > Ever since commit 4600767d294d ("patman: Refactor how the default
>> > subcommand works"), when I use patman on the Linux tree I get grumbles
>> > about unknown tags. This is because the Linux default making
>> > process_tags be False wasn't working anymore.
>> >
>> > It appears that the comment claiming that the defaults propagates
>> > through all subparsers no longer works for some reason.
>> >
>> > We're already looking at all the subparsers anyway. Let's just update
>> > each one.
>> >
>> > Fixes: 4600767d294d ("patman: Refactor how the default subcommand works")
>> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
>> > Tested-by: Brian Norris <briannorris@chromium.org>
>> > Reviewed-by: Brian Norris <briannorris@chromium.org>
>> > ---
>> >
>> > (no changes since v1)
>> >
>> >  tools/patman/settings.py | 41 +++++++++++++++++++++-------------------
>> >  1 file changed, 22 insertions(+), 19 deletions(-)
>> >
>> > diff --git a/tools/patman/settings.py b/tools/patman/settings.py
>> > index 7c2b5c196c06..5eefe3d1f55e 100644
>> > --- a/tools/patman/settings.py
>> > +++ b/tools/patman/settings.py
>> > @@ -244,28 +244,31 @@ def _UpdateDefaults(main_parser, config):
>> >                    if isinstance(action, argparse._SubParsersAction)
>> >                    for _, subparser in action.choices.items()]
>> >
>> > +    unknown_settings = set(name for name, val in config.items('settings'))
>> > +
>> >      # Collect the defaults from each parser
>> > -    defaults = {}
>> >      for parser in parsers:
>> >          pdefs = parser.parse_known_args()[0]
>> > -        defaults.update(vars(pdefs))
>> > -
>> > -    # Go through the settings and collect defaults
>> > -    for name, val in config.items('settings'):
>> > -        if name in defaults:
>> > -            default_val = defaults[name]
>> > -            if isinstance(default_val, bool):
>> > -                val = config.getboolean('settings', name)
>> > -            elif isinstance(default_val, int):
>> > -                val = config.getint('settings', name)
>> > -            elif isinstance(default_val, str):
>> > -                val = config.get('settings', name)
>> > -            defaults[name] = val
>> > -        else:
>> > -            print("WARNING: Unknown setting %s" % name)
>> > -
>> > -    # Set all the defaults (this propagates through all subparsers)
>> > -    main_parser.set_defaults(**defaults)
>> > +        defaults = dict(vars(pdefs))
>> > +
>> > +        # Go through the settings and collect defaults
>> > +        for name, val in config.items('settings'):
>> > +            if name in defaults:
>> > +                default_val = defaults[name]
>> > +                if isinstance(default_val, bool):
>> > +                    val = config.getboolean('settings', name)
>> > +                elif isinstance(default_val, int):
>> > +                    val = config.getint('settings', name)
>> > +                elif isinstance(default_val, str):
>> > +                    val = config.get('settings', name)
>> > +                defaults[name] = val
>> > +                unknown_settings.discard(name)
>> > +
>> > +        # Set all the defaults
>> > +        parser.set_defaults(**defaults)
>> > +
>> > +    for name in sorted(unknown_settings):
>> > +        print("WARNING: Unknown setting %s" % name)
>>
>> Can you see if 4780f7d8a6b ("patman: Fix defaults not propagating to
>> subparsers") [1] addresses this problem? The implementation is different,
>> but I believe these should have the same effect.
> 
> To my mind the logic of your patch is a bit harder to follow, but I
> believe you're correct that it accomplishes the same thing. ...and my
> quick test also seems to confirm that yours works fine. Too bad it
> wasn't already in "-next" or it would have saved me a bit of time...
> 
> I'm curious whether you agree that the logic in my patch is a little
> simpler. Should I re-post it as a squashed revert of yours and then
> apply mine and call it a "simplify" instead of a bugfix? ...or just
> leave yours alone? If we leave yours alone, I guess my patch #2 needs
> a trivial rebase to fix a merge conflict.

IMO my version is simpler, but that is mainly because I thought of it.

I have no objection to your rearranging, as long as it works afterwards.

--Sean

Re: [PATCH v2 1/6] patman: Fix updating argument defaults from settings

[Doug Anderson]

Hi,

On Thu, Jul 7, 2022 at 8:11 AM Sean Anderson <sean.anderson@seco.com> wrote:
>
> On 7/6/22 8:07 PM, Doug Anderson wrote:
> > Hi,
> >
> > On Tue, Jul 5, 2022 at 11:16 AM Sean Anderson <sean.anderson@seco.com> wrote:
> >>
> >> Hi Doug,
> >>
> >> On 7/1/22 4:23 PM, Douglas Anderson wrote:
> >> > Ever since commit 4600767d294d ("patman: Refactor how the default
> >> > subcommand works"), when I use patman on the Linux tree I get grumbles
> >> > about unknown tags. This is because the Linux default making
> >> > process_tags be False wasn't working anymore.
> >> >
> >> > It appears that the comment claiming that the defaults propagates
> >> > through all subparsers no longer works for some reason.
> >> >
> >> > We're already looking at all the subparsers anyway. Let's just update
> >> > each one.
> >> >
> >> > Fixes: 4600767d294d ("patman: Refactor how the default subcommand works")
> >> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> >> > Tested-by: Brian Norris <briannorris@chromium.org>
> >> > Reviewed-by: Brian Norris <briannorris@chromium.org>
> >> > ---
> >> >
> >> > (no changes since v1)
> >> >
> >> >  tools/patman/settings.py | 41 +++++++++++++++++++++-------------------
> >> >  1 file changed, 22 insertions(+), 19 deletions(-)
> >> >
> >> > diff --git a/tools/patman/settings.py b/tools/patman/settings.py
> >> > index 7c2b5c196c06..5eefe3d1f55e 100644
> >> > --- a/tools/patman/settings.py
> >> > +++ b/tools/patman/settings.py
> >> > @@ -244,28 +244,31 @@ def _UpdateDefaults(main_parser, config):
> >> >                    if isinstance(action, argparse._SubParsersAction)
> >> >                    for _, subparser in action.choices.items()]
> >> >
> >> > +    unknown_settings = set(name for name, val in config.items('settings'))
> >> > +
> >> >      # Collect the defaults from each parser
> >> > -    defaults = {}
> >> >      for parser in parsers:
> >> >          pdefs = parser.parse_known_args()[0]
> >> > -        defaults.update(vars(pdefs))
> >> > -
> >> > -    # Go through the settings and collect defaults
> >> > -    for name, val in config.items('settings'):
> >> > -        if name in defaults:
> >> > -            default_val = defaults[name]
> >> > -            if isinstance(default_val, bool):
> >> > -                val = config.getboolean('settings', name)
> >> > -            elif isinstance(default_val, int):
> >> > -                val = config.getint('settings', name)
> >> > -            elif isinstance(default_val, str):
> >> > -                val = config.get('settings', name)
> >> > -            defaults[name] = val
> >> > -        else:
> >> > -            print("WARNING: Unknown setting %s" % name)
> >> > -
> >> > -    # Set all the defaults (this propagates through all subparsers)
> >> > -    main_parser.set_defaults(**defaults)
> >> > +        defaults = dict(vars(pdefs))
> >> > +
> >> > +        # Go through the settings and collect defaults
> >> > +        for name, val in config.items('settings'):
> >> > +            if name in defaults:
> >> > +                default_val = defaults[name]
> >> > +                if isinstance(default_val, bool):
> >> > +                    val = config.getboolean('settings', name)
> >> > +                elif isinstance(default_val, int):
> >> > +                    val = config.getint('settings', name)
> >> > +                elif isinstance(default_val, str):
> >> > +                    val = config.get('settings', name)
> >> > +                defaults[name] = val
> >> > +                unknown_settings.discard(name)
> >> > +
> >> > +        # Set all the defaults
> >> > +        parser.set_defaults(**defaults)
> >> > +
> >> > +    for name in sorted(unknown_settings):
> >> > +        print("WARNING: Unknown setting %s" % name)
> >>
> >> Can you see if 4780f7d8a6b ("patman: Fix defaults not propagating to
> >> subparsers") [1] addresses this problem? The implementation is different,
> >> but I believe these should have the same effect.
> >
> > To my mind the logic of your patch is a bit harder to follow, but I
> > believe you're correct that it accomplishes the same thing. ...and my
> > quick test also seems to confirm that yours works fine. Too bad it
> > wasn't already in "-next" or it would have saved me a bit of time...
> >
> > I'm curious whether you agree that the logic in my patch is a little
> > simpler. Should I re-post it as a squashed revert of yours and then
> > apply mine and call it a "simplify" instead of a bugfix? ...or just
> > leave yours alone? If we leave yours alone, I guess my patch #2 needs
> > a trivial rebase to fix a merge conflict.
>
> IMO my version is simpler, but that is mainly because I thought of it.
>
> I have no objection to your rearranging, as long as it works afterwards.

No worries then. I'll drop my patch #1 and post a rebase of the rest
of the series.

-Doug