ksh: quote empties

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

ksh: quote empties

Martijn Dekker
$ ksh -c 'trap "" CONT; trap'
trap --  CONT

That is not "suitable for re-entry into the shell". Empty words must be
quoted, or they disappear. Expected output:

trap -- '' CONT

Patch below. OK?

- M.

Index: misc.c
===================================================================
RCS file: /cvs/src/bin/ksh/misc.c,v
retrieving revision 1.70
diff -u -p -r1.70 misc.c
--- misc.c 9 Apr 2018 17:53:36 -0000 1.70
+++ misc.c 14 Apr 2018 22:44:39 -0000
@@ -966,6 +966,12 @@ print_value_quoted(const char *s)
  const char *p;
  int inquote = 0;

+ /* Check for empty */
+ if (!*p) {
+ shprintf("''");
+ return;
+ }
+
  /* Test if any quotes are needed */
  for (p = s; *p; p++)
  if (ctype(*p, C_QUOTE))

Reply | Threaded
Open this post in threaded view
|

Re: ksh: quote empties

Philip Guenther-2
On Sun, 15 Apr 2018, Martijn Dekker wrote:
> $ ksh -c 'trap "" CONT; trap'
> trap --  CONT
>
> That is not "suitable for re-entry into the shell". Empty words must be
> quoted, or they disappear. Expected output:
>
> trap -- '' CONT
>
> Patch below. OK?

That also changes the output of set, export, and alias:

: morgaine; ksh -c 'alias foo=""; alias -p; export foo=; set; export -p' | grep foo
alias foo=
foo=
export foo=
: morgaine; obj/ksh -c 'alias foo=""; alias -p; export foo=; set; export -p' | grep foo
alias foo=''
foo=''
export foo=''
: morgaine;

Are there any regress tests affected by this?


> --- misc.c 9 Apr 2018 17:53:36 -0000 1.70
> +++ misc.c 14 Apr 2018 22:44:39 -0000
> @@ -966,6 +966,12 @@ print_value_quoted(const char *s)
>   const char *p;
>   int inquote = 0;
>
> + /* Check for empty */
> + if (!*p) {
> + shprintf("''");
> + return;
> + }
> +

My bikeshed would move the handling into the unquoted case to make it just
a test+set:

--- misc.c      15 Mar 2018 16:51:29 -0000      1.69
+++ misc.c      15 Apr 2018 01:00:31 -0000
@@ -971,6 +971,9 @@ print_value_quoted(const char *s)
                if (ctype(*p, C_QUOTE))
                        break;
        if (!*p) {
+               /* handle zero-length value */
+               if (p == s)
+                       s = "''";
                shprintf("%s", s);
                return;
        }


...but I'm find with the original diff too.


Philip

Reply | Threaded
Open this post in threaded view
|

Re: ksh: quote empties

Klemens Nanni-2
On Sat, Apr 14, 2018 at 06:03:29PM -0700, Philip Guenther wrote:

> On Sun, 15 Apr 2018, Martijn Dekker wrote:
> > $ ksh -c 'trap "" CONT; trap'
> > trap --  CONT
> >
> > That is not "suitable for re-entry into the shell". Empty words must be
> > quoted, or they disappear. Expected output:
> >
> > trap -- '' CONT
> >
> > Patch below. OK?
>
> That also changes the output of set, export, and alias:
>
> : morgaine; ksh -c 'alias foo=""; alias -p; export foo=; set; export -p' | grep foo
> alias foo=
> foo=
> export foo=
> : morgaine; obj/ksh -c 'alias foo=""; alias -p; export foo=; set; export -p' | grep foo
> alias foo=''
> foo=''
> export foo=''
> : morgaine;
It also badly effects non-empty cases:

        $ ksh -c alias
        autoload='typeset -fu'
        functions='typeset -f'
        hash='alias -t'
        history='fc -l'
        integer='typeset -i'
        local=typeset
        login='exec login'
        nohup='nohup '
        r='fc -s'
        stop='kill -STOP'
        type='whence -v'
        $ ./obj/ksh -c alias
        autoload=''
        functions=''
        hash=''
        history=''
        integer=''
        local=''
        login=''
        nohup=''
        r=''
        stop=''
        type=''

Reply | Threaded
Open this post in threaded view
|

Re: ksh: quote empties

Philip Guenther-2
On Sun, 15 Apr 2018, Klemens Nanni wrote:
> It also badly effects non-empty cases:
...
> $ ./obj/ksh -c alias
> autoload=''
> functions=''

Hah!  The original diff i actually broken (it tests the wrong variable)
but I fixed that by accident when I manually made the diff in my tree!

So, uh, I'm no longer fine with the original diff...


Philip

Reply | Threaded
Open this post in threaded view
|

Re: ksh: quote empties

Martijn Dekker
In reply to this post by Philip Guenther-2
Op 15-04-18 om 03:03 schreef Philip Guenther:

