dragora-members
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Dragora-members] Qi 2.0rc10

From: Matias Fonzo
Subject: Re: [Dragora-members] Qi 2.0rc10
Date: Wed, 15 Jul 2020 18:31:56 -0300
User-agent: Roundcube Webmail/1.4.6 
El 2020-07-14 09:52, Michael Siegel escribió:

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. :)


Thanks.


    * 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.
I prefer to use quotes when echo or whatever when there are (separated) spaces. 


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.
That's needed for the input of ed(1). '$' means the last line in the buffer followed by the commands.
I don't know how to do in another way in order to avoid the warning from shellcheck. 



  In qi.in line 881:
                      rm -rf -- "$rootdir${packagedir}/$replace"
                                ^-- SC2115: Use "${var:?}" to ensure
  this never expands to / .
Note that rootdir is a prefix to a defined and given directory by the user. The same one is validated and qualify previously in Qi:
We cannot use "${rootdir:?}" for every line marked by shellcheck because it will defeat its purpose of having a prefix directory, the value will be equal to '?' if not defined - if we follow the suggestion from shellcheck... 



  In qi.in line 1060:
      PRVDIR="${TMPDIR}/${name}.${RANDOM-0}$$"
                                ^-- SC2039: In POSIX sh, RANDOM is
  undefined.
If the shell does not define the environment variable RANDOM, its value will be equal to 0 here. Otherwise it takes advantage of RANDOM for the shells that define it.
reply via email to
[Prev in Thread] Current Thread [Next in Thread]