[U-Boot] [PATCH 0/2] cmd: auto-complete args starting with a $

[U-Boot] [PATCH 0/2] cmd: auto-complete args starting with a $

[Boris Brezillon]

Hello,

It's pretty common to pass arguments that start with a $ and are then
expanded by the shell, and I'm this kind of lazy guy that hits tab all
the time and expects the shell to suggest something appropriate. So
here is a patchset adding support for ${} auto-completion and using the
new helper from the mtd command.

Using it elsewhere should be pretty easy (I already patched env set to
use it too [1]).

Note that this patch series depends on [2].

Regards,

Boris

[1]https://github.com/bbrezillon/u-boot/commit/9f37c5b6471c7479aa043524d31531f06128771c
[2]https://patchwork.ozlabs.org/project/uboot/list/?series=79552

Boris Brezillon (2):
  common: command: Provide a dollar_complete() helper
  cmd: mtd: auto-complete args starting with a $ when appropriate

 cmd/mtd.c         | 40 ++++++++++++++++++++++++++++++++-----
 common/command.c  | 22 ++++++++++++++++++---
 env/common.c      | 50 ++++++++++++++++++++++++++++++++++++++++++++---
 include/command.h |  2 ++
 include/common.h  |  3 ++-
 5 files changed, 105 insertions(+), 12 deletions(-)

-- 
2.17.1

[U-Boot] [PATCH 1/2] common: command: Provide a dollar_complete() helper

[Boris Brezillon]

Add an helper to auto-complete arguments starting with a '$' with what's
available in the environment.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
 common/command.c  | 22 ++++++++++++++++++---
 env/common.c      | 50 ++++++++++++++++++++++++++++++++++++++++++++---
 include/command.h |  2 ++
 include/common.h  |  3 ++-
 4 files changed, 70 insertions(+), 7 deletions(-)

diff --git a/common/command.c b/common/command.c
index 19f0534a76ea..754ab9bbc396 100644
--- a/common/command.c
+++ b/common/command.c
@@ -143,22 +143,38 @@ int cmd_usage(const cmd_tbl_t *cmdtp)
 
 #ifdef CONFIG_AUTO_COMPLETE
 
+static char env_complete_buf[512];
+
 int var_complete(int argc, char * const argv[], char last_char, int maxv, char *cmdv[])
 {
-	static char tmp_buf[512];
 	int space;
 
 	space = last_char == '\0' || isblank(last_char);
 
 	if (space && argc == 1)
-		return env_complete("", maxv, cmdv, sizeof(tmp_buf), tmp_buf);
+		return env_complete("", maxv, cmdv, sizeof(env_complete_buf),
+				    env_complete_buf, false);
 
 	if (!space && argc == 2)
-		return env_complete(argv[1], maxv, cmdv, sizeof(tmp_buf), tmp_buf);
+		return env_complete(argv[1], maxv, cmdv,
+				    sizeof(env_complete_buf),
+				    env_complete_buf, false);
 
 	return 0;
 }
 
+int dollar_complete(int argc, char * const argv[], char last_char, int maxv,
+		    char *cmdv[])
+{
+	/* Make sure the last argument starts with a $. */
+	if (argc < 1 || argv[argc - 1][0] != '$' ||
+	    last_char == '\0' || isblank(last_char))
+		return 0;
+
+	return env_complete(argv[argc - 1], maxv, cmdv, sizeof(env_complete_buf),
+			    env_complete_buf, true);
+}
+
 /*************************************************************************************/
 
 int complete_subcmdv(cmd_tbl_t *cmdtp, int count, int argc,
diff --git a/env/common.c b/env/common.c
index 3317cef35522..aa9a097bced0 100644
--- a/env/common.c
+++ b/env/common.c
@@ -241,31 +241,75 @@ void env_relocate(void)
 }
 
 #if defined(CONFIG_AUTO_COMPLETE) && !defined(CONFIG_SPL_BUILD)
