Re: select syntax violates the POLA

[Robert Elz]

    Date:        Thu, 1 Apr 2021 13:18:07 -0400
    From:        Greg Wooledge <greg@wooledge.org>
    Message-ID:  <YGYAT/Cr1z6/F5Or@wooledge.org>

  | It's amazing how many people manage to post their code with NO comments
  | or explanations of what it's supposed to do, what assumptions are being
  | made about the inputs, etc.  This leaves us to guess.

Agreed, it is a problem in general.   It just happens that in this
case I can see exactly what is intended (as I know the environment)
so I don't need to guess.

  | I'm guessing that /usr/src/pkg/*/* is a list of files, not a list of
  | directories.  You're guessing that it's a list of directories.

No, I know it is a list of directories.  No guessing involved.

  | In the original code, if it's a list of directories, then we have
  | "ls firstdir seconddir" which expands to a list of mangled filenames

Yes, the original was broken.   But that was not what the message was
about, it was about the syntax of select.   That the rest of the example
code wouldn't have worked was kind of not the point, he wasn't asking
for assistance with that.

What he wanted to know was why his usage of select was giving a syntax error.
Chet answered that.

  | Now, this is obviously just a guess, and I could be extremely wrong
  | about it.  For one thing, they used the variable name "d".  That seems
  | to indicate that it should be a list of directories, rather than a list
  | of files.

More than that, after one of them is selected ("select dir ...") the
code did "cd $dir" (you even commented on the lack of quotes in that
statement) - so a directory name is clearly what is wanted.   Even if
you didn't know the environment, this makes that very clear.

  | So, perhaps their code is simply *broken*, and where they *thought* they
  | would be getting a list of directories, they actually get a list of
  | mangled useless filenames, because they forgot the -d argument to ls.

Yes.

  |
  | >   | d=(/usr/src/pkg/*/"$1")
  | > 
  | > definitely not that, the quotes are wrong in any case
  |
  | They are not!

They are.   As I said, when used $1 is likely to be a pattern 'p5-*'
or similar, and it wants to locate all directories named starting with
p5- not the one (non-existing) directory named literally "p5-*".

  | > Alternatively
  | >   d=( $( ls -d /usr/src/pkg/*/$1 ) )
  |
  | This needs quotes around "$1"

Again, no it doesn't.   Quote everything is a nice starting point,
but it does not universally apply.   Filename expansion of the $1
is definitely wanted here.

  | but yes, it's quite possible that this was
  | their intent.  Of course, this is still *broken*, but it only breaks if
  | one of the directory names contains whitespace characters.

They don't.   They never will.   Of course it could break if IFS was set
to something unusual, but since this script is (I presume) not doing that,
that's also not an issue.

  | It's likely
  | that they didn't happen to have any such directories in their test
  | environment, and therefore, this bug would go unnoticed for a long time.

It would never be noticed, as there will never be a category (the '*')
or package (the $1) name containing a space or any other meta-char.
There are rules for the names.

Too much of pkgsrc (which is what this is dealing with, the NetBSD
packaging system, also available for lots of other systems, including
various linux variants, Solaris, OSX, and more) depends on the names
having specific characteristics for this to ever be changed.

  | > or just
  | >   d=( $( printf %s\\n /usr/src/pkg/*/$1 ) )
  |
  | This is not much better.  It still breaks on whitespace.

It would.  There is none.

  | If a list of directories (not their contents) is the desired outcome,
  | then my original code is perfectly fine:
  |
  | d=(/usr/src/pkg/*/"$1")
  |
  | This will expand to something like
  |
  | d=(/usr/src/pkg/vendor1/netcat /usr/src/pkg/vendor2/netcat)

Except that it can't.    There can be only one "netcat" in all of the
category directories, because those things aren't used (or not really
used) for the compiled binary packages.   If you restrict the $1 to being
a literal string (by quoting it) then there could never be more than one
name returned, so using an array, and using select to choose the actual
one which is wanted, would be a waste of coding effort.

I'm not blaming you for not knowing all this, but like I said, I know the
environment, and I can see exactly what is being attempted.

  | > Just to be sure.    Personally I'd do
  | > 
  | >   set -- /usr/src/pkg/*/$1
  | > 
  | > and then simply use the positional parameters.
  |
  | This still needs quotes around "$1",

No, it doesn't.

  | and it's basically the same as the array assignment -- just less flexible,
  | and more suited to sh than bash.

Exactly.   It will work with any shell, bash doesn't need to be available.

  | > Yes.   But it turns out not to matter in this case, as none of
  | > the names will ever contain anything but "normal" characters
  | > (no spaces, newlines, asterisks, ...)
  |
  | Famous Last Words

In this case, guaranteed.

kre