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

feat(blame): Support custom rev_opts for blame #1485

Merged
merged 1 commit into from Aug 31, 2022

Conversation

thehale
Copy link
Contributor

@thehale thehale commented Aug 30, 2022

The git blame CLI offers a repeated -C option that can be used to detect
lines that move within/between files. While a slower operation, it yields more
accurate authorship reports.
https://git-scm.com/docs/git-blame#Documentation/git-blame.txt--Cltnumgt

While GitPython does enable passing custom kwargs to the command line git
invocation, the fact that kwargs is a dictionary (i.e. no duplicate keys) means
that there was no way to request the -C option in git blame more than once.

This commit adds an optional rev_opts parameter to the blame method which
accepts a list of strings to propagate to the CLI invocation of git blame. By
using a List[str] for rev_opts, users of GitPython can pass now the -C
option multiple times to get more detailed authorship reports from git blame.

@thehale
Copy link
Contributor Author

thehale commented Aug 30, 2022

At this point, I would appreciate a second pair of eyes to help diagnose why the test suite keeps failing on Python 3.7.

The one test that's currently failing was passing in my second commit (c0225b3), and my latest commit only re-wrote a small portion of the new test I added as part of the original PR.

Is this a flaky test that just needs a re-run? or is there something deeper that I'm somehow polluting in my latest revision?

@Byron
Copy link
Member

Byron commented Aug 31, 2022

Thanks for your contribution, it's appreciated.

I retried the CI a couple of times but it appears to consistently fail. Now trying main again to see if this is a new general issue, as I also wouldn't think this test should break after this change.

@Byron
Copy link
Member

Byron commented Aug 31, 2022

And the results are in: main is still working, which means something here has broken this one one test, consistently across python versions: test/test_base.py::TestBase::test_with_rw_remote_and_rw_repo.

As it still makes no sense to me, could you revert all your changes so this branch is the same content as main to see if this fixes it? I'd expect not, but I don't know what else to try. It appears an ls-remote call has a file-not-found error, and generally it can't connect to the server running at localhost, which probably means it crashed or shut down.

The `git blame` CLI offers a repeated `-C` option that can be used to detect
lines that move within/between files. While a slower operation, it yields more
accurate authorship reports.
https://git-scm.com/docs/git-blame#Documentation/git-blame.txt--Cltnumgt

While GitPython does enable passing custom kwargs to the command line `git`
invocation, the fact that kwargs is a dictionary (i.e. no duplicate keys) means
that there was no way to request the `-C` option in `git blame` more than once.

This commit adds an optional `rev_opts` parameter to the `blame` method which
accepts a list of strings to propagate to the CLI invocation of `git blame`. By
using a `List[str]` for `rev_opts`, users of GitPython can pass now the `-C`
option multiple times to get more detailed authorship reports from `git blame`.
@thehale
Copy link
Contributor Author

thehale commented Aug 31, 2022

I saw earlier that you aren't a fan of force-pushes, but in this case, I found it much easier to squash my three commits into the one logical change that they are, then revert that single commit.

As expected, reverting this branch back to the code in main is passing on all counts, so there definitely seems to be some unexpected coupling between the test test_with_rw_remote_and_rw_repo and my strictly additive change to the blame() function.


something here has broken this one test, consistently across python versions

Was there a CI run where Python 3.9 was broken? I'm only seeing failures for Python 3.7 and Python 3.8 and 3.10.

I'm not sure if that observation is helpful or not. Like you, I'm trying to scrounge up anything reproducible related to this failing test.

@thehale
Copy link
Contributor Author

thehale commented Aug 31, 2022

Well... reverting the revert appears to be passing. Do you see anything that explains that behavior?

@thehale
Copy link
Contributor Author

thehale commented Aug 31, 2022

Theoretically, if the reverted revert passes, then so should the standalone squashed commit. Would you like me to force push the branch with just the squashed commit, or just keep the double revert in the history?

@Byron
Copy link
Member

Byron commented Aug 31, 2022

Thanks so much for your help! Force pushing should do the job.
My guess is that it’s about the number of commits, as the test suite sees the GitPython repository, it’s not isolated unfortunately.

@thehale
Copy link
Contributor Author

thehale commented Aug 31, 2022

Thanks so much for your help! Force pushing should do the job.

Done, and it looks like tests passed again!

My guess is that it’s about the number of commits, as the test suite sees the GitPython repository, it’s not isolated unfortunately.

Well, at least it's an effective way to require PRs to stay small! XD


Thanks for your generosity with debugging this PR and open sourcing this powerful library!

@Byron Byron added this to the v3.1.28 - Bugfixes milestone Aug 31, 2022
@Byron
Copy link
Member

Byron commented Aug 31, 2022

🎉 Thanks for pulling through, this was much harder than it should have been 😅.

@Byron Byron merged commit bec6157 into gitpython-developers:main Aug 31, 2022
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.

None yet

2 participants