Handle empty environment variable updates #4326
Conversation
Empty strings cause corrupt environment files, since two tabs are written in a row which are of course lexed as a single piece of whitespace. Alter the format so that the @ symbol on its own means an empty string (I'd have used something closer to $\epsilon$, but it'd never be portable...). The single string @ is represented by \@. Signed-off-by: David Allsopp <david.allsopp@metastack.com>
There was a problem hiding this comment.
Change seems fine;
one thing I am still not very comfortable about is that we don't support unsetting variables (this is particularly true for opam env --revert), and we set them to the empty string instead. It has more or less worked until now, and fixing it would need specific variable-unsetting code for all supported shells with all the pitfalls that involves, which is why it's still like that.
The reason I mention it here is that maybe we should provision a syntax for that as well (at first when @rjbou told me about this PR I thought that's what the @ syntax was about)
|
Windows doesn't support empty environment variables, so for the most part I think it's better to (try to) head towards a world where we treat "unset" and "empty" as the same thing. |
|
Has this been cherry-picked to |
|
Nop, not yet |
Handle empty environment variable updates
Handle empty environment variable updates
Handle empty environment variable updates
Handle empty environment variable updates
This is apparently a piece of lost history on my part, but it's resurfaced in #4325. Empty strings cause corrupt environment files, since two tabs are written in a row which are of course lexed as a single piece of whitespace.
For better or worse, my solution in 2016 was to write an empty string as
@. The ambiguity with the string"@"is resolved by writing that as\@(since the format already allows\to escape any character.This can go on 2.0 with one caveat - it would mean that any environment updates which intentionally set a value to be
@will now be empty. That seems unlikely, but it may be reason enough to fix this for 2.1 only.