-int env_complete(char *var, int maxv, char *cmdv[], int bufsz, char *buf)
+int env_complete(char *var, int maxv, char *cmdv[], int bufsz, char *buf,
+		 bool dollar_comp)
 {
 	ENTRY *match;
 	int found, idx;
 
+	if (dollar_comp) {
+		/*
+		 * When doing $ completion, the first character should
+		 * obviously be a '$'.
+		 */
+		if (var[0] != '$')
+			return 0;
+
+		var++;
+
+		/*
+		 * The second one, if present, should be a '{', as some
+		 * configuration of the u-boot shell expand ${var} but not
+		 * $var.
+		 */
+		if (var[0] == '{')
+			var++;
+		else if (var[0] != '\0')
+			return 0;
+	}
+
 	idx = 0;
 	found = 0;
 	cmdv[0] = NULL;
 
+
 	while ((idx = hmatch_r(var, idx, &match, &env_htab))) {
 		int vallen = strlen(match->key) + 1;
 
-		if (found >= maxv - 2 || bufsz < vallen)
+		if (found >= maxv - 2 ||
+		    bufsz < vallen + (dollar_comp ? 3 : 0))
 			break;
 
 		cmdv[found++] = buf;
+
+		/* Add the '${' prefix to each var when doing $ completion. */
+		if (dollar_comp) {
+			strcpy(buf, "${");
+			buf += 2;
+			bufsz -= 3;
+		}
+
 		memcpy(buf, match->key, vallen);
 		buf += vallen;
 		bufsz -= vallen;
+
+		if (dollar_comp) {
+			/*
+			 * This one is a bit odd: vallen already contains the
+			 * '\0' character but we need to add the '}' suffix,
+			 * hence the buf - 1 here. strcpy() will add the '\0'
+			 * character just after '}'. buf is then incremented
+			 * to account for the extra '}' we just added.
+			 */
+			strcpy(buf - 1, "}");
+			buf++;
+		}
 	}
 
 	qsort(cmdv, found, sizeof(cmdv[0]), strcmp_compar);
 
 	if (idx)
-		cmdv[found++] = "...";
+		cmdv[found++] = dollar_comp ? "${...}" : "...";
 
 	cmdv[found] = NULL;
 	return found;
diff --git a/include/command.h b/include/command.h
index 461b17447c0d..c512ec6854d0 100644
--- a/include/command.h
+++ b/include/command.h
@@ -84,6 +84,8 @@ static inline bool cmd_is_repeatable(cmd_tbl_t *cmdtp)
 
 #ifdef CONFIG_AUTO_COMPLETE
 extern int var_complete(int argc, char * const argv[], char last_char, int maxv, char *cmdv[]);
+int dollar_complete(int argc, char * const argv[], char last_char, int maxv,
+                    char *cmdv[]);
 extern int cmd_auto_complete(const char *const prompt, char *buf, int *np, int *colp);
 #endif
 
diff --git a/include/common.h b/include/common.h
index 8b561370326f..2c776adc5fe0 100644
--- a/include/common.h
+++ b/include/common.h
@@ -237,7 +237,8 @@ static inline int env_set_addr(const char *varname, const void *addr)
 }
 
 #ifdef CONFIG_AUTO_COMPLETE
-int env_complete(char *var, int maxv, char *cmdv[], int maxsz, char *buf);
+int env_complete(char *var, int maxv, char *cmdv[], int maxsz, char *buf,
+		 bool dollar_comp);
 #endif
 int get_env_id (void);
 
-- 
2.17.1

[U-Boot] [PATCH 2/2] cmd: mtd: auto-complete args starting with a $ when appropriate

[Boris Brezillon]

It's quite usual to have RAM or flash address stored in env vars.
Use $ auto-completion for such arguments.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
 cmd/mtd.c | 40 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 35 insertions(+), 5 deletions(-)

diff --git a/cmd/mtd.c b/cmd/mtd.c
index cda702d18b16..b13dbef3e888 100644
--- a/cmd/mtd.c
+++ b/cmd/mtd.c
@@ -517,6 +517,36 @@ static int mtd_name_complete(int argc, char * const argv[], char last_char,
 
 	return n_found;
 }
+
+static int subcmd_complete(int maxargs, int argc, char * const argv[],
+			   char last_char, int maxv, char *cmdv[])
+{
+	if (argc > maxargs)
+		return 0;
+
+	if (argc <= 2)
+		return mtd_name_complete(argc, argv, last_char, maxv, cmdv);
+
+	return dollar_complete(argc, argv, last_char, maxv, cmdv);
+}
+
+static int four_args_complete(int argc, char * const argv[], char last_char,
+			      int maxv, char *cmdv[])
+{
+	return subcmd_complete(5, argc, argv, last_char, maxv, cmdv);
+}
+
+static int three_args_complete(int argc, char * const argv[], char last_char,
+			       int maxv, char *cmdv[])
+{
+	return subcmd_complete(4, argc, argv, last_char, maxv, cmdv);
+}
+
+static int one_arg_complete(int argc, char * const argv[], char last_char,
+			    int maxv, char *cmdv[])
+{
+	return subcmd_complete(2, argc, argv, last_char, maxv, cmdv);
+}
 #endif /* CONFIG_AUTO_COMPLETE */
 
 static char mtd_help_text[] =
