New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reinstate failglob behaviour for most commands #2719

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@anordal
Contributor

anordal commented Feb 8, 2016

Expand globs to zero arguments (nullglob) only for set, for and count.

The warning about failing globs, and setting the accompanying $status,
now happens regardless of mode, interactive or not.

It is assumed that the above commands are the common cases where
nullglob behaviour is desirable.
More importantly, doing this with set is a real feature enabler,
since the resulting empty array can be passed on to any command.

The previous behaviour was actually all nullglob (since commit
cab115c), but this was undocumented;
the failglob warning was still printed in interactive mode,
and the documentation was bragging about failglob behaviour.

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Feb 9, 2016

Member

Without running this:

I think I'd be okay with this behavior (it's basically what I suggested).

At the very least you'll want to look at test5, which fails because you also print the error message for case *something* - you'll need to adjust it (and yes, I do think we'll want it for case as well because that's a major source of errors because it overloads the "*")

Otherwise it's worth discussing, especially:

  • Do we want a way to mark a certain glob as nullable? Like a "nullglob" command?
  • Do we want a way to figure out after the fact if a glob failed?

Of course both of these could be left out - we could tell people to use variables if they want that.

And one thing I'll have to test is if the error message is suppressable.

Member

faho commented Feb 9, 2016

Without running this:

I think I'd be okay with this behavior (it's basically what I suggested).

At the very least you'll want to look at test5, which fails because you also print the error message for case *something* - you'll need to adjust it (and yes, I do think we'll want it for case as well because that's a major source of errors because it overloads the "*")

Otherwise it's worth discussing, especially:

  • Do we want a way to mark a certain glob as nullable? Like a "nullglob" command?
  • Do we want a way to figure out after the fact if a glob failed?

Of course both of these could be left out - we could tell people to use variables if they want that.

And one thing I'll have to test is if the error message is suppressable.

@faho faho added the enhancement label Feb 9, 2016

@faho faho added this to the next-2.x milestone Feb 9, 2016

@faho faho added the bug label Feb 9, 2016

@anordal

This comment has been minimized.

Show comment
Hide comment
@anordal

anordal Feb 9, 2016

Contributor

Thanks for the review! Yes, I read your posts on the matter, and the last one inspired me to try if this was possible.

I'm looking into the case failure, but I think it's bedtime for me now.

Contributor

anordal commented Feb 9, 2016

Thanks for the review! Yes, I read your posts on the matter, and the last one inspired me to try if this was possible.

I'm looking into the case failure, but I think it's bedtime for me now.

@anordal

This comment has been minimized.

Show comment
Hide comment
@anordal

anordal Feb 9, 2016

Contributor

Now I get it, case *something*, from beforehand, actually checks if a file exists that matches the glob. This is also documented.

This commit accidentally "fixes" it by disallowing the syntax — what I should do is modify the expected result.

Contributor

anordal commented Feb 9, 2016

Now I get it, case *something*, from beforehand, actually checks if a file exists that matches the glob. This is also documented.

This commit accidentally "fixes" it by disallowing the syntax — what I should do is modify the expected result.

Reinstate failglob behaviour for most commands
Expand globs to zero arguments (nullglob) only for set, for and count.

The warning about failing globs, and setting the accompanying $status,
now happens regardless of mode, interactive or not.

It is assumed that the above commands are the common cases where
nullglob behaviour is desirable.
More importantly, doing this with `set` is a real feature enabler,
since the resulting empty array can be passed on to any command.

The previous behaviour was actually all nullglob (since commit
cab115c), but this was undocumented;
the failglob warning was still printed in interactive mode,
and the documentation was bragging about failglob behaviour.
@anordal

This comment has been minimized.

Show comment
Hide comment
@anordal

anordal Feb 9, 2016

Contributor

test5 is now fixed by expecting this error output:

No matches for wildcard '*ee*'.
fish:  case *ee*
            ^

Now, I'm struggling to reproduce the "bind.expect" failure in the clang test.

Contributor

anordal commented Feb 9, 2016

test5 is now fixed by expecting this error output:

No matches for wildcard '*ee*'.
fish:  case *ee*
            ^

Now, I'm struggling to reproduce the "bind.expect" failure in the clang test.

@ridiculousfish

This comment has been minimized.

Show comment
Hide comment
@ridiculousfish

ridiculousfish Feb 10, 2016

Member

The bind.expect failure is a gnarly timing issue due to ASAN (I think!), though we've also had trouble reproing it so I may easily be wrong. Thanks for being conscientious and you can ignore that one.

Member

ridiculousfish commented Feb 10, 2016

The bind.expect failure is a gnarly timing issue due to ASAN (I think!), though we've also had trouble reproing it so I may easily be wrong. Thanks for being conscientious and you can ignore that one.

# Lists the .foo files, or warns if there aren't any.
set foos *.foo
if test (count $foos) -ge 1

This comment has been minimized.

@faho

faho Feb 15, 2016

Member

The test is actually unnecessary - count will return 1 if given no arguments (and 0 otherwise), so if count $foos >/dev/null works just as well. As would if set -q foos[1].

@faho

faho Feb 15, 2016

Member

The test is actually unnecessary - count will return 1 if given no arguments (and 0 otherwise), so if count $foos >/dev/null works just as well. As would if set -q foos[1].

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Feb 15, 2016

Member

I actually can't find anything wrong with the behavior here.

@ridiculousfish: I'd like to merge this and believe we can add any other nullglob command or syntax later. Okay?

Member

faho commented Feb 15, 2016

I actually can't find anything wrong with the behavior here.

@ridiculousfish: I'd like to merge this and believe we can add any other nullglob command or syntax later. Okay?

@ridiculousfish

This comment has been minimized.

Show comment
Hide comment
@ridiculousfish

ridiculousfish Feb 15, 2016

Member

Reviewed, looks most excellent. Merged with rebase as 62b76b2 . Thanks!!

Member

ridiculousfish commented Feb 15, 2016

Reviewed, looks most excellent. Merged with rebase as 62b76b2 . Thanks!!

@anordal

This comment has been minimized.

Show comment
Hide comment
@anordal

anordal Feb 17, 2016

Contributor

Thank you guys, I'm delighted.

Contributor

anordal commented Feb 17, 2016

Thank you guys, I'm delighted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment