gh-99547: Add isjunction methods for checking if a path is a junction

[csm10495]

Implementation of gh-99547.

This is my first try at getting something into cpython.. so please go easy on me. Thanks!

[bedevere-bot]

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

[ghost]

All commit authors signed the Contributor License Agreement.

[zooba]

Code looks good, but we'll also need docs updated (at least the os.path docs and What's New for 3.12)

[csm10495]

@zooba is there a wiki or something on which files generate the docs? I had thought maybe it was from the code. Happy to update docs :)

[csm10495]

Whelp.. doing this on a phone is tough.. reopened

[barneygale]

shutil.py also checks st_reparse_tag for IO_REPARSE_TAG_MOUNT_POINT. Could it be updated to use the new function, perhaps?

[eryksun]

shutil.py also checks st_reparse_tag for IO_REPARSE_TAG_MOUNT_POINT. Could it be updated to use the new function, perhaps?

We could eliminate _rmtree_isdir(). For example:

        try:
            is_dir = (entry.is_dir(follow_symlinks=False) and
                      not entry.is_junction())
        except OSError:
            is_dir = False

This is relatively cheap. On Windows, both tests use the cached stat info from the directory entry. On POSIX, the is_junction() test is always false.

To replace _rmtree_islink(), we could check isjunction() inline. For example:

            if os.path.islink(path) or os.path.isjunction(path):
                # symlinks to directories are forbidden, see bug #1669
                raise OSError("Cannot call rmtree on a symbolic link or junction")

However, this calls lstat() twice, so it might be better to keep _rmtree_islink().

If islink() had a strict option that defaulted to true, then we could call os.path.islink(path, strict=False) to broaden the scope to include junctions, or to include all name-surrogate reparse points. The check for a name-surrogate reparse point is st_reparse_tag & 0x2000_0000.

[csm10495]

I'm not a big fan of needing to call stat again if we don't need to.

Part of me thinks that stat objects should have methods similar to pathlib.Path's is_ methods for checking things based on a cached stat. We can kind of cheat to get there with a DirEntry but idk.

I mean I know we have some stat IS_ functions but I guess I like methods on objects better than loose functions.

Regardless: do we feel as though these optimizations should be part of the current issue or should maybe be a post merge task?

I kind of feel as though we're stepping a tiny bit out of the scope of the current task.

[barneygale]

I don't consider the shutil.py thing blocking :)

[eryksun]

Regardless: do we feel as though these optimizations should be part of the current issue or should maybe be a post merge task?

Replacing _rmtree_isdir() would only use the cached stat value of a DirEntry. I think it's worthwhile for this pull request.

I know we have some stat IS_ functions

Junctions don't have a POSIX file type other than as a directory, S_IFDIR. If we add a test function for junctions to the stat module, it can't follow the S_IS* pattern that checks the POSIX file type in st_mode. It still might be worthwhile to implement this.

[csm10495]

It's just continually adding things on to the same PR is kind of discouraging for me as a new contributor.

I'd greatly prefer having all asks at once (so they can be attacked at once) as opposed to small ones continually given over and over.

[eryksun]

It's just continually adding things on to the same PR is kind of discouraging for me as a new contributor.

Okay. I'm sorry if you feel discouraged. This pull request is outstanding for a new contributor.

[eryksun]

@zooba, since your last review there was a minor change to the documentation, and a test was fixed. If that's all okay with you, then this is ready to be merged as soon as Charles says so.

[csm10495]

Ok i removed _rmtree_is_dir() and replaced its usage with is_dir/is_junction checks. Unit tests pass. If its ok, i'm good with a merge at this point.

[csm10495]

I wonder if one day is_dir should be updated to return False on OSError internally.. regardless I've updated the implementation to handle that case here. I'll leave that to a separate thread.

Updated. If it all passes, i'm still good for merge here.

[eryksun]

I wonder if one day is_dir should be updated to return False on OSError internally..

The rationale is in PEP 471, notes on exception handling.

[zooba]

Thanks! Great job, and congratulations on your first contribution!