Refactor ConfigEntry and related iterators for safer memory management

[beamerblvd]

ConfigEntry is holding on to git_config_entry pointers for its entire lifespan, but this isn't safe or necessary. When getting a single entry without an iterator, this isn't really a concern. However, when entries are iterated with git_config_next, the previously-allocated iterator entry object is re-used. If ConfigEntry is still holding onto the git_config_entry, its values could change inappropriately and unexpectedly. The solution to this is to eagerly copy the underlying C values into the Python structure and move on.

Additionally, git_config_entry_free should never be used for iterated entries. Freeing the iterator automatically frees any entries it allocated, and manually freeing the entry can cause a crash. git_config_entry_free should be called only for entries retrieved without an iterator.

This pull request, which is a subset of the changes initially proposed in #1473, fixes this memory misuse by refactoring ConfigEntry and its related iterators.

[jdavid]

Thanks. This is the review from Kimi:

1. No tests for the fix. A memory-safety refactor of this size (+201/−93 in config.py) ships with zero test changes. Please add regression tests, e.g.: collect list(config) and assert values remain intact after iteration ends; interleave two iterators and assert earlier entries aren't corrupted; valueless-key get_bool behavior. These are exactly the scenarios from Access violation accessing repo config entries #970.

2. Docstring: multivar "first value" is wrong. The new get_bool/get_int docstrings say "If the config var found is a multivar, only the first value is returned." Verified: git_config_get_entry returns the last value (['first', 'last'] → get_entry gives 'last'), matching git config --get semantics.

3. Docstring: "empty value" listed as truthy is wrong. get_bool/parse_bool say truthy values include "an empty value / NULL". Verified: Config.parse_bool('') is False (empty string is falsy); only a missing/NULL value is truthy. Suggest "a missing value (NULL)".

[beamerblvd]

Thanks. This is the review from Kimi:

1. No tests for the fix. A memory-safety refactor of this size (+201/−93 in config.py) ships with zero test changes. Please add regression tests, e.g.: collect list(config) and assert values remain intact after iteration ends; interleave two iterators and assert earlier entries aren't corrupted; valueless-key get_bool behavior. These are exactly the scenarios from Access violation accessing repo config entries #970.
2. Docstring: multivar "first value" is wrong. The new get_bool/get_int docstrings say "If the config var found is a multivar, only the first value is returned." Verified: git_config_get_entry returns the last value (['first', 'last'] → get_entry gives 'last'), matching git config --get semantics.
3. Docstring: "empty value" listed as truthy is wrong. get_bool/parse_bool say truthy values include "an empty value / NULL". Verified: Config.parse_bool('') is False (empty string is falsy); only a missing/NULL value is truthy. Suggest "a missing value (NULL)".

All great points:

1. I've added tests not only for iterating and confirming values aren't corrupted, but also additional tests for parse_bool and parse_int to test expected behavior for a wider range of values, including expected exceptions.
2. Indeed, this was a misreading of libgit2 code on my part. The docstring has been fixed and the new tests assert this behavior.
3. Indeed, again, a misreading on my part. I've updated the docstring.

[jdavid]

Feedback regarding the latest commit:

GitError now carries two args, changing str(e) everywhere

errors.py now does raise GitError(message, giterr.klass). Since GitError is a plain C-defined exception, str() of a two-arg exception is the tuple repr:

  _pygit2.GitError: ('some message', 7) 

This affects nearly every libgit2 error raised through check_error (all real errors have a klass). Evidence it's already biting: the commit had to patch settings.py:72
from str(git_err) to git_err.args[0]. Test-suite match= assertions still pass (substring search), but:

• Users' tracebacks and logs will show ('message', 7) instead of message.
• Any downstream code doing str(e) == ..., e.args == (msg,), or formatting str(e) breaks.

Suggestion: attach the class as an attribute instead of a second arg —

  if giterr != ffi.NULL and giterr.klass != 0: 
      error = GitError(message)
      error.klass = giterr.klass
      raise error
  raise GitError(message)

(_pygit2 exceptions are heap types; instances accept attributes.) Then config.py checks getattr(e, 'klass', None) == C.GIT_ERROR_CONFIG, settings.py can revert to its
original form, and str(e) is unchanged. If you do keep the two-arg form, it needs a prominent changelog note as a breaking change.

Breaking change in parse_bool/parse_int

The change to ValueError is good, but it would break user code catching GitError. Instead it may raise InvalidError which inherits from both GitError and ValueError.

ConfigEntry.__eq__/__hash__ nits

Two nits: __eq__ should return NotImplemented (not False) for non-ConfigEntry operands so reflected comparison works; and it compares self._raw_name to other.raw_name — private vs public accessor, harmonize.

[beamerblvd]

I have implemented your latest suggestions and fixed the issues you pointed out.