cmd: auto-complete args starting with a $

PATCH New Boris Brezillon 2 participants 10 messages Dec 3, 2018 – Dec 4, 2018

lore

Share & tools

lore

Boris Brezillon

author

2018-12-03 22:07 UTC #0 lore raw

cover cmd: auto-complete args starting with a $

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

Signature · 1 line

2.17.1

Boris Brezillon

author

2018-12-03 22:07 UTC #1 lore raw

PATCH 1/2 common: command: Provide a dollar_complete() helper

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

Signed-off-by Boris Brezillon <[email protected]>

---
 common/command.c  | 22 ++++++++++++++++++---
 env/common.c      | 50 ++++++++++++++++++++++++++++++++++++++++++++---
 include/command.h |  2 ++
 include/common.h  |  3 ++-
 4 files changed, 70 insertions(+), 7 deletions(-)

common/command.c +19 -3

index 19f0534a76ea..754ab9bbc396 100644
@@ -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,

env/common.c +47 -3

78 lines not on this page · raw message

include/command.h +2 -0

index 461b17447c0d..c512ec6854d0 100644
@@ -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

include/common.h +2 -1

index 8b561370326f..2c776adc5fe0 100644
@@ -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);

Signature · 1 line

2.17.1

Boris Brezillon

author

2018-12-03 22:07 UTC #2 lore raw

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

In reply to 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 <[email protected]>

---
 cmd/mtd.c | 40 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 35 insertions(+), 5 deletions(-)

cmd/mtd.c +35 -5

index cda702d18b16..b13dbef3e888 100644
@@ -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));

Signature · 1 line

2.17.1

Wolfgang Denk

2018-12-04 09:44 UTC #3 lore raw

cover cmd: auto-complete args starting with a $

In reply to Boris Brezillon

Dear Boris,

Quoted 6 lines · In message <[email protected]> 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

Signature · 6 lines

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>

Boris Brezillon

author

2018-12-04 09:54 UTC #4 lore raw

cover cmd: auto-complete args starting with a $

Quoted 11 lines · On Tue, 04 Dec 2018 10:44:19 +0100 Wolfgang Denk <[email protected]> wrote:

Dear Boris,
Quoted 6 lines · In message <[email protected]> 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.

Quoted 3 lines

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.

Quoted 4 lines

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.

Wolfgang Denk

2018-12-04 10:14 UTC #5 lore raw

cover cmd: auto-complete args starting with a $

Dear Boris,

Quoted 11 lines · In message <20181204105448.63b9af8c@bbrezillon> you wrote:

Quoted 8 lines
Quoted 6 lines
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?

Quoted 8 lines

Quoted 2 lines
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.

Quoted 6 lines

Quoted 3 lines
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

Signature · 5 lines

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

Boris Brezillon

author

2018-12-04 10:33 UTC #6 lore raw

cover cmd: auto-complete args starting with a $

Hi Wolfgang,

Quoted 10 lines · On Tue, 04 Dec 2018 11:14:31 +0100 Wolfgang Denk <[email protected]> wrote:

Dear Boris,
Quoted 7 lines · In message <20181204105448.63b9af8c@bbrezillon> you wrote:
Quoted 7 lines
Quoted 6 lines
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.

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
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.

Quoted 14 lines

Quoted 9 lines
Quoted 3 lines
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.

Quoted 5 lines

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.

Quoted 1 line

-> 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 ${.

Quoted 7 lines

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.

Quoted 10 lines

Quoted 7 lines
Quoted 4 lines
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

Wolfgang Denk

2018-12-04 13:00 UTC #7 lore raw

cover cmd: auto-complete args starting with a $

Dear Boris,

Quoted 5 lines · In message <20181204113313.577178ac@bbrezillon> you wrote:

Quoted 3 lines
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.

Quoted 3 lines

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

Signature · 5 lines

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

Boris Brezillon

author

2018-12-04 13:11 UTC #8 lore raw

cover cmd: auto-complete args starting with a $

Quoted 9 lines · On Tue, 04 Dec 2018 14:00:47 +0100 Wolfgang Denk <[email protected]> wrote:

Dear Boris,
Quoted 5 lines · In message <20181204113313.577178ac@bbrezillon> you wrote:
Quoted 3 lines
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.

Quoted 8 lines

OK, then I misread the patches.
Quoted 3 lines
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---

cmd/echo.c +3 -2

index 5b018d9349ab..857f268c6a61 100644
@@ -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
 );

Boris Brezillon

author

2018-12-04 13:54 UTC #9 lore raw

cover cmd: auto-complete args starting with a $

In reply to Boris Brezillon

Quoted 10 lines · On Tue, 4 Dec 2018 11:33:13 +0100 Boris Brezillon…

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

common/command.c +8 -3

index 754ab9bbc396..b9a44d4ea7a8 100644
@@ -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) {