search

mvdan / sh

A shell parser, formatter, and interpreter with bash support; includes shfmt
https://pkg.go.dev/mvdan.cc/sh/v3
BSD 3-Clause "New" or "Revised" License
7.14k stars 338 forks source link

interp: Return invalid globs when nullglob is set #862

Closed lollipopman closed 2 years ago

lollipopman commented 2 years ago

Prior to this commit invalid globs were returned as null when nullglob was set, which resulted in test expressions failing:

$ shopt -s nullglob; [ -n butter ] && echo bubbles "-n": executable file not found in $PATH

After this commit invalid globs are returned verbatim when nullglob is set.

mvdan commented 2 years ago

I'm a bit confused by your commit message: butter is not an invalid glob, as far as I can tell.

lollipopman commented 2 years ago

I'm a bit confused by your commit message: butter is not an invalid glob, as far as I can tell.

Sorry, I didn't mean to make the example confusing, the invalid glob is the first opening of the test expression, [, on master when running the example in gosh, you get:

$ shopt -s nullglob; [ -n butter ] && echo bubbles
"-n": executable file not found in $PATH

because with nullglob enabled the current code does not append the invalid glob, so the opening bracket is removed from the expression.

mvdan commented 2 years ago

Ah, I see, thanks for the clarification.

Do we need to add InvalidGlobError? It's the only error that the method ever returns, so we can avoid extending the API and simply use err != nil in the caller to tell whether the glob is valid or not.

Otherwise the change sounds good to me!

mvdan commented 2 years ago

Err, scratch that. I hadn't noticed that the call is to the glob method, which then indirectly calls pattern.Regexp. We do need to differentiate the error. So I think what you've done is good.

lollipopman commented 2 years ago

Err, scratch that. I hadn't noticed that the call is to the glob method, which then indirectly calls pattern.Regexp. We do need to differentiate the error. So I think what you've done is good.

I also considered having the glob function return nil, nil when the glob is invalid and then having the caller differentiate between a nil slice and an empty slice. That method worked, but it seemed a little too clever and brittle.

mvdan commented 2 years ago

Yeah, I agree that's too subtle - I don't like APIs that give different meanings to nil vs empty.

lollipopman commented 2 years ago

Thanks for the careful review @mvdan, I believe this is now ready to merge.

mvdan commented 2 years ago

Thanks to you!