[PATCH 1/2] commands: test: simplify argv handling

[PATCH 1/2] commands: test: simplify argv handling

[Marco Felsch]

Decrement argc first before check the closing ']' to avoid the
*argv[argc - 1]. No functional change.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 commands/test.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/commands/test.c b/commands/test.c
index c845cec017..c1b84c42ef 100644
--- a/commands/test.c
+++ b/commands/test.c
@@ -75,11 +75,11 @@ static int do_test(int argc, char *argv[])
 	struct stat statbuf;
 
 	if (*argv[0] == '[') {
-		if (*argv[argc - 1] != ']') {
+		argc--;
+		if (*argv[argc] != ']') {
 			printf("[: missing `]'\n");
 			return 1;
 		}
-		argc--;
 	}
 
 	/* args? */
-- 
2.39.2

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

[Marco Felsch]

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
+	prompt "test bash-compatibility"
+	help
+	  Enable minimal support for bash compatible tests: [[ ]] to allow
+	  pattern matching via: == and !=
+
+# end CMD_TEST
+endif
+
 config CMD_TRUE
 	tristate
 	default y
diff --git a/commands/test.c b/commands/test.c
index c1b84c42ef..77e15aeb03 100644
--- a/commands/test.c
+++ b/commands/test.c
@@ -11,9 +11,13 @@
 #include <command.h>
 #include <fs.h>
 #include <linux/stat.h>
+#ifdef CONFIG_CMD_TEST_BASH_COMP
+#include <fnmatch.h>
+#endif
 
 typedef enum {
 	OPT_EQUAL,
+	OPT_EQUAL_BASH,
 	OPT_NOT_EQUAL,
 	OPT_ARITH_EQUAL,
 	OPT_ARITH_NOT_EQUAL,
@@ -36,6 +40,7 @@ typedef enum {
 
 static char *test_options[] = {
 	[OPT_EQUAL]			= "=",
+	[OPT_EQUAL_BASH]		= "==",
 	[OPT_NOT_EQUAL]			= "!=",
 	[OPT_ARITH_EQUAL]		= "-eq",
 	[OPT_ARITH_NOT_EQUAL]		= "-ne",
@@ -67,18 +72,37 @@ static int parse_opt(const char *opt)
 	return -1;
 }
 
+static int string_comp(const char *left_op, const char *right_op, bool bash_test)
+{
+#ifdef CONFIG_CMD_TEST_BASH_COMP
+	if (bash_test)
+		return fnmatch(right_op, left_op, 0);
+#endif
+	return strcmp(left_op, right_op);
+}
+
 static int do_test(int argc, char *argv[])
 {
 	char **ap;
 	int left, adv, expr, last_expr, neg, last_cmp, opt, zero;
 	ulong a, b;
 	struct stat statbuf;
+	bool bash_test = false;
 
 	if (*argv[0] == '[') {
 		argc--;
-		if (*argv[argc] != ']') {
-			printf("[: missing `]'\n");
-			return 1;
+		if (!strncmp(argv[0], "[[", 2)) {
+			if (strncmp(argv[argc], "]]", 2) != 0) {
+				printf("[[: missing `]]'\n");
+				return 1;
+			}
+			if (IS_ENABLED(CONFIG_CMD_TEST_BASH_COMP))
+				bash_test = true;
+		} else {
+			if (*argv[argc] != ']') {
+				printf("[: missing `]'\n");
+				return 1;
+			}
 		}
 	}
 
@@ -183,10 +207,11 @@ static int do_test(int argc, char *argv[])
 			b = simple_strtol(ap[2], NULL, 0);
 			switch (parse_opt(ap[1])) {
 			case OPT_EQUAL:
-				expr = strcmp(ap[0], ap[2]) == 0;
+			case OPT_EQUAL_BASH:
+				expr = string_comp(ap[0], ap[2], bash_test) == 0;
 				break;
 			case OPT_NOT_EQUAL:
-				expr = strcmp(ap[0], ap[2]) != 0;
+				expr = string_comp(ap[0], ap[2], bash_test) != 0;
 				break;
 			case OPT_ARITH_EQUAL:
 				expr = a == b;
@@ -233,7 +258,11 @@ static int do_test(int argc, char *argv[])
 	return expr;
 }
 
+#ifdef CONFIG_CMD_TEST_BASH_COMP
+static const char * const test_aliases[] = { "[", "[[", NULL};
+#else
 static const char * const test_aliases[] = { "[", NULL};
+#endif
 
 BAREBOX_CMD_HELP_START(test)
 BAREBOX_CMD_HELP_TEXT("Options:")
-- 
2.39.2

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

[Ahmad Fatoum]

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_
Does this really need its own symbol? Can't you just use CONFIG_GLOB
and amend the help text?

> +	prompt "test bash-compatibility"
> +	help
> +	  Enable minimal support for bash compatible tests: [[ ]] to allow
> +	  pattern matching via: == and !=
> +
> +# end CMD_TEST
> +endif
> +
>  config CMD_TRUE
>  	tristate
>  	default y
> diff --git a/commands/test.c b/commands/test.c
> index c1b84c42ef..77e15aeb03 100644
> --- a/commands/test.c
> +++ b/commands/test.c
> @@ -11,9 +11,13 @@
>  #include <command.h>
>  #include <fs.h>
>  #include <linux/stat.h>
> +#ifdef CONFIG_CMD_TEST_BASH_COMP
> +#include <fnmatch.h>
> +#endif
>  
>  typedef enum {
>  	OPT_EQUAL,
> +	OPT_EQUAL_BASH,
>  	OPT_NOT_EQUAL,
>  	OPT_ARITH_EQUAL,
>  	OPT_ARITH_NOT_EQUAL,
> @@ -36,6 +40,7 @@ typedef enum {
>  
>  static char *test_options[] = {
>  	[OPT_EQUAL]			= "=",
> +	[OPT_EQUAL_BASH]		= "==",
>  	[OPT_NOT_EQUAL]			= "!=",
>  	[OPT_ARITH_EQUAL]		= "-eq",
>  	[OPT_ARITH_NOT_EQUAL]		= "-ne",
> @@ -67,18 +72,37 @@ static int parse_opt(const char *opt)
>  	return -1;
>  }
>  
> +static int string_comp(const char *left_op, const char *right_op, bool bash_test)
> +{
> +#ifdef CONFIG_CMD_TEST_BASH_COMP
> +	if (bash_test)
> +		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.

Also can't the #ifdef be replaced with a IS_ENABLED()

> +	return strcmp(left_op, right_op);
> +}
> +
>  static int do_test(int argc, char *argv[])
>  {
>  	char **ap;
>  	int left, adv, expr, last_expr, neg, last_cmp, opt, zero;
>  	ulong a, b;
>  	struct stat statbuf;
> +	bool bash_test = false;
>  
>  	if (*argv[0] == '[') {
>  		argc--;
> -		if (*argv[argc] != ']') {
> -			printf("[: missing `]'\n");
> -			return 1;
> +		if (!strncmp(argv[0], "[[", 2)) {

This is equivalent to argv[0][1] == '['

> +			if (strncmp(argv[argc], "]]", 2) != 0) {
> +				printf("[[: missing `]]'\n");
> +				return 1;
> +			}
> +			if (IS_ENABLED(CONFIG_CMD_TEST_BASH_COMP))
> +				bash_test = true;
> +		} else {
> +			if (*argv[argc] != ']') {
> +				printf("[: missing `]'\n");
> +				return 1;
> +			}
>  		}
>  	}
>  
> @@ -183,10 +207,11 @@ static int do_test(int argc, char *argv[])
>  			b = simple_strtol(ap[2], NULL, 0);
>  			switch (parse_opt(ap[1])) {
>  			case OPT_EQUAL:
> -				expr = strcmp(ap[0], ap[2]) == 0;
> +			case OPT_EQUAL_BASH:
> +				expr = string_comp(ap[0], ap[2], bash_test) == 0;
>  				break;
>  			case OPT_NOT_EQUAL:
> -				expr = strcmp(ap[0], ap[2]) != 0;
> +				expr = string_comp(ap[0], ap[2], bash_test) != 0;
>  				break;
>  			case OPT_ARITH_EQUAL:
>  				expr = a == b;
> @@ -233,7 +258,11 @@ static int do_test(int argc, char *argv[])
>  	return expr;
>  }
>  
> +#ifdef CONFIG_CMD_TEST_BASH_COMP
> +static const char * const test_aliases[] = { "[", "[[", NULL};
> +#else
>  static const char * const test_aliases[] = { "[", NULL};
> +#endif
>  
>  BAREBOX_CMD_HELP_START(test)
>  BAREBOX_CMD_HELP_TEXT("Options:")

-- 
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 |

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

[Marco Felsch]

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.

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.

> > +	prompt "test bash-compatibility"
> > +	help
> > +	  Enable minimal support for bash compatible tests: [[ ]] to allow
> > +	  pattern matching via: == and !=
> > +
> > +# end CMD_TEST
> > +endif
> > +
> >  config CMD_TRUE
> >  	tristate
> >  	default y
> > diff --git a/commands/test.c b/commands/test.c
> > index c1b84c42ef..77e15aeb03 100644
> > --- a/commands/test.c
> > +++ b/commands/test.c
> > @@ -11,9 +11,13 @@
> >  #include <command.h>
> >  #include <fs.h>
> >  #include <linux/stat.h>
> > +#ifdef CONFIG_CMD_TEST_BASH_COMP
> > +#include <fnmatch.h>
> > +#endif
> >  
> >  typedef enum {
> >  	OPT_EQUAL,
> > +	OPT_EQUAL_BASH,
> >  	OPT_NOT_EQUAL,
> >  	OPT_ARITH_EQUAL,
> >  	OPT_ARITH_NOT_EQUAL,
> > @@ -36,6 +40,7 @@ typedef enum {
> >  
> >  static char *test_options[] = {
> >  	[OPT_EQUAL]			= "=",
> > +	[OPT_EQUAL_BASH]		= "==",
> >  	[OPT_NOT_EQUAL]			= "!=",
> >  	[OPT_ARITH_EQUAL]		= "-eq",
> >  	[OPT_ARITH_NOT_EQUAL]		= "-ne",
> > @@ -67,18 +72,37 @@ static int parse_opt(const char *opt)
> >  	return -1;
> >  }
> >  
> > +static int string_comp(const char *left_op, const char *right_op, bool bash_test)
> > +{
> > +#ifdef CONFIG_CMD_TEST_BASH_COMP
> > +	if (bash_test)
> > +		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.

> > +	return strcmp(left_op, right_op);
> > +}
> > +
> >  static int do_test(int argc, char *argv[])
> >  {
> >  	char **ap;
> >  	int left, adv, expr, last_expr, neg, last_cmp, opt, zero;
> >  	ulong a, b;
> >  	struct stat statbuf;
> > +	bool bash_test = false;
> >  
> >  	if (*argv[0] == '[') {
> >  		argc--;
> > -		if (*argv[argc] != ']') {
> > -			printf("[: missing `]'\n");
> > -			return 1;
> > +		if (!strncmp(argv[0], "[[", 2)) {
> 
> This is equivalent to argv[0][1] == '['

I would keep the strncmp.

Regards,
  Marco

> > +			if (strncmp(argv[argc], "]]", 2) != 0) {
> > +				printf("[[: missing `]]'\n");
> > +				return 1;
> > +			}
> > +			if (IS_ENABLED(CONFIG_CMD_TEST_BASH_COMP))
> > +				bash_test = true;
> > +		} else {
> > +			if (*argv[argc] != ']') {
> > +				printf("[: missing `]'\n");
> > +				return 1;
> > +			}
> >  		}
> >  	}
> >  
> > @@ -183,10 +207,11 @@ static int do_test(int argc, char *argv[])
> >  			b = simple_strtol(ap[2], NULL, 0);
> >  			switch (parse_opt(ap[1])) {
> >  			case OPT_EQUAL:
> > -				expr = strcmp(ap[0], ap[2]) == 0;
> > +			case OPT_EQUAL_BASH:
> > +				expr = string_comp(ap[0], ap[2], bash_test) == 0;
> >  				break;
> >  			case OPT_NOT_EQUAL:
> > -				expr = strcmp(ap[0], ap[2]) != 0;
> > +				expr = string_comp(ap[0], ap[2], bash_test) != 0;
> >  				break;
> >  			case OPT_ARITH_EQUAL:
> >  				expr = a == b;
> > @@ -233,7 +258,11 @@ static int do_test(int argc, char *argv[])
> >  	return expr;
> >  }
> >  
> > +#ifdef CONFIG_CMD_TEST_BASH_COMP
> > +static const char * const test_aliases[] = { "[", "[[", NULL};
> > +#else
> >  static const char * const test_aliases[] = { "[", NULL};
> > +#endif
> >  
> >  BAREBOX_CMD_HELP_START(test)
> >  BAREBOX_CMD_HELP_TEXT("Options:")
> 
> -- 
> 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 |
> 
> 

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

[Sascha Hauer]

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 |

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

[Sascha Hauer]

On Tue, Jul 04, 2023 at 12:58:13AM +0200, Marco Felsch wrote:
> Add [[ expression ]] support which allow pattern matching like:
> 
>   - [[ "foo1" == "foo*" ]]

It's a bit unfortunate that in barebox we require putting the pattern in
"", whereas in bash it only works without the "".

I guess there's not much we can do about this unless we turn the '['
command into a shell builtin.

>   - [[ "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
> +	prompt "test bash-compatibility"
> +	help
> +	  Enable minimal support for bash compatible tests: [[ ]] to allow
> +	  pattern matching via: == and !=
> +
> +# end CMD_TEST
> +endif
> +
>  config CMD_TRUE
>  	tristate
>  	default y
> diff --git a/commands/test.c b/commands/test.c
> index c1b84c42ef..77e15aeb03 100644
> --- a/commands/test.c
> +++ b/commands/test.c
> @@ -11,9 +11,13 @@
>  #include <command.h>
>  #include <fs.h>
>  #include <linux/stat.h>
> +#ifdef CONFIG_CMD_TEST_BASH_COMP
> +#include <fnmatch.h>
> +#endif

No need to include this conditionally.

>  
>  typedef enum {
>  	OPT_EQUAL,
> +	OPT_EQUAL_BASH,
>  	OPT_NOT_EQUAL,
>  	OPT_ARITH_EQUAL,
>  	OPT_ARITH_NOT_EQUAL,
> @@ -36,6 +40,7 @@ typedef enum {
>  
>  static char *test_options[] = {
>  	[OPT_EQUAL]			= "=",
> +	[OPT_EQUAL_BASH]		= "==",
>  	[OPT_NOT_EQUAL]			= "!=",
>  	[OPT_ARITH_EQUAL]		= "-eq",
>  	[OPT_ARITH_NOT_EQUAL]		= "-ne",
> @@ -67,18 +72,37 @@ static int parse_opt(const char *opt)
>  	return -1;
>  }
>  
> +static int string_comp(const char *left_op, const char *right_op, bool bash_test)
> +{
> +#ifdef CONFIG_CMD_TEST_BASH_COMP
> +	if (bash_test)
> +		return fnmatch(right_op, left_op, 0);
> +#endif
> +	return strcmp(left_op, right_op);
> +}
> +
>  static int do_test(int argc, char *argv[])
>  {
>  	char **ap;
>  	int left, adv, expr, last_expr, neg, last_cmp, opt, zero;
>  	ulong a, b;
>  	struct stat statbuf;
> +	bool bash_test = false;
>  
>  	if (*argv[0] == '[') {
>  		argc--;
> -		if (*argv[argc] != ']') {
> -			printf("[: missing `]'\n");
> -			return 1;
> +		if (!strncmp(argv[0], "[[", 2)) {
> +			if (strncmp(argv[argc], "]]", 2) != 0) {
> +				printf("[[: missing `]]'\n");
> +				return 1;
> +			}
> +			if (IS_ENABLED(CONFIG_CMD_TEST_BASH_COMP))
> +				bash_test = true;

The if() is unnecessary.

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 |

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

[Marco Felsch]

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 |
>