> On Sun, 15 Apr 2018, Martijn Dekker wrote:
>> $ ksh -c 'trap "" CONT; trap'
>> trap --  CONT
>>
>> That is not "suitable for re-entry into the shell". Empty words must be
>> quoted, or they disappear. Expected output:
>>
>> trap -- '' CONT
>>
>> Patch below. OK?
>
> That also changes the output of set, export, and alias:

Yes, but harmlessly. ('readonly -p' and 'typeset', too.)

The change also matches the behaviour of every other POSIXy shell,
including AT&T ksh88 and ksh93.

An alternative would be to special-case empty trap actions in the 'trap'
builtin, but in my view that would be (a) ugly and (b) asking for future
bugs if another builtin ever needs to print separate shell-quoted words.

> Are there any regress tests affected by this?

I should have mentioned that I ran them, and the results before and
after the patch are identical (11 failed "as expected", 245 passed).

> My bikeshed would move the handling into the unquoted case to make it just
> a test+set:
>
> --- misc.c      15 Mar 2018 16:51:29 -0000      1.69
> +++ misc.c      15 Apr 2018 01:00:31 -0000
> @@ -971,6 +971,9 @@ print_value_quoted(const char *s)
>                  if (ctype(*p, C_QUOTE))
>                          break;
>          if (!*p) {
> +               /* handle zero-length value */
> +               if (p == s)
> +                       s = "''";
>                  shprintf("%s", s);
>                  return;
>          }
>
>
> ...but I'm find with the original diff too.

FWIW, your way seems fine to me as well.

Thanks,

- M.

Reply | Threaded
Open this post in threaded view
|

Re: ksh: quote empties

Martijn Dekker
In reply to this post by Philip Guenther-2
Op 15-04-18 om 03:41 schreef Philip Guenther:

> On Sun, 15 Apr 2018, Klemens Nanni wrote:
>> It also badly effects non-empty cases:
> ...
>> $ ./obj/ksh -c alias
>> autoload=''
>> functions=''
>
> Hah!  The original diff i actually broken (it tests the wrong variable)
> but I fixed that by accident when I manually made the diff in my tree!
>
> So, uh, I'm no longer fine with the original diff...

D'oh!

That's embarrassing. Sorry about that. :-/

- M.

Index: misc.c
===================================================================
RCS file: /cvs/src/bin/ksh/misc.c,v
retrieving revision 1.70
diff -u -p -r1.70 misc.c
--- misc.c 9 Apr 2018 17:53:36 -0000 1.70
+++ misc.c 15 Apr 2018 02:06:55 -0000
@@ -966,6 +966,12 @@ print_value_quoted(const char *s)
  const char *p;
  int inquote = 0;

+ /* Check for empty */
+ if (!*s) {
+ shprintf("''");
+ return;
+ }
+
  /* Test if any quotes are needed */
  for (p = s; *p; p++)
  if (ctype(*p, C_QUOTE))

Reply | Threaded
Open this post in threaded view
|

Re: ksh: quote empties

Martijn Dekker
Op 15-04-18 om 04:09 schreef Martijn Dekker:

> Op 15-04-18 om 03:41 schreef Philip Guenther:
>> On Sun, 15 Apr 2018, Klemens Nanni wrote:
>>> It also badly effects non-empty cases:
>> ...
>>>     $ ./obj/ksh -c alias
>>>     autoload=''
>>>     functions=''
>>
>> Hah!  The original diff i actually broken (it tests the wrong variable)
>> but I fixed that by accident when I manually made the diff in my tree!
>>
>> So, uh, I'm no longer fine with the original diff...
>
> D'oh!
>
> That's embarrassing. Sorry about that. :-/

Here are a couple of regression tests.

- M.

Index: regress/bin/ksh/syntax.t
===================================================================
RCS file: /cvs/src/regress/bin/ksh/syntax.t,v
retrieving revision 1.1
diff -u -p -r1.1 syntax.t
--- regress/bin/ksh/syntax.t 2 Dec 2013 20:39:44 -0000 1.1
+++ regress/bin/ksh/syntax.t 15 Apr 2018 02:44:48 -0000
@@ -8,3 +8,36 @@ expected-stderr-pattern:
  /syntax error/
  ---

+name: syntax-quoted-output-1
+description:
+ Check that 'trap' outputs properly quoted trap actions
+stdin:
+ trap "" INFO
+ trap foo USR1
+ trap foo\ bar USR2
+ trap
+expected-exit: e == 0
+expected-stdout:
+ trap -- '' INFO
+ trap -- foo USR1
+ trap -- 'foo bar' USR2
+---
+
+name: syntax-quoted-output-2
+description:
+ Check that 'alias' outputs properly quoted aliases
+stdin:
+ unalias -a
+ alias empty=
+ alias oneword=foo
+ alias twowords=foo\ bar
+ alias extrablanks=" foo bar "
+ alias
+expected-exit: e == 0
+expected-stdout:
+ empty=''
+ extrablanks=' foo bar '
+ oneword=foo
+ twowords='foo bar'
+---
+