[arch-projects] [initscripts] [PATCH 04/12] functions: fix indentation

[Tom Gundersen]

On Jun 25, 2011, at 20:11, Dave Reisner <d at falconindy.com> wrote:

> On Sat, Jun 25, 2011 at 12:15:56PM +0200, Kurt J. Bosch wrote:
>> ---
>> functions |   50 +++++++++++++++++++++++++-------------------------
>> 1 files changed, 25 insertions(+), 25 deletions(-)
>> 
>> diff --git a/functions b/functions
>> index 61cf3dd..9c881c1 100644
>> --- a/functions
>> +++ b/functions
>> @@ -285,17 +285,17 @@ kill_everything() {
>>    # Terminate all processes
>>    stat_busy "Sending SIGTERM To Processes"
>>    run_hook "$1_prekillall"
>> -    local pid k5args=""
>> -    for pid in ${omit_pids[@]}; do
>> -        k5args+=" -o $pid"
>> -    done
>> -    /sbin/killall5 -15 $k5args &>/dev/null
>> -    /bin/sleep 5
>> +        local pid k5args=""
>> +        for pid in ${omit_pids[@]}; do
>> +            k5args+=" -o $pid"
>> +        done
> 
> I'd rather just see this whole chunk of code go away. Building the extra
> variable is moot when you can just use a PE:
> 
>  killall 5 -15 ${omit_pids[@]/#/-o }
> 
> d
> 
>> +        /sbin/killall5 -15 $k5args &>/dev/null
>> +        /bin/sleep 5
>>    stat_done
>> 
>>    stat_busy "Sending SIGKILL To Processes"
>> -    /sbin/killall5 -9 $k5args &>/dev/null
>> -    /bin/sleep 1
>> +        /sbin/killall5 -9 $k5args &>/dev/null
>> +        /bin/sleep 1
>>    stat_done
>> 
>>    run_hook "$1_postkillall"
>> @@ -371,13 +371,13 @@ NETFS="nonfs,nonfs4,nosmbfs,nocifs,nocodafs,noncpfs,nosysfs,noshfs,nofuse,nofuse
>> # Check local filesystems
>> fsck_all() {
>>    stat_busy "Checking Filesystems"
>> -    FSCK_OUT=/dev/stdout
>> -    FSCK_ERR=/dev/stdout
>> -    FSCK_FD=
>> -    FORCEFSCK=
>> -    [[ -f /forcefsck ]] || in_array forcefsck $(< /proc/cmdline) && FORCEFSCK="-- -f"
>> -    run_hook sysinit_prefsck
>> -    fsck -A -T -C$FSCK_FD -a -t "$NETFS,noopts=_netdev" $FORCEFSCK >$FSCK_OUT 2>$FSCK_ERR
>> +        FSCK_OUT=/dev/stdout
>> +        FSCK_ERR=/dev/stdout
>> +        FSCK_FD=
>> +        FORCEFSCK=
>> +        [[ -f /forcefsck ]] || in_array forcefsck $(< /proc/cmdline) && FORCEFSCK="-- -f"
>> +        run_hook sysinit_prefsck
>> +        fsck -A -T -C$FSCK_FD -a -t "$NETFS,noopts=_netdev" $FORCEFSCK >$FSCK_OUT 2>$FSCK_ERR
>>    local fsckret=$?
>>    if (( fsckret > 1 )); then
>>        stat_fail
>> @@ -426,9 +426,9 @@ fsck_reboot() {
>> 
>> mount_all() {
>>    stat_busy "Mounting Local Filesystems"
>> -    run_hook sysinit_premount
>> -    mount -a -t $NETFS -O no_netdev
>> -    run_hook sysinit_postmount
>> +        run_hook sysinit_premount
>> +        mount -a -t $NETFS -O no_netdev
>> +        run_hook sysinit_postmount
>>    stat_done
>> }
>> 
>> @@ -502,13 +502,13 @@ fi
>> set_consolefont() {
>>    [[ $CONSOLEFONT ]] || return 0
>>    stat_busy "Loading Console Font: $CONSOLEFONT"
>> -    #CONSOLEMAP in UTF-8 shouldn't be used
>> -    [[ $CONSOLEMAP && ${LOCALE,,} =~ utf ]] && CONSOLEMAP=""
>> -    local i
>> -    for i in /dev/tty[0-9]*; do
>> -        /usr/bin/setfont ${CONSOLEMAP:+-m ${CONSOLEMAP}} \
>> -        $CONSOLEFONT -C ${i} &>/dev/null
>> -    done
>> +        #CONSOLEMAP in UTF-8 shouldn't be used
>> +        [[ $CONSOLEMAP && ${LOCALE,,} =~ utf ]] && CONSOLEMAP=""
>> +        local i
>> +        for i in /dev/tty[0-9]*; do
>> +            /usr/bin/setfont ${CONSOLEMAP:+-m ${CONSOLEMAP}} \
>> +                $CONSOLEFONT -C ${i} &>/dev/null
>> +        done
>>    if (( $? )); then
>>        stat_fail
>>    elif [[ $CONSOLEMAP ]]; then
>> -- 
>> 1.7.1
>> 
> 
> I never really understood why we indented like this. Since we seem to
> be split 50/50, I'd argue that it might be better to just separate the
> code with whitespace, e.g.
> 
> # more code above here...
> 
> stat_busy
> foo
> bar
> baz
> stat_done
> 
> # more code below here...
> 
> This way, we don't have to worry about what happens when there could be
> a stat_fail somewhere in between that would make us have to wonder wtf
> to do with indenting.
> 
> d

The reason for the indentation was to make it obvious how stat_{busy,fail,done} match up. There were some errors due to mismatches before.

I think it is relatively clear how to DTRT: if stat_fail/stat_done is one indentation level below stat_busy, then unindent it. Otherwise (e.g., if it is inside a conditional), don't.

That said, I'm wondering if it wouldn't be clearer if we just used status, and when needed put these code blocks inside functions.

-t