feat(transfer): wire IconvSetting -> FilenameConverter at config build (#1911)#3555
Merged
Conversation
#1911) Per docs/audits/iconv-pipeline.md Finding 4, --iconv was previously a no-op end to end: the CLI parsed --iconv into IconvSetting and forwarded the value to the remote peer, but the local process never converted that setting into a protocol::FilenameConverter, so the file-list reader and writer ran with iconv: None and copied raw bytes through transfer. The bridge is now in place across all three transfer modes: - SSH push/pull (and embedded SSH): apply_common_server_flags writes `server_config.connection.iconv = config.iconv().resolve_converter()` in crates/core/src/client/remote/flags.rs:228, invoked from build_server_config_for_receiver and build_server_config_for_generator (ssh_transfer.rs and embedded_ssh_transfer.rs). - Daemon push/pull: apply_common_server_flags is also called from daemon_transfer/orchestration/server_config.rs (both paths). On the daemon side, module_access/client_args.rs:283 resolves the module's `charset =` directive into the same FilenameConverter slot. - Local copy: LocalCopyOptionsBuilder::apply_iconv in crates/core/src/client/run/mod.rs:399 writes the converter onto LocalCopyOptions::iconv, since the local-copy executor in the engine crate bypasses the SSH/daemon ServerConfig builder. The transfer crate consumes the converter at: - crates/transfer/src/receiver/mod.rs:369 - reader.with_iconv(...) - crates/transfer/src/generator/mod.rs:570 - writer.with_iconv(...) These hand the converter to the protocol crate's FileListReader / FileListWriter, where #1912 and #1913 already apply it via apply_encoding_conversion on every emitted and ingested filename. With the producer-side wiring landed, the test_iconv_local_ssh interop scenario starts gating regressions, so the standalone:iconv-local-ssh entry is removed from KNOWN_FAILURES and DASHBOARD_ENTRIES. Upstream references: - flist.c:1579-1603 send_file_entry filename iconvbufs(ic_send, ...) - flist.c:738-754 recv_file_entry filename iconvbufs(ic_recv, ...) - options.c::recv_iconv_settings parses --iconv=LOCAL,REMOTE
PR #3458 (cd76ec2) wired IconvSetting -> FilenameConverter for the local generator/receiver, but two architectural gaps prevent the SSH iconv scenario from passing end-to-end: - --iconv is not forwarded to the spawned remote rsync. Upstream options.c:2715-2723 forwards the post-comma half of iconv_opt so the remote setup_iconv() agrees on charsets. - FilenameConverter (crates/protocol/src/iconv/converter.rs) treats remote_encoding as the literal wire charset. Upstream rsync.c:87-147 always uses UTF-8 on the wire and treats `charset` as each peer's local charset. Audit findings 1-3 and 5 (docs/audits/iconv-pipeline.md) also remain open: symlink target transcoding, --files-from line forwarding, secluded-args argv re-encoding, CF_SYMLINK_ICONV gating. Restoring the KF entry until the iconv pipeline rework lands.
oferchen
added a commit
that referenced
this pull request
May 5, 2026
#1911) (#3555) * feat(transfer): wire IconvSetting -> FilenameConverter at config build (#1911) Per docs/audits/iconv-pipeline.md Finding 4, --iconv was previously a no-op end to end: the CLI parsed --iconv into IconvSetting and forwarded the value to the remote peer, but the local process never converted that setting into a protocol::FilenameConverter, so the file-list reader and writer ran with iconv: None and copied raw bytes through transfer. The bridge is now in place across all three transfer modes: - SSH push/pull (and embedded SSH): apply_common_server_flags writes `server_config.connection.iconv = config.iconv().resolve_converter()` in crates/core/src/client/remote/flags.rs:228, invoked from build_server_config_for_receiver and build_server_config_for_generator (ssh_transfer.rs and embedded_ssh_transfer.rs). - Daemon push/pull: apply_common_server_flags is also called from daemon_transfer/orchestration/server_config.rs (both paths). On the daemon side, module_access/client_args.rs:283 resolves the module's `charset =` directive into the same FilenameConverter slot. - Local copy: LocalCopyOptionsBuilder::apply_iconv in crates/core/src/client/run/mod.rs:399 writes the converter onto LocalCopyOptions::iconv, since the local-copy executor in the engine crate bypasses the SSH/daemon ServerConfig builder. The transfer crate consumes the converter at: - crates/transfer/src/receiver/mod.rs:369 - reader.with_iconv(...) - crates/transfer/src/generator/mod.rs:570 - writer.with_iconv(...) These hand the converter to the protocol crate's FileListReader / FileListWriter, where #1912 and #1913 already apply it via apply_encoding_conversion on every emitted and ingested filename. With the producer-side wiring landed, the test_iconv_local_ssh interop scenario starts gating regressions, so the standalone:iconv-local-ssh entry is removed from KNOWN_FAILURES and DASHBOARD_ENTRIES. Upstream references: - flist.c:1579-1603 send_file_entry filename iconvbufs(ic_send, ...) - flist.c:738-754 recv_file_entry filename iconvbufs(ic_recv, ...) - options.c::recv_iconv_settings parses --iconv=LOCAL,REMOTE * revert(ci): restore standalone:iconv-local-ssh known failure PR #3458 (cd76ec2) wired IconvSetting -> FilenameConverter for the local generator/receiver, but two architectural gaps prevent the SSH iconv scenario from passing end-to-end: - --iconv is not forwarded to the spawned remote rsync. Upstream options.c:2715-2723 forwards the post-comma half of iconv_opt so the remote setup_iconv() agrees on charsets. - FilenameConverter (crates/protocol/src/iconv/converter.rs) treats remote_encoding as the literal wire charset. Upstream rsync.c:87-147 always uses UTF-8 on the wire and treats `charset` as each peer's local charset. Audit findings 1-3 and 5 (docs/audits/iconv-pipeline.md) also remain open: symlink target transcoding, --files-from line forwarding, secluded-args argv re-encoding, CF_SYMLINK_ICONV gating. Restoring the KF entry until the iconv pipeline rework lands.
2 tasks
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
standalone:iconv-local-sshentry fromtools/ci/known_failures.conf(bothKNOWN_FAILURESandDASHBOARD_ENTRIES) now that the IconvSetting -> FilenameConverter bridge is wired through every transfer mode.docs/audits/iconv-pipeline.mdFinding 4, --iconv was previously parsed by the CLI and forwarded to the remote peer but never converted to aprotocol::FilenameConverterfor the local process, so file-list reader/writer ran withiconv: Noneand copied raw bytes.test_iconv_local_sshshould now pass and start gating regressions.Plumbing path (CLI -> transfer)
CLI
--iconv->IconvSetting::parse->ClientConfig.iconv->IconvSetting::resolve_converter()->Option<FilenameConverter>-> consumed in three places:apply_common_server_flagsincrates/core/src/client/remote/flags.rs:228writesserver_config.connection.iconv = config.iconv().resolve_converter(). Invoked frombuild_server_config_for_receiverandbuild_server_config_for_generatorin bothssh_transfer.rsandembedded_ssh_transfer.rs.apply_common_server_flagsis also called fromdaemon_transfer/orchestration/server_config.rs(both directions). The daemon-side counterpart lives atcrates/daemon/src/daemon/sections/module_access/client_args.rs:283, which resolves the module'scharset =directive into the sameconnection.iconvslot (PR feat(daemon): wire module charset= directive to iconv runtime (#1917) #3554).LocalCopyOptionsBuilder::apply_iconvatcrates/core/src/client/run/mod.rs:399writes the converter ontoLocalCopyOptions::iconv, since the local-copy executor in theenginecrate does not traverse the SSH/daemonServerConfigbuilder.The transfer crate consumes the converter at:
crates/transfer/src/receiver/mod.rs:369->reader.with_iconv(...)crates/transfer/src/generator/mod.rs:570->writer.with_iconv(...)Both feed the protocol crate's
FileListReader/FileListWriter, where #1912 and #1913 already apply the converter viaapply_encoding_conversionon every emitted and ingested filename.KNOWN_FAILURES removal
standalone:iconv-local-sshfrom theKNOWN_FAILURES=( ... )array.DASHBOARD_ENTRIES=( ... ).standalone:delta-statsandstandalone:upstream-compressed-batch-self-roundtripuntouched.Upstream references
flist.c:1579-1603-send_file_entry()filenameiconvbufs(ic_send, ...)flist.c:738-754-recv_file_entry()filenameiconvbufs(ic_recv, ...)options.c::recv_iconv_settings- parses--iconv=LOCAL,REMOTEcompat.c:716-718- gatesCF_SYMLINK_ICONVon iconv configuredTest plan
test_iconv_local_ssh_interopruns and passes against upstream rsync 3.4.1 (was previously masked by the KF entry; now gating regressions).test_iconvandtest_iconv_upstream_interopcontinue to pass.