Skip to content

WIP: Implement simple tilde expansion and globbing #3919

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

Closed
wants to merge 3 commits into from
Closed

Conversation

timholy
Copy link
Member

@timholy timholy commented Aug 2, 2013

This at least partially implements #1136. This does not support ~someotheruser notation.

Once I learned more about how backticks worked, it was so dirt-simple that I'm suspicious this is overlooking something big. @StefanKarpinski in particular please review.

I'd really like to get this merged for 0.2. Globbing would be even nicer. Is shell-parse the right place to put that? It seems quite a bit more complicated.

@timholy
Copy link
Member Author

timholy commented Aug 2, 2013

OK, I went ahead and added simple globbing. Aside from the limitations listed above glob(), I'm hoping one of our regex gurus can tell me how to implement it with less than 3 calls to replace().

@timholy
Copy link
Member Author

timholy commented Aug 2, 2013

FYI the Travis error on the 2nd patch seems totally unrelated (it couldn't build the items in the deps/ directory).

@staticfloat
Copy link
Member

I restarted the Travis build for you, everything looks green now.

@timholy
Copy link
Member Author

timholy commented Aug 2, 2013

Didn't know you could do that!

@staticfloat
Copy link
Member

I'm not sure you can! It's only people who have some kind of permissions on this Github repo. You can find out if you can or not by dropping down the gear icon on the top right on that Travis page. Useful to figure out if something was a transient failure, or something really having to do with this code.

x = string(ENV["HOME"], x[2:end])
end
if search(x, ['*', '?']) != 0
paths = glob(x)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this glob expansion should be eager (as it is here), or delayed until the command is spawned.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I first read this, I was inclined towards lazy (delayed until command execution), but then I realized that I would really like to be able to do this:

julia> `~./julia`
`/Users/stefan/.julia`

which implies eager expansion. Likewise for globbing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, there are actually three times at which globbing can occur:

  1. at backtick parsing time
  2. at backtick evaluation time
  3. at command execution time

This is (1), you're talking about (3) and I think that (2) is actually right.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will any of these options fix this?

julia> ;ls a*
ERROR: Cannot access a*: no such file or directory

julia> ;cd ~/Documents; ls a*
ERROR: Cannot access a*: no such file or directory

julia> ;cd ~/Documents
/home/tim/Documents

julia> ;ls a*
./admin:
<...lots of files...>

./applications:
<...lots of files...>

...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not clear on what the issue is here. Can you explain what's happening?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I append two shell commands with a ;, for this to work as it does in the shell, the globbing needs to run after the cd command is finished. With the current buggy eager implementation (glob at parsing time), it tries to resolve a* before the cd command executes.

I'm vague on how spawn handles these things, but since it's a single backticks expression does that pose some challenges?

Or I could ask this another way: in the lazy vs eager discussion, I'm not quite on yours and @vtjnash's wavelength yet. I might need more examples of things that we want to work---and things we don't want to break---that might require some care in the implementation.

@timholy
Copy link
Member Author

timholy commented Aug 8, 2013

Given that 0.2 seems to be approaching rapidly (and this feature has the potential for subtle breakage), and because I think that currently my "altruistic" coding time is better-spent on graphics, I'm going to let this one sit. I'm going to mark it "up for grabs" so no one expects that I'm going to get back to this anytime soon.

@StefanKarpinski
Copy link
Member

Yes, unfortunately, I think it needs the same level of careful attention that was spent on the original ability to call out to shell commands with shell-compatible escaping and quoting. Let's definitely wait until post-0.2.

@lucasb-eyer
Copy link
Contributor

More info for when that careful attention will be taken:

  • --foo=~/bar, gets expanded in bash, but --foo~/bar doesn't get expanded. In zsh, none of those get expanded.
  • Both bash and zsh support (and I love) {}-expansion: echo {f,g}oo{b,c,d}ar gets expanded to foobar foocar foodar goobar goocar goodar.

@JeffBezanson
Copy link
Member

Bump. Can this be put in shape quickly and merged, or should we move to 0.4?

@timholy
Copy link
Member Author

timholy commented Jan 17, 2014

Would be lovely. But I don't think I'm the right person for this particular job---it's just not my skill set.

@StefanKarpinski
Copy link
Member

This is a pretty tricky feature to really get right – and the kind of feature that it's better not to have than to have a broken version of. If people think it's a priority, I can work on it though. I believe that @kbara was working on proper user home directory lookup (which is a very difficult thing to do well portably). Maybe we can get that merged as a first step.

@blakejohnson
Copy link
Contributor

Is this closed because Tim indicated that continuing it was not in his wheelhouse?

@StefanKarpinski
Copy link
Member

So there are two pieces: user expansion and globbing. @vtjnash has implemented POSIX globbing in Glob.jl. A good approach to handling globbing would be to move the code from Glob.jl into Base and hook it up to backtick parsing. User expansion is a little harder. We need to use the correct APIs on each system for mapping user names to home directories – I'm not even sure that those APIs are. Maybe looking at an MIT or BSD-license shell implementation would be a good start.

@quinnj
Copy link
Member

quinnj commented Mar 24, 2015

Or python's os.expanduser function.

On Tue, Mar 24, 2015 at 10:39 AM, Stefan Karpinski <[email protected]

wrote:

So there are two pieces: user expansion and globbing. @vtjnash
https://github.com/vtjnash has implemented POSIX globbing in Glob.jl
https://github.com/vtjnash/Glob.jl. A good approach to handling
globbing would be to move the code from Glob.jl into Base and hook it up to
backtick parsing. User expansion is a little harder. We need to use the
correct APIs on each system for mapping user names to home directories
– I'm not even sure that those APIs are. Maybe looking at an MIT or
BSD-license shell implementation would be a good start.


Reply to this email directly or view it on GitHub
#3919 (comment).

@timholy
Copy link
Member Author

timholy commented Mar 24, 2015

@blakejohnson, if it were an issue, then it should stay open until the issue was addressed. But this was a pull request---one that is probably not a good starting point for implementation---so I agree with @Keno's having closed it.

@Sean1708
Copy link
Contributor

@StefanKarpinski I made some changes to the Unix implementation of expanduser in #8668 though there were some (non-fatal) issues with the linux build. I'm not sure how much it'll help but it might make a good starting point.

@KristofferC KristofferC deleted the tildeglob branch June 4, 2018 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants