Always use GIF89a for comments

[raygard]

Fix bug that allows GIFs with long comments to be written as GIF87a.

Fixes #6291 .

Changes proposed in this pull request:

• Change version feature test in _get_global_header() to use GIF89a for long comments.
• Add test in test_comment_over_255() to check this.

[radarhere]

Suggested change

	extensionKey == "comment" and len(info[extensionKey]) == 0
	extensionKey == "comment" and info[extensionKey]

This seems simpler to me?

[raygard]

@radarhere, thank you for reviewing my PR.

The test needs to determine that info[comment] is an empty string to keep version as 87a. Your suggestion info[extensionKey] does the opposite. I was attempting to extrapolate from not (1 <= len(info[extensionKey]) <= 255) (which originated with PR #1896 but was superseded by PR #3479) to retain the length test. I think it probably could be info[extensionKey] == "" or even not info[extensionKey]. The 255 was there to begin with, I think, to fit with the maximum length of the single data block that was allowed before PR #3479.

[radarhere]

You're right - I meant not info[extensionKey]. That was just a mistake on my part.

[radarhere]

This forces GIF89a for all comments, not just "long comments". So wouldn't it be more straightforward to modify test_comment, rather than test_comment_over_255?

[raygard]

@radarhere,

This forces GIF89a for all comments, not just "long comments". So wouldn't it be more straightforward to modify test_comment, rather than test_comment_over_255?

Pillow already uses GIF89a for shorter comments, so adding assert reread.info["version"] == b"GIF89a" in test_comment would not show that the PR actually fixed anything (but it couldn't hurt to add that in test_comment too). You have more experience than I with automated unit tests, but I recall you telling me that one reason for these tests is to prevent regressions, so (as I understand) a test should fail with the old code and pass with the new code. I tried to do that with this and also with test_read_multiple_comments and test_write_comment in my other recent PRs. (OTOH test_write_no_comment tests that an empty comment="" arg to .save() will leave no comment at all, and that will work before and after my PRs; I just thought it would be a good thing to have a test for it.)

[radarhere]

Ok, thanks. That's perfectly fair.

[radarhere]

I've created https://github.com/raygard/Pillow/pull/1 with a suggestion from me, to add the check to test_comment() as well, and a modified copy of your suggestion from #6291 (comment) to simplify the version check.