GeezerVison

Most changes to Python require a NEWS entry.

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

All commit authors signed the Contributor License Agreement.

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

@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 :)

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

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

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.

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.

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

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.

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.

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.

@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.

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.

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.

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.

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