Hi,
On Thu, Jul 7, 2022 at 8:11 AM Sean Anderson <sean.ander...@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.ander...@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 <diand...@chromium.org>
> >> > Tested-by: Brian Norris <briannor...@chromium.org>
> >> > Reviewed-by: Brian Norris <briannor...@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
