Re: [PATCH] etc/ksh.kshrc - unify command substitution

[Raf Czlonka]

Hi all,

So there's one OK - anyone else?

Would anyone be so kind as to commit it, please? :^)

Regards,

Raf

On Mon, Oct 23, 2017 at 06:32:03PM BST, Alexander Hall wrote:
> I'm OK with this.
> 
> /Alexander
> 
> 
> On October 23, 2017 3:29:57 PM GMT+02:00, Raf Czlonka  
> wrote:
> >What say you?
> >
> >On Tue, Aug 29, 2017 at 08:44:43PM BST, Raf Czlonka wrote:
> >> Ping.
> >> 
> >> Anyone?
> >> 
> >> On Sun, Jul 16, 2017 at 01:43:32PM BST, Raf Czlonka wrote:
> >> > Hi all,
> >> > 
> >> > Further simplification - 'ps | grep' can be replaced by pgrep(1)
> >> > and if-then-fi by &&.
> >> > 
> >> > > While there:
> >> > > 
> >> > > - remove ':' (null utility) from the very first line of the file
> >-
> >> > >   I *do* understand what it does but it doesn't seem like it's
> >needed
> >> > >   at all, unless I'm missing something (as is the case with some
> >idioms)
> >> > > [...]
> >> > > 
> >> > 
> >> > As it transpired, this does indeed seem to be an old idiom denoting
> >> > a Bourne shell script.
> >> > 
> >> > To quote rpe@: "I guess it's fine to remove the : line in 2017."
> >> > 
> >> > I agree.
> >> > 
> >> > Thanks again to Robert for all the feedback and suggestions.
> >> > 
> >> > Regards,
> >> > 
> >> > Raf
> >> > 
> >> > Index: etc/ksh.kshrc
> >> > ===
> >> > RCS file: /cvs/src/etc/ksh.kshrc,v
> >> > retrieving revision 1.28
> >> > diff -u -p -r1.28 ksh.kshrc
> >> > --- etc/ksh.kshrc15 Jul 2017 07:11:42 -  1.28
> >> > +++ etc/ksh.kshrc16 Jul 2017 11:49:55 -
> >> > @@ -1,4 +1,3 @@
> >> > -:
> >> >  #   $OpenBSD: ksh.kshrc,v 1.28 2017/07/15 07:11:42 tb Exp $
> >> >  #
> >> >  # NAME:
> >> > @@ -74,9 +73,7 @@ case "$-" in
> >> >  xterm*)
> >> >  ILS='\033]1;'; ILE='\007'
> >> >  WLS='\033]2;'; WLE='\007'
> >> > -if ps -p $PPID -o command | grep -q telnet; then
> >> > -export TERM=xterms
> >> > -fi
> >> > +pgrep -qxs $PPID telnet && export TERM=xterms
> >> >  ;;
> >> >  *)  ;;
> >> >  esac

Re: [PATCH] etc/ksh.kshrc - unify command substitution

[Alexander Hall]

I'm OK with this.

/Alexander


On October 23, 2017 3:29:57 PM GMT+02:00, Raf Czlonka  
wrote:
>What say you?
>
>On Tue, Aug 29, 2017 at 08:44:43PM BST, Raf Czlonka wrote:
>> Ping.
>> 
>> Anyone?
>> 
>> On Sun, Jul 16, 2017 at 01:43:32PM BST, Raf Czlonka wrote:
>> > Hi all,
>> > 
>> > Further simplification - 'ps | grep' can be replaced by pgrep(1)
>> > and if-then-fi by &&.
>> > 
>> > > While there:
>> > > 
>> > > - remove ':' (null utility) from the very first line of the file
>-
>> > >   I *do* understand what it does but it doesn't seem like it's
>needed
>> > >   at all, unless I'm missing something (as is the case with some
>idioms)
>> > > [...]
>> > > 
>> > 
>> > As it transpired, this does indeed seem to be an old idiom denoting
>> > a Bourne shell script.
>> > 
>> > To quote rpe@: "I guess it's fine to remove the : line in 2017."
>> > 
>> > I agree.
>> > 
>> > Thanks again to Robert for all the feedback and suggestions.
>> > 
>> > Regards,
>> > 
>> > Raf
>> > 
>> > Index: etc/ksh.kshrc
>> > ===
>> > RCS file: /cvs/src/etc/ksh.kshrc,v
>> > retrieving revision 1.28
>> > diff -u -p -r1.28 ksh.kshrc
>> > --- etc/ksh.kshrc  15 Jul 2017 07:11:42 -  1.28
>> > +++ etc/ksh.kshrc  16 Jul 2017 11:49:55 -
>> > @@ -1,4 +1,3 @@
>> > -:
>> >  # $OpenBSD: ksh.kshrc,v 1.28 2017/07/15 07:11:42 tb Exp $
>> >  #
>> >  # NAME:
>> > @@ -74,9 +73,7 @@ case "$-" in
>> >xterm*)
>> >ILS='\033]1;'; ILE='\007'
>> >WLS='\033]2;'; WLE='\007'
>> > -  if ps -p $PPID -o command | grep -q telnet; then
>> > -  export TERM=xterms
>> > -  fi
>> > +  pgrep -qxs $PPID telnet && export TERM=xterms
>> >;;
>> >*)  ;;
>> >esac

