Properly support skipping of non-mandatory extensions #1066
Conversation
codablock
force-pushed
the
fix-unknown-extensions
branch
from
c5317d5
to
f008dec
Compare
|
Some reasoning behind the first commit: I looked into how git figures out how many extensions are present and found https://github.com/git/git/blob/master/read-cache.c#L2047. It knows when to stop when go-git does not know the file size because it is not passed around. I decided to not change this as it would lead to an api-incompatible change. So instead, I decided to perform a peek based test to see if enough bytes are remaining that an extension could possibly be present. |
Before this, go-git was relying on the peeked header to not include a valid 4 char string header. While doing this, it did not differentiate between the errournously read final hash and an unknown extension. This made it impossible to properly skip unknown optional extensions while detecting EOF early enough.
codablock
force-pushed
the
fix-unknown-extensions
branch
from
f008dec
to
d3aec57
Compare
There was a problem hiding this comment.
@codablock please add some tests to ensure we don't get regressions in the future.
Before this, go-git would prematurely bail out of extensions processing when an unknown extension was encountered. This had two issues: 1. It did not account for mandatory (lower case header) extensions 2. It did not properly update the calculated hash, leading to an "invalid checksum" error.
codablock
force-pushed
the
fix-unknown-extensions
branch
from
d3aec57
to
02bb5ca
Compare
|
@pjbgf Handled all the review comments. I also added a test for invalid index hashes and truncated extensions. |
codablock
force-pushed
the
fix-unknown-extensions
branch
from
02bb5ca
to
f763fd3
Compare
|
Test failures seem to be unrelated? |
See the 2 individual commits.
This fix might cause some existing repos to fail now, because go-git did not properly fail on
linkextensions (split index). I assume it's fine to fail, as this extension is mandatory anyway and behaviour is actually undefined/buggy when handling of it is skipped.Fixes: #299