Fish hangs on autocompletion because of Grep descend into sub-dirs #2245

Closed
loolooyyyy opened this Issue Jul 27, 2015 · 6 comments

Comments

Projects
None yet
3 participants
@loolooyyyy

There is not much information I can provide (yet) because the situation is a little bit ugly, when the fish shell hangs, well, everything on my laptop hangs.

It's related to grep. the most useful information I've been able to get so far is this, printed to terminal after I hit tab when pwd is ~/.config (I was going to check ~/.config/fish to see what plugins are available).

cd .con...
grep: .config/yyy/zz0: No such file or directory
grep: .config/yyy/zz1: No such device or address
grep: .config/yyy/zz2: No such file or directory
grep: .cache/tilda/locks/lock_00000_0: Permission denied
fig/fish

I had a chance to take a glance at the top output before everything crashed and yes, it was grep using 55% of ram, 99% of CPU. Autocompletion for cd argument does not need to descend into sub-directories, and obviously grep is called with an -R switch.

D'oh! almost forgot! being a shell plugin junky I'm using oh-my-fish. but I saw this behaviour before using any plugin (less sever but it was there).

So, something is better than nothing on the issue queue, I guess. I'll come back when I have more information.

@loolooyyyy loolooyyyy changed the title from Fish hangs on autocompletion because of Grep to Fish hangs on autocompletion because of Grep descend into sub-dirs Jul 27, 2015

@loolooyyyy

This comment has been minimized.

Show comment
Hide comment
@loolooyyyy

loolooyyyy Jul 27, 2015

Found it. had my aliases copied from oh-my-zsh from common-aliases to fish directly not checking the content. there was a line saying:

 alias sgrep='grep -R -n -H -C 5 --exclude-dir={.git,.svn,CVS} '

removed it (the -R is there!) and everything is just fine.

Found it. had my aliases copied from oh-my-zsh from common-aliases to fish directly not checking the content. there was a line saying:

 alias sgrep='grep -R -n -H -C 5 --exclude-dir={.git,.svn,CVS} '

removed it (the -R is there!) and everything is just fine.

@loolooyyyy loolooyyyy closed this Jul 27, 2015

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Jul 27, 2015

Member

There's actually still an issue here - that re-defining "sgrep" causes issues in completions.

"sgrep" is a fish function that uses grep without GREP_OPTIONS, and it's used in 37 fish completion scripts or functions, among which __fish_complete_cd and __fish_contains_opt, which are used in countless other places.

The simple solution here is to call sgrep __fish_sgrep instead which solves it for this particular case (and should probably be done anyway), but there's a larger issue since I don't think all completion scripts always use command $command, so aliases/functions can still wreak havoc.

Member

faho commented Jul 27, 2015

There's actually still an issue here - that re-defining "sgrep" causes issues in completions.

"sgrep" is a fish function that uses grep without GREP_OPTIONS, and it's used in 37 fish completion scripts or functions, among which __fish_complete_cd and __fish_contains_opt, which are used in countless other places.

The simple solution here is to call sgrep __fish_sgrep instead which solves it for this particular case (and should probably be done anyway), but there's a larger issue since I don't think all completion scripts always use command $command, so aliases/functions can still wreak havoc.

faho added a commit to faho/fish-shell that referenced this issue Jul 27, 2015

Rename sgrep to __fish_sgrep
Makes it harder to cause issues with aliases, see fish-shell#2245
@ridiculousfish

This comment has been minimized.

Show comment
Hide comment
@ridiculousfish

ridiculousfish Jul 27, 2015

Member

It would be nice to migrate off of grep entirely onto the string stuff in #156.

Member

ridiculousfish commented Jul 27, 2015

It would be nice to migrate off of grep entirely onto the string stuff in #156.

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Jul 28, 2015

Member

Well, that's a long-term solution to the grep-specific instance of the issue (and we could presumably already replace some grep usage with while read fish; switch $fish; case "flounder"; dosomething; end; end if we really wanted), but it doesn't fix other commands that are being called in completion scripts - there are currently at least (only counting the ones that start the line) 165 occurences of printf, 24 for systemctl, 13 for cat, 8 for sed.... that aren't qualified with "command" in share/functions and share/completions (grep -Ri "^[[:space:]]*[[:alnum:]]*" --only-matching --no-filename completions/ functions/ | grep "[[:alnum:]]*" --only-matching | sort | uniq -c | sort --numeric-sort).

