fix: skip MSYS2 path conversion for rsync-over-SSH remote paths

[vicjayjay]

Summary

When using MSYS2 rsync with the -e ssh flag and a remote destination containing an MSYS2-style path (e.g., user@host:/c/path), rsync fails with:

rsync error: syntax or usage error (code 1) at main.c(1415) - The source and destination cannot both be remote.

Root Cause

In arg_heuristic_with_exclusions() (winsup/cygwin/path.cc), the MSYS2 path conversion heuristic treats the /c/ portion of user@host:/c/path as a Windows drive-letter path and converts it. The converted path is then passed to SSH, causing rsync on the remote side to misparse the path.

The Fix

Added a check for remote-path syntax before invoking the conversion:

const char *at_sign = strchr(arg, '@');
const char *colon   = strchr(arg, ':');
if (at_sign && colon && at_sign < colon)
  return (char*)arg;

This reliably identifies SCP/rsync remote-path syntax (user@host:/path) and skips path conversion for those arguments. The @: pattern is unique to remote-path syntax — a Windows drive letter never has @ before :.

Why This Fix Is Correct

Argument pattern	@ before : ?	Action
/c/Users/file.txt	NO	Convert (local MSYS2 path)
user@host:/c/path	YES	SKIP conversion (remote path)
ssh://host/path	NO	Not affected (already skipped)

Testing

Before fix:

rsync -az -e "ssh -p 2222" "/c/Users/test/" "user@host:/c/backup/"
# rsync error: source and destination cannot both be remote

After fix:

rsync -az -e "ssh -p 2222" "/c/Users/test/" "user@host:/c/backup/"
# Works correctly

Regression check: Local MSYS2 paths (/c/Users/...) continue to be converted normally.

Issue

Follow-up to: #330

[dscho]

(I fixed the PR base branch, it was targeting master instead of the current default branch, msys2-3.6.7.)

[dscho]

You almost found the correct spot where this new logic needs to live, but it's not quite here. 11 lines further down, the convert() function is called, that is closer to the correct location. The actual location where things like this need to happen is the find_path_start_and_type() function, I think.

If you look carefully, you will see that this function avoids performance hitters like strchr().

