[linux-lvm] [PATCH 01/35] fsadm: Add "create" command

[Stephane Chazelas]

2011-09-22 11:28:36 +0200, Lukas Czerner:
[...]
> > > +	if [ $? -ne 0 ]; then
> > > +		error "FAILED. Exitting!"
> > > +	fi
> > 
> > "$@" || error ...
> 
> I guess it is just a matter of taste.
> 
> > 
> > or
> > 
> > if ! "$@"; then
> >   error ...
> > fi
> 
> That is really not very readable.
[...]

That is how shell works. The syntax is

if
  list-of-commands
then
  other-list-of-commands
else
  yet-another-list-of-commands
fi

I find [ "$?" -ne 0 ] above very confusion and illegible myself.

Why would you run another command that fails if the last one
succeeded? 

> > > +}
> > > +
> > > +is_natural() {
> > > +	test "$1" -ge 0 &> /dev/null && return 1
> > 
> > 
> > &> is a bashism.
> 
> Probably (#!/bin/bash) :)

But it is called "fsadm.sh", that's misleading.

[...]
> > Again, I think that should be either:
> > 
> > set --
> > [ -n "$YES" ] &&
> >   set -- -F
> > dry "mkfs.$fstyp" "$@" -b "$(($bsize*1024))" -E stride="${stride},stripe-width=${stripewidth}" "$device"
> > 
> > or:
> > 
> > force=
> > [ -n "$YES" ] &&
> >   force=-F
> > 
> > eval 'dry "mkfs.$fstyp" '"$force"' -b "$(($bsize*1024))" -E stride="${stride},stripe-width=${stripewidth}" "$device"'
> 
> Ok, I do understand the that I should rather use 'eval', but I do not
> understand why you're trying to get rid of the 'if', it is a bit longer,
> so what ? But is is also more obvious.

"if" is fine, it was just to save me some typing.

> > 
> > [...]
> > > +	is_natural $NEWSIZE
> > > +	[ $? -ne 1 ] && error "$NEWSIZE is not valid number for file system size"
> > 
> > With a fixed is_natural,
> > 
> > is_natural "$NEWSIZE" || error ...
> > 
> > [...]
> > > +	for i in $@; do
> > 
> > for i do
> 
> I am not sure what is wrong with that, it is more obvious and it works
> just fine.

for i in "$@"; do

would have been fine. Again, I can't think of any circumstance
where leaving $@ unquoted would make sense.

-- 
Stephane