-
Notifications
You must be signed in to change notification settings - Fork 56
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
fix(init): Support dots in final path item #1425
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,6 +24,7 @@ use flox_rust_sdk::models::pkgdb::scrape_input; | |
use flox_rust_sdk::models::search::{do_search, PathOrJson, Query, SearchParams, SearchResult}; | ||
use indoc::formatdoc; | ||
use log::debug; | ||
use path_dedot::ParseDot; | ||
use toml_edit::{DocumentMut, Formatted, Item, Table, Value}; | ||
use tracing::instrument; | ||
|
||
|
@@ -79,6 +80,7 @@ impl Init { | |
EnvironmentName::from_str(DEFAULT_NAME)? | ||
} else { | ||
let name = dir | ||
.parse_dot()? | ||
Comment on lines
82
to
+83
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nb: while this mitigates the error we're seeing, we could normalize the path earlier either for this method (up in L74) or at the argument parsing level. Since relative paths have yet to cause issues outside of this context we may as well keep it local here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It also makes sense to use and report the original path in any error messages so that it matches the user's CLI arguments. |
||
.file_name() | ||
.map(|n| n.to_string_lossy().to_string()) | ||
.context("Can't init in root")?; | ||
|
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's curious to me is that this appears to be the same crate as https://github.com/magiclen/path-absolutize (same author as this: https://github.com/magiclen/path-dedot).
Though the former seems to be at least partially a wrapper around this crate.
Makes me a bit suspicious but may be not too noteworthy.
If we're truly concerned, the meat of this is a single ~100loc function, or we copy in the 25loc cargo implementation which does less but arguably enough if its enough for cargo.
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.
Suspicious in what kind of way, like a supply chain attack? 🤔
It doesn't do what we need which is to resolve the path without any
.
or..
so that we can take a name from the final path item.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.
For the cargo one you can join the oath with fwd first if it's relative or otherwise normalize if it's already absolute but🤔
And yea suspicious for sums supply chain attacks. Not that I have any reason to expect anything any more than with other crates. Just the fact that this library is in crates.io twice for some reason is unusual that's all
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.
Then we've implemented some portion of
path-dedot
(albeit without the caching that we don't use/need) and need our own unit tests around a separate function. My one takeaway from looking at all of the issues and implementations is that correct parsing is deceptively un-trivial and it'd be easier to use something off-the-shelf.Gotcha. I think it's OK, in that
path-absolutize
is a super-set ofpath-dedot
, and just happens to expose it if you want access to both.