Skip to content
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

fix: bugs in detecting empty git diffs and end of patch meta block #599

Merged
merged 1 commit into from
Nov 8, 2024

Conversation

j-lanson
Copy link
Collaborator

@j-lanson j-lanson commented Nov 8, 2024

Resolves a few bugs in the plugins/git diff parser that I encountered while trying to run plugins/entropy on numpy@no version.

  1. Our format specifier for get_commits() caused GPG key info to be printed and parsed, but locally I was getting an error in the GPG processing C code in git. Other team member got a different error, with git seemingly trying to do validation against GPG signatures in the git history when GPG was not set up on his machine.

  2. Some diffs that just delete a file do not have the 1-2 lines prefixed with +++ or --- between the patch metadata and patch chunks. We were naively using opt(line) to capture these, which would eat up the start of another diff in some cases. Now we check for those lines explicitly.

  3. Some other commits (such as those just used to force the CI to re-run) have zero output in our customized git log invocation used for parsing diffs. Without any changes, we have no way to detect these commits, and our # parsed diffs != # num parsed commits. My solution was to add ~~~ to this argument "--pretty=tformat:, causing each diff to have a header of ~~~ printed whether or not there is anything else to print for that diff. This way, we can at least count empty diffs. Now we successfully parse all commit diffs for a large repo like numpy which has 28,000+ commits

@j-lanson j-lanson added the type: bug Something isn't working label Nov 8, 2024
@j-lanson j-lanson added this to the 3.8.0 milestone Nov 8, 2024
@j-lanson j-lanson self-assigned this Nov 8, 2024
Copy link
Collaborator

@alilleybrinker alilleybrinker left a comment

Choose a reason for hiding this comment

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

LGTM!

@alilleybrinker alilleybrinker merged commit 2fab9d3 into main Nov 8, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants