git_patch_from_diff may return a NULL patch without an error

[jorio]

Diff.__getitem__ used to assume (incorrectly) that when git_patch_from_diff returns 0 (success), it also creates a valid git_patch.

However, per the docs for git_patch_from_diff, "For an unchanged file (...) no git_patch will be created, the output will be set to NULL".

In practice, git_patch_from_diff doesn't always return a NULL patch if the file is unchanged. However, I found a scenario involving CRLF filters that consistently triggers this edge case (success code with NULL patch).

This PR makes Diff.__getitem__ return None in this case. This prevents passing NULL to wrap_patch, which would cause undefined behavior in release builds (because this assertion would be bypassed).

[jorio]

Ah, the new mypy checks failed (they weren't set up to run automatically in my fork before I submitted the PR) because Diff.__getitem__ may now return None, and many unit tests just assume that they can use diff[number] to get a valid patch.

Some thoughts:

• Should Diff.__getitem__ raise an exception if libgit2 declines to create the patch? This way, we wouldn't have to return None, but iterating on a Diff may be interrupted halfway through by an exception.
• Or, should we put the burden on the user to check that every single patch gotten via Diff[] or Diff.__iter__ is not None?

[jdavid]

• Should Diff.getitem raise an exception if libgit2 declines to create the patch? This way, we wouldn't have to return None, but iterating on a Diff may be interrupted halfway through by an exception.
• Or, should we put the burden on the user to check that every single patch gotten via Diff[] or Diff.iter is not None?

The second one is better in my opinion, the way it's already in this PR.
The first one would require the user to handle it as well, but with a try/except, which is worse in my opinion.

[jorio]

Hi @jdavid! My latest changes are ready for review. I think pygit2 would benefit from eliminating this undefined behavior (until then, the test scenario can cause a segfault in my git client ;))

PS: I'm not sure why the "Wheels for linux-arm" check failed to start...

[jdavid]

Thanks @jorio and sorry for the delay.

I have a question, there has been a string of mypy related changes, what do you think about it?

For the record, in this PR mypy complained because of the union-attr rule, and to make mypy happy the test code is now more complicated that it would have been otherwise.

[jorio]

Thank you @jdavid for the merge! Sorry it took me a while to get back to you about mypy. Here's my opinion:

In this case, I think the warnings about SomeType | None unions are a bit pedantic. They force us to add boilerplate for rare failure cases where we'd raise an exception anyway.

If we have a unit test that should handle the failure case but doesn't, I'd prefer to just let the test fail, so that we're forced to review the failure case.

In an ideal world, code coverage should give us the peace of mind that the test suite covers the failure cases - for example in this case, where Diff.__iter__ returns None. (However, this specific case originates on the C side, and I'm not sure we have coverage for the C code...)

[jdavid]

For example I would write (in the tests):

files = [patch.delta.new_file.path for patch in diff_safeiter(diff)]

As:

files = [patch.delta.new_file.path for patch in diff if patch is not None]

To me the fact that now patch may be None is not necessarily an error, but a possibility that the user should handle, and the way to handle it is testing whether it's None or not.