@@ -548,12 +578,12 @@ static char mtd_help_text[] =
 U_BOOT_CMD_WITH_SUBCMDS(mtd, "MTD utils", mtd_help_text,
 		U_BOOT_SUBCMD_MKENT(list, 1, 1, do_mtd_list),
 		U_BOOT_SUBCMD_MKENT_COMPLETE(read, 5, 0, do_mtd_io,
-					     mtd_name_complete),
+					     four_args_complete),
 		U_BOOT_SUBCMD_MKENT_COMPLETE(write, 5, 0, do_mtd_io,
-					     mtd_name_complete),
+					     four_args_complete),
 		U_BOOT_SUBCMD_MKENT_COMPLETE(dump, 4, 0, do_mtd_io,
-					     mtd_name_complete),
+					     three_args_complete),
 		U_BOOT_SUBCMD_MKENT_COMPLETE(erase, 4, 0, do_mtd_erase,
-					     mtd_name_complete),
+					     three_args_complete),
 		U_BOOT_SUBCMD_MKENT_COMPLETE(bad, 2, 1, do_mtd_bad,
-					     mtd_name_complete));
+					     one_arg_complete));
-- 
2.17.1

[U-Boot] [PATCH 0/2] cmd: auto-complete args starting with a $

[Wolfgang Denk]

Dear Boris,

In message <20181203220726.19370-1-boris.brezillon@bootlin.com> you wrote:
> 
> It's pretty common to pass arguments that start with a $ and are then
> expanded by the shell, and I'm this kind of lazy guy that hits tab all
> the time and expects the shell to suggest something appropriate. So
> here is a patchset adding support for ${} auto-completion and using the
> new helper from the mtd command.

You mean, this feature depends on MTD support?  this is not goot.

Also, you should make this feature configurable.  Not everybody may
want to use it or may have the memory available.

Technically - should autocompletion not be prevented for escaped
'$' characters, i. e. hitting TAB after a '\$' sequence should NOT
expand?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
If you use modules, you pay the price. Sane embedded solutions
running in "tight" environments don't use modules :-)
    -- Benjamin Herrenschmidt in <1258234866.2140.451.camel@pasglop>

[U-Boot] [PATCH 0/2] cmd: auto-complete args starting with a $

[Boris Brezillon]

On Tue, 04 Dec 2018 10:44:19 +0100
Wolfgang Denk <wd@denx.de> wrote:

> Dear Boris,
> 
> In message <20181203220726.19370-1-boris.brezillon@bootlin.com> you wrote:
> > 
> > It's pretty common to pass arguments that start with a $ and are then
> > expanded by the shell, and I'm this kind of lazy guy that hits tab all
> > the time and expects the shell to suggest something appropriate. So
> > here is a patchset adding support for ${} auto-completion and using the
> > new helper from the mtd command.  
> 
> You mean, this feature depends on MTD support?  this is not goot.

Not at all, MTD is just the first user of this dollar_complete()
helper. That's what I meant.

> 
> Also, you should make this feature configurable.  Not everybody may
> want to use it or may have the memory available.

It already depends on CONFIG_AUTO_COMPLETE, and each command has to
explicitly call the helper to support ${} auto-completion. Not that $
auto-completion only makes sense for some arguments, not all of them.
It's the responsibility of the command itself to decide when it should
be used.

> 
> Technically - should autocompletion not be prevented for escaped
> '$' characters, i. e. hitting TAB after a '\$' sequence should NOT
> expand?

It's already the case. The argument has to start with $ or ${ to be
auto-completed.

[U-Boot] [PATCH 0/2] cmd: auto-complete args starting with a $

[Wolfgang Denk]

Dear Boris,

