Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update porcelain.status to output string #890

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

DedSecer
Copy link
Contributor

Issues #889

@jelmer
Copy link
Owner

jelmer commented Jul 29, 2021

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

@codecov
Copy link

codecov bot commented Jul 30, 2021

Codecov Report

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

Impacted file tree graph

@@           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
Copy link
Owner

jelmer commented Jul 30, 2021

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

@DedSecer
Copy link
Contributor Author

So shouldn’t we change the porcelain.status?

@jelmer
Copy link
Owner

jelmer commented Aug 14, 2021

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

# 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]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@DedSecer DedSecer requested a review from jelmer August 22, 2021 03:32
@jelmer jelmer marked this pull request as draft October 7, 2021 22:14
@jelmer jelmer force-pushed the master branch 2 times, most recently from f1ae053 to cd30df4 Compare October 16, 2024 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants