Repository navigation
Enable path globbing for native commands - #1639
Andy Jordan (andyleejordan) wants to merge 1 commit into
Conversation
E.g. `ls *.md` expands to `ls /path/to/README.md /path/to/CHANGELOG.md` If expansion fails, the argument is sent literally, so `ls foo*` results in `/usr/bin/ls: cannot access foo*: No such file or directory`. Don't glob expand if the verbatim marker was used.
|
|
||
| foreach (string path in resolvedPaths) | ||
| { | ||
| arg = path; |
There was a problem hiding this comment.
There are cases in the following loop where arg gets modified, so path cannot be used as it's an iterator. So arg = path seemed to be the cleanest solution (and meant not having to rename the variable elsewhere).
|
The globber doesn't appear to be handling escapes. |
|
The failures are legit - in some cases we can't glob. At a minimum, I think we can't do globbing on Windows at all - Windows tools implement globbing themselves, so even if we did something, it might work differently than the tool we're calling. That said, we still need to be smarter somehow as the test failure demonstrates (basically At a minimum, I think we need a way to escape globbing characters, but that's messy. The question is - where do process escapes? If it was just in the native command processor, it's less risky, but also less useful. If we do it in the globbing code, we also run a greater risk of breaking something. I'm fine with punting the escape character for now, make that an issue, but put the globbing under |
|
Removed review needed since tests are failing... |
|
Closing until I get back to this. Thanks for the review! |
origin/master.Enable path globbing for native commands
E.g.
ls *.mdexpands tols /path/to/README.md /path/to/CHANGELOG.mdIf expansion fails, the argument is sent literally, so
ls foo*results in/usr/bin/ls: cannot access foo*: No such file or directory.Don't glob expand if the verbatim marker was used.
/cc Jason Shirk (@lzybkr) James Truher (@JamesWTruher)
After tracing what the native command parameter binder does, it seems most reasonable to add this logic to
appendOneNativeArgument. I know that doesn't sound right, but in fact it already deals with being sent an argument that gets expanded to a list. However I could be completely wrong, as the code around here is a bit strange.When a
parameter.ParameterNameSpecifiedis true, we don't want to glob, as this correlates to-flag. Moreover, theCommandParameterInternalshould probably be updated to interpret--flagas aParameter(instead of anArgument); as it does not currently.We want to glob when
parameter.ArgumentSpecifiedis true, and only once it's been converted to a string (or expanded to a set of strings) (which happens in the poorly namedappendOneNativeArgumentfunction).We do not want to glob if the
--%verbatim marker was seen.I need to add tests, but figured I'd get this up for review.
Also I already have one known bug: globbing doesn't appear to work for
.dotfiles. So although*.mdworks,.gi*and*git*does not work. But it doesn't have anything to do with these changes, it fails similarly forGet-ChildItemtoo. I'll file a bug report tomorrow.