libutil: treat an existing empty environment variable as empty, not absent - #16345
Open
awsmadi wants to merge 4 commits into
Open
libutil: treat an existing empty environment variable as empty, not absent#16345awsmadi wants to merge 4 commits into
awsmadi wants to merge 4 commits into
Conversation
awsmadi
marked this pull request as ready for review
August 21, 2026 19:25
Member
|
BTW see https://github.com/NixOS/nix/blob/master/CONTRIBUTING.md forgive my guessing but you might be missing an |
Ericson2314
reviewed
Aug 21, 2026
Comment on lines
25
to
28
| if (resultSize == 0) { | ||
| return std::nullopt; | ||
| return OsString{}; | ||
| } | ||
|
|
Member
There was a problem hiding this comment.
Do we even need this special case, or will value.resize(resultSize); below just do the right thing?
Let's delete the branch if possible, but still keep the comment.
xokdvium
reviewed
Aug 21, 2026
xokdvium
left a comment
xokdvium
left a comment
Contributor
There was a problem hiding this comment.
Can we get a libutil test for this?
…bsent getEnvOs() returned nullopt when the second GetEnvironmentVariableW call stored zero characters. Reaching that call already implies the sizing call succeeded, which implies the variable exists -- so zero there means the variable is present and its value is empty, not that it is missing. Windows distinguishes the two cases and this discarded the distinction. For an existing empty variable the sizing call returns 1, the character count required including the terminator, and the buffered call then returns 0. For an absent variable both calls return 0 and set ERROR_ENVVAR_NOT_FOUND. Measured under Wine 11.0 with a kernel32-only probe: an empty value is representable and survives inheritance. A launcher exporting VAR= produces a process whose environment block carries the literal entry "VAR=" and whose sizing call returns 1 with no error, while an unset variable returns 0 with ERROR_ENVVAR_NOT_FOUND. SetEnvironmentVariableW(name, L"") stores an empty value; only passing NULL removes the entry. So setEnvOs(k, "") followed by getEnvOs(k) now round-trips, and an inherited empty variable is no longer reported as missing. The alternative fix is to test GetLastError() == ERROR_ENVVAR_NOT_FOUND after the second call rather than inferring existence from the first. That is the documented disambiguator and it is immune to a concurrent deleter, but it needs SetLastError(0) beforehand, because a zero return from an existing empty variable is a success and leaves any earlier error code in place. Inferring from the sizing call needs no error handling and is sound for every case this process creates. Assisted-by: Claude Code (claude-opus-5)
awsmadi
force-pushed
the
pr/getenvos-empty-not-absent
branch
from
August 24, 2026 15:28
5ea9704 to
12640d4
Compare
Per review: resize(resultSize) already returns the empty value, so the branch was redundant. Comment kept, as asked. nix-util-tests under Wine unchanged at 782 passed / 8 skipped / 0 failed. Assisted-by: Claude Code (claude-opus-5)
Per review. emptyValueIsPresent fails on Windows without the change in this PR. 807 tests pass natively, and the suite compiles for mingw. Assisted-by: Claude Code (claude-opus-5)
On Windows it reports success as -1, the inverse of the POSIX convention the header describes. Keeping the tests off that means they pass either way. Assisted-by: Claude Code (claude-opus-5)
Member
|
Please squash the history, as I have mentioned in the other PRs. Also, I suspect we will have a problem where the environment variable tests keep on conflicting with one another. That may call for combining these PRs into one. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
getEnvOsreturnednulloptwhen the secondGetEnvironmentVariableWstored zerocharacters. But reaching that call means the sizing call already succeeded, which
means the variable exists, so zero there means the value is empty rather than
missing.
Windows distinguishes the two and this discarded the distinction. Per the documented
contract, an existing empty variable returns 1 from the sizing call (the count
including the terminator) and 0 from the buffered call. An absent one returns 0 from
both and sets
ERROR_ENVVAR_NOT_FOUND.Unix already behaves correctly:
getEnvon an empty variable yields an engagedoptional holding
"". So the same calling code took different branches on the twoplatforms, and one consequence is that an empty-string guard in
FileTransferSettingsis unreachable on Windows.Following review, the branch is gone entirely rather than corrected.
value.resize(resultSize)on a zero count gives the empty string, and returning itgives an engaged optional, which is what was wanted. Thanks @Ericson2314.
Two tests in
libutil-tests, per @xokdvium.getEnvOs.emptyValueIsPresentfails onWindows without this change. They pass on both platforms, since Unix has always had
the behaviour being asserted.