Re: [PATCH] etc/ksh.kshrc - unify command substitution

[Raf Czlonka]

What say you?

On Tue, Aug 29, 2017 at 08:44:43PM BST, Raf Czlonka wrote:
> Ping.
> 
> Anyone?
> 
> On Sun, Jul 16, 2017 at 01:43:32PM BST, Raf Czlonka wrote:
> > Hi all,
> > 
> > Further simplification - 'ps | grep' can be replaced by pgrep(1)
> > and if-then-fi by &&.
> > 
> > > While there:
> > > 
> > > - remove ':' (null utility) from the very first line of the file -
> > >   I *do* understand what it does but it doesn't seem like it's needed
> > >   at all, unless I'm missing something (as is the case with some idioms)
> > > [...]
> > > 
> > 
> > As it transpired, this does indeed seem to be an old idiom denoting
> > a Bourne shell script.
> > 
> > To quote rpe@: "I guess it's fine to remove the : line in 2017."
> > 
> > I agree.
> > 
> > Thanks again to Robert for all the feedback and suggestions.
> > 
> > Regards,
> > 
> > Raf
> > 
> > Index: etc/ksh.kshrc
> > ===
> > RCS file: /cvs/src/etc/ksh.kshrc,v
> > retrieving revision 1.28
> > diff -u -p -r1.28 ksh.kshrc
> > --- etc/ksh.kshrc   15 Jul 2017 07:11:42 -  1.28
> > +++ etc/ksh.kshrc   16 Jul 2017 11:49:55 -
> > @@ -1,4 +1,3 @@
> > -:
> >  #  $OpenBSD: ksh.kshrc,v 1.28 2017/07/15 07:11:42 tb Exp $
> >  #
> >  # NAME:
> > @@ -74,9 +73,7 @@ case "$-" in
> > xterm*)
> > ILS='\033]1;'; ILE='\007'
> > WLS='\033]2;'; WLE='\007'
> > -   if ps -p $PPID -o command | grep -q telnet; then
> > -   export TERM=xterms
> > -   fi
> > +   pgrep -qxs $PPID telnet && export TERM=xterms
> > ;;
> > *)  ;;
> > esac

Re: [PATCH] etc/ksh.kshrc - unify command substitution

[Raf Czlonka]

Ping.

Anyone?

On Sun, Jul 16, 2017 at 01:43:32PM BST, Raf Czlonka wrote:
> Hi all,
> 
> Further simplification - 'ps | grep' can be replaced by pgrep(1)
> and if-then-fi by &&.
> 
> > While there:
> > 
> > - remove ':' (null utility) from the very first line of the file -
> >   I *do* understand what it does but it doesn't seem like it's needed
> >   at all, unless I'm missing something (as is the case with some idioms)
> > [...]
> > 
> 
> As it transpired, this does indeed seem to be an old idiom denoting
> a Bourne shell script.
> 
> To quote rpe@: "I guess it's fine to remove the : line in 2017."
> 
> I agree.
> 
> Thanks again to Robert for all the feedback and suggestions.
> 
> Regards,
> 
> Raf
> 
> Index: etc/ksh.kshrc
> ===
> RCS file: /cvs/src/etc/ksh.kshrc,v
> retrieving revision 1.28
> diff -u -p -r1.28 ksh.kshrc
> --- etc/ksh.kshrc 15 Jul 2017 07:11:42 -  1.28
> +++ etc/ksh.kshrc 16 Jul 2017 11:49:55 -
> @@ -1,4 +1,3 @@
> -:
>  #$OpenBSD: ksh.kshrc,v 1.28 2017/07/15 07:11:42 tb Exp $
>  #
>  # NAME:
> @@ -74,9 +73,7 @@ case "$-" in
>   xterm*)
>   ILS='\033]1;'; ILE='\007'
>   WLS='\033]2;'; WLE='\007'
> - if ps -p $PPID -o command | grep -q telnet; then
> - export TERM=xterms
> - fi
> + pgrep -qxs $PPID telnet && export TERM=xterms
>   ;;
>   *)  ;;
>   esac

