Re: [LTP] [PATCH] Fix some bashisms

["Jiří Paleček" <jpalecek@web.de>]

On Tue, 07 Jul 2009 02:31:25 +0200, Garrett Cooper <yanegomi@gmail.com>  
wrote:

> 2009/7/6 Jiří Paleček <jpalecek@web.de>:
>> On Tue, 07 Jul 2009 01:33:29 +0200, Garrett Cooper <yanegomi@gmail.com>
>> wrote:
>>
>>> On Mon, Jul 6, 2009 at 3:47 PM, Jiri Palecek<jpalecek@web.de> wrote:
>>>>
>>>> On Monday 06 July 2009 19:45:08 Garrett Cooper wrote:
>>>>>
>>>>> 2. All numeric test(1) comparisons could and should be switched from:
>>>>>
>>>>> command
>>>>> if [ $? = 0 ]; then
>>>>>
>>>>> to:
>>>>>
>>>>> if command; then
>>>>>
>>>>> for brevity.
>>>>
>>>> Why should them be changed? This is only a cosmetic change, which  
>>>> would
>>>> make the patch unnecessarily larger (when it's already a little too  
>>>> large
>>>> IMHO). If it could be in the former form till now, I think it can  
>>>> stay there
>>>> a few months/years.
>>>>
>>>> IMHO this usage is still much better than
>>>>
>>>>  command; RC=$?
>>>>  if [ $RC = 0 ] ...
>>>>  ... RC is not read later ...
>>>>
>>>> or (incorrect)
>>>>
>>>>  command || RC=$?
>>>>  if [ $RC = 0 ] ...
>>>
>>> Yes, I agree with what you said about the above usage, but if you're
>>> not using $? for other than just the [ $? -ne 0 ], then I wouldn't
>>> even bother doing that, because if command; then saves a fork-exec,
>>> and an additional line in the source. As long as it's readable and
>>> doesn't expand multiple lines with a trailing \, you're fine.
>>
>> Are you talking only about efficiency and bash scripts? :-)
>
> Efficiency, and more importantly readability.
>
>> Anyway, if you want to save the fork-exec, you can just use a shell  
>> with a
>> test builtin (ksh, bash, dash ... and I believe many others do).
>>
>> If you are concerned about readability, I suggest considering another
>> alternative:
>>
>>  command || {
>>   do_something_on_error
>>  }
>>
>>  command && {
>>   do_something_when_ok
>>  }
>>
>> Of course, usable only when there's no "else" branch.
>
> That's ugly and hard to read with large logic blocks.

I don't think so; I have no problems with a similar syntax in C, moreover,  
I consider the braces more distinguishable than keywords.

>> Still, I think the original form is correct and quite straightforward. I
>> would probably not use the extra test myself, but I don't feel like it  
>> needs
>> changing. Just looking at the patch, I see places where the check is  
>> done
>> really suspiciously:
>>
>>
>>        tst_resm TINFO "Test #1: changing mtu size of eth0:1 device."
>>
>> -       ip link set eth0:1 mtu 300 &>$LTPTMP/tst_ip.err
>> +       ip link set eth0:1 mtu 300 >$LTPTMP/tst_ip.err 2>&1
>>        if [ $RC -ne 0 ]
>>        then
>>                tst_res TFAIL $LTPTMP/tst_ip.err \
>>
>> I think this needs much more attention than "calling [ twice" or "extra  
>> line
>> in the script".
>
> Yeah, true. Why not run everything through set -u then?

It would not help, IMO. At least in this particular case, the variable is  
properly initialized.

Regards
     Jiri Palecek

------------------------------------------------------------------------------
Enter the BlackBerry Developer Challenge  
This is your chance to win up to $100,000 in prizes! For a limited time, 
vendors submitting new applications to BlackBerry App World(TM) will have 
the opportunity to enter the BlackBerry Developer Challenge. See full prize 
details at: http://p.sf.net/sfu/blackberry
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list