Skip to content

Conversation

@amirhosseinghanipour
Copy link
Contributor

This pull request proposes a minor stylistic simplification for the physical_path function in the pwd utility.

The current implementation is perfectly correct and performant and correctly handles the platform-specific behaviors of env::current_dir(). It correctly avoids a redundant canonicalize() call on Unix while applying it on other platforms like Windows where it's necessary.

This PR suggests a small refactor to reduce code duplication. By calling env::current_dir() once and then using #[cfg] blocks to apply the canonicalize() call only where needed, the function becomes slightly more DRY.

Before:

fn physical_path() -> io::Result<PathBuf> {
    // std::env::current_dir() is a thin wrapper around libc::getcwd().

    // On Unix, getcwd() must return the physical path:
    // https://pubs.opengroup.org/onlinepubs/9699919799/functions/getcwd.html
    #[cfg(unix)]
    {
        env::current_dir()
    }

    // On Windows we have to resolve it.
    // On other systems we also resolve it, just in case.
    #[cfg(not(unix))]
    {
        env::current_dir().and_then(|path| path.canonicalize())
    }
}

After:

fn physical_path() -> io::Result<PathBuf> {
    // std::env::current_dir() is a thin wrapper around libc::getcwd().
    let path = env::current_dir()?;

    // On Unix, getcwd() must return the physical path:
    // https://pubs.opengroup.org/onlinepubs/9699919799/functions/getcwd.html
    #[cfg(unix)]
    {
        Ok(path)
    }

    // On Windows we have to resolve it.
    // On other systems we also resolve it, just in case.
    #[cfg(not(unix))]
    {
        path.canonicalize()
    }
}

I want to emphasize that this is purely a stylistic suggestion intended to enhance clarity. The functionality and performance are identical, as any modern compiler will optimize both versions to the same machine code. I recognize the original implementation is a very direct and efficient way to express the logic. This alternative is simply another valid approach that some developers may find cleaner.

Thank you for considering this minor improvement. I appreciate the high quality of the existing codebase.

@github-actions
Copy link

GNU testsuite comparison:

Skip an intermittent issue tests/misc/tee (fails in this run but passes in the 'main' branch)

@sylvestre
Copy link
Contributor

it doesn't hurt to merge it but not sure it simplifies much

@sylvestre sylvestre merged commit 9c01ef5 into uutils:main Jul 29, 2025
78 checks passed
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.

2 participants