update porcelain.status to output string #890
Conversation
|
Unfortunately it's not as simple as this - this changes the API of porcelain.status(), breaking any existing callers. |
Codecov Report
@@ Coverage Diff @@
## master #890 +/- ##
=======================================
Coverage 84.64% 84.64%
=======================================
Files 91 91
Lines 22349 22352 +3
Branches 2403 2406 +3
=======================================
+ Hits 18917 18920 +3
Misses 3009 3009
Partials 423 423
Continue to review full report at Codecov.
|
|
This still changes the API - any code in applications that use dulwich that expected bytestrings will now receive plain strings and break. |
|
So shouldn’t we change the porcelain.status? |
|
We need to figure out a path forward - if we change this, it's going break everybody who relies on this API. There are a couple of possibilities:
|
| # 2. Get status of unstaged | ||
| index = r.open_index() | ||
| normalizer = r.get_blob_normalizer() | ||
| filter_callback = normalizer.checkin_normalize | ||
| unstaged_changes = list(get_unstaged_changes(index, r.path, filter_callback)) | ||
| unstaged_changes = [file.decode() for file in unstaged_changes] |
There was a problem hiding this comment.
This is going to decode with the current encoding (e.g. utf-8) which may not work if the filename contains bytes that can't be decoded with the current encoding (e.g. utf-8) or even if they can it may not result in the string you were expecting (file system encoding and display encoding may be different).
There was a problem hiding this comment.
Can we set the default encoding in the script which using dulwich to resolve it?
There was a problem hiding this comment.
That would be inconsistent with the way the rest of Dulwich works. And if you're making the caller worry about the encoding, why not let them do the decoding as well?
There was a problem hiding this comment.
Generally, it can decode the bytes with defaultcoding, if it doesn't work, we just need to use # -*- coding: xxx -*- at the top of the file.(I think if the defaultcoding is incorrect, there will be other problems too)
There was a problem hiding this comment.
The encoding on top of the file is for the source file itself, not for decoding at run time.
If you're a user of Dulwich and you have a repo that happens to use latin1 as encoding while your current encoding is utf8, how would that work?
There was a problem hiding this comment.
But if current encoding doesn't match to the default encoding, Dulwich.status will still output bytes unreadable. Then what should we do?
There was a problem hiding this comment.
For some uses cases, bytes are sufficient. You also can choose what to do in that case if you want to display the output - you can ignore invalid characters, replace them with surrogates or e.g. use chardet to try to figure out the proper encoding if it isn't utf8.
There was a problem hiding this comment.
I'd be open to returning a surrogateescape-decoded string here, since it's possible for the caller to undo that and retrieve the original bytes but that would still be a breaking change for existing users of the API. We should also force utf8 in that case (since that's the convention for git repositories) rather than whatever the users' encoding is.
There was a problem hiding this comment.
I think the function in porcelain should work like the git subcommands(a higher function), or I should decode the bytes after using porcelain.status.
Issues #889