Personally, I think the most intuitive thing is that user-defined functions/variables don't influence what happens in functions or scripts, but that'd be a huge behavioral change.

Edit: There are also at least 8 unqualified occurences of ls, and it's pretty usual to wrap that with some arguments.

Member

faho commented Jul 28, 2015

Well, that's a long-term solution to the grep-specific instance of the issue (and we could presumably already replace some grep usage with while read fish; switch $fish; case "flounder"; dosomething; end; end if we really wanted), but it doesn't fix other commands that are being called in completion scripts - there are currently at least (only counting the ones that start the line) 165 occurences of printf, 24 for systemctl, 13 for cat, 8 for sed.... that aren't qualified with "command" in share/functions and share/completions (grep -Ri "^[[:space:]]*[[:alnum:]]*" --only-matching --no-filename completions/ functions/ | grep "[[:alnum:]]*" --only-matching | sort | uniq -c | sort --numeric-sort).

Personally, I think the most intuitive thing is that user-defined functions/variables don't influence what happens in functions or scripts, but that'd be a huge behavioral change.

Edit: There are also at least 8 unqualified occurences of ls, and it's pretty usual to wrap that with some arguments.

@ridiculousfish

This comment has been minimized.

Show comment
Hide comment
@ridiculousfish

ridiculousfish Jul 28, 2015

Member

fish (at least as I see it) isn't like an OS kernel. that presents a fixed interface and bulletproof abstractions. It's more like Smalltalk, with visible exposed wiring, and you can tweak or have a conversation with your environment. It gives users more power, but in turn must exercise some care. It's reasonable to expect that if you modify printf incompatibly, bad things will happen.

So I'm opposed to an unqualified rule like "completions may only invoke commands or __fish_... functions" (not to say that anyone is proposing that). It's fine for fish to invoke unqualified cat or sed.

But in some cases fish should be more mindful of possible replacements. ls is certainly one of them and I agree we should get out of the business of calling that programmatically. (We should use globbing instead!)

Thanks for doing this survey @faho!

Member

ridiculousfish commented Jul 28, 2015

fish (at least as I see it) isn't like an OS kernel. that presents a fixed interface and bulletproof abstractions. It's more like Smalltalk, with visible exposed wiring, and you can tweak or have a conversation with your environment. It gives users more power, but in turn must exercise some care. It's reasonable to expect that if you modify printf incompatibly, bad things will happen.

So I'm opposed to an unqualified rule like "completions may only invoke commands or __fish_... functions" (not to say that anyone is proposing that). It's fine for fish to invoke unqualified cat or sed.

But in some cases fish should be more mindful of possible replacements. ls is certainly one of them and I agree we should get out of the business of calling that programmatically. (We should use globbing instead!)

Thanks for doing this survey @faho!

faho added a commit to faho/fish-shell that referenced this issue Jul 28, 2015

Rename sgrep to __fish_sgrep
Makes it harder to cause issues with aliases, see fish-shell#2245
@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Jul 28, 2015

Member

I guess I can live with that. How about this very-much-qualified rule, then:

  • If the user overrides a well-known command with something non-compatible, they get to keep the pieces (say they alias grep with "grep --color=always" or with "rm -rf $HOME/*") - though I'd say to still use "command" or "builtin" when in doubt
  • Internal fish scripts (completions and functions) should not use obvious names like "sgrep" for things that don't have a well-known definition

?

Member

faho commented Jul 28, 2015

I guess I can live with that. How about this very-much-qualified rule, then:

  • If the user overrides a well-known command with something non-compatible, they get to keep the pieces (say they alias grep with "grep --color=always" or with "rm -rf $HOME/*") - though I'd say to still use "command" or "builtin" when in doubt
  • Internal fish scripts (completions and functions) should not use obvious names like "sgrep" for things that don't have a well-known definition

?

faho added a commit to faho/fish-shell that referenced this issue Aug 21, 2015

Rename sgrep to __fish_sgrep
Makes it harder to cause issues with aliases, see fish-shell#2245

faho added a commit that referenced this issue Sep 9, 2015

Rename sgrep to __fish_sgrep
Makes it harder to cause issues with aliases, see fish-shell#2245
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment