Fix repository crash if path passed is not a str #593
Conversation
|
Ok so that build was a catastrophe, will fix it in a bit. |
|
Hello, could you please provide a unit test? If possible the unit test should be the first commit, and the fix would be the second commit. Extra points for making a new branch on top of current master. Note too that pygit2 supports both Python 2.7 and 3, and that Thanks |
Yeah, that difference is the whole point. If pygit2 is passed a path represented as I'm actually not sure if this is pygit2's problem or the problem of the applications using pygit2's API. I have the feeling that lots of other parts of pygit2 will also fail if they are passed paths as (I mainly made this PR and issue #588 because tracking down where the bytes came from in powerline was more difficult and I couldn't find a clear position in the pygit2 docs.) |
|
I have added a test and rebased on |
| def __init__(self, *args, **kwargs): | ||
| super(Repository, self).__init__(*args, **kwargs) | ||
| def __init__(self, path, *args, **kwargs): | ||
| if not isinstance(path, str): |
There was a problem hiding this comment.
Shouldn't it be something like six.string_types here? Because this will surely not work/break on py3
There was a problem hiding this comment.
six wasn't a dependency of this project so I was hesitant to include it.
It will actually not break though. On py2, you'll pass a str or unicode to this function. unicode has .decode and will decode to str. py2 str is accepted by Repository just fine. On py3 you'll pass a str or bytes to this function. bytes will .decode to str. py3 str is also accepted by Repository.
If you pass anything else then it'll break with a vague error about not having .decode, but this is the best I could come up with without six.
Basically pygit2 tries to be a good Python 2 citizen and good Python 3 citizen, which develops a kind of At the C level, the relevant line is at In Python 2 this is interpreted as bytes or unicode. In Python 3 this only accepts unicode. |
|
I have applied the failing test. Now about the fix. |
|
+1 to @pypingou idea of using six. As the Python side of pygit2 grows six will be useful for this and other Then yes the test would be: I leave you, @pypingou or @thomwiggers some time to add six as dependency 😉 .. otherwise I will do it. It would be interesting too, but not required, to add tests for paths with non ascii chars. |
|
FYI: I've looked into fixing it in the C API instead of in the python code, but that does not seem possible because the needed converter is only available in py3.2+: https://docs.python.org/3.5/c-api/unicode.html#c.PyUnicode_FSDecoder |
|
I've rebased on master, slightly modified my tests and am now using |
Add unit test for bytes repository paths Add a unicode path test for Repositories
Tries to decode any non-string objects (such as bytes) Introduces `six` as a dependency Closes #588
This perhaps isnt' the most elegant fix, but otherwise all callers need to be patched.
Closes #588