kornelski / pngquant

Lossy PNG compressor — pngquant command based on libimagequant library
https://pngquant.org
Other
5.25k stars 486 forks source link

Usage of batch processing in README.md is misleading #420

Closed michael-o closed 1 month ago

michael-o commented 1 month ago

The README says:

batch conversion of multiple files: pngquant *.png

Let's try:

$ /tmp/pngquant/bin/pngquant --skip-if-larger --verbose --ext .png -f 'inputs/Bilder.toy/*'
inputs/Bilder.toy/*:
  error: cannot open inputs/Bilder.toy/* for reading
There were errors quantizing 1 file out of a total of 1 file.

Does not work. Reason: The README relies on the fact that the tool is run from a shell which supports file globbing, but the tool itself does not use glob(3). Better replace with multiple filenames. As soon as I run this example from a non-shell, e.g., Java, Python, popen it will fail.

kornelski commented 1 month ago

It doesn't work, because you've put '*' inside quotes. In quotes it means exactly this character, nothing else. Without quotes it gets expanded to multiple files.

michael-o commented 1 month ago

It doesn't work, because you've put '*' inside quotes. In quotes it means exactly this character, nothing else. Without quotes it gets expanded to multiple files.

Have you actually understood what I have written? Please reopen. I don't expect you to change the code, but simply fix the documentation.

kornelski commented 1 month ago

The documentation is correct. pngquant *.png as written in the documentation works fine, and pngquant '*.png' doesn't work due to how globbing works. The example you've shown here is the invalid one.

michael-o commented 1 month ago

No, you ate confusing shell wildcard expansion either application level globbing. If disabled it won't work either: https://stackoverflow.com/a/11456496

kornelski commented 1 month ago

If we're talking about unixish systems, then relying on shell expansion is normal. ls '*.png' behaves the same. * is a valid filename character, so calling glob on already-expanded arguments could expand twice. That is a non-standard behavior, and it could have unintended consequences.

Command-spawning APIs that pass arguments literally without going through shell do this intentionally. This allows tools to invoke pngquant with specific file names, and not worry that it will process other files. If pngquant applied glob itself, then callers that expect to pass a literal path would be required to escape glob characters themselves, and that is unusual.

michael-o commented 1 month ago

If we're talking about unixish systems, then relying on shell expansion is normal. ls '*.png' behaves the same. * is a valid filename character, so calling glob on already-expanded arguments could expand twice. That is a non-standard behavior, and it could have unintended consequences.

Command-spawning APIs that pass arguments literally without going through shell do this intentionally. This allows tools to invoke pngquant with specific file names, and not worry that it will process other files. If pngquant applied glob itself, then callers that expect to pass a literal path would be required to escape glob characters themselves, and that is unusual.

Yes, that is correct and that is why the README is confusing. It implies to rely on shell globbing, not application globbing. I would just recommend change the docs to make it clear about globbing or not mention globbing (wildcard expansion) at all.