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

Fix similar 'save image' filename overwriting existing file instead of incrementing #3484

Closed
wants to merge 1 commit into from

Conversation

Jobus0
Copy link

@Jobus0 Jobus0 commented May 15, 2024

Problem

On Windows, saving an image with a filename that already exists with a different case causes it to overwrite the existing image file instead of incrementing the name.

For example, if I first save with a filename_prefix of "abc", and then save again with "Abc", the second image will overwrite the first without warning.

Expected Behavior

The filename should increment despite the difference in case, resulting in "abc_00001_.png" and "Abc_00002_.png", because Windows's file system is case insensitive.

Solution

Add .casefold() to the code that counts the number of files with the same name, making it case insensitive.

comfyanonymous added a commit that referenced this pull request May 16, 2024
@comfyanonymous
Copy link
Owner

58f8388

os.path.normcase seems to be the proper function to use for this case.

@NeedsMoar
Copy link

os.path.normcase

Most of the time it will be since people don't mess with these flags often, but the function itself is technically incorrect for Windows. According to the python docs it just drops case info on Windows, when it really needs to be
NTFS allows setting case sensitivity within the filesystem on a per-volume or per-directory basis. ReFS allows per volume at least, probably per-directory as well.

I leave it off because differentiating files within a directory by case only (at least in English) is borderline psychotic in terms of keeping files organized and just leads to an amplification of errors like this or even weirder issues. What happens when you have ComfyUI_, comfyui_, and then typo ComfYUI_ ? The main reason I can think of for setting it would be on Windows Server or regular windows running an NFS server that's being shared to Linux machines so it can be used for incremental backups or similar more easily, although most of the linux backup utils I've messed with encapsulate everything in .tar / .tar.bz2 by default as a convenience thanks to the horrible network speeds most computers have. Motherboards finally starting to ship with 10GbE ports as a default roughly a decade after it was outdated is a kind of sad state of things. For the cost of the average high end mobo they should really be shipping with 25Gb or 40Gb QSFP with a free transceiver, but I'm getting off-topic.

The actual bug here is in Pillow's Image.save and probably Image.open. In Image.save, this exists:
PIL\Image.py

        created = False
        if open_fp:
            created = not os.path.exists(filename)
            if params.get("append", False):
                # Open also for reading ("+"), because TIFF save_all
                # writer needs to go back and edit the written data.
                fp = builtins.open(filename, "r+b")
            else:
                fp = builtins.open(filename, "w+b")

So it has the inconsistent and braindead behavior of truncating everything but tiff files when it opens them to save. I don't think that; judging by the comment and the flags; that the person who wrote that knew what the w parameter, the + parameter, or the r parameter for mode actually do and when they need to / should be used. Apparently they think that 'r' means "write but don't append", '+' means "read", and 'w' means "write and append". I think this code snippet alone is an indication that everyone involved in writing Pillow should have their computers taken away and donated to some underprivileged kids somewhere along with being forced to pay for C++ or maybe Rust lessons... or if they're younger one of the newer advanced LOGO variants; there's one of them that allows moving the turtle in N-dimensional projected space which is a bit much for most kids at the LOGO age, but the 3D version of it would probably be a ton of entertainment for people who aren't ready to learn object oriented programming.

I didn't dig through the gigantic overkill class to see if there's any kind of filename check elsewhere, although I think if it did they wouldn't be checking again right there. That check is used very weirdly. 'created' isn't set anywhere else.

        try:
            save_handler(self, fp, filename)
        except Exception:
            if open_fp:
                fp.close()
            if created:
                try:
                    os.remove(filename)
                except PermissionError:
                    pass
            raise

So we make sure to remove the file if it fails to save the image, but only if it doesn't exist before an attempt is made to open & truncate for r/w or open for r/w. No exception handling is done for builtins.open which throws an OSError if it fails, so arguably the most important call in this method is the thing they didn't bother checking for success on.

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.

None yet

3 participants