Repository navigation
Preserve ideographic spaces when copying text #95
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
Open
Ehsan-hash
wants to merge
1
commit into
github:main
Choose a base branch
from
Ehsan-hash:fix/preserve-ideographic-spaces
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jonrohan do you have thoughts on this? 👀 preserving U+3000 fixes the Japanese text issue, but it sounds like it could weaken the command-copy protection from #87/#88. not sure which behavior we want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed the parsing difference locally without fetching or executing any remote content: Bash retains U+3000 in the URL argument before the pipe, while an ASCII space terminates that argument. The Japanese text-preservation benefit therefore does not establish that a global exception is appropriate for command-copying callers.
The current PR remains a proposal. I will hold further changes to this exception pending the maintainers' decision on the security tradeoff and whether the intended behavior needs to depend on the caller. This agent-run check is not a substitute for the requested human security decision.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for checking. The parsing difference is real, and I'm not disputing it. I opened #94, so here is the other side of the tradeoff: normalizing U+3000 doesn't remove the mismatch between what users see and what they copy. It moves it.
In Japanese environments, U+3000 commonly appears inside quoted arguments of shell commands:
With normalization, the copied command silently targets a different file or searches for a different string than the one displayed. So for command-copying callers too, normalization can make the copied command behave differently from what the user sees, and in this case it happens to legitimate, everyday text rather than to a crafted edge case.
Also, unlike U+00A0, U+3000 is visibly wider than an ASCII space, especially in monospace code blocks, so the spoofing risk is lower than in the case #87 addressed.
For these reasons, I think preserving U+3000, as this PR does, is the safer default overall. If you decide to keep normalization for some callers, I'd ask that U+3000 at least be preserved in non-shell code blocks (CSV, JSON, plain text, etc.). That alone would fix the data-corruption case in #94.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A small correction to my previous comment. English isn't my first language, and I used an AI assistant to help write it. In the process, a compromise ended up in the last paragraph that I don't actually agree with.
Please disregard the suggestion to preserve U+3000 only in non-shell code blocks. As the examples show, shell commands are exactly where silently changing U+3000 causes the most harm, so that compromise is hard for me to accept. My position is that U+3000 should be preserved everywhere, including in shell code blocks, as this PR does.