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

recognize the new packed-ref header format #689

Merged
merged 1 commit into from
Nov 2, 2017

Conversation

bjb
Copy link

@bjb bjb commented Oct 13, 2017

Related to issue 687. I also had the problem after updating my git version.

Please feel free to remove all the comments explaining the change.

I must admit this is the first time I've looked at the git source, so I may not have done this right. But, it works for me : -)

as long as line contains "peeled", accept it
fixes the PackingType of packed-Refs not understood:

pack-refs with: peeled fully-peeled sorted

problem

as long as line contains "peeled", accept it
fixes the PackingType of packed-Refs not understood:
# pack-refs with: peeled fully-peeled sorted
problem
@bjb
Copy link
Author

bjb commented Oct 13, 2017

I'm not familiar with "appveyor", it seems a windows version of python is objecting to this patch? Maybe it doesn't like the syntax: if 'substr' not in 'mystring'? If that is the case, I can rewrite in another syntax. Is there a recommended method for windows versions of python? Or am I completely off base?

How about using "find"? Find was not recommended in the python documentation for this kind of application because it is inefficient I guess ... but if it is the only thing that will work I can use it.

@bjb bjb closed this Oct 13, 2017
@bjb
Copy link
Author

bjb commented Oct 13, 2017

Oops, I did not mean to close it ...

@bjb bjb reopened this Oct 13, 2017
@codecov-io
Copy link

Codecov Report

Merging #689 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #689   +/-   ##
=======================================
  Coverage   92.95%   92.95%           
=======================================
  Files          61       61           
  Lines       10065    10065           
=======================================
  Hits         9356     9356           
  Misses        709      709
Impacted Files Coverage Δ
git/refs/symbolic.py 96.44% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f237620...5a358f2. Read the comment docs.

@bjb
Copy link
Author

bjb commented Oct 14, 2017

I found the details for the appveyor failures (test console output). It says, in each of several python versions, after 500 lines of test output, that it cannot find the git executable. It is doing the "refresh" test in each case.. I don't think I can address that.

Re: coverage ... again I don't know how to make these tests pass. Maybe there needs to be more tests, to cover the case where the word "peel" did not occur on the packed-refs header for example.

Or maybe I completely misunderstand.

@rbdixon
Copy link

rbdixon commented Oct 30, 2017

@bjb -- thank you! This issue stopped development here as we sorted it out. This one-liner fixes the issue nicely. Hope to see it merged soon.

@gawbul
Copy link

gawbul commented Nov 1, 2017

@bjb same here - updated file with this update and fixed issues I was having.

@Byron Byron merged commit 66328d7 into gitpython-developers:master Nov 2, 2017
@Byron
Copy link
Member

Byron commented Nov 2, 2017

Thanks a lot for your contribution!
I hope this merge will already help some people.
However, it would be nice if you could also stitch a test together to make the intent of the code executable :).

@bjb
Copy link
Author

bjb commented Nov 3, 2017

Hmm, I'll have to learn more about these packed_refs then ... Also must learn about testing in GitPython.

In the test, should I create a repo and then use it, or does the test suite make a repo for the tests to use? I think this "peeled sorted" thing requires a certain version of git, how do I specify that in the test? Thanks for whatever pointers you can send me. I will try to do this regardless.

@jgavris
Copy link

jgavris commented Nov 9, 2017

Hi, just wondering when the next release will be cut? A bunch of people have upgraded git and need this.

@bjb
Copy link
Author

bjb commented Nov 10, 2017

There are already some tests that cover this problem.

When using git version 2.15 and GitPython version 2.1.7 (ccff34d)

The following tests ERROR when running
"nosetests git/test/test_refs.py"

ERROR: test_head_checkout_detached_head (git.test.test_refs.TestRefs)
ERROR: test_head_reset (git.test.test_refs.TestRefs)
ERROR: test_heads (git.test.test_refs.TestRefs)
ERROR: test_is_valid (git.test.test_refs.TestRefs)
ERROR: test_reflog (git.test.test_refs.TestRefs)
ERROR: test_refs (git.test.test_refs.TestRefs)
ERROR: test_tag_base (git.test.test_refs.TestRefs)
ERROR: test_tags (git.test.test_refs.TestRefs)
ERROR: test_tags_author (git.test.test_refs.TestRefs)

(all but the "test_heads" test complain about the reported error:
TypeError: PackingType of packed-Refs not understood: '# pack-refs with: peeled fully-peeled sorted'; test_heads has a different error but it is also cleared up with the fix.)

And when using git version 2.15 and GitPython version 5a358f2 (where the fix went in)

and running "nosetests git/test/test_refs.py", all the same tests pass:

nosetests git/test/test_refs.py 2>&1 | less
............

Ran 12 tests in 3.531s

OK

Please let me know if this is sufficient or if you'd like something else done.

@beatgammit
Copy link

Just pinging because a release would be very nice to have.

@Byron
Copy link
Member

Byron commented Nov 19, 2017

@beatgammit Sorry for all these delays, I am now working towards merging all pending PRs, making Travis happy (flake8 failure), to pave the way towards a release. Let's hope I will finish this today.

@kennethreitz
Copy link

I can confirm that this patch is hitting me as well.

@theckman
Copy link

theckman commented Dec 1, 2017

I've opened a new issue to track a new release of GitPython to include this fix. I didn't want to step on anyone toes, but felt it would be an easier way for all of us to track a new release that includes this fix:

@kennethreitz
Copy link

\o/

@Byron
Copy link
Member

Byron commented Dec 11, 2017

@thiblahute Sorry for letting you (and probably many more) wait for so long. I am once again working on it, and think chances are I will actually make that release today as I am on vacation and fortunately have nothing else to do :D!

wmfgerrit pushed a commit to wikimedia/integration-quibble that referenced this pull request Apr 26, 2018
On a local machine, when using git 2.15, GitPython before 2.1.8 will
fail to recognize an instruction and raise:

  Type of packed-Refs not understood: '# pack-refs with: peeled
  fully-peeled sorted'

That is fixed since GitPython 2.1.8:
gitpython-developers/GitPython#689

We do not want versions 2.1.2 to 2.1.7 that have a performance
regression.

In Docker containers I want to rely on the python3-git python (2.1.1)
over the version from PyPI.  Both Jessie and Stretch ship 2.1.1 and have
older git versions, they are not affected by the issue.

Hence extend the requirement so that it accepts the Debian package
version (2.1.1 < 2.2.0).
Blacklist the versions we do not want (2.1.2 to 2.1.7).
Allow a version from Pypi (2.1.8+) when one uses a recent git.

Bug: T193057
Change-Id: I47367016c496e73fa18ad6d17dc08a39d5d4283d
riley-martine pushed a commit to riley-martine/GitPython that referenced this pull request Dec 7, 2023
recognize the new packed-ref header format
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.

10 participants