Fix color lookup after reusing a palette entry

[sergioperezcheco]

Palette entries and their color lookup diverge after reuse

A full palette can reuse an index that is absent from the image histogram. ImagePalette.getcolor() updates the bytes at that index but leaves the replaced color in colors. A subsequent request for that color returns the reused index, so public drawing operations silently use the wrong color.

This reproduces on current main through putpixel():

from PIL import Image

im = Image.new("P", (3, 1))
im.putpalette([channel for i in range(256) for channel in (i, i, i)])
im.putpixel((0, 0), (255, 0, 0))
im.putpixel((1, 0), (255, 255, 255))
print([im.convert("RGB").getpixel((x, 0)) for x in range(3)])
# Before: [(255, 0, 0), (255, 0, 0), (0, 0, 0)]
# After:  [(255, 0, 0), (255, 255, 255), (0, 0, 0)]

Keep the lookup consistent with the palette bytes

Invalidate the color lookup only when replacing an existing palette entry, then register the new color after updating the bytes. Rebuilding also preserves duplicate colors at their remaining indices; deleting the old color alone would lose that information. Appending a new entry retains the existing incremental lookup path.

The regressions cover RGB/RGBA palettes, replacement of unique and duplicate colors, repeated requests for the newly allocated color, and a public putpixel() reproduction with a PNG save/reopen check. The existing test_new_color expected a cache entry for black after its unused slot had been replaced; it now checks that the stale entry is absent while retaining its rendered-pixel assertion. A release note describes the behavior change.

Validation

Built the checkout and its native extensions locally with Python 3.11.15 on macOS arm64. All five new regression cases fail against the unchanged implementation.

After the fix:

• python3 selftest.py: 59 passed.
• python3 -m pytest Tests/test_imagepalette.py Tests/test_imageops.py Tests/test_imagedraw.py Tests/test_image_putpalette.py Tests/test_image.py Tests/test_file_png.py Tests/test_file_gif.py -q: 725 passed, 2 skipped, 1 xfailed. The skips are missing IPython and WebP support; the xfail is the existing test_line_h_s1_w2.
• Ruff 0.16.6, Black 26.5.1, mypy on the three changed Python files, Bandit on ImagePalette.py, Sphinx-lint on the release notes, and git diff --check pass. Black ran with --fast because the project's target includes Python 3.15 while the local interpreter is 3.11; mypy and pytest ran separately.

The full test suite and full pre-commit suite were not run. Prepared with AI assistance.

[radarhere]

I've created sergioperezcheco#1 with a suggestion.

[sergioperezcheco]

Thanks for the suggestion. I've applied the release-note wording change in 913bb26.

I tested the suggested getcolor implementation against the existing reused-index cases. The non-duplicate cases and pixel round-trip pass, but both duplicate cases fail: when entries 1 and 2 share a color and entry 1 is reused, deleting the cached mapping also loses the still-valid entry 2. Requesting the old color then reuses entry 1 again rather than returning 2.

An incremental update could search the remaining palette entries for a replacement mapping when deleting the cached index. The current cache rebuild handles that case directly, which is why I'd prefer to keep the duplicate-color tests. I haven't merged the suggested branch; happy to discuss an incremental variant that preserves this behavior.

[radarhere]

both duplicate cases fail: when entries 1 and 2 share a color and entry 1 is reused, deleting the cached mapping also loses the still-valid entry 2. Requesting the old color then reuses
entry 1 again rather than returning 2.

For my benefit, and anyone else reading this, let me go through the 'RGB' 'duplicate' test case step by step.

Step 1. In both versions, the palette starts off as (0, 0, 0), (1, 1, 1), (1, 1, 1), (3, 3, 3)
Step 2. User asks for (255, 0, 0). The palette becomes (0, 0, 0), (255, 0, 0), (1, 1, 1), (3, 3, 3). Here is where things diverge. In your version, colors remembers both (255, 0, 0) and (1, 1, 1). In mine, colors forgets about (1, 1, 1). It was a duplicate, and my code updates one and forgets about the other.
Step 3. User asks for (1, 1, 1). In your version, the palette is unchanged, and so (1, 1, 1) is index 2. In my version, (255, 0, 0) is recognised as an unused colour. The user didn't draw it on the image, they only asked for the colour. So index 1 is updated, and the palette reverts back to (0, 0, 0), (1, 1, 1), (1, 1, 1), (3, 3, 3). This means that the color index returned is 1. This isn't the number your test code expects, but it isn't necessarily wrong.

An incremental update could search the remaining palette entries for a replacement mapping when deleting the cached index. The current cache rebuild handles that case directly, which is why I'd prefer to keep the duplicate-color tests. I haven't merged the suggested branch; happy to discuss an incremental variant that preserves this behavior.

So I understand that my suggestion doesn't do what you're saying. I don't have a firm stance here, but I'm not sure I understand why it should?

After Step 2, (1, 1, 1) is unused at index 2. Aside from the fact that remapping all of the indexes to remove it would be overkill, I wouldn't mind if it was removed from the palette entirely at that point. The palette is connected to the image, and I don't think the palette has any external meaning. We have no way of knowing that the user will ask for (1, 1, 1) afterwards, and even if they do, like in your test... so what? The user wanted red in Step 2, but they never used it, and now Step 3 wants something else, so why does it matter if red is removed? In the big picture, the user is requesting arbitrary RGB colours be allocated into an image with a potentially full palette. There is no plan that can guarantee that the palette indexes they are requesting stay the same when we're pushing up against the 256 colour limit. Sure, there happens to be a duplicate entry in your scenario that means that the first index stays the same, but that's a lucky happenstance. It's not something that the user should be counting on.

In short - maybe you value indexes staying the same, or maybe you value not changing palette entries, but neither are guaranteed behaviour no matter what we decide here.

[sergioperezcheco]

You are right that allocating red without drawing it does not establish a rendering failure. I strengthened the duplicate-color cases in f9c20e6: they now draw red into the reused entry before requesting the remaining gray color, then check both rendered pixels. With all palette entries now in use, forgetting the surviving gray mapping raises ValueError rather than just selecting another unused index. This tests successful drawing of an already-present color, not stable indices for unused allocations. All 56 checks on this head are successful.