In message <20181204105448.63b9af8c@bbrezillon> you wrote:
>
> > > It's pretty common to pass arguments that start with a $ and are then
> > > expanded by the shell, and I'm this kind of lazy guy that hits tab all
> > > the time and expects the shell to suggest something appropriate. So
> > > here is a patchset adding support for ${} auto-completion and using the
> > > new helper from the mtd command.  
> > 
> > You mean, this feature depends on MTD support?  this is not goot.
>
> Not at all, MTD is just the first user of this dollar_complete()
> helper. That's what I meant.

But is this not based on the code of mtd_name_complete() which is
only availabole when MTD is present?

> > Also, you should make this feature configurable.  Not everybody may
> > want to use it or may have the memory available.
>
> It already depends on CONFIG_AUTO_COMPLETE, and each command has to
> explicitly call the helper to support ${} auto-completion. Not that $
> auto-completion only makes sense for some arguments, not all of them.
> It's the responsibility of the command itself to decide when it should
> be used.

Um... this is not hat I would expect - if we implement
auto-completion for variables, it should IMO work similar to what a
standard shell is doing.

In Bash, I can do:

-> foo=1234
-> echo $f<TAB>
$f     $flag  $foo
-> echo $f

i. e. this is a feature of the shell and not of any command.
Implementing this a zillion times for each of the commands in
inacceptable.  Also, implementing it for one command and not for
another makese no sense - that would violate the Principle of Least
Surprise, as you have something which looks convenient but does not
work everywhere.

> > Technically - should autocompletion not be prevented for escaped
> > '$' characters, i. e. hitting TAB after a '\$' sequence should NOT
> > expand?
>
> It's already the case. The argument has to start with $ or ${ to be
> auto-completed.

We should implement this such that behaviour is similar to what we
see on a standard shell.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
What we hope ever to do with ease, we must learn  first  to  do  with
diligence.                                           - Samuel Johnson

[U-Boot] [PATCH 0/2] cmd: auto-complete args starting with a $

[Boris Brezillon]

Hi Wolfgang,

On Tue, 04 Dec 2018 11:14:31 +0100
Wolfgang Denk <wd@denx.de> wrote:

> Dear Boris,
> 
> In message <20181204105448.63b9af8c@bbrezillon> you wrote:
> >  
> > > > It's pretty common to pass arguments that start with a $ and are then
> > > > expanded by the shell, and I'm this kind of lazy guy that hits tab all
> > > > the time and expects the shell to suggest something appropriate. So
> > > > here is a patchset adding support for ${} auto-completion and using the
> > > > new helper from the mtd command.    
> > > 
> > > You mean, this feature depends on MTD support?  this is not goot.  
> >
> > Not at all, MTD is just the first user of this dollar_complete()
> > helper. That's what I meant.  
> 
> But is this not based on the code of mtd_name_complete() which is
> only availabole when MTD is present?

Nope. See patch 1, the code is completely independent from the mtd cmd.

> 
> > > Also, you should make this feature configurable.  Not everybody may
> > > want to use it or may have the memory available.  
> >
> > It already depends on CONFIG_AUTO_COMPLETE, and each command has to
> > explicitly call the helper to support ${} auto-completion. Not that $
> > auto-completion only makes sense for some arguments, not all of them.
> > It's the responsibility of the command itself to decide when it should
> > be used.  
> 
> Um... this is not hat I would expect - if we implement
> auto-completion for variables, it should IMO work similar to what a
> standard shell is doing.
> 
> In Bash, I can do:
> 
> -> foo=1234
> -> echo $f<TAB>  
> $f     $flag  $foo

And that's pretty much how it works except that right now, echo has to
be patched to support $ auto-completion.

> -> echo $f  

