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

Multiple fixes for bugs around --show-email and --since=<date> #32

Merged
merged 8 commits into from Jun 10, 2020
Merged

Multiple fixes for bugs around --show-email and --since=<date> #32

merged 8 commits into from Jun 10, 2020

Conversation

ghost
Copy link

@ghost ghost commented May 11, 2020

Hello git-fame maintainers,

I have a PR for you that resolves several bugs we ran into while using this tool. Please see the commit messages for descriptions of the changes.

Known bug resolved: #31
New bug resolved: Crash to stack trace when using a selection range (e.g. --since=) and --show-email
New bug resolved: Committers that ever used more than one name (as judged by string comparison) with the same email would only have the stats earned by one of those names, causing data loss.

Cory Carson (Salesforce) added 2 commits May 11, 2020 12:14
Fix for data loss when users use multiple names (as judged by string comparison) with the same email. Otherwise, the last entry iterated "wins" and all other entries get lost.
Exclude git blame entries that exist outside of the requested range (e.g. with --since=<date>). Otherwise, the user with the nearest commit to the boundary earns the LOC count.

Incidentally, this also fixes a crash to stack trace when you request a date-filtered git-fame with --show-email; the auth_stats attributed the boundary lines to a (wrong) author, and that author has no commit messages in the specified range. This can be forced to trigger by requesting a git-fame since the future (e.g. --since=2024) because then one lucky owner gets attributed all of the stat; since all authors have no commits, so there's no emails to perform a lookup against whomever that lucky owner was.
@coveralls
Copy link

coveralls commented May 11, 2020

Coverage Status

Coverage decreased (-1.1%) to 94.985% when pulling 274df71 on ccarson-publicsalesforce:master into fd9c79c on casperdcl:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.2%) to 95.468% when pulling c5fcc5f on ccarson-publicsalesforce:master into eb15381 on casperdcl:master.

@ghost
Copy link
Author

ghost commented May 11, 2020

^ travis-ci build/test succeeds. The failure is from the Deploy steps, which I have no control over.

@casperdcl
Copy link
Owner

Hello git-fame maintainers,

Hah love this. If you feel the need to refer to an unfunded OS project's sole maintainer in the plural, fine by me :D

PRs always even more welcome than sponsorship.

gitfame/_gitfame.py Outdated Show resolved Hide resolved
Copy link
Owner

@casperdcl casperdcl left a comment

Choose a reason for hiding this comment

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

one minor comment; rest looks good

@codecov-commenter
Copy link

codecov-commenter commented May 22, 2020

Codecov Report

Merging #32 into master will decrease coverage by 0.49%.
The diff coverage is 77.77%.

@@            Coverage Diff             @@
##           master      #32      +/-   ##
==========================================
- Coverage   94.78%   94.29%   -0.50%     
==========================================
  Files           4        4              
  Lines         326      333       +7     
  Branches       69       69              
==========================================
+ Hits          309      314       +5     
- Misses         11       13       +2     
  Partials        6        6              

Copy link
Owner

@casperdcl casperdcl left a comment

Choose a reason for hiding this comment

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

Thanks a lot, @ccarson-publicsalesforce - just updated to simplify the regex and keep flake8 happy - would be great if you could check if it works for you!

@casperdcl casperdcl mentioned this pull request Jun 10, 2020
@casperdcl casperdcl merged commit 86418b9 into casperdcl:master Jun 10, 2020
casperdcl added a commit that referenced this pull request Mar 9, 2023
- fixes #85
- fixes regression from #32
@casperdcl casperdcl mentioned this pull request Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--since has somewhat confusing behaviour
3 participants