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

Fix brackets #4632

Closed
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@faho
Copy link
Member

faho commented Jan 2, 2018

Description

This is a double-whammy to remove some of the annoyances from brace/bracket-expansion (the code calls it "brackets", I've only ever heard "braces" elsewhere).

It changes a literal {} to expand to itself, which fixes issue #1109, and makes find -exec nicer to use.

It also changes it so successive commas in braces are seen as successive empty items, rather than alternating between an empty item and a literal ",". So x{,,,}y is no longer seen as xy x,y xy (because the second "," is seen as a literal "," rather than a separator), but as "xy xy xy xy". That fixes #3002. (To get the literal comma, it can be \\ escaped or quoted still)

Both of these are in the "theoretically backwards-incompatible, but unlikely to break stuff" category. The former especially will only break code that does useless stuff - currently a {} could be removed everywhere without any changes, so why have it at all?

TODOs:

  • Changes to fish usage are reflected in user documentation/manpages - the first has been, the second behavior was never documented AFAICT.
  • Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.md

faho added some commits Jan 2, 2018

Make literal "{}" expand to itself
This caused major annoyances with e.g. `find -exec`, and it's utterly
useless - "{}" expands to nothing, so why have it at all?

Fixes #1109.
Don't count successive "," as literal in brace expansion
This was highly surprising.

Fixes #3002.

@faho faho added the enhancement label Jan 2, 2018

@faho faho added this to the fish-3.0 milestone Jan 2, 2018

faho added some commits Jan 2, 2018

Fix {} tests
I've misplaced an empty line, sorry!
@faho

This comment has been minimized.

Copy link
Member

faho commented Jan 2, 2018

Note: The {,,,} behavior dates back to 149594f - the initial revision. The comment has been added later on, but I have no idea if there's been any prior discussion on it that justified the feature. To me, it just seems wrong(tm).

@mqudsi

This comment has been minimized.

Copy link
Contributor

mqudsi commented Jan 3, 2018

That is awesome, thanks for tackling this! I agree the alternating comma behavior is weird, I'm presuming \, still specifies an explicit comma, which would be the obvious approach.

Any chance you could implement #3802 while you're at it?

@faho

This comment has been minimized.

Copy link
Member

faho commented Jan 3, 2018

I'm presuming , still specifies an explicit comma, which would be the obvious approach.

Yes. Which is why I'm oblivious as to why the current behavior was a thing in the first place, and why this is rather unlikely to cause any issues in practice.

Any chance you could implement #3802 while you're at it?

No. That would require touching the parser/tokenizer (since expand.cpp receives BRACKET_END for the space IIUC), and I do not understand that yet.

@faho

This comment has been minimized.

Copy link
Member

faho commented Jan 7, 2018

Merged as 55ebf44..d67b4d6 (that last was because of some weirdness, possibly related to git rerere).

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