[webkit-reviews] review denied: [Bug 220781] [Flatpak SDK] Flatpak 1.10 environment variable issues : [Attachment 418118] Patch

[bugzilla-daemon at webkit.org]

Adrian Perez <aperez at igalia.com> has denied Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 220781: [Flatpak SDK] Flatpak 1.10 environment variable issues
https://bugs.webkit.org/show_bug.cgi?id=220781

Attachment 418118: Patch

https://bugs.webkit.org/attachment.cgi?id=418118&action=review




--- Comment #3 from Adrian Perez <aperez at igalia.com> ---
Comment on attachment 418118
  --> https://bugs.webkit.org/attachment.cgi?id=418118
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=418118&action=review

> Tools/flatpak/flatpakutils.py:854
> +	       if envvar.startswith("LC"):

It would be safer to list the local-related LC_* variable names only:

  if envvar in ("LC_CTYPE=", "LC_NUMERIC", "LC_TIME", "LC_COLLATE",
		"LC_MONETARY", "LC_MESSAGES", "LC_PAPER", "LC_NAME",
		"LC_ADDRESS", "LC_TELEPHONE", "LC_MEASUREMENT",
		"LC_IDENTIFICATION", "LC_ALL"):

If the above feels like too much of a mouthful (though I think it's fine),
at least I would make this check “.startswith("LC_")” — to make sure some
other variable like “LCFOO” is not accidentally picked by the loop.

Also: This check does not match the “LANG” variable, which can also be
used to setup locale attributes. Is that on purpose, or maybe missing a
check here?