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

change how bare `{}` is handled so `find -exec {}` isn't broken #1109

Closed
hrldcpr opened this Issue Nov 11, 2013 · 33 comments

Comments

Projects
None yet
@hrldcpr
Copy link

hrldcpr commented Nov 11, 2013

"reopening" #95

i think it's bad that simple, standard find commands fail with a perplexing error message in fish, due to its expansion of empty curly braces to empty string. e.g.:

> find . -name '*.txt' -exec cat {} +
find: -exec: no terminating ";" or "+"

as discussed in #95 the fix is to quote '{}' but this is kind of hard to figure out

@xfix

This comment has been minimized.

Copy link
Member

xfix commented Nov 11, 2013

I believe this is a duplicate of #354.

@hrldcpr

This comment has been minimized.

Copy link

hrldcpr commented Nov 11, 2013

@glitchmr well, I'm not saying fish should get rid of brace expansion in general, just the case of empty braces.

Basically, having the same behavior as bash here might be good, given that man find uses empty braces all over the place, it's confusing when it doesn't work.

@xfix

This comment has been minimized.

Copy link
Member

xfix commented Nov 11, 2013

Now that I look into it, empty brace expansion doesn't work like other brace expansions (and I already assumed they are really buggy before...). Assuming we have following function (as alternative to brace expansions - it doesn't work with newlines or globs, but other than that, it works like brace expansions):

function list        
    for elem in $argv
        echo $elem
    end
end

If we have one or two elements, this function and brace expansion work identically.

glitchmr@tomato /> list a(list 1)
a1
glitchmr@tomato /> list a{1}
a1
glitchmr@tomato /> list a(list 1 2)
a1
a2
glitchmr@tomato /> list a{1,2}
a1
a2
glitchmr@tomato /> 

However, this is not the case with empty brace expansion, as empty brace expansion expands to empty string, not an empty list.

glitchmr@tomato /> list a(list)
glitchmr@tomato /> list a{}
a
glitchmr@tomato /> 

However, let's assume that we expand empty array.

glitchmr@tomato /> list a(list $array)
glitchmr@tomato /> list a{$array}
glitchmr@tomato /> 

