interp: Return invalid globs when nullglob is set

[lollipopman]

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]

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

[lollipopman]

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]

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]

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]

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]

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

[mvdan]

SGTM with a naming nit

[lollipopman]

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

[mvdan]

Thanks to you!