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

[Kurt J. Bosch]

Tom Gundersen, 2011-06-25 21:10:
> 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

Hmm... Currently we have the checks for skipping the entire thing as 
well as the stat_busy messages within the functions. This makes sense 
for three reasons:
a) It allows to disable say set_consolefont in a sysinit hook by just 
unsetting $CONSOLEFONT and then call it later via some rc.multi hook 
without the need to duplicate the checking code (which isn't always that 
simple). This actually happens to avoid destroying a splash screen right 
in the middle of boot up.
b) It allows to override the check and also the message together with 
the code instead of the code block only.
c) It allows to have more than one code block in one single function 
(like in kill_everything)

So, when using status, we would end up with somthing like

set_consolefont() {
      [[ $CONSOLEFONT ]] || return 0
      status "Loading Console Font: $CONSOLEFONT" \
          set_consolefont_inner
}

set_consolefont_inner() {
     ...
}

-- 
Kurt