Skip to content

git_patch_from_diff may return a NULL patch without an error #1412

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jorio
Copy link
Contributor

@jorio jorio commented Aug 19, 2025

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
Copy link
Contributor Author

jorio commented Aug 19, 2025

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
Copy link
Member

jdavid commented Aug 20, 2025

  • 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.

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