Re: [PATCH 2/2] commands: test: add based support for bash-test style

[Marco Felsch <m.felsch@pengutronix.de>]

On 23-07-04, Sascha Hauer wrote:
> On Tue, Jul 04, 2023 at 10:32:22AM +0200, Marco Felsch wrote:
> > Hi Ahmad,
> > 
> > On 23-07-04, Ahmad Fatoum wrote:
> > > On 04.07.23 00:58, Marco Felsch wrote:
> > > > Add [[ expression ]] support which allow pattern matching like:
> > > > 
> > > >   - [[ "foo1" == "foo*" ]]
> > > >   - [[ "foo" != "bar" ]]
> > > > 
> > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > > ---
> > > >  commands/Kconfig | 13 +++++++++++++
> > > >  commands/test.c  | 39 ++++++++++++++++++++++++++++++++++-----
> > > >  2 files changed, 47 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/commands/Kconfig b/commands/Kconfig
> > > > index 4d3ff631a8..615e96aa9d 100644
> > > > --- a/commands/Kconfig
> > > > +++ b/commands/Kconfig
> > > > @@ -1219,6 +1219,19 @@ config CMD_TEST
> > > >  		  !, =, !=, -eq, -ne, -ge, -gt, -le, -lt, -o, -a, -z, -n, -d, -e,
> > > >  		  -f, -L; see 'man test' on your PC for more information.
> > > >  
> > > > +if CMD_TEST
> > > > +
> > > > +config CMD_TEST_BASH_COMP
> > > > +	tristate
> > > > +	select CONFIG_FNMATCH
> > > 
> > > select FNMATCH, no CONFIG_
> > 
> > Right.
> > 
> > > Does this really need its own symbol? Can't you just use CONFIG_GLOB
> > > and amend the help text?
> > 
> > The implementation was inspired by busybox and they use fnmatch.
> > Checking glob.c I don't see any difference, therefore I would keep the
> > fnmatch.
> 
> glob() is the wrong function anyway. glob() expands patterns to existing
> files. You want to do pattern matching only not looking into the VFS.
> fnmatch() is the right function here.
> 
> Besides, glob() uses fnmatch(), so by using glob() you would add even
> more code.
> 
> > 
> > I wasn't sure if every user need the bash(ism) support and sometimes
> > bootloader partitions are really small due to legacy reasons. Therefore
> > I went this way to let the user the choice to enable/disable the
> > support.
> 
> You can't do much in barebox without globalvar support which already
> uses fnmatch(), so it's likely enabled anyway.

Ah right, I will drop it then.

Thanks,
Marco

> 
> Sascha
> 
> > > > +		return fnmatch(right_op, left_op, 0);
> > > > +#endif
> > > 
> > > Scripts that execute differently depending on a barebox config option
> > > is not good user experience. I think it would be better if [[ is an
> > > error if support is missing.
> > 
> > I was thinking about a warning which is printed once you use [[ and
> > didn't enabled it before. Then I read man bash again and [[ supports
> > '=' as well which equals the [ function. Therefore I dropped the
> > warning.
> > 
> > > Also can't the #ifdef be replaced with a IS_ENABLED()
> > 
> > I don't think so, since the fnmatch is added conditional to keep the
> > code base the same if not enabled.
> 
> Use IS_ENABLED(). The linker will discard unused functions for you. We
> use this all over the place.
> 
> Sascha
> 
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
>