Skip to content
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 Physical mode to realpath #2459

Merged
merged 1 commit into from
Aug 21, 2021

Conversation

jaggededgedjustice
Copy link
Contributor

@jaggededgedjustice jaggededgedjustice commented Jun 27, 2021

This adds the 'Physical Mode' and 'Logical Mode' switches to realpath, which control when symlinks are resolved.

For other utilities that use the canonicalize function I have preserved the current behaviour.

@jaggededgedjustice jaggededgedjustice force-pushed the realpath-add-physical-mode branch 3 times, most recently from 96771ef to 94fc6f1 Compare June 27, 2021 12:16
Copy link
Member

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

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

Nice work! Great to see this functionality become available for other utils as well. I just have some small suggestions and please update the doc comment on the canonizalize function to include the ResolveMode argument.

Comment on lines 232 to 236
match fs::canonicalize(result) {
Err(e) => return Err(e),
Ok(path) => {
result = path;
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
match fs::canonicalize(result) {
Err(e) => return Err(e),
Ok(path) => {
result = path;
}
result = canonicalize(result)?;

Comment on lines 82 to 94
.arg(
Arg::with_name(OPT_LOGICAL)
.short("L")
.long(OPT_LOGICAL)
.help("resolve '..' components before symlinks"),
)
.arg(
Arg::with_name(OPT_PHYSICAL)
.short("P")
.long(OPT_PHYSICAL)
.help("resolve symlinks as encountered (default)"),
)
Copy link
Member

Choose a reason for hiding this comment

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

I think these options should override each other. See overrides_with

Comment on lines 75 to 81
pub enum ResolveMode {
/// Resolve symlinks as encountered when processing the path
Physical,

/// Resolve '..' elements before symlinks
Logical,
}
Copy link
Member

Choose a reason for hiding this comment

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

I might be totally wrong here and it's a remnant of the existing implementation, but I feel like the concerns are not well separated between ResolveMode and CanonicalizeMode. I would expect something like:

enum ResolveMode {
    None,
    Logical,
    Physical,
}

enum MissingHandling {
    Normal,
    Existing,
    Missing,
}

But maybe that doesn't make sense for a reason I'm not seeing...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I think your suggestion is correct. The CanonicalizeMode is a remnant of the existing implementation and the options for handling symlinks should be grouped together.

@jaggededgedjustice jaggededgedjustice force-pushed the realpath-add-physical-mode branch 2 times, most recently from 48bbb15 to fcc7572 Compare July 4, 2021 14:28
@jaggededgedjustice
Copy link
Contributor Author

I'm not sure why the windows tests are failing, as I don't have a windows box set up to test on. Any suggestions for potential fixes would be appreciated.

@tertsdiepraam
Copy link
Member

I've previously tested some utils on wine, maybe that would work? I could also take a look at it later

@jaggededgedjustice
Copy link
Contributor Author

Ok, I have discovered the cause for the test failures. When using the Physical mode, the path is passed to std::fs::canonicalize from the documentation

On Windows, this converts the path to use extended length path syntax, which allows your program to use longer path names, but means you can only join backslash-delimited paths to it, and it may be incompatible with other applications (if passed to the application on the command-line, or written to a file another application may read).

The \\?\ at the start of the path is the indicator for the 'extended length path syntax'. So we can either leave the function as is and change the tests so they pass on windows or strip out the \\?\ when running on windows and using physical mode.

@jaggededgedjustice jaggededgedjustice force-pushed the realpath-add-physical-mode branch from fcc7572 to 9d287f6 Compare July 17, 2021 13:26
@jaggededgedjustice
Copy link
Contributor Author

Ok, I have a windows vm that I can test with and I made a change to remove the extended path length prefix. Now there's another failure, which also happens on my vm when I run cargo test but if I run the command directly then it works as it should, so I need to do more investigation.

@jaggededgedjustice jaggededgedjustice force-pushed the realpath-add-physical-mode branch 2 times, most recently from cf371c2 to efe2c6e Compare July 25, 2021 20:02
@sylvestre
Copy link
Contributor

Did you look how it performs again the gnu testsuite ?
I see "PASS: 171" when we are usually around 175

@jaggededgedjustice
Copy link
Contributor Author

I had a look and the behaviour of readlink changed, I'll get it fixed.

This adds the 'Physical Mode' and 'Logical Mode' switches to realpath, which control when symlinks are resolved.
@jaggededgedjustice jaggededgedjustice force-pushed the realpath-add-physical-mode branch from efe2c6e to 0e04f95 Compare August 1, 2021 16:06
@jaggededgedjustice
Copy link
Contributor Author

The problem with readlink has been fixed now, so this should be good to merge.

@miDeb
Copy link
Contributor

miDeb commented Aug 21, 2021

Thanks! The test failures are all because of a bug in a sort test that has since been fixed, so let's merge this!

@miDeb miDeb merged commit 5e07d58 into uutils:master Aug 21, 2021
@jfinkels
Copy link
Collaborator

Partly fixes issue #2253.

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.

5 participants