Re: [PATCH] etc/ksh.kshrc - unify command substitution

[Raf Czlonka]

Hi all,

Further simplification - 'ps | grep' can be replaced by pgrep(1)
and if-then-fi by &&.

> While there:
> 
> - remove ':' (null utility) from the very first line of the file -
>   I *do* understand what it does but it doesn't seem like it's needed
>   at all, unless I'm missing something (as is the case with some idioms)
> [...]
> 

As it transpired, this does indeed seem to be an old idiom denoting
a Bourne shell script.

To quote rpe@: "I guess it's fine to remove the : line in 2017."

I agree.

Thanks again to Robert for all the feedback and suggestions.

Regards,

Raf

Index: etc/ksh.kshrc
===
RCS file: /cvs/src/etc/ksh.kshrc,v
retrieving revision 1.28
diff -u -p -r1.28 ksh.kshrc
--- etc/ksh.kshrc   15 Jul 2017 07:11:42 -  1.28
+++ etc/ksh.kshrc   16 Jul 2017 11:49:55 -
@@ -1,4 +1,3 @@
-:
 #  $OpenBSD: ksh.kshrc,v 1.28 2017/07/15 07:11:42 tb Exp $
 #
 # NAME:
@@ -74,9 +73,7 @@ case "$-" in
xterm*)
ILS='\033]1;'; ILE='\007'
WLS='\033]2;'; WLE='\007'
-   if ps -p $PPID -o command | grep -q telnet; then
-   export TERM=xterms
-   fi
+   pgrep -qxs $PPID telnet && export TERM=xterms
;;
*)  ;;
esac

Re: [PATCH] etc/ksh.kshrc - unify command substitution

[Raf Czlonka]

On Fri, Jul 07, 2017 at 05:47:46AM BST, Raf Czlonka wrote:
> Hi all,
> 
> I've noticed that etc/ksh.kshrc uses both types of command substitution
> `command` and $(command). The below diff unifies it and uses
> $(command) notation consistently.
> 
> While there, [...] remove basename(1) invocation and use parameter
> expansion instead.
> [...]
> 

Hi all,

Take two - only the above. Already got OKs from rpe@ so, if there
are no objections, would someone be so kind as to committing it,
please? :^)

Cheers,

Raf

Index: etc/ksh.kshrc
===
RCS file: /cvs/src/etc/ksh.kshrc,v
retrieving revision 1.27
diff -u -p -r1.27 ksh.kshrc
--- etc/ksh.kshrc   14 Sep 2016 18:34:51 -  1.27
+++ etc/ksh.kshrc   15 Jul 2017 05:45:01 -
@@ -39,7 +39,7 @@ case "$-" in
0) PS1S='# ';;
esac
PS1S=${PS1S:-'$ '}
-   HOSTNAME=${HOSTNAME:-`uname -n`}
+   HOSTNAME=${HOSTNAME:-$(uname -n)}
HOST=${HOSTNAME%%.*}
 
PROMPT="$USER:!$PS1S"
@@ -49,8 +49,8 @@ case "$-" in
PS1=$PPROMPT
# $TTY is the tty we logged in on,
# $tty is that which we are in now (might by pty)
-   tty=`tty`
-   tty=`basename $tty`
+   tty=$(tty)
+   tty=${tty##*/}
TTY=${TTY:-$tty}
# $console is the system console device
console=$(sysctl kern.consdev)
@@ -117,7 +117,7 @@ case "$-" in
alias o='fg %-'
alias df='df -k'
alias du='du -k'
-   alias rsize='eval `resize`'
+   alias rsize='eval $(resize)'
 ;;
 *) # non-interactive
 ;;
@@ -142,6 +142,6 @@ function pre_path {
 }
 # if $1 is in path, remove it
 function del_path {
-   no_path $* || eval ${2:-PATH}=`eval echo :'$'${2:-PATH}: |
-   sed -e "s;:$1:;:;g" -e "s;^:;;" -e "s;:\$;;"`
+   no_path $* || eval ${2:-PATH}=$(eval echo :'$'${2:-PATH}: |
+   sed -e "s;:$1:;:;g" -e "s;^:;;" -e "s;:\$;;")
 }

