[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Dragora-members] Qi 2.0rc10
From: |
Michael Siegel |
Subject: |
Re: [Dragora-members] Qi 2.0rc10 |
Date: |
Mon, 13 Jul 2020 19:38:17 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux i686; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 |
Am 04.07.20 um 22:35 schrieb Matias Fonzo:
>
> Qi 2.0rc10 for review, attached.
Okay, I'll keep this concise. This is just what I found having a rather
quick look over the source.
* L 46: Usage: $PROGRAM COMMAND [OPTION...] [FILE]...
* POSIX would also allow for simply saying "[OPTIONS]"[1]. I think
that's a good way to do it in this case.
* POSIX is not really clear on this, but I think it should rather be
"[FILE...]".
* This should then also be adjusted in the manual page.
* L 112, 380: printf "%s\\n"
* Wouldn't it be better to say: printf '%s\n'
* L 262–265: You might want to quote those substitutions, even though
they probably won't contain spaces. I mean, you're already doing it
in the assignment block right above that.
* L 323: outdir=${outdir}/${arch}
* Might want to quote the substitution.
* L 1506: \unalias -a; # Unset all possible aliases.
* Why is there a backslash before "unalias"?
* Also, I'm not sure if that is needed because, usually aliases
don't apply when running scripts, IIRC. We'd need to verify that
in POSIX, mksh(1) and the Bash Reference Manual.
Generally, I'd go through the code again and look for unquoted variable
expansions and decide whether to better quote them or not. I mean, it is
more consistent to just quote every substitution, except for where you
don't quote things on purpose, I think.
Also, maybe you could convert all usage of `echo' to `printf'. Maybe
that would even be more convenient/practical in some cases.
Last but not least, I'd say the argument parsing should be done a bit
differently, but I'd have to have a closer look on exactly how it should
be done. The basic idea is: Parse global options first, then parse
commands and have the function implementing a particular command parse
its respective options.
--Michael
[1] See point 5. in
https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html#tag_12_01
- [Dragora-members] Qi 2.0rc10, Matias Fonzo, 2020/07/04
- Re: [Dragora-members] Qi 2.0rc10,
Michael Siegel <=
- Re: [Dragora-members] Qi 2.0rc10, Matias Fonzo, 2020/07/14
- Re: [Dragora-members] Qi 2.0rc10, Michael Siegel, 2020/07/14
- Re: [Dragora-members] Qi 2.0rc10, Matias Fonzo, 2020/07/15
- Re: [Dragora-members] Qi 2.0rc10, Michael Siegel, 2020/07/18
- Re: [Dragora-members] Qi 2.0rc10, Matias Fonzo, 2020/07/18
- Re: [Dragora-members] Qi 2.0rc10, Matias Fonzo, 2020/07/18