update porcelain.status to output string

[DedSecer]

Issues #889

[jelmer]

Unfortunately it's not as simple as this - this changes the API of porcelain.status(), breaking any existing callers.

[codecov]

Codecov Report

Merging #890 (e58448b) into master (2f9248d) will increase coverage by 0.00%.
The diff coverage is 88.88%.

@@           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           

Impacted Files	Coverage Δ
dulwich/cli.py	0.00% <0.00%> (ø)
dulwich/porcelain.py	81.17% <100.00%> (+0.07%)	⬆️
dulwich/tests/test_porcelain.py	99.76% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f9248d...e58448b. Read the comment docs.

[jelmer]

This still changes the API - any code in applications that use dulwich that expected bytestrings will now receive plain strings and break.

[DedSecer]

So shouldn’t we change the porcelain.status?

[jelmer]

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:

• We break it, but do so at a major release and announce it as part of the API changes
• We add a flag to enable the new behaviour; this is a little bit icky since we'll have to support both behaviours going forward
• We can add another status() function that has the new behaviour

[jelmer]

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

[DedSecer]

Can we set the default encoding in the script which using dulwich to resolve it?

[jelmer]

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?

[DedSecer]

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)

[jelmer]

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?

[DedSecer]

But if current encoding doesn't match to the default encoding, Dulwich.status will still output bytes unreadable. Then what should we do?

[DedSecer]

@jelmer

[jelmer]

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.

[jelmer]

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.

[DedSecer]

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.