Re: select syntax violates the POLA

[Eli Schwartz]

On 4/1/21 1:18 PM, Greg Wooledge wrote:
> On Thu, Apr 01, 2021 at 11:58:13PM +0700, Robert Elz wrote:
>>
>>   | If $1 is not a directory, then you want:
>>
>> It is a directory, or I'd guess, quite likely a pattern chosen
> 
> 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.
> 
> 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.
> 
> 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
> without directory components.  And since there's more than one directory,
> the array ends up looking like:
> 
> d=(README foo.c foo.h foo README bar.c bar.h foo.c foo.h foobar)
> 
> with repeated filenames, and no knowledge of which filename came out of
> which subdirectory.
> 
> I was assuming that this was *not* how it was intended to work, and thus,
> I made the guess that /usr/src/pkg/*/$1 expanded to a list of *files*,
> and therefore generated a list of full pathnames, e.g.
> 
> d=(/usr/src/pkg/applesauce/Makefile.PL /usr/src/pkg/bananabread/Makefile.PL)
> 
> and then the user can make a meaningful selection from such a list.
> 
> 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.
> 
> 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.
> 
>>   | d=(/usr/src/pkg/*/"$1")
>>
>> definitely not that, the quotes are wrong in any case
> 
> They are not!

The quotes are wrong, *iff* the intent is that people should be allowed
to pass quoted globs as $1 and let those be expanded to produce d=()

Which, I can see where Robert is coming from in assuming this.

>> (but apart from
>> that, if filename expansion happens in array assignments (it doesn't in
>> normal ones, and I dislike arrays, so ...) then without the quotes that
>> might work.
>>
>> Alternatively
>>      d=( $( ls -d /usr/src/pkg/*/$1 ) )
> 
> This needs quotes around "$1" 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.  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.
> 
>> or just
>>      d=( $( printf %s\\n /usr/src/pkg/*/$1 ) )
> 
> This is not much better.  It still breaks on whitespace.
> 
> 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)
> 
>> 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", and it's basically the same as
> the array assignment -- just less flexible, and more suited to sh than
> bash.  But certainly, this is viable in many cases.
> 
>> 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™

It's not too much of a leap if the names are required to be NetBSD ports
tree names of packages, which presumably ban such characters and enforce
that ban in their tooling.

It's not something one would be wise to get into the habit of doing
elsewhere, and the quotes don't hurt, so why not add them... unless,
again, we assume the intent of the script is in fact to pass in globs.

These "Famous Last Words™" are really just a fancy way to repeat your
initial, valid objection that the OP gave zero comments or explanation
and therefore you're left to guess at the intention without having any
contextual knowledge about what /usr/src/pkg is or why one would want to
cd into any of them and run make.

I suspect Robert does know quite a bit about it though. :)

-- 
Eli Schwartz
Arch Linux Bug Wrangler and Trusted User

OpenPGP_signature
Description: OpenPGP digital signature