Respect os.Pathlike

[George-Ogden]

Fixes #2085
Replaces instances of str(path) with path.__fspath__() for more general usage.
I also moved the clone tests into a separate file that existed before but only contained a single test.

[George-Ogden]

Also, I notice you skip mypy because it produces errors. Why not pin the version and increment it periodically?

[Byron]

That's a great incentive, thanks so much! Using __fspath__() seems like it's using a private API, so it looks strange to my untrained eye. But if it's correct, I guess it's worth doing anyway?

Also, I notice you skip mypy because it produces errors. Why not pin the version and increment it periodically?

I don't see why this wouldn't work, great idea! Please feel free to set this up in your next PR :).

[Byron]

It seems that I won't get my copilot review here, so I wonder why you'd not be calling os.fspath(path) instead?

[George-Ogden]

It seems that I won't get my copilot review here, so I wonder why you'd not be calling os.fspath(path) instead?
I agree that's much nicer and also gets rid of the if not isinstance(path, str) checks.

[George-Ogden]

I don't see why this wouldn't work, great idea! Please feel free to set this up in your next PR :).

Sure thing

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Byron]

This looks better indeed.
Something I think will be desirable is to actually add tests for non-decodable paths for the functions/types that are affected. Otherwise, how do we know it's working?

Feel free to use AI for that, it's quite good at this usually.

The idea is to prove that the changes actually make something possible that wasn't possible before.

[Byron]

URL is not a path though.

[George-Ogden]

Here, url can be a path if you're cloning a local repo, and if it's not os.fspath will leave strings alone.

[Copilot]

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[George-Ogden]

I've added a few tests, but lots of the calls work on internal APIs, so they won't make much difference. In these cases, calling os.fspath is still better as it makes the intention clearer. I also made a tool to check for redundant uses and the only one is git/util.py:420.

[Byron]

That's great, thanks a lot!

What should be shown in the tests is that it can now handle filepaths that don't decode with the standard encoding.

Candidate tests are:

• clone from a filesystem path
• index.add with a strange path
• and probably more - you could intentionally break a piece of code that changed fspath and see which tests fail, then it's clear where to add a test with a 'special' path.

Thanks for making this happen, and thanks for your understanding - we can't just make changes hoping it will work or doesn't make things work, but there must be real evidence that this is desirable. And we can only have that with tests that fail without this change.

[Byron]

I am putting the PR back to draft until there are tests that use the new "can handle paths encoding independently from the runtime encoding" to prove the changes are effective. Thanks again.

[George-Ogden]

Sorry, I meant to add this comment with the changes I made yesterday:
I've gone through all the changes and made sure that they apply (and removed the ones that didn't). As a result, there are more tests for submodules and index, as well as the existing ones I added.

For example, the clone_from_pathlike test means that all the fs.path conversions in Repo.base are required and removing them will cause that test to fail. Is that what you mean by "can handle paths encoding independently from the runtime encoding"? If not, I can add tests that will do that.

[Byron]

No problem!

What I mean is that the point of this conversion is to prevent decoding issues related to paths. I.e. the user passes a filesystem path, but internally GitPython runs str() on it which then tries to re-encode the path-bytes to the python runtime default encoding. This typically fails as soon as there is one non-ascii character.

You'd have to go back to main and add such tests which should fail, to then show that this now works with the changes in this branch, for a particular scenario - like one of the ones I mentioned, but there might be more.

[George-Ogden]

I've added some non-ASCII characters into the tests. However, the main point of the pull request is with objects that follow the os.Pathlike type hint, such as this:

import os
from dataclasses import dataclass

@dataclass
class CustomPathlike:
    path: str
    def __fspath__(self) -> str:
        return self.path

custom_pathlike = CustomPathlike("folder/file")
str(custom_pathlike) # "CustomPathlike(path='folder/file')" - fails in "git add CustomPathlike(path='folder/file')"
os.fspath(custom_pathlike) # "folder/file"  - works in "git add folder/file"

I realise that that was unclear in both the PR and the original issue (so I've added this example to the original issue).