Re: MAKEDEV (and code highlighted by PR bin/52280)

[christos%astron.com@localhost (Christos Zoulas)]

In article <10114.1496863546%andromeda.noi.kre.to@localhost>,
Robert Elz  <kre%munnari.OZ.AU@localhost> wrote:
>The recent PR bin/52280 which discovered a piece of lack of thought in
>my sh $LINENO code, was triggered by this section of code in MAKEDEV ...
>
>        # Now we need exactly one node-creation method.
>        case $(( $($do_mtree && echo 1 || echo 0) + \
>                $($do_pax && echo 1 || echo 0) + \ 
>                $($do_mknod && echo 1 || echo 0) + \
>                $($do_specfile && echo 1 || echo 0) ))
>        in
>        1)      : OK ;;
>        *)      die "-m, -p, -s, and -t options are mutually exclusive" ;;
>        esac
>
>First, let me say that there's nothing technically wrong with that code,
>and I wouldn't change it before the bug was fixed (though any of a number
>of changes could have hidden the bug, so it is good that it was written
>just this way).
>
>But now the bug is fixed ...
>
>#1:   the \'s at end of the lines there are totally unnecessary (those were,
>leaving aside my role in writing the code, the primary bug trigger, dealing
>with line counting and elided newlines, accurately, is tricky.)
>
>The general rule is that you need a \ at end of line in sh if it is possible
>that the command could (correctly) end at that point.
>
>Since here we're in the middle of a $(( )) expression, nothing "ends" until
>we get the )) so we can never need a \ newline in there
>
>The same applies in a $( ) command substitution which won't end until the )
>is seen, though there a command being executed might need to be continued
>over multiple lines, and then that might need a \ newline.
>
>Similarly after any of the | || && operators, which need a subsequent
>command to be complete, and after most reserved words (if while... even !)
>some other command is required, so no \ is needed before a newline.
>
>
>#2:   running 4 command substitutions in order to generate an arith expression
>seems excessive, if we wanted to do it that way, something more like
>
>        case $(( $( echo 0
>                    $do_mtree && echo + 1
>                    $do_pax && echo + 1
>                    $do_mknod && echo + 1
>                    $do_specfile && echo + 1
>                  )
>              ))
>        in
>
>would be better, just one cmdsub to achieve the same result.
>[Irrelevant aside, in general it is safer to use printf(1) than echo(1)
> but here it doesn't matter.]
>
>
>#3:   all this looks like an attempt to program sh with pseudo-C code,
>so rather than either of those:
>
>        case ",${do_mtree},${do_pax},${do_mknod},$do_specfile)," in
>        ( *,true,*,true,* ) 
>                die "-m, -p, -s, and -t options are mutually exclusive"
>                ;;
>        esac
>
>looks more like the sh way.   Here the quotes around the case word look
>unneeded (and it is correct that the effect would be the same without them
>as each of those variables is just "true" or "false") it is better to
>include them, not only "just in case" the variable happens to contain white
>space or a meta-character ('*' etc), neither of which is going to happen
>here, and we know that - but also because when you use the "" you are telling
>the shell that you do not want filename, or field splitting to happen, and
>with that knowledge, the shell has less work to do and is more efficient.
>So, do always quote things unless you really need the unquoted result, adding
>quotes does not make the shell work harder, it reduces the workload.
>
>In this, earlier code has already assured that at least one of the
>do_xxx's is true), but:
>
>
>#4:  And then speaking of that earlier code, it is...
>
>        if ! ( $do_mtree || $do_pax || $do_mknod || $do_specfile ); then
>		# code to set one of them to true
>	fi
>
>In that the grouping is needed (though we could apply De Morgan's law to
>remove it) but it doesn't need to be a sub-shell, better would be ...
>
>        if ! { $do_mtree || $do_pax || $do_mknod || $do_specfile ;}; then
>
>That is, in sh, use { } where you would use ( ) in C or mathematics, but
>just remember that while ( and ) are operators, { and } are just reserved
>words, so need more care in how they are placed.
>
>But even better, we can combine that test into the test for more than
>one option set, by adding...
>
>        ( ,false,false,false,false, )
>		# code to set one of them
>		;;
>
>to the case statement above.  Again, that's the sh way.
>
>
>#5:  But now writing about unnecessary sub-shells, a little later we have this:
>
>        # do_mtree or do_pax internally implies do_specfile.
>        # This happens after checking for mutually-exclusive options.
>        if ($do_mtree || $do_pax) && ! $do_specfile; then
>                do_specfile=true
>                opts="${opts} -s"
>        fi
>
>where again a sub-shell is not needed, and we could use
>
>        if { $do_mtree || $do_pax; } && ! $do_specfile; then
>
>(note the change of spacing and the extra ';')   But that's still C programming
>in sh, in sh the operator precedence rules are different than C, and the
>grouping is not needed here at all, just
>
>        if $do_mtree || $do_pax && ! $do_specfile; then
>
>works fine - sh evaluates and-or lists left to right, making a decision
>at each operator whether the next command (pipeline) needs to be evaluated
>or not.  So if do_mtree is true we do not evaluate do_pax, and proceed to
>! do_specfile.  If do_mtree is false, we evaluate do_pax, and if that is true,
>we do the do_specfile case (and invert the result).   If both do_mtree and
>do_pax are both false then at the && we skip the do_spacfile expansion
>(and its !).
>
>Since in any of those cases we are then at the end of the and-or list
>whatever status we're left with from the last command evaluated is the result.
>
>That's not how C would evaluate it, but this is not C.
>
>So, given all that, does anyone object if I commit the following patch
>(to src/etc/MAKEDEV.tmpl) ?

Go for it.

christos