paste: support multi-byte delimiters and GNU escape sequences #10840
Conversation
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
ChrisDryden
force-pushed
the
paste-multibyte-delimiters
branch
from
5865c10 to
321cb08
Compare
Collaborator
Author
|
Seems like github is down? |
This comment was marked as outdated.
This comment was marked as outdated.
ChrisDryden
force-pushed
the
paste-multibyte-delimiters
branch
from
321cb08 to
b084aaa
Compare
Contributor
|
I think you can re-run 1 job only instead of force-pushing, |
ChrisDryden
force-pushed
the
paste-multibyte-delimiters
branch
from
b084aaa to
1650f0e
Compare
oech3
reviewed
Feb 9, 2026
| grep -rl '\$abs_path_dir_' tests/*/*.sh | xargs -r "${SED}" -i "s|\$abs_path_dir_|${UU_BUILD_DIR//\//\\/}|g" | ||
| # Some tests use $abs_top_builddir/src for shebangs: point them to the uutils build dir. | ||
| grep -rl '\$abs_top_builddir/src' tests/*/*.sh tests/*/*.pl | xargs -r "${SED}" -i "s|\$abs_top_builddir/src|${UU_BUILD_DIR//\//\\/}|g" | ||
| grep -rl '\$ENV{abs_top_builddir}/src' tests/*/*.pl | xargs -r "${SED}" -i "s|\$ENV{abs_top_builddir}/src|${UU_BUILD_DIR//\//\\/}|g" |
Collaborator
Author
There was a problem hiding this comment.
Whoops that is unrelated but it changes two skips to passes, was working on that locally and it got added
Contributor
There was a problem hiding this comment.
But does not mean that we are not using uutils binaries at here if we don't sed?
Collaborator
Author
There was a problem hiding this comment.
According to the logs I think we deleted the gnu coreutils binaries from that env so it means that it just skips because its unable to find a binary.
Contributor
There was a problem hiding this comment.
Can we simply symlink our bins to abs_top_builddir for all tests at once?
|
GNU testsuite comparison: |
ChrisDryden
force-pushed
the
paste-multibyte-delimiters
branch
from
1650f0e to
143fec3
Compare
|
GNU testsuite comparison: |
Contributor
|
oh, nice |
sylvestre
reviewed
Feb 9, 2026
| /// Returns 1 for empty, invalid, or incomplete sequences. | ||
| pub fn mb_char_len(bytes: &[u8]) -> usize { | ||
| if bytes.is_empty() { | ||
| return 1; |
Contributor
There was a problem hiding this comment.
returning 1 when empty is confusing
can you please document why ?
sylvestre
reviewed
Feb 9, 2026
src/uu/paste/src/paste.rs
Outdated
| translate!("paste-error-delimiter-unescaped-backslash", "delimiters" => delimiters), | ||
| )); | ||
| fn parse_delimiters(delimiters: &OsString) -> UResult<Box<[Box<[u8]>]>> { | ||
| let bytes = uucore::os_string_to_vec(delimiters.clone())?; |
Contributor
There was a problem hiding this comment.
can we use OsStr::as_bytes() on Unix or borrowing instead of cloning the OsString ?
sylvestre
reviewed
Feb 9, 2026
| _ => { | ||
| // Unknown escape: strip backslash, use the following character(s) | ||
| let len = mb_char_len(&bytes[i..]); | ||
| vec.push(Box::from(&bytes[i..i + len])); |
Contributor
There was a problem hiding this comment.
Potential panic if mb_char_len returns non-zero but bytes[i..] is shorter than returned length, no?
|
GNU testsuite comparison: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
This PR is fairly large in scope and has a few design decisions that probably warrant some additional discussion. I have been prototyping many approaches to how to deal with the issue of getting the multi-byte character sequence lengths and all of the solutions I could think of that would call the libc implementation ultimately would take a bunch of unsafe code and require FFI's and still create compatibility issues when it comes to different platforms not supporting that API.
To work backwards from what the goal was, I went through all of the glibc locales to see which ones actually had multi-byte decoding rules that were unique from UTF-8 and how many of those overlapped with one another. That led to to discover that currently there are only 5 different rule sets for this calculation and that you can determine which encoding to use from the env variables for which locale to use. Then it appears that some of the locales are hardcoded to specific encodings and the only two that I could find when searching all of the glibc locales was
"zh_CN" | "zh_SG"and"zh_TW" | "zh_HK"There were two new GNU tests that were related to this type of locale stuff: paste.pl and multi-byte.sh that this PR addresses.
This can help with the other issues in the queue:
#10184
#9712
#7600
#3075
#5831
#3997