Re: [Dragora-members] Qi 2.0rc10

[Michael Siegel]

Am 14.07.20 um 13:20 schrieb Matias Fonzo:
> Hi Michael,
> 
> Thanks for taking the time in review Qi 2.0rc10! ;-)
> 
> El 2020-07-13 14:38, Michael Siegel escribió:
>> 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.
> 
> I changed it to [OPTIONS].  While [FILE]... prevails as I see nothing
> wrong with it, it is widely used in GNU tools.

Okay, I see. My impression is that the POSIX description of how to do
that exactly right is so confusing that, basically, everyone has their
own way of doing it. And if GNU says [FILE]..., then let it be that.

>>   * L 112, 380: printf "%s\\n"
>>     * Wouldn't it be better to say: printf '%s\n'
> 
> Yes.  This was pointed by
> https://github.com/koalaman/shellcheck/wiki/SC1117 before.
> I changed it to printf '%s\n' now.

Nice.

[...]

>>   * L 1506: \unalias -a;        # Unset all possible aliases.
>>     * Why is there a backslash before "unalias"?
> 
> To avoid invoking a possible alias of "unalias".

Yeah, I just found it in POSIX and wanted to tell you to ignore this
question, but you were quicker. :)

>>     * 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.
> 
> This would put off any possible declaration of aliases from the
> configuration file, I'm not sure if anyone will declare useful aliases
> there to be used in the recipe...

I see.

>> 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.
> 
> Agree.  I will take a look on the code for such cases.
> 
>> Also, maybe you could convert all usage of `echo' to `printf'. Maybe
>> that would even be more convenient/practical in some cases.
> 
> I use echo (without options) to print a few lines or lines that I feel
> will have no special formatting.  Printf is quite useful when you want
> formatting and fast when printing several lines in one go.

Yeah, that's probably fine. You don't actually need the "" in those
cases, though. It could just be `echo' (says POSIX). But that's not
really important.

>> 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.
>>
> 
> Yes and no.
> 
> That's what I did before when I was trying to implement the new
> interface.  I found this to be both impractical and confusing, as it
> adds a third (optional) level to the command line order or attempt.
> 
> Consider that for the global option to work, the program needs a command
> as an argument, which is mandatory.  The global option would then
> fulfill its function based on a specific command.  So, in Qi, there are
> no global options as such (that is, general options that apply to all
> commands); instead, these are common options that can exist between one
> command and another, currently there are no options in Qi that apply to
> all commands.
> 
> Global options seem to be an illusion more than anything else, in
> practice you could type:
> 
>     qi -N install package.tlz
> 
> Note that -N in the first position (before the command) is a Global
> option.  But if you or someone forgets to type the -G option at this
> position, you'll want to match the option for the "install" command
> after it, so that it takes effect:
> 
>     qi install -N package.tlz
> 
> Not to mention that making the global options require a little more code
> in Qi to work, and this will be optional, since the user may not write
> the global option in that corresponding position.
> 
> It's clearer and simpler if you know what the order of Qi's interface is:
> 
>     qi COMMAND [OPTIONS] [FILE]...

Okay, I'll have to think about this again. In the meantime, you could
have a look at

  https://vcs.malbolge.net/msi/mote/-/blob/master/mote#L1056

for a code example of what I meant. Maybe that doesn't make sense for
Qi, though. Anyway, let's just leave it as it is for 2.0, if it works
reliably.

There were a few more issues when I ran shellcheck on rc10, the most
important being:

  In qi.in line 543:
              echo ",s/^\\(release\\)=.*/\\1=${release}/"$'\nw' | \
                                                         ^-- SC2039: In
  POSIX sh, $'..' is undefined.


  In qi.in line 881:
                      rm -rf -- "$rootdir${packagedir}/$replace"
                                ^-- SC2115: Use "${var:?}" to ensure
  this never expands to / .


  In qi.in line 1060:
      PRVDIR="${TMPDIR}/${name}.${RANDOM-0}$$"
                                ^-- SC2039: In POSIX sh, RANDOM is
  undefined.



Best
Michael