You might find inspiration in afa529d how this could be done in a better way. For example, you could initialize char *colon = NULL; just before the loop, then set colon = it; in the case ':' arm, and then add an if (!colon) goto skip_p2w; in the `case '@': arm.

But even so, that logic might very well catch too much, i.e. by mistake prevent legitimate paths from being converted. @ characters are allowed in filenames, after all, and colons can be path separators.

And even if the "@ before :" pattern is probably to broad, at the same time it is also too narrow. You could, after all, have a section like this in your ~/.ssh/config (I have plenty of those in mine):

Host my-machine
HostName xyz.example.com
User vicjayjay

And then you can call rsync abc my-machine:/c/xyz/. In fact, I rarely ever specify my user name explicitly in any SSH address, I pretty much always use those nicknames defined in ~/.ssh/config. And you'll note that your "@ before :" pattern does not handle this situation well...

[vicjayjay]

@dscho Thanks for the detailed review — updated in bdc781d.

Changes:

• Moved the logic from arg_heuristic_with_exclusions() in path.cc into find_path_start_and_type() in msys2_path_conv.cc
• Replaced strchr() scans with in-loop tracking: colon is set on the first ':' encountered, then in case '@' we check if (!colon) goto skip_p2w;
• Removed the old block from path.cc entirely

Limitations acknowledged in the commit message:

• SSH config aliases (my-machine:/path without @) are not covered — we can't reasonably parse ~/.ssh/config from here
• @ in filenames could theoretically false-positive, though @ before the first : in a real Windows/MSYS2 path seems unlikely in practice

Happy to refine further if you have a better approach for the alias case.

[vicjayjay]

CI note: the 4 failed jobs are unrelated to this change:

• CLANG64-clang, UCRT64-clang, CLANGARM64-clang: error: target not found: mingw-w64-clang-aarch64-openmp — the openmp package appears to have been removed from the MSYS2 repos recently. All gcc test variants pass.
• MSYS-gcc: GitHub Actions network timeout (api.github.com:443 unreachable during setup-msys2 download) — transient infrastructure issue.

The build itself succeeded (10m40s) and all gcc-based test jobs (UCRT64-gcc, MINGW64-gcc, MINGW32-gcc) pass cleanly.

[dscho]

• CLANG64-clang, UCRT64-clang, CLANGARM64-clang: error: target not found: mingw-w64-clang-aarch64-openmp — the openmp package appears to have been removed from the MSYS2 repos recently. All gcc test variants pass.

That's indeed a regression. More precisely, the openmp package never existed as such, only as an alias for llvm-openmp and that one was (accidentally?) removed. I opened msys2/msys2-tests#94 to fix that. Once that's merged, we can re-run.

• MSYS-gcc: GitHub Actions network timeout (api.github.com:443 unreachable during setup-msys2 download) — transient infrastructure issue.

Yes, this was transient. GitHub seems to have a rough time lately. I re-ran it, and it passed.

[dscho]

I am still worried that this might mess with path lists, i.e. colon-separated values in the form as e.g. PATH.

Besides, we're not even ensuring that there is a colon here, so even abc@ would match, or @abc, which is clearly unwanted.

Personally, I'd try to match a pattern like this: [sequence-of-alnum-dashes-and-dots]@[sequence-of-alnum-dashes-and-dots]:/[alnum]. That might be overkill, but it would be a lot less likely to introduce inadvertent side effects.

And to be clear: I would not implement this as a state machine, even if that would be the fastest, performance-wise. I would probably introduce a char *at = NULL that, before it is assigned, runs something like:

// Does the string so far consist only of alphanumerical characters, dots or dashes,
// i.e. does it look like a username?
#define ALNUMDD ".-0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"
if (strspn(src, ALNUMDD) == it)
  at = it;

and then, in the : arm, if this is the first : and at != NULL (i.e. the string started with something like <username>@) do the same dance with strspn(at + 1, ALNUMDD) to verify the host part, and then verify the path part via: it[1] == '/' && isalnum(it[2]).

Oh, and I think this needs an addition to: https://github.com/msys2/msys2-tests/blob/main/runtime/path_convert/src/main.cpp

[vicjayjay]

@dscho Updated in 1ffd11f with the stricter pattern you suggested.

What changed:

The loose !colon check is replaced with strict strspn()-based validation:

#define ALNUMDD ".-0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"
const char *at = NULL;

// In case '@': record only if prefix is valid username chars
if (!at && strspn(*src, ALNUMDD) == (size_t)(it - *src))
    at = it;

// In case ':': skip only if hostname is valid AND colon is followed by /[alnum]
if (at && strspn(at + 1, ALNUMDD) == (size_t)(it - at - 1)
    && it + 2 < end && it[1] == '/' && isalnum(it[2]))
    goto skip_p2w;

This matches the pattern [alnum-dot-dash]@[alnum-dot-dash]:/[alnum] — so:

• user@host:/c/path → skipped (valid SCP syntax)
• abc@ → not skipped (at never set, no colon)
• @abc → not skipped (empty prefix fails strspn check)
• file@name with no colon → not skipped
• C:/path@name:/foo → not skipped (prefix contains /, fails username check)
• PATH-style lists like /usr/bin:/usr/local/bin → not skipped (no @)

Re: tests — I'll add cases to msys2-tests/runtime/path_convert/src/main.cpp in a follow-up or as part of this PR if you prefer. Let me know.

Thanks for the openmp fix (msys2/msys2-tests#94) — once that merges the CI should go green.

[dscho]

@vicjayjay wow that was fast. Did you use an AI agent or what 😆

[dscho]

Re: tests — I'll add cases to msys2-tests/runtime/path_convert/src/main.cpp in a follow-up or as part of this PR if you prefer. Let me know.

Since those tests live in a different repository, I prefer the changes in a parallel PR ;-)

[mati865]

@vicjayjay wow that was fast. Did you use an AI agent or what 😆

Yep, you are basically talking with an openclaw agent the whole time.