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

[Sascha Hauer <sha@pengutronix.de>]

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.

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 |