Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Rewrite __fish_complete_cd #2289

Closed
wants to merge 4 commits into from
Closed

Rewrite __fish_complete_cd #2289

wants to merge 4 commits into from

Conversation

faho
Copy link
Member

@faho faho commented Aug 10, 2015

This no longer uses "eval" (which is scary), and is a bit shorter (which
is nice).

I'd appreciate testing since there's bound to be a ton of edge-cases.

Sorry for the noise, I'll throw pillows at myself for the appropriate amount of time, as soon as I can figure out how to do that (maybe that's what selfie-sticks are good for?).

@faho
Rewrite __fish_complete_cd
f6f8ee2 
This no longer uses "eval" (which is scary), and is a bit shorter (which
is nice).
@lledey
Copy link
Contributor

lledey commented Aug 10, 2015

realpath is not available on OS X so it just throws an error, and there is apparently no easy way I could find to reproduce the behavior, except maybe make it a builtin (bash seems to have a realpath module but not included by default).

@faho
Copy link
Member Author

faho commented Aug 10, 2015

Hmm... I think in this particular case (realpath .), we should just be able to get away with replacing it with "$PWD" (as I've already done further down), since fish sanitizes that anyway - or is there anything I'm missing?

@lledey
Copy link
Contributor

lledey commented Aug 10, 2015

If "$PWD" is sanitized and trustworthy it should not make any difference I think.

@faho
cd completion: Replace "realpath ." with $PWD
f0505b0 
OSX doesn't have realpath by default

This is assuming that $PWD is sanitized, but the worst that could happen
is that we print an unnecessary description
@faho
cd completion: Replace cd-ing with test -x
51dd9f4 
If something's a directory and "executable" for us, that should mean we
should be able to cd to it.
This was referenced Aug 13, 2015
Closed
Closed
@faho
Copy link
Member Author

faho commented Aug 21, 2015

Anyone to test this, particularly on OSX?

It fixes #2299 and probably part of #2300, so I'd like to merge it.

@lledey
Copy link
Contributor

lledey commented Aug 21, 2015

I personally haven't found any issue with this on OS X, but I only used it in "classic" cases so maybe you should get feedback from other people too.

@ridiculousfish
Copy link
Member

I'll be glad to review it (once I'm done with the strings mega-patch!)

@faho
Copy link
Member Author

faho commented Aug 24, 2015

Oh, this would also fix (the cd-part of) #562 - as I said in #2300, there seems to be a more general part of the problem, since selecting from the menu works, while directly entering (e.g.) "/tmp/[" and pressing TAB will complete, but not escape the "[".

This was referenced Aug 26, 2015
Closed
Closed
@faho
cd completion: Switch to string
7f52b85 
This also fixes a bug where "$HOME" would be replaced with "~"
everywhere in the string, so /tmp$HOME/something ($HOME starts with a "/") would become "/tmp~/something".
@faho
Copy link
Member Author

faho commented Sep 29, 2015

@ridiculousfish: Ping? I've been using this since I proposed it and haven't stumbled over any more bugs, but I'd rather have someone check it because there could be a ton of edgecases.

@ridiculousfish ridiculousfish self-assigned this Sep 29, 2015
@ridiculousfish
Copy link
Member

Assigning to myself to remember to review it.

@faho
Copy link
Member Author

faho commented Sep 29, 2015

The merge conflict is because of the sgrep thing, btw.

@faho
Copy link
Member Author

faho commented Oct 5, 2015

@ridiculousfish: Ping?

@ridiculousfish
Copy link
Member

Sorry for the delay...I added one more comment. I think it should be good to go after addressing that.

@faho
Copy link
Member Author

faho commented Oct 7, 2015

Squashed rebase merge as 0a99772.

I've tried listing all the bugs this fixes or improves, but I'm not sure I caught them all.

Thanks for the review!

@faho faho closed this Oct 7, 2015
@zanchey zanchey added this to the next-2.x milestone Oct 7, 2015
@ridiculousfish
Copy link
Member

This is a sweet fix. Thanks for taking it on!

@faho faho added the release notes Something that is or should be mentioned in the release notes label Oct 26, 2015
@faho faho deleted the cd-no-poopyhead branch December 16, 2015 18:07
This was referenced Jan 3, 2016
Closed
Closed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@ridiculousfish ridiculousfish

Labels
release notes Something that is or should be mentioned in the release notes
Projects
None yet
Milestone
fish 2.3.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@faho @lledey @ridiculousfish @zanchey