fix: fix incoherent beginning whitespace#1933

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

Byron merged 3 commits intogitpython-developers:mainfromcardoeng:main

Jun 18, 2024

Merged

fix: fix incoherent beginning whitespace#1933

Byron merged 3 commits intogitpython-developers:mainfromcardoeng:main

Jun 18, 2024

Conversation

Copy link

Contributor

cardoeng commentedJun 18, 2024

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 spacesometimes when doing a diff (depending on whether the space is ina_path orb_path).

Python 3.12.3 [...] on linuxType "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 astrip being done to the path (see the only line changed indiff.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 thestrip (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'

cardoeng added3 commits

June 17, 2024 09:28

fix: fix beginning whitespace error

d35998f

test: add test for diff with beginning whitespace

9910a88

style: ruff

97fad9c

Byron approved these changes

Jun 18, 2024

View reviewed changes

Copy link

Member

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 thestrip("\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 nostrip() call?

Byron added the waiting for feedback label

Jun 18, 2024

Copy link

ContributorAuthor

cardoeng commentedJun 18, 2024

I tried removing thestrip() 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 ofa_path andb_path in certain cases if we remove completely thestrip(). (I am not fully familiar with the internal working of GitPython, so I cannot really give more details as to why...)