fix(realpath): To error out in long paths#13223
Conversation
| if path_str.ends_with("/.") || path_str.ends_with("/./") { | ||
| abs.metadata()?; // raise no such file or directory error | ||
| for component in p.components() { | ||
| if let Some(name) = component.as_os_str().to_str() { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@sylvestre review that |
|
Does this fix #13224 too? |
I would appreciate a more polite message |
|
oh sorry, yes you are right, we need talk in a polite manner, since we are humans |
|
also, please ping me only when the testsuite is green and there are tests |
Merging this PR will degrade performance by 12.95%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | df_with_path |
242 µs | 304.4 µs | -20.51% |
| ❌ | Simulation | sort_ascii_c_locale |
16 ms | 16.7 ms | -4.67% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing HackingRepo:patch-3 (e172782) with main (02dfaa2)
Footnotes
-
46 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
Added tests to check behavior with long path components.
|
GNU testsuite comparison: |
|
Hi, @sylvestre now all tests passed and added tests and fix working, can you merge? i appreciate it or still |
| const OPT_RELATIVE_TO: &str = "relative-to"; | ||
| const OPT_RELATIVE_BASE: &str = "relative-base"; | ||
|
|
||
| const MAX_PATH: usize = 255; |
|
after i restore the tests, they failing that was weird, i just added the max limit then failed, i will see why |
|
the openbsd test failing because error: extern location for proc_macro2 does not exist: /home/runner/work/coreutils/coreutils/target/debug/deps/libproc_macro2-24e159babf15897f.rlib
--> /home/tester/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/yoke-derive-0.8.2/src/lifetimes.rs:5:5
|
5 | use proc_macro2::Span;
| ^^^^^^^^^^^
error[E0463]: can't find crate for `syn`
--> /home/tester/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/yoke-derive-0.8.2/src/lifetimes.rs:6:5
|
6 | use syn::ext::IdentExt as _;
| ^^^ can't find crate
error: extern location for quote does not exist: /home/runner/work/coreutils/coreutils/target/debug/deps/libquote-c6c049b8eedf5bdc.rlib
--> /home/tester/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/yoke-derive-0.8.2/src/lib.rs:25:5
|
25 | use quote::quote;
| ^^^^^
error[E0463]: can't find crate for `synstructure`
--> /home/tester/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/yoke-derive-0.8.2/src/lib.rs:29:5
|
29 | use synstructure::Structure;
| ^^^^^^^^^^^^ can't find crateabout soomething different not about realpath at all, it causing issues |
|
the 1 failing last job, is about install unrelated fully to the pr, realpath is the thing we fixed, not install that just some problems occurs with CI/CD sometimes, ci/cd race issues sometime tests successed, however the important tests passed and only 1 job failing that happen in tons of prs. but however only 1 failing not a huge problem |
|
Hi, @sylvestre i appreciate it, when you have time, can you please review it again all tests passes, the 1 failing is unrelated to the pr and restored previous tests and ignored rsplit to cspell not fails and now passed, can you review and merge? |
| .to_str() | ||
| .is_some_and(|name| name.len() > MAX_PATH) | ||
| }) { | ||
| return Err(Error::new(ErrorKind::InvalidInput, "File name too long")); |
There was a problem hiding this comment.
please use the translate! macro here
|
@cakebaker what about that one? can you review it, i appreciate it |
|
hi, @sylvestre review now. i added translate! no breaking change, or logic change, so what do you think, i appreciate it, when having time |
| .to_str() | ||
| .is_some_and(|name| name.len() > MAX_PATH) | ||
| }) { | ||
| return Err(Error::new(ErrorKind::InvalidInput, translate!("File name too long"))); |
There was a problem hiding this comment.
it isn't the way translate works
please look at other places to see how it works
| abs.metadata()?; // raise no such file or directory error | ||
| let p_str = p.to_string_lossy(); | ||
| let has_trailing_dot = | ||
| p_str.ends_with("/.") || p_str.ends_with("/..") || p_str == "." || p_str == ".."; |
There was a problem hiding this comment.
we don't have a function already doing that in the code base ?
Fixes #13154
Fixes #13224