Re: [PATCH] etc/ksh.kshrc - unify command substitution

[Raf Czlonka]

On Mon, Jul 10, 2017 at 06:48:25PM BST, Robert Peichaer wrote:
> On Fri, Jul 07, 2017 at 05:47:46AM +0100, Raf Czlonka wrote:
> > Hi all,
> > 
> > I've noticed that etc/ksh.kshrc uses both types of command substitution
> > `command` and $(command). The below diff unifies it and uses
> > $(command) notation consistently.
> > 
> > While there:
> > 
> > - remove ':' (null utility) from the very first line of the file -
> >   I *do* understand what it does but it doesn't seem like it's needed
> >   at all, unless I'm missing something (as is the case with some idioms)
> > - remove basename(1) invocation and use parameter expansion instead
> > - simplify one if conditional by replacing it with && and grouping
> >   commands
> 
> Having sent a lot of similar diffs myself, I recommend to only change
> one thing in a diff and not to mix stuff. Better send two diffs with one
> type of change each than one diff with multiple.

Hi Robert,

Thanks for the feedback and OKs.

The other changes seemed like too small a ones to warrant separate diffs.

> > Regards,
> > 
> > Raf
> > 
> > Index: etc/ksh.kshrc
> > ===
> > RCS file: /cvs/src/etc/ksh.kshrc,v
> > retrieving revision 1.27
> > diff -u -p -u -r1.27 ksh.kshrc
> > --- etc/ksh.kshrc   14 Sep 2016 18:34:51 -  1.27
> > +++ etc/ksh.kshrc   7 Jul 2017 04:38:58 -
> > @@ -1,4 +1,3 @@
> > -:
> >  #  $OpenBSD: ksh.kshrc,v 1.27 2016/09/14 18:34:51 rpe Exp $
> >  #
> >  # NAME:
> > @@ -39,7 +38,7 @@ case "$-" in
> > 0) PS1S='# ';;
> > esac
> > PS1S=${PS1S:-'$ '}
> > -   HOSTNAME=${HOSTNAME:-`uname -n`}
> > +   HOSTNAME=${HOSTNAME:-$(uname -n)}
> 
> OK
> 
> > HOST=${HOSTNAME%%.*}
> >  
> > PROMPT="$USER:!$PS1S"
> > @@ -49,8 +48,8 @@ case "$-" in
> > PS1=$PPROMPT
> > # $TTY is the tty we logged in on,
> > # $tty is that which we are in now (might by pty)
> > -   tty=`tty`
> > -   tty=`basename $tty`
> > +   tty=$(tty)
> > +   tty=${tty##*/}
> 
> OK
> 
> > TTY=${TTY:-$tty}
> > # $console is the system console device
> > console=$(sysctl kern.consdev)
> > @@ -74,9 +73,8 @@ case "$-" in
> > xterm*)
> > ILS='\033]1;'; ILE='\007'
> > WLS='\033]2;'; WLE='\007'
> > -   if ps -p $PPID -o command | grep -q telnet; then
> > +   { ps -p $PPID -o command | grep -q telnet; } &&
> > export TERM=xterms
> > -   fi
> 
> If at all this would be
> 
>   ps -p $PPID -o command | grep -q telnet &&
>   export TERM=xterms
> 
> But I doubt this is any improvement compared to if-then-else.
> In contrast to the installer script, there is no need for a
> terse shell scripting style.

I "automatically" grouped them as to ensure the exit code of the
whole sequence rather than the last command - obviously here I've
done it unnecessarily.

While I tried to remove if-then-fi (I only ever try doing this if there
is no *else*) I could not see the wood for the trees - the above
*can* actually be improved using pgrep(1):

pgrep -P $PPID -q telnet && export TERM=xterms

We not only got rid of if-then-fi, but there's also one process less.

That's surely an improvement! :^)

