[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 at 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 at chromium.org>
>> > Tested-by: Brian Norris <briannorris at chromium.org>
>> > Reviewed-by: Brian Norris <briannorris at 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