Skip to content

Split lines by new line characters #441

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

Merged
merged 2 commits into from
May 24, 2016

Conversation

jonathanchu
Copy link
Contributor

What's this PR do?

Fixes a bug where there could be special characters in the commit message, causing the line splits to be improperly detected when parsing the blame metadata.

This commit opts to split lines by the new line character instead of letting splitlines() do this. This helps catch the issue when there are special characters in the line, particular the commit summary section.

How should this be manually tested?

git/test/test_repo.py has been modified to catch this edge case. Please do run all tests or this one test in particular.

Opt to split lines by the new line character instead of letting
`splitlines()` do this. This helps catch the issue when there are
special characters in the line, particular the commit summary section.

if line.strip() == '' or line.strip() == b'':
# Skip over empty lines
continue
Copy link
Contributor

Choose a reason for hiding this comment

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

Ohh, this actually has nothing to do with the ^M chars in the fixture. It's because of this:

>>> s = 'foo\nbar\n'
>>> s.splitlines()
['foo', 'bar']
>>> s.split('\n')
['foo', 'bar', '']
               ^^

@nvie nvie merged commit 903826a into gitpython-developers:master May 24, 2016
@nvie
Copy link
Contributor

nvie commented May 24, 2016

Thanks, this is now pulled!

@Byron Byron added this to the v2.0.3 - Bugfixes milestone May 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants