[PATCH 2/2] Split bash compatible extensions into separate options.

[Kang-Che Sung]

On Wed, Jan 11, 2017 at 4:09 PM, Denys Vlasenko
<vda.linux at googlemail.com> wrote:
> On Tue, Jan 10, 2017 at 8:17 PM, Kang-Che Sung <explorer09 at gmail.com> wrote:
>> All of the bash compatibility options will now depend on
>> CONFIG_*_BASH_COMPAT, and CONFIG_*_BASH_COMPAT become an option that
>> alone doesn't add any code.
>>
>> Splitting these options allows more flexibility in configuration
>
> Configuration shouldn't strive to be maximally flexible.
>
> It floods the user with way too many options.
> For example, now LS has eight suboptions.
> Imagine this instead:
>
> [*] ls
> [*]   Enable -1    One column output
> [*]   Enable -a    Include entries which start with .
> [*]   Enable -A    Like -a, but exclude . and ..
> [*]   Enable -C    List by columns
> [*]   Enable -x    List by lines
> [*]   Enable -d    List directory entries instead of contents
> [*]   Enable -L    Follow symlinks
> [*]   Enable -H    Follow symlinks on command line
> [*]   Enable -R    Recurse
> [*]   Enable -p    Append / to dir entries
> [*]   Enable -F    Append indicator (one of */=@|) to entries
> [*]   Enable -l    Long listing format
> [*]     Enable -c  sort by ctime
> [*]     Enable -t  sort by mtime
> [*]     Enable -u  sort by atime
> [*]   Enable -i    List inode numbers
> [*]   Enable -n    List numeric UIDs and GIDs instead of names
> [*]   Enable -s    List allocated blocks
> [*]   Enable -e    List full date and time
> [*]   Enable -h    List sizes in human readable format (1K 243M 2G)
> [*]   Enable -r    Sort in reverse order
> [*]   Enable -S    Sort by size
> [*]   Enable -X    Sort by extension
> [*]   Enable -v    Sort by version
> [*]   Enable -w N  Assume the terminal is N columns wide
> [*]   Enable --color
> [*]     Produce colored ls output by default
>
> For every applet and every option. That would be ridiculous.
>
> Ridiculous we can live with, but there is a bigger problem too.
> More options means more #ifdefs and more weird interactions
> between different options, making code less maintainable.
> Because of this reason, some options (e.g. in top) were even
> _removed_ (permanently enabled) because code was too tangled.
>
> therefore options should be introduced if there is a reasonable
> chance user actually needs to control them separately,
> and if they control a nontrivial amount of code (i.e. not 20 bytes).
>
> If user needs bash compat, it means he is not running on an extremely
> constrained machine (otherwise he can well spend a bit of time and rewrite
> all his scripts to be POSIX-compatible). Since the machine is not
> too constrained, it's okay to just enable all of them and not worry
> about having a few more kbytes of code for a bashism he doesn't need.

I don't buy your argument, unfortunately.

My points:

1. I can read if any of the shell's feature is too trivial that it'll
be not worth
implementing or making a configure option at all. The bash-compatible
'source' command behavior, that I suggested you to revert, is one of the
example. (busybox ash implemented that wrong, and I know user can
easily have 'source' search for current directory by adding '.' to PATH,
meaning that it'll be not worthy to implement it, even though it could
implement right.)

2. The bash-compat extensions are already scattered around various
places in the source code. And each of the
#if ENABLE_ASH_BASH_COMPAT ... #endif blocks only cover one or
two features that can break up easily. They share little common code.

3. If people don't like too many Kconfig options, they don't have to.
Nothing stops you from making them in-C macros instead:

    #if ENABLE_ASH_BASH_COMPAT
    # define ENABLE_ASH_DOLLAR_SQUOTE    /* $'...' */
    # define ENABLE_ASH_SUBSTR_EXPANSION /* ${var:pos:len} */
    # define ENABLE_ASH_PATTERN_SUBST    /* ${var/pattern/repl} */
    # define ENABLE_ASH_REDIR_OUTPUT2    /* &>file ; >&file */
    # define ENABLE_ASH_FUNCTION2        /* function keyword */
    # define ENABLE_ASH_SOURCE           /* source keyword */
    # define ENABLE_ASH_TEST2            /* [[ EXPR ]] */
    # define ENABLE_ASH_OPT_PIPEFAIL     /* -o pipefail */
    # define ENABLE_ASH_HOSTNAME_VAR     /* $HOSTNAME */
    # define ENABLE_ASH_SHLVL_VAR        /* $SHLVL */
    #endif

    #if ENABLE_HUSH_BASH_COMPAT
    # define ENABLE_HUSH_BRACE_EXPANSION
    # define ENABLE_HUSH_SUBSTR_EXPANSION
    # define ENABLE_HUSH_PATTERN_SUBST
    # define ENABLE_HUSH_SOURCE
    # define ENABLE_HUSH_TEST2
    # define ENABLE_HUSH_HOSTNAME_VAR
    #endif

4. If we want to argue with "trivial", then I think only a few of the above fits
('source', 'function', '$SHLVL', and '$HOSTNAME')
The others, such as substring expansion and pattern substitution, actually
add a couple of subroutines that I don't think just 20 bytes of code could
make it.

5. I would wonder why HUSH_BRACE_EXPANSION is separate from
HUSH_BASH_COMPAT in the first place...