Excerpts from Mike Frysinger's message of Tue Oct 12 22:57:11 +0200 2010:
> On Tuesday, October 12, 2010 16:26:31 Jeroen Roovers wrote:
> > On Tue, 12 Oct 2010 22:09:06 +0200 Dirkjan Ochtman wrote:
> > > On Fri, Oct 1, 2010 at 15:07, Peter Volkov wrote:
> > > > [a very thorough review of the openvpn ebuild]
> > >
> > > Thanks for reviewing, I've fixed most of the issues.
> > >
> > > >> if [[ -n $(ls /etc/openvpn/*/local.conf 2>/dev/null) ]] ;
> > > >>
> > > >> then
> > > >
> > > > I'd suggested [ -e /etc/openvpn/*/local.conf ] here, but probably
> > > > there are better alternatives. Also ${ROOT} is missed here.
> > >
> > > I've put ${ROOT} in, are there no better alternatives? I don't think
> > > anyone mentioned any.
> >
> > for foo in ${ROOT}/etc/openvpn/*/local.conf; do
> > [ -e ${foo} ] && bar ${foo}
> > done
> >
> > If no ${ROOT}/etc/openvpn/*/local.conf is found, it returns the exact
> > string; which doesn't exist so Nothing Happens.
>
> i'd say doing a loop is worse than a `ls` hack. and this has quoting
> problems, but that's ancillary ...
> -mike

What about defining following function?

any_exists() {
local f

for f; do
[[ -e $f ]] && return 0
done

return 1
}
--
Amadeusz Żołnowski

PGP key fpr: C700 CEDE 0C18 212E 49DA 4653 F013 4531 E1DB FAB5

On Wednesday, October 13, 2010 04:23:16 Amadeusz Żołnowski wrote:
> Excerpts from Mike Frysinger's message of Tue Oct 12 22:57:11 +0200 2010:
> > On Tuesday, October 12, 2010 16:26:31 Jeroen Roovers wrote:
> > > On Tue, 12 Oct 2010 22:09:06 +0200 Dirkjan Ochtman wrote:
> > > > On Fri, Oct 1, 2010 at 15:07, Peter Volkov wrote:
> > > > > [a very thorough review of the openvpn ebuild]
> > > >
> > > > Thanks for reviewing, I've fixed most of the issues.
> > > >
> > > > >> if [[ -n $(ls /etc/openvpn/*/local.conf 2>/dev/null) ]] ;
> > > > >>
> > > > >> then
> > > > >
> > > > > I'd suggested [ -e /etc/openvpn/*/local.conf ] here, but probably
> > > > > there are better alternatives. Also ${ROOT} is missed here.
> > > >
> > > > I've put ${ROOT} in, are there no better alternatives? I don't think
> > > > anyone mentioned any.
> > >
> > > for foo in ${ROOT}/etc/openvpn/*/local.conf; do
> > >
> > > [ -e ${foo} ] && bar ${foo}
> > >
> > > done
> > >
> > > If no ${ROOT}/etc/openvpn/*/local.conf is found, it returns the exact
> > > string; which doesn't exist so Nothing Happens.
> >
> > i'd say doing a loop is worse than a `ls` hack. and this has quoting
> > problems, but that's ancillary ...

> What about defining following function?
>
> any_exists() {
> local f
>
> for f; do
> [[ -e $f ]] && return 0
> done
>
> return 1
> }

perhaps if it had a better name and were in a common location (eclass)
-mike

On Wednesday, October 13, 2010 03:08:24 Vaeth wrote:
> Mike Frysinger wrote:
> > > for foo in ${ROOT}/etc/openvpn/*/local.conf; do
> > >
> > > [ -e ${foo} ] && bar ${foo}
> > >
> > > done
> >
> > i'd say doing a loop is worse than a `ls` hack.
>
> Why do you think so? No external program on which you must rely,
> and if you put a "break" in there, the loop is just syntactical.
> Here is another possibility if you do not need positional
> parameters anymore:
>
> set -- "${ROOT}"/etc/openvpn/*/local.conf
> if test -e "${1}"
> then ...
> fi
>
> And if you do need, you can write a function:
>
> Test2() {
> test "${1}" "${2}"
> }
>
> if Test2 -e "${ROOT}"/etc/openvpn/*/local.conf
> then ...
> fi

relying on an external program to get a single line of readable code is better
than multiple lines of code that attempt to do the same thing. your further
examples here are even worse on many levels.
-mike

Mike Frysinger wrote:

> relying on an external program

The point is that external programs can have all sorts of undesired
side effects. For instance, if an directory is not readable (or
readable but not executable or is on $FILESYSTEM with who-knows-what
permissions or accessability problems) it can cause unexpected
errors which otherwise are treated by "standard" shell behavior:
*This* is why the ls is hackish here, not the readability.

About the readability, one can always have different opinions,
but your attack is inappropriate:

> to get a single line of readable code is better than
> multiple lines of code that attempt to do the same thing.

First, my suggestions are not "multiple lines" but only
exactly *two* lines (i.e. only one additional line):
Please compare

set -- "${ROOT}"/etc/openvpn/*/local.conf
if test -e "${1}"

with the "single line of readable code" which contains not less code,
but is only squashed into one line:

if [[ -e $(ls -1 -- "${ROOT}"/etc/openvpn/*/local.conf) ]]

Is this really so much more readable?
(The "--" can be omitted in both code pieces iff portage has a test
that ROOT does not start with "-").

And if you really want only readability, you should like even much
more my second suggestion which could also be squashed as two lines:

Exists() { test -e "${1}"; }
if Exists "${ROOT}"/etc/openvpn/*/local.conf

I would say this is *way* more readable than the complex 1-liner.
Of course, opinions may differ, but I think an unfounded attack as

> your further examples here are even worse on many levels.

is not appropriate here.

Regards
Martin

Excerpts from Mike Frysinger's message of Wed Oct 13 16:13:58 +0200 2010:
> On Wednesday, October 13, 2010 04:23:16 Amadeusz Żołnowski wrote:
> > Excerpts from Mike Frysinger's message of Tue Oct 12 22:57:11 +0200 2010:
> > > On Tuesday, October 12, 2010 16:26:31 Jeroen Roovers wrote:
> > > > On Tue, 12 Oct 2010 22:09:06 +0200 Dirkjan Ochtman wrote:
> > > > > On Fri, Oct 1, 2010 at 15:07, Peter Volkov wrote:
> > > > > > [a very thorough review of the openvpn ebuild]
> > > > >
> > > > > Thanks for reviewing, I've fixed most of the issues.
> > > > >
> > > > > >> if [[ -n $(ls /etc/openvpn/*/local.conf 2>/dev/null) ]] ;
> > > > > >>
> > > > > >> then
> > > > > >
> > > > > > I'd suggested [ -e /etc/openvpn/*/local.conf ] here, but probably
> > > > > > there are better alternatives. Also ${ROOT} is missed here.
> > > > >
> > > > > I've put ${ROOT} in, are there no better alternatives? I don't think
> > > > > anyone mentioned any.
> > > >
> > > > for foo in ${ROOT}/etc/openvpn/*/local.conf; do
> > > >
> > > > [ -e ${foo} ] && bar ${foo}
> > > >
> > > > done
> > > >
> > > > If no ${ROOT}/etc/openvpn/*/local.conf is found, it returns the exact
> > > > string; which doesn't exist so Nothing Happens.
> > >
> > > i'd say doing a loop is worse than a `ls` hack. and this has quoting
> > > problems, but that's ancillary ...
>
> > What about defining following function?
> >
> > any_exists() {
> > local f
> >
> > for f; do
> > [[ -e $f ]] && return 0
> > done
> >
> > return 1
> > }
>
> perhaps if it had a better name and were in a common location (eclass)
> -mike

So give it a better name. :-) In this case 'ls' shouldn't hurt anybody,
but such function solves problem in much more elegant manner -
regardless it's definied in an ebuild or eclass.
--
Amadeusz Żołnowski

PGP key fpr: C700 CEDE 0C18 212E 49DA 4653 F013 4531 E1DB FAB5

On Wed, 13 Oct 2010 10:13:58 -0400
Mike Frysinger <vapier@gentoo.org> wrote:

> > any_exists() {
> > local f
> >
> > for f; do
> > [[ -e $f ]] && return 0
> > done
> >
> > return 1
> > }
>
> perhaps if it had a better name and were in a common location (eclass)

has_file()?

--
Best regards,
Michał Górny

Excerpts from Michał Górny's message of Wed Oct 13 18:41:54 +0200 2010:
> On Wed, 13 Oct 2010 10:13:58 -0400
> Mike Frysinger <vapier@gentoo.org> wrote:
>
> > > any_exists() {
> > > local f
> > >
> > > for f; do
> > > [[ -e $f ]] && return 0
> > > done
> > >
> > > return 1
> > > }
> >
> > perhaps if it had a better name and were in a common location (eclass)
>
> has_file()?

What it would mean? „Has�?
--
Amadeusz Żołnowski

PGP key fpr: C700 CEDE 0C18 212E 49DA 4653 F013 4531 E1DB FAB5

On Wed, 13 Oct 2010 18:51:31 +0200
Amadeusz Żołnowski <aidecoe@aidecoe.name> wrote:

> > has_file()?
>
> What it would mean? „Has�?

It's reference to has() function specified by PMS. 'The file system has
a file named one-of ${@}' :P.

--
Best regards,
Michał Górny

On Wednesday, October 13, 2010 13:26:24 Michał Górny wrote:
> On Wed, 13 Oct 2010 18:51:31 +0200 Amadeusz Żołnowski wrote:
> > > has_file()?
> >
> > What it would mean? „Has�?
>
> It's reference to has() function specified by PMS. 'The file system has
> a file named one-of ${@}' :P.

the has functions are typically used to look up an item in a list. i'd go
with "path_exists" since this is doing -e and not -f.
-mike

On Wednesday, October 13, 2010 12:35:20 Vaeth wrote:
> Mike Frysinger wrote:
> > relying on an external program
>
> The point is that external programs can have all sorts of undesired
> side effects. For instance, if an directory is not readable (or
> readable but not executable or is on $FILESYSTEM with who-knows-what
> permissions or accessability problems) it can cause unexpected
> errors which otherwise are treated by "standard" shell behavior:
> *This* is why the ls is hackish here, not the readability.

and if you had read all my replies, you would see i already pointed out this
problem with the original code

> but your attack is inappropriate:

might want to review the meanings of these words. i said your examples were
worse, not better. i didnt say anything about you personally.

the rest of your e-mail isnt worth responding to, so i wont bother. as most
likely will any follow up to this.
-mike