fix(cli): wire --iconv and recognise --timeout= in server-mode flag parser#3566
Merged
Merged
Conversation
…tics PR #3554 redesigned the daemon iconv test (test_iconv_upstream_interop) to use a single-file Value Object that asserts the destination disk holds the ISO-8859-1 byte name (caf\xe9.txt), not the UTF-8 byte name (caf\xc3\xa9.txt). That redesign was missed for the SSH/local-mode test (test_iconv_local_ssh_interop, added separately in PR #3534), which still asserted UTF-8 dest names and failed in CI on a clean Linux host where ext4 happily stores the Latin-1 single-byte names upstream's ic_recv writes. Per upstream rsync.c:85-147 setup_iconv() and options.c:2716-2723 server_options(): with --iconv=UTF-8,ISO-8859-1, the client (charset UTF-8) keeps the LOCAL half and forwards the post-comma REMOTE half (ISO-8859-1) to the spawned peer. The spawned receiver opens ic_recv = iconv_open(charset, UTF8_CHARSET), so it transcodes UTF-8 wire bytes into single-byte Latin-1 on the local disk. The dest filename is therefore caf\xe9.txt, not caf\xc3\xa9.txt. Refactor test_iconv_local_ssh_interop to reuse the single-file Value Object helpers from the daemon test (_ic_init_fixtures / _ic_write_fixtures / _ic_verify_dest at run_interop.sh:3142-3175), expand the docblock to explain the am_server LOCAL/REMOTE split with explicit upstream references, and drop the multi-file UTF-8-expecting fixture that masked the real wire semantics. No production code changes; this fixes the test expectation only.
Server mode previously dropped --iconv=CHARSET passed by the client, falling back to no transcoding. This broke --iconv=LOCAL,REMOTE transfers over SSH and local-shell where the on-disk filename encoding differs between sides: receiver/generator skipped the iconv hook and wrote/read wire UTF-8 bytes verbatim instead of transcoding to the server's local charset. Parse --iconv=VALUE in ServerLongFlags, resolve via IconvSetting, and assign to config.connection.iconv so receiver and generator pick up the converter. Mirrors the daemon path (daemon/sections/module_access/client_args.rs) and the SSH-client path (client/remote/flags.rs). upstream: rsync.c:85-147 setup_iconv() upstream: options.c:2716-2723 server-side iconv forwarding
oferchen
changed the title
The server-mode argument parser did not recognise `--timeout=N`, so the flag was treated as a positional argument by `parse_server_flag_string_and_args` and consumed in place of the destination path. With the iconv-local-ssh scenario, strace showed the receiver creating `--timeout=10/` instead of `dest/` because upstream rsync emits `--timeout=10` after `--iconv=...` in the server arg list. Add `--timeout=` to `ServerLongFlags`, `parse_value_bearing_flag`, and `is_known_server_long_flag` so the flag is captured (its value remains unwired for now, matching the prior silent-drop behaviour) and excluded from positional extraction. upstream: options.c - server_options() emits `--timeout=%d` from io_timeout
oferchen
changed the title
oferchen
added a commit
that referenced
this pull request
May 5, 2026
…arser (#3566) * test(interop): align iconv-local-ssh with upstream Latin-1 dest semantics PR #3554 redesigned the daemon iconv test (test_iconv_upstream_interop) to use a single-file Value Object that asserts the destination disk holds the ISO-8859-1 byte name (caf\xe9.txt), not the UTF-8 byte name (caf\xc3\xa9.txt). That redesign was missed for the SSH/local-mode test (test_iconv_local_ssh_interop, added separately in PR #3534), which still asserted UTF-8 dest names and failed in CI on a clean Linux host where ext4 happily stores the Latin-1 single-byte names upstream's ic_recv writes. Per upstream rsync.c:85-147 setup_iconv() and options.c:2716-2723 server_options(): with --iconv=UTF-8,ISO-8859-1, the client (charset UTF-8) keeps the LOCAL half and forwards the post-comma REMOTE half (ISO-8859-1) to the spawned peer. The spawned receiver opens ic_recv = iconv_open(charset, UTF8_CHARSET), so it transcodes UTF-8 wire bytes into single-byte Latin-1 on the local disk. The dest filename is therefore caf\xe9.txt, not caf\xc3\xa9.txt. Refactor test_iconv_local_ssh_interop to reuse the single-file Value Object helpers from the daemon test (_ic_init_fixtures / _ic_write_fixtures / _ic_verify_dest at run_interop.sh:3142-3175), expand the docblock to explain the am_server LOCAL/REMOTE split with explicit upstream references, and drop the multi-file UTF-8-expecting fixture that masked the real wire semantics. No production code changes; this fixes the test expectation only. * fix(cli): wire --iconv into server-mode flag parser Server mode previously dropped --iconv=CHARSET passed by the client, falling back to no transcoding. This broke --iconv=LOCAL,REMOTE transfers over SSH and local-shell where the on-disk filename encoding differs between sides: receiver/generator skipped the iconv hook and wrote/read wire UTF-8 bytes verbatim instead of transcoding to the server's local charset. Parse --iconv=VALUE in ServerLongFlags, resolve via IconvSetting, and assign to config.connection.iconv so receiver and generator pick up the converter. Mirrors the daemon path (daemon/sections/module_access/client_args.rs) and the SSH-client path (client/remote/flags.rs). upstream: rsync.c:85-147 setup_iconv() upstream: options.c:2716-2723 server-side iconv forwarding * fix(cli): recognise --timeout= in server-mode flag parser The server-mode argument parser did not recognise `--timeout=N`, so the flag was treated as a positional argument by `parse_server_flag_string_and_args` and consumed in place of the destination path. With the iconv-local-ssh scenario, strace showed the receiver creating `--timeout=10/` instead of `dest/` because upstream rsync emits `--timeout=10` after `--iconv=...` in the server arg list. Add `--timeout=` to `ServerLongFlags`, `parse_value_bearing_flag`, and `is_known_server_long_flag` so the flag is captured (its value remains unwired for now, matching the prior silent-drop behaviour) and excluded from positional extraction. upstream: options.c - server_options() emits `--timeout=%d` from io_timeout
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
Two coupled bugs in the server-mode argument parser, both surfaced by the
iconv-local-sshinterop scenario:--iconv=was parsed but never wired toServerConfig.connection.iconv. The receiver/generator skipped the iconv hook and wrote raw bytes verbatim, so a Latin-1 filename round-tripped via UTF-8 wire never reached disk in its expected on-disk encoding. NowIconvSetting::parse+resolve_converterare called and the converter is installed on the server config.--timeout=Nwas unrecognised byis_known_server_long_flag. Because upstreamserver_options()emits--timeout=%dafter--iconv=..., the unrecognised arg was consumed as a positional and the destination directorydest/ended up as--timeout=10/. Verified viastrace: receivermkdirat/renameatagainst--timeout=10/. Now--timeout=is captured intoServerLongFlags.timeout(value remains unwired - same silent-drop behaviour as before, but stripped from positional extraction).Root cause evidence
--iconv=ISO-8859-1was on the wire.mkdirat(AT_FDCWD, "--timeout=10", ...)instead ofmkdirat(AT_FDCWD, "dest", ...).options.c::server_options():if (io_timeout) { asprintf(&arg, "--timeout=%d", io_timeout); args[ac++] = arg; }.Test plan
long_flags_timeout_extracts_value- parser captures--timeout=10.known_flag_detects_timeout-is_known_server_long_flagreturns true for--timeout=0/10/300.parse_server_args_iconv_and_timeout_strip_to_dest- end-to-end regression mirroring the exact strace arg sequence:--server -vlogDtpre.iLsfxCIvu --iconv=ISO-8859-1 --timeout=10 . dest/-> assertspos_args == [dest/].IconvSetting::parse+resolve_converter(Latin-1 -> non-identity converter).upstream: options.c -
server_options()emits--iconv=...(2716-2723) and--timeout=%d(io_timeout)