libgit2 / pygit2

Python bindings for libgit2
https://www.pygit2.org/
Other
1.59k stars 383 forks source link

Pass blame flags to blame options object. #1083

Closed boehmseb closed 3 years ago

boehmseb commented 3 years ago

The GIT_BLAME_* flags passed to Repository.blame() were never assigned to the git_blame_options struct meaning that they never had any effect on the blame results. This PR fixes this issue by forwarding these flags to the options object.

I can confirm that the GIT_BLAME_IGNORE_WHITESPACE flag is handled properly when I test this fix locally. But if you want me to add some tests please let me know. Since I am new to this project I'm not entirely sure how you handle test inputs that are git repositories and would require some help.

Resolves #925

jdavid commented 3 years ago

Yes please, add a test.

We already have a number of repositories for testing purposes, https://github.com/libgit2/pygit2/tree/master/test/data Maybe one of them works for your test.

They're exposed as fixtures (see test/conftest.py) so it's very easy to use them, e.g. from test/test_blame.py:

def test_blame_index(testrepo):
    blame = testrepo.blame(PATH)

    assert len(blame) == 3
    [...]
boehmseb commented 3 years ago

It does not seem as if one of the existing repositories works for that test. I need a repository that lets me observe the effect of any of the git blame flags. The simplest should be either IGNORE_WHITESPACE or USE_MAILMAP. Should I add a new test repository for this or can I add a commit to an existing one? And how would I do that: is it as simple as tar-ing a git repository and adding a fixture in the former case?

jdavid commented 3 years ago

Better add a new commit, to data/testrepo.tar if possible. For example:

$ cd test/data
$ tar xf testrepo.tar
$ cd testrepo
# Do your changes
$ git add [...]                                      # Don't add bye.txt
$ git commit -m "[...]"
$ cd ..
$ tar cf testrepo.tar testrepo
# Verify the tests pass including the new one
$ git add test/data/testrepo.tar
$ rm test/data/testrepo -r
[...]

If you add a commit to an existing repo then you don't need a new fixture.

boehmseb commented 3 years ago

There are quite some tests failing if I add a commit to that repo simply because the HEAD id changes. Should I just fix those tests or do you suggest a different approach then?

jdavid commented 3 years ago

Oh. Choose the approach that is easier for you.

boehmseb commented 3 years ago

The alternative would be adding another repository. I'm fine with both.

I have another problem however: I'm not sure how to access the GIT_BLAME_* constants in the test. When I use pygit in my project, those constants are defined in the pygit2 module, but that does not seem to work here. I assume those constants are somehow generated from the C sources, but I have no idea how that works. I figured that one out.

boehmseb commented 3 years ago

I decided to added a new repository (essentially a copy of testrepo with an additional whitespace-only commit) since that was way easier than modifying all the other tests to work with a modified testrepo. The test I added passes the IGNORE_WHITESPACE flag to blame, so the blame output should ignore my additional commit and therefore, be identical to the blame for the testrepo.