For the record, not all shell configs expand $foo, some require that foo
be enclosed in ${} (${foo}), that's why the current proposal always
auto-completes $ as ${.

> 
> i. e. this is a feature of the shell and not of any command.
> Implementing this a zillion times for each of the commands in
> inacceptable.  Also, implementing it for one command and not for
> another makese no sense - that would violate the Principle of Least
> Surprise, as you have something which looks convenient but does not
> work everywhere.

Okay, I understand. I can try to hook that up at the common/command.c
level.

> 
> > > Technically - should autocompletion not be prevented for escaped
> > > '$' characters, i. e. hitting TAB after a '\$' sequence should NOT
> > > expand?  
> >
> > It's already the case. The argument has to start with $ or ${ to be
> > auto-completed.  
> 
> We should implement this such that behaviour is similar to what we
> see on a standard shell.

Apart from the "make that available to everyone" comment, is there
anything else you think should be changed? IOW, what is not compliant
with a standard shell auto-completion in my proposal?

Regards,

Boris

[U-Boot] [PATCH 0/2] cmd: auto-complete args starting with a $

[Wolfgang Denk]

Dear Boris,

In message <20181204113313.577178ac@bbrezillon> you wrote:
>
> > But is this not based on the code of mtd_name_complete() which is
> > only availabole when MTD is present?
>
> Nope. See patch 1, the code is completely independent from the mtd cmd.

OK, then I misread the patches.

> Apart from the "make that available to everyone" comment, is there
> anything else you think should be changed? IOW, what is not compliant
> with a standard shell auto-completion in my proposal?

I can't say easily from the code.  I'd have to see this running (in
the sandbox, for example).

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"The greatest warriors are the ones who fight for peace."
- Holly Near

[U-Boot] [PATCH 0/2] cmd: auto-complete args starting with a $

[Boris Brezillon]

On Tue, 04 Dec 2018 14:00:47 +0100
Wolfgang Denk <wd@denx.de> wrote:

> Dear Boris,
> 
> In message <20181204113313.577178ac@bbrezillon> you wrote:
> >  
> > > But is this not based on the code of mtd_name_complete() which is
> > > only availabole when MTD is present?  
> >
> > Nope. See patch 1, the code is completely independent from the mtd cmd.  
> 
> OK, then I misread the patches.
> 
> > Apart from the "make that available to everyone" comment, is there
> > anything else you think should be changed? IOW, what is not compliant
> > with a standard shell auto-completion in my proposal?  
> 
> I can't say easily from the code.  I'd have to see this running (in
> the sandbox, for example).

Actually, that's how I tested it. You can easily test it with the echo
command after applying patch 1 of this series + the following diff.

--->8---
diff --git a/cmd/echo.c b/cmd/echo.c
index 5b018d9349ab..857f268c6a61 100644
--- a/cmd/echo.c
+++ b/cmd/echo.c
@@ -47,9 +47,10 @@ static int do_echo(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
        return 0;
 }
 
-U_BOOT_CMD(
+U_BOOT_CMD_COMPLETE(
        echo,   CONFIG_SYS_MAXARGS,     1,      do_echo,
        "echo args to console",
        "[args..]\n"
-       "    - echo args to console; \\c suppresses newline"
+       "    - echo args to console; \\c suppresses newline",
+       dollar_complete
 );

[U-Boot] [PATCH 0/2] cmd: auto-complete args starting with a $

[Boris Brezillon]

On Tue, 4 Dec 2018 11:33:13 +0100
Boris Brezillon <boris.brezillon@bootlin.com> wrote:

> > 
> > i. e. this is a feature of the shell and not of any command.
> > Implementing this a zillion times for each of the commands in
> > inacceptable.  Also, implementing it for one command and not for
> > another makese no sense - that would violate the Principle of Least
> > Surprise, as you have something which looks convenient but does not
> > work everywhere.  
> 
> Okay, I understand. I can try to hook that up at the common/command.c
> level.

I thought it would be much more complicated: here is the diff adding $
auto-completion for everyone. The only drawback I see with this approach
is that cmd/sub-cmd arg auto-completion cannot be bound to cmd->maxargs
anymore, but I guess that's acceptable.

--->8---
diff --git a/common/command.c b/common/command.c
index 754ab9bbc396..b9a44d4ea7a8 100644
--- a/common/command.c
+++ b/common/command.c
@@ -373,9 +373,14 @@ int cmd_auto_complete(const char *const prompt, char *buf, int *np, int *colp)
        /* separate into argv */
        argc = make_argv(tmp_buf, sizeof(argv)/sizeof(argv[0]), argv);
 
-       /* do the completion and return the possible completions */
-       i = complete_cmdv(argc, argv, last_char,
-                         sizeof(cmdv) / sizeof(cmdv[0]), cmdv);
+       /* first try a $ completion */
+       i = dollar_complete(argc, argv, last_char,
+                           sizeof(cmdv) / sizeof(cmdv[0]), cmdv);
+       if (!i) {
+               /* do the completion and return the possible completions */
+               i = complete_cmdv(argc, argv, last_char,
+                                 sizeof(cmdv) / sizeof(cmdv[0]), cmdv);
+       }
 
        /* no match; bell and out */
        if (i == 0) {