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

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 :).

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

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

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

Sure thing

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.

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, theclone_from_pathlike test means that all thefs.path conversions inRepo.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.

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 runsstr() 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 tomain 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.

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

importosfromdataclassesimportdataclass@dataclassclassCustomPathlike:path:strdef__fspath__(self)->str:returnself.pathcustom_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).

Thanks a lot, this definitely helped me understand that these changes aren't meant to address that one thing I thought they do 😅.

My feeling is that the added tests don't actually fail on master, and thus should be removed. But if they do, please feel free to put them into a separate PR so I see them failing.
Either way, once I hear from you I think the PR can be merged.

See#2088 for failing tests

Thanks for giving it a shot. Admittedly,@2088 is surprisingly complex and fails in more places than just the two I thought it would fail in. To my mind, a cherry-pick of1710626 ontomaster would have done the trick (or a manual port of these tests), instead there isa lot of new commits.

Do you want to try again, or remove the added tests here in absence of a demonstration of failure?

I've done that in#2089

Thanks a lot for your patience, just one more thing:1710626 adds tests which I believe should have shown that they won't work onmaster, but work here. The idea is that the encoding of the files can't be determined if Python would try to decode them, which a lot of code here does.

If you don't feel like this has a chance to be fixed, then I think these tests can be removed by force-pushing without this commit for this PR to be merged. Otherwise, there can be a PR onmaster that shows how these two tests are failing because of some decoding issue, and this PR will be merged as well.

Thank you, and… we will get there :).

However, both

The difference in

I see, thanks for clarifying and digging out the non-unicode tests onmain which have been there before and seemed to be working already.

Then I even more so think that the tests in1710626 as duplicates of the tests above serve no purpose. ThePathLike test is already done.
What am I missing?

All reverted! I agree that the tests don't add much.

Alright, let's do this!
Thanks for bearing with me here.

Thanks, that's great 🥳. Can you release a new version please?

I will do this at the end of the year, time for more fixes and features to be collected.