In my opinion, {} is incorrectly expanded to nothing (it doesn't simply kill token). As the behavior is already unlike what could be expected (see empty array case), and it's pointless, I believe it could be killed in fish 2.2.0, to make find command work properly without escaping.

@hrldcpr

This comment has been minimized.

Copy link

hrldcpr commented Nov 11, 2013

@glitchmr cool, I like that reasoning. thanks for looking into it!

@jbarlow83

This comment has been minimized.

Copy link

jbarlow83 commented Jan 16, 2014

+1. I found this behaviour in fish quite bewildering -- and there certainly isn't value in having {} expand to nothing.

If nothing else, at least replace the generic error message for a special error message for find - but it's probably easier to just fix the parser.

@kballard

This comment has been minimized.

Copy link
Contributor

kballard commented Sep 4, 2014

There is a potential value in having {} expand to nothing, which is consistency of behavior when generating fishscript strings to pass to eval. Similarly, {$empty} should behave the same as {} unless there's a very good reason not to.

On a related note, {a} is generally quite useless, except it's repurposed as a way to separate a {$variable}name from what follows. I found this unfortunate a long time ago as it makes using @{u} syntax in git commands quite annoying. But fish has chosen to make {a} evaluate to a, and for consistency's sake, {} should probably evaluate to the empty string instead of being left intact as the string {}.

@hrldcpr

This comment has been minimized.

Copy link

hrldcpr commented Sep 4, 2014

I'd be happy with either of these solutions:

  • a more helpful error message when you run find -exec with {}
  • {} not evaluating to empty

Seems like the first option is probably the better way to go.

Basically the current failures of find -exec are very confusing in fish—at first I thought find simply couldn't be used in fish, and I doubt I'm alone.

@jbarlow83

This comment has been minimized.

Copy link

jbarlow83 commented Sep 5, 2014

Standard commands written for bash (without variables or scripting) can usually be pasted into fish without modification - which is an awesome feature. The curly brace thing is one of the few cases where this breaks down - and find is quite widely used.

Anyway, if opting for the more helpful error message, find, and parallel both use curly braces for argument distribution in their subshells. Examples straight from the man pages of both will fail in fish. Since it's quite possible someone is writing find ... | rm -rf or parallel rm {} it would be well worth complaining if find or parallel with curly braces show up in a pipeline. sysadmins will thank you. parallel also uses {n} as well where n is an integer, for reordering arguments; not sure if find has that as well.

@kballard

This comment has been minimized.

Copy link
Contributor

kballard commented Sep 5, 2014

We could certainly make the literal token {} into a parse error. I'm still mildly concerned about the possibility of a string being constructed for eval that contains it, but the find issue is probably much more common than this edge case.

@kballard kballard added the enhancement label Sep 5, 2014

@ridiculousfish

This comment has been minimized.

Copy link
Member

ridiculousfish commented Sep 5, 2014

It seems like the zero-argument {} and single-argument {foo} braces are simultaneously the most commonly typed and the least useful expansions in fish. bash and zsh also don't expand these, and presumably have survived the fallout from eval'ing strings. I think on balance making both {} and {a} not expand specially (echo foo{bar} outputs foo{bar}) will lead to the least amount of confusion and annoyance.

That may be because I personally never use brace expansion, so I'm not attached to the syntax. If there is a heavy user please weigh in!

@kballard

This comment has been minimized.

Copy link
Contributor

kballard commented Sep 5, 2014

@ridiculousfish I thought you intentionally expanded {a} for the purposes of {$foo}bar? I swear there was an issue about this, where I complained because it interferes with Git's @{upstream} notation, but I can't find it.

Personally, I'm in favor of making {a} not expand, just like bash. Instead of {$foo}bar, people can always use something like $foo''bar.

@jbarlow83

This comment has been minimized.

Copy link

jbarlow83 commented Sep 6, 2014

I realized that

find . -type d -exec rm -f {}/* \;   # do not do this in fish

(i.e., delete all files in subdirectories of the working directory)

will perform the following in fish:

rm -f /*  # don't do this anywhere

...while it works as intended in bash/zsh. This is similar to what I raised before, I think it strengthens the case to give a practical example of something someone could do, rather than my vague speculation about possible nasty consequences.

grep a{1,3} is another example where expansion will have other-than-intended effects. (fish runs the command as grep a1,3 which is probably not the regex the user wanted.)

Not expanding is looking better and better.

@kballard

This comment has been minimized.

Copy link
Contributor

kballard commented Sep 6, 2014

@jbarlow83 Eek. I am very much in favor right now of making {} expand to {}, but if that doesn't happen, at the very least it needs to be fixed to kill the token.

@kballard

This comment has been minimized.

Copy link
Contributor

kballard commented Sep 6, 2014

@jbarlow83

grep a{1,3} is another example where expansion will have other-than-intended effects. (fish runs the command as grep a1,3 which is probably not the regex the user wanted.)

Huh? Fish expands that to grep a1 a3 just like Bash does.

@jbarlow83

This comment has been minimized.

Copy link

jbarlow83 commented Sep 6, 2014

You are right about grep; it would need to be quoted in bash.

@anordal

This comment has been minimized.

Copy link
Contributor

anordal commented Nov 4, 2014

We could just drop the whole brace expansion syntax if

  1. we had a decent command to replace it (see also #1187), and
  2. tab completion worked as good with command substitution as it already does with brace expansion.

Suggestion: a{1,3}a(vector 1 3),

function vector --description 'vertical echo'
    for i in $argv
        echo $i
    end
end

I'm saying this as a heavy-user of brace expansion, because the command, not syntax principle tends to be more powerful.

@faho

This comment has been minimized.

Copy link
Member

faho commented Jul 16, 2015

Duplicate of #434, though both titles are just special cases of a general problem: Brace expansion is parsed in an unintuitive (for bash/zsh users) manner.

@faho faho added duplicate and removed enhancement labels Dec 11, 2015

@faho faho removed this from the next-major milestone Dec 11, 2015

@stephane-chazelas

This comment has been minimized.

Copy link

stephane-chazelas commented Aug 4, 2016

Note that brace expansion was introduced by csh. Csh (back in the end of the 70s) explicitly made a documented special case for {} alone to be left alone (though {}foo would expand to foo).

All other shells that implemented brace expansion thereafter (tcsh, ksh, zsh, bash, yash at least) copied that behaviour ({} left alone, not the {}foo -> foo part (and {foo} -> foo)).

I can't imagine anyone relying on {} expanding to an empty argument. I'd expect people would use '' or "" for that instead, so I don't expect aligning with other shells would break anything (the eval case seems a bit contrived).

rc is another shell where {} needs to be quoted, but that's not for brace expansion there ({ and } are used in the syntax of the language).

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Aug 4, 2016

See also issue #3002. It's always dangerous to change long established behavior. But in this particular case I think it is warranted. The find ... -exec some_command {} use case is particularly compelling because people don't expect the shell to eat the braces and doing so can have catastrophic consequences. Can anyone provide a good example of how changing fish to not elide a bare, free standing, {} would break fish scripts? If not I recommend we change fish to behave like bash in this regard.

@krader1961 krader1961 reopened this Aug 4, 2016

@jbarlow83

This comment has been minimized.

Copy link

jbarlow83 commented Aug 4, 2016

I agree that is a mistake in fish's design that should be corrected.

Tab completion is great, but does not help with people converting scripts or copy-pasting command lines.

Here's an example of a safe bash command that turns into rm -rf /* in fish (I gave this example earlier up).

find . -type d -exec rm -f {}/* \;   # do not do this in fish

@faho faho removed the duplicate label Aug 4, 2016

@faho faho added this to the next-major milestone Aug 4, 2016

@stephane-chazelas

This comment has been minimized.

Copy link

stephane-chazelas commented Aug 5, 2016

Here's an example of a safe bash command that turns into rm -rf /* in fish (I gave this example earlier up).

find . -type d -exec rm -f {}/* \;   # do not do this in fish

That one works the same in csh/tcsh (the original implementation of brace expansion) as in fish though. In csh, only {} on its own is left alone.

Also note that find -exec rm -f '{}/*' \; would not work as rm doesn't do globbing, and find -exec cmd '{}word' \; is not standard. (portably, you can only rely on {} on its own to be expanded by find to the file name).

@ridiculousfish ridiculousfish self-assigned this Aug 6, 2016

@ridiculousfish

This comment has been minimized.

Copy link
Member

ridiculousfish commented Aug 6, 2016

It seems like we're all in favor of making {} expand to nothing itself. Assigning this to myself so I remember to fix it, but if someone would like to take it on, by all means please feel free.

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Aug 7, 2016

It seems like we're all in favor of making {} expand to nothing.

Since that's the current behavior I assume you meant to say "making {} expand to itself."

@jbarlow83

This comment has been minimized.

Copy link

jbarlow83 commented Aug 7, 2016

@ridiculousfish {} already expands to nothing; the suggestion is to make {} expand to {} since all other shells seem to that.

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Aug 7, 2016

Also, TBD is whether we should follow csh or bash: echo {}foo yields foo in csh and {}foo in bash. Of course the csh behavior is incompatible with fish's cartesian product behavior so we probably want the bash behavior.

@ridiculousfish

This comment has been minimized.

Copy link
Member

ridiculousfish commented Aug 7, 2016

Derp, yes, I meant expand to itself. Comment corrected. Thanks.

With that corrected, naively I would expect echo {}foo to expand to itself. Making echo {}foo expand to foo seems like a weird special case - is there any reason to prefer that?

@stephane-chazelas

This comment has been minimized.

Copy link

stephane-chazelas commented Aug 7, 2016

If you want to align with ksh/zsh/bash/yash, there's also the question of echo {foo}, and then of echo {$var} which at the moment fish recommends as the equivalent of ${var} when not quoted.

Currently in fish, {} is a list of one empty element and {$a} is the list of elements of $a (With set a, {$a} is an empty list, with set a '', that's a list of one empty element like {}). In that context, having {}var expand to {}var, when set a; {$a}foo expands to nothing and set a ''; {$a}foo expands to foo may seem strange.

Note that there are some differences between all the ksh (ksh93, pdksh/mksh), bash, zsh and yash -o braceexpand implementations.

Generally for all those, {...} must contain at least one , to count as a brace expansion. I suppose the rationale is because you don't need brace expansion if there's only one value in them ({foo}bar is the same as foobar}.

Now, there's the question of the interaction with other expansions. Except maybe in zsh and yash (which behave more like fish), brace expansion looks more like an after-thought.

In bash, brace expansion is done very early which can cause some surprises like

$ bash -c 'a=a b=b; echo {$a,$b}-c'
a-c b-c
$ bash -c 'a=a b=b; echo {$a,$b}_c'

$

(That's interpreted as echo $a_c $b_c, you'd actually need:

bash -c 'a=a b=b; echo {${a},${b}}_c

or better:

bash -c 'a=a b=b; echo {"$a","$b"}_c

bash is also the only shell to give this result to this kind of construct:

$ bash -c 'echo {a,b,c}$((++x))'
a1 b2 c3
$ bash -c 'echo {a,b,c}$(readlink /proc/self)'
a27241 b27243 c27245

(those arithmetic and command substitutions evaluated several times)

Playing with the limits of the parser there can be interesting:

~$ bash -c 'echo $[{1+1,2*3,4/4}]'
2 6 1
~$ bash -c 'echo $(({1+1,2*3,4/4}))'
bash: {1+1,2*3,4/4}: syntax error: operand expected (error token is "{1+1,2*3,4/4}")

In ksh, that's the opposite problem:

$ ksh93 -c 'a=1,2; echo {$a,3}x'
1x 2x 3x

Quoting helps in pdksh/mksh, but not ksh93:

$ ksh93 -c 'a=1,2; echo {"$a",3}x'
1x 2x 3x
$ mksh -c 'a=1,2; echo {"$a",3}x'
1,2x 3x

In mksh, it can get worse though:

mksh -c 'a="1,2}{4"; echo {$a,3}x'
14x 13x 24x 23x

One thing I noticed is that all shells (including fish and csh) treat {a,{1,2},b} as {a,1,2,b}. However with both fish and zsh:

$ fish -c 'set v a b; echo {1,$v,2}x'
1x ax 2x 1x bx 2x
$ zsh -c 'v=(a b); echo {1,$^v,2}x'
1x ax 2x 1x bx 2x

Another thing fish and zsh have in common:

$ zsh -c 'echo ~ma{n,il}'
/var/cache/man /var/mail
$ fish -c 'echo ~ma{n,il}'
/var/cache/man /var/mail

(bash as well, but that's not surprising for bash)

@krader1961 krader1961 modified the milestones: fish-future, next-major Mar 15, 2017

@krader1961 krader1961 changed the title find -exec somewhat broken change how bare `{}` is handled so `find -exec {}` isn't broken Mar 15, 2017

@zanchey

This comment has been minimized.

Copy link
Member

zanchey commented Jul 9, 2017

If we're doing a 3.0 (syntax-breaking) release, let's add this.

@zanchey zanchey modified the milestones: fish-3.0, fish-future Jul 9, 2017

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Jul 9, 2017

If we're doing a 3.0 (syntax-breaking) release, let's add this.

Agreed, although there is the question of exactly which behavior we want. For example, csh or bash.

@faho

This comment has been minimized.

Copy link
Member

faho commented Jul 9, 2017

I don't see how csh's behavior is useful here.

We want to change the behavior for a literal {} because that is currently utterly useless - it always expands to nothing, so why use it in the first place? That also applies to {}foo expanding to foo - what use is the {} prefix there?

Its also consistent to have {} expand to itself (or "not expand" depending on how you look at it) in both cases - when standing alone, and when used as part of a word.

That means:

echo {} # prints {}
echo foo{}bar # prints foo{}bar
echo foo{a,{}}bar # prints fooabarr foo{}bar
echo foo{{}}bar # prints foo{}bar

In other words, something is only a brace expansion if it matches {.+}

@asymmetric

This comment has been minimized.

Copy link

asymmetric commented Dec 19, 2017

So do I understand correctly that this change would have to wait for a major version to be released?

@faho

This comment has been minimized.

Copy link
Member

faho commented Dec 19, 2017

Strictly speaking, yes. And... good news: The next fish version is planned to be 3.0.

@mqudsi mqudsi closed this Jan 2, 2018

@mqudsi mqudsi reopened this Jan 2, 2018

@mqudsi

This comment has been minimized.

Copy link
Contributor

mqudsi commented Jan 2, 2018

I ran into an edge case yesterday where I actually used the bare {} syntax - I wanted to do something to files that shared a sequence number with files of a different name but the same sequence number, so I actually used this syntax:

rm file1_{(for n in (seq 20 100)
    if test -e file2_$n
        echo $n
    end
end)}

except I didn’t use rm. The real problem isn’t that {} is useless (even aside from the fact that using two for loops would have been more idiomatic and easier on the eyes) but that you almost never can use the result of an empty expansion since the behavior of Unix commands generally changes when the number of arguments provided dwindles down to zero, ie the command above actually doesn’t do what I would want since passing zero arguments to rm is not the same as not executing rm.

Basically, even when the intended expansion of braces is an empty set, they are still useless 😃

faho added a commit to faho/fish-shell that referenced this issue 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 fish-shell#1109.

@faho faho referenced this issue Jan 2, 2018

Closed

Fix brackets #4632

2 of 3 tasks complete

@faho faho closed this in 55ebf44 Jan 7, 2018

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