Skip to content

Conversation

@ethomson
Copy link
Member

@ethomson ethomson commented Jul 2, 2015

Presently, cl_getenv wraps getenv on posixy systems and does some UTF16-UTF8 magic on Windows. The return code must be freed on Windows and must not be on Unix boxes. Ugh.

p_getenv just wraps getenv on all systems, which means it's ANSI code page on Windows. (This hasn't really bitten us, yet, as we don't read potentially Unicode values in the library, only in the tests.) But still, ugh again.

This introduces git__getenv which is a UTF-8 aware getenv everywhere. Plus some helpery clar stuff because we can.

@ethomson
Copy link
Member Author

ethomson commented Jul 2, 2015

Bleh. cl_getenv is now too simplistic as it coalesces not found and empty strings.

Introduce `git__getenv` which is a UTF-8 aware `getenv` everywhere.
Make `cl_getenv` use this to keep consistent memory handling around
return values (free everywhere, as opposed to only some platforms).
@ethomson
Copy link
Member Author

ethomson commented Jul 2, 2015

Okay, this is ready for review. This only affects tests so I'm happy to punt on it until 0.23 lands.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of these NULLs are redundant. Static data is initialized to zero. It's not a big deal, but it's a part of the diff that doesn't do anything.

@carlosmn
Copy link
Member

carlosmn commented Jul 3, 2015

Yeah, let's hold off on merging it until after the release.

carlosmn added a commit that referenced this pull request Jul 6, 2015
git__getenv: utf-8 aware env reader
@carlosmn carlosmn merged commit ea445e0 into libgit2:master Jul 6, 2015
max630 added a commit to max630/libgit2 that referenced this pull request Aug 28, 2016
This reverts commit ea445e0, reversing
changes made to 3c83111.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants