rsync >= 3.2.4 quotes arguments itself, making unison fail when it quotes arguments

[rrthomas]

See the discussion of --old-args in rsync(1) for how to handle this; also:

If you use scripts that have been written to manually apply extra quoting to the remote rsync args (or to require remote arg splitting), you can ask rsync to let your script handle the extra escaping. This is done by either adding the --old-args option to the rsync runs in the script (which requires a new rsync) or exporting RSYNC_OLD_ARGS=1 and RSYNC_PROTECT_ARGS=0 (which works with old or new rsync versions).

Or copyquoterem's default could be made to depend on the rsync version.

[rrthomas]

(On reflection this issue must surely have been reported already, as rsync 3.2.4 was released nearly a year ago; but I have searched closed issues and can't find a similar one! I also did a quick scan of the git commit messages, where rsync is hardly ever mentioned.)

[gdt]

More importantly, bugs in rsync are not bugs in unison!! But seriously: looks like you opened an issue in wrong repo.

[rrthomas]

@gdt, as far as I can tell, this is a deliberate change to rsync, not a bug. (It is also being backported to earlier versions of rsync in security updates to e.g. Ubuntu. See https://bugs.launchpad.net/ubuntu/+source/rsync/+bug/2009706 )

Which is the right repo to file this issue in? It's is a problem with unison, and this seems to be the repo for unison.

[gdt]

If it's a unison issue, please make it clear what is wrong in unison, and which version of unison you find the bug in, and a reproduction recipe. My system has rsync 3.2.7 and I have seen no problems. I don't claim there are none -- but it's necessary to explain what the problem is.

[rrthomas]

Sure thing, sorry @gdt!

Version info:

$ unison -version
unison version 2.52.1 (ocaml 4.08.1)
$ rsync --version
rsync  version 3.2.7  protocol version 31
…

Unison setup:

$ cat .unison/test.prf
root = /home/rrt/tmp
root = ssh://localhost//home/rrt/tmp2
copythreshold = 0
$ pwd
/home/rrt
$ ls -l tmp
total 4.7M
-rw-r--r-- 1 rrt rrt 4.7M Mar  5 22:33 foo.opus
$ ls -l tmp2
total 0

First try with default options fails:

$ unison -confirmbigdel -batch -ui text test
Unison 2.52.1 (ocaml 4.08.1): Contacting server...
Connected [//dwks//home/rrt/tmp -> //dwks//home/rrt/tmp2]

Looking for changes
  Waiting for changes from server
Reconciling changes
new file ---->            foo.opus  
local        : new file           modified on 2023-03-05 at 22:33:24  size 4891675   rw-r--r--
dwks         : absent
Propagating updates
Unison 2.52.1 (ocaml 4.08.1) started propagating changes at 00:21:53.27 on 10 Mar 2023
[BGN] Copying foo.opus from /home/rrt/tmp to //dwks//home/rrt/tmp2
rsync --partial --inplace --compress '/home/rrt/tmp/foo.opus' 'localhost:'\''/home/rrt/tmp2/.unison.foo.opus.2df6cd84726ef48ed510863854360572.unison.tmp'\'''
Failed: External copy program did not create target file (or bad length): .unison.foo.opus.2df6cd84726ef48ed510863854360572.unison.tmp
Failed [foo.opus]: External copy program did not create target file (or bad length): .unison.foo.opus.2df6cd84726ef48ed510863854360572.unison.tmp
Unison 2.52.1 (ocaml 4.08.1) finished propagating changes at 00:21:53.84 on 10 Mar 2023, 0.577 s
Saving synchronizer state
Synchronization incomplete at 00:21:53  (1 item transferred, 0 skipped, 1 failed)
  failed: foo.opus

Second try with -copyquoterem false succeeds:

$ unison -copyquoterem false -confirmbigdel -batch -ui text test
Unison 2.52.1 (ocaml 4.08.1): Contacting server...
Connected [//dwks//home/rrt/tmp -> //dwks//home/rrt/tmp2]

Looking for changes
  Waiting for changes from server
Reconciling changes
new file ---->            foo.opus  
local        : new file           modified on 2023-03-05 at 22:33:24  size 4891675   rw-r--r--
dwks         : absent
Propagating updates
Unison 2.52.1 (ocaml 4.08.1) started propagating changes at 00:23:38.24 on 10 Mar 2023
[BGN] Copying foo.opus from /home/rrt/tmp to //dwks//home/rrt/tmp2
rsync --partial --inplace --compress '/home/rrt/tmp/foo.opus' 'localhost:/home/rrt/tmp2/.unison.foo.opus.2df6cd84726ef48ed510863854360572.unison.tmp'
[END] Copying foo.opusUnison 2.52.1 (ocaml 4.08.1) finished propagating changes at 00:23:38.91 on 10 Mar 2023, 0.671 s
Saving synchronizer stateSynchronization complete at 00:23:38  (1 item transferred, 0 skipped, 0 failed)

[tleedjarv]

Looking at https://repology.org/project/rsync/versions (a summary can be seen at https://repology.org/project/rsync/badges ), it seems to me that the easiest course of action is to make copyquoteterm false by default; and possibly mention in the release notes that users of rsync < 3.2.4 beware. This pref is currently special-cased to be true for rsync only and clearly that special-casing is no longer needed.

[rrthomas]

I have implemented a more general solution to this problem in #866, which obviates the need for copyquoterem and argument quoting by calling the external program without a shell.

[gdt]

I wonder if we have unison check the rsync version (once, when rsync is first used) and do the right thing. rsync from a year ago is going to be present on some fraction of unison installs, and certainly many will have it and many will not.

[tleedjarv]

I wonder if we have unison check the rsync version (once, when rsync is first used) and do the right thing. rsync from a year ago is going to be present on some fraction of unison installs, and certainly many will have it and many will not.

Not worth it. This issue will only bite those users who make an active choice to use external rsync as the copyprog. I'd imagine that nobody's really been bit by this issue so far, also evidenced by https://github.com/bcpierce00/unison/issues/865#issuecomment-1462909142

[rrthomas]

Not worth it. This issue will only bite those users who make an active choice to use external rsync as the copyprog. I'd imagine that nobody's really been bit by this issue so far, also evidenced by #865 (comment)

You only have to set copythreshold to run into the problem; you don't have to set copyprog or copyprogrest. That seemed a fairly low bar to me (and I set copythreshold=1000 myself for improved performance, I think; it's been a while since I did it, so I don't remember exactly!).

[tleedjarv]

You only have to set copythreshold to run into the problem; you don't have to set copyprog or copyprogrest. That seemed a fairly low bar to me (and I set copythreshold=1000 myself for improved performance, I think; it's been a while since I did it, so I don't remember exactly!).

Fair enough. But on that note, I think copyprog should be used for very special tricks only. Using it plainly for copying (via copythreshold) instead of Unison's own copying is probably just messy and more fragile without any direct benefits. I know that during the early days of Unison it was seen as way for improved performance but I very much doubt it is still the case today. Then again, I don't tend to sync large VM images that often.

I haven't checked this but my feeling is that normal users should always steer clear of external copyprogs and all these preferences should be moved into the "Expert" category. I'd be happy if someone had real hard data to base this decision on (one way or another then).

[rrthomas]

I think copyprog should be used for very special tricks only. Using it plainly for copying (via copythreshold) instead of Unison's own copying is probably just messy and more fragile without any direct benefits. I know that during the early days of Unison it was seen as way for improved performance but I very much doubt it is still the case today. Then again, I don't tend to sync large VM images that often.

I've been using Unison for over 20 years, and I don't think I changed copythreshold until a few years ago. (Unfortunately I can't find any notes about why I did it.) I indeed sync large files from time to time.

I haven't checked this but my feeling is that normal users should always steer clear of external copyprogs and all these preferences should be moved into the "Expert" category. I'd be happy if someone had real hard data to base this decision on (one way or another then).

I agree things should be that way, though I feel that really only rsync should be used, perhaps with special options. I guess Unison makes a lot of assumptions about the properties of a copy program and what it does.

[gdt]

Is there any recent data that shows that this complexity is worth it, vs a null hypothesis that we should just rip out the code for the entire concept?

[rrthomas]

Is there any recent data that shows that this complexity is worth it, vs a null hypothesis that we should just rip out the code for the entire concept?

I just did a lot of searching online and in my mail archive, but without the old mailing list (which sadly I didn't keep all of), I feel hobbled, and I can't find anything explicit about why rsync might be faster than Unison for large files; only plenty of evidence that that's what people use it for. I presume someone on unison-hackers will know. (I just tried subscribing but I get an error about needing to provide a valid email address; I have written to the list owner.)

Why not remove Unison's internal copying, and just use rsync, replacing copyprog and copyprogrest with a single preference to specify extra rsync options? (I bet there are good reasons, but I'd like to know what they are!)

[gdt]

This is turning into a disucssion that belongs on hackers rather than a bug. I will follow up there.

[rrthomas]

This is turning into a disucssion that belongs on hackers rather than a bug. I will follow up there.

I'll be there as soon as my subscription is unblocked!

[gdt]

Fixed by merge of #866