> > ;;
> > *)  ;;
> > esac
> > @@ -117,7 +115,7 @@ case "$-" in
> > alias o='fg %-'
> > alias df='df -k'
> > alias du='du -k'
> > -   alias rsize='eval `resize`'
> > +   alias rsize='eval $(resize)'
> 
> OK
> 
> >  ;;
> >  *) # non-interactive
> >  ;;
> > @@ -142,6 +140,6 @@ function pre_path {
> >  }
> >  # if $1 is in path, remove it
> >  function del_path {
> > -   no_path $* || eval ${2:-PATH}=`eval echo :'$'${2:-PATH}: |
> > -   sed -e "s;:$1:;:;g" -e "s;^:;;" -e "s;:\$;;"`
> > +   no_path $* || eval ${2:-PATH}=$(eval echo :'$'${2:-PATH}: |
> > +   sed -e "s;:$1:;:;g" -e "s;^:;;" -e "s;:\$;;")
> 
> OK
> 
> >  }
> > 
> 
> -- 
> -=[rpe]=-

Best regards,

Raf

Re: [PATCH] etc/ksh.kshrc - unify command substitution

[Robert Peichaer]

On Fri, Jul 07, 2017 at 05:47:46AM +0100, Raf Czlonka wrote:
> Hi all,
> 
> I've noticed that etc/ksh.kshrc uses both types of command substitution
> `command` and $(command). The below diff unifies it and uses
> $(command) notation consistently.
> 
> While there:
> 
> - remove ':' (null utility) from the very first line of the file -
>   I *do* understand what it does but it doesn't seem like it's needed
>   at all, unless I'm missing something (as is the case with some idioms)
> - remove basename(1) invocation and use parameter expansion instead
> - simplify one if conditional by replacing it with && and grouping
>   commands

Having sent a lot of similar diffs myself, I recommend to only change
one thing in a diff and not to mix stuff. Better send two diffs with one
type of change each than one diff with multiple.

> Regards,
> 
> Raf
> 
> Index: etc/ksh.kshrc
> ===
> RCS file: /cvs/src/etc/ksh.kshrc,v
> retrieving revision 1.27
> diff -u -p -u -r1.27 ksh.kshrc
> --- etc/ksh.kshrc 14 Sep 2016 18:34:51 -  1.27
> +++ etc/ksh.kshrc 7 Jul 2017 04:38:58 -
> @@ -1,4 +1,3 @@
> -:
>  #$OpenBSD: ksh.kshrc,v 1.27 2016/09/14 18:34:51 rpe Exp $
>  #
>  # NAME:
> @@ -39,7 +38,7 @@ case "$-" in
>   0) PS1S='# ';;
>   esac
>   PS1S=${PS1S:-'$ '}
> - HOSTNAME=${HOSTNAME:-`uname -n`}
> + HOSTNAME=${HOSTNAME:-$(uname -n)}

OK

>   HOST=${HOSTNAME%%.*}
>  
>   PROMPT="$USER:!$PS1S"
> @@ -49,8 +48,8 @@ case "$-" in
>   PS1=$PPROMPT
>   # $TTY is the tty we logged in on,
>   # $tty is that which we are in now (might by pty)
> - tty=`tty`
> - tty=`basename $tty`
> + tty=$(tty)
> + tty=${tty##*/}

OK

>   TTY=${TTY:-$tty}
>   # $console is the system console device
>   console=$(sysctl kern.consdev)
> @@ -74,9 +73,8 @@ case "$-" in
>   xterm*)
>   ILS='\033]1;'; ILE='\007'
>   WLS='\033]2;'; WLE='\007'
> - if ps -p $PPID -o command | grep -q telnet; then
> + { ps -p $PPID -o command | grep -q telnet; } &&
>   export TERM=xterms
> - fi

If at all this would be

ps -p $PPID -o command | grep -q telnet &&
export TERM=xterms

But I doubt this is any improvement compared to if-then-else.
In contrast to the installer script, there is no need for a
terse shell scripting style.

>   ;;
>   *)  ;;
>   esac
> @@ -117,7 +115,7 @@ case "$-" in
>   alias o='fg %-'
>   alias df='df -k'
>   alias du='du -k'
> - alias rsize='eval `resize`'
> + alias rsize='eval $(resize)'

OK

>  ;;
>  *)   # non-interactive
>  ;;
> @@ -142,6 +140,6 @@ function pre_path {
>  }
>  # if $1 is in path, remove it
>  function del_path {
> - no_path $* || eval ${2:-PATH}=`eval echo :'$'${2:-PATH}: |
> - sed -e "s;:$1:;:;g" -e "s;^:;;" -e "s;:\$;;"`
> + no_path $* || eval ${2:-PATH}=$(eval echo :'$'${2:-PATH}: |
> + sed -e "s;:$1:;:;g" -e "s;^:;;" -e "s;:\$;;")

OK

>  }
> 

-- 
-=[rpe]=-