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: fix incoherent beginning whitespace #1933

Merged
merged 3 commits into from
Jun 18, 2024

Conversation

cardoeng
Copy link
Contributor

Some repositories contain files / directories beginning by a space in their history (example : react-component/field-form@0e81dc0).

The current version of GitPython returns this space sometimes when doing a diff (depending on whether the space is in a_path or b_path).

Python 3.12.3 [...] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import git
>>> git.Repo.clone_from("https://github.com/react-component/field-form", "field-form") # example repository
<git.repo.base.Repo '/tmp/field-form/.git'>
>>> r = git.Repo("field-form")
>>> c = r.commit("0e81dc0d69d198d644b44eb4f84d875777c03581") # commit where there is a space
>>> d1 = c.diff(c.parents[0])[0] # first diff
>>> d1.a_path
'.github/workflows/main.yml'
>>> d1.b_path
' .github/workflows/main.yml' # note the space in the beginning
>>> d2 = c.parents[0].diff(c)[0] # same diff but inverted commits
>>> d2.a_path
'.github/workflows/main.yml' # there is no space
>>> d2.b_path
'.github/workflows/main.yml'

This is due to a strip being done to the path (see the only line changed in diff.py). This PR make a simple change by not stripping the spaces, but only new lines (we can also add other characters if needed) so the behavior stays coherent and the white space is given when doing a diff. An alternative would be to remove the strip (some tests started failing when doing so...).

>>> r = git.Repo("field-form")
>>> c = r.commit("0e81dc0d69d198d644b44eb4f84d875777c03581")
>>> d1 = c.diff(c.parents[0])[0] 
>>> d1.a_path
'.github/workflows/main.yml'
>>> d1.b_path
' .github/workflows/main.yml' # note the space
>>> d2 = c.parents[0].diff(c)[0]
>>> d2.a_path
' .github/workflows/main.yml' # the space is there
>>> d2.b_path
'.github/workflows/main.yml'

Copy link
Member

@Byron Byron 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 for this fix, that's better!

I wonder if the strip("\n") has to happen at all, it's unusual to have newlines in paths anyway, and if they are there, they should not be removed.

In other words, what happens if there is no strip() call?

@cardoeng
Copy link
Contributor Author

I tried removing the strip() call, and it seems two tests related to copied or renamed files during diff started failing :

FAILED test/test_diff.py::TestDiff::test_diff_with_copied_file - AssertionError: 'test2.txt\n' != 'test2.txt'
FAILED test/test_diff.py::TestDiff::test_diff_with_rename - AssertionError: 'that\n' != 'that'

A \n seems to be added at the end of a_path and b_path in certain cases if we remove completely the strip(). (I am not fully familiar with the internal working of GitPython, so I cannot really give more details as to why...)

@Byron
Copy link
Member

Byron commented Jun 18, 2024

Thanks for trying it out, and it seems there is nothing left to do here but to hit merge.

Thanks again for contributing :)!

@Byron Byron merged commit 4c21e51 into gitpython-developers:main Jun 18, 2024
26 checks passed
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.

2 participants