Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add which function for finding executables in PATH #2440
base: master
Are you sure you want to change the base?
Add which function for finding executables in PATH #2440
Changes from all commits
6fb2400
f1980b5
0c6f5e8
389b2ae
2a535c0
34f2ea6
740c0ae
c89e182
aad3831
5aa3c07
e7e9d56
6a98c39
1f40ec3
d642b1d
ea958df
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about this version? I found the original logic a little hard to follow:
(Also included some comments which don't need to go into the final PR.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally find my implementation easier to understand, but that might only be because I wrote it.
The main difference between our implementations seems to be how we determine whether to resolve via
PATH
. Mine checkscommand.components.count()
: anything with more than one component is considered a relative path and skipsPATH
resolution. Meanwhile, yours checkscommand.components.next()
: anything whose first element is.
,..
, or/
is considered a relative path and skipsPATH
resolution.Crucially, they differ on how they handle something like
which("dir/cmd")
, which has more than one component, but the first component is aComponent::Normal("dir")
. Thus, my implementation would consider it the same as./dir/cmd
(i.e., resolved relative to CWD), while yours would resolve it using thePATH
variable.I wrote a small test, and (at least on macOS with
sh
,bash
, andzsh
) it seems like my implementation has the correct behavior in this instance:The other differences are:
Path::new(command)
rather thanPathBuf::from(command)
. I've adopted this change since it's more efficient (and alsolexiclean()
to remove extraneous..
or.
components. However, at least thewhich
implementation on my system (macOS) does not do this./
) implicitly, through the behavior ofPath::join()
(TIL about this behavior!). But I personally think it's clearer to handle this case explicitly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yah, you're right, in addition to your test, I looked at the [POSIX shell standard](https://pubs.opengroup.org/onlinepubs/9699919799.2018edition/utilities/V3_chap02.html_ (search for "command search and execution"), and it says that for any path with a, the
PATH
variable isn't consulted.So I think
command.components.count()
is best. (Which, if we're doing that, it makes sense to use that to check for an empty command, and not.is_empty()
)GNU which does actually remove
..
:So I'm in favor of using
lexiclean
, since it makes the returned paths easier to read, in case they return..
or.
.I personally like using the relative path behavior to do joining, but I think that's kind of a wash, since it's a little weird.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't aware of that, thanks for finding that behavior! But if someone really wants to clean up the path, can't they do something like
clean(which("some/../funny/path"))
? Or evencanonicalize(which("some/../funny/path"))
?The argument against cleaning the path is that doing so removes information that can no longer be recovered. I'm wary of over-normalization for weird edge cases like the example mentioned in the Rust Path::components() docs.
Admittedly, I can't personally foresee any scenario where this relative path information might actually be useful, so I'm strongly against calling
lexiclean()
here. But since the user can opt into normalization with other Just functions likeclean
andcanonicalize
, shouldn't we leave this to the user?