Open HaleTom opened 4 years ago
Actually, on reading closely, --inplace
implies partial
anyway:
The option implies --partial (since an interrupted transfer does
not delete the file), but conflicts with --partial-dir and
--delay-updates.
So, the defaults for the two commands may as well be the same:
copyprog = rsync --archive --inplace --compress --no-whole-file --protect-args --sparse
copyprogrest = rsync --archive --inplace --compress --no-whole-file --protect-args --sparse
@HaleTom I am not sure I am following. Can you make a fresh clear statement of what is wrong, relative to 2.51.2 or master?
I observed the same: I am trying to rely on --checksum to retransfer modified files, but with --apend-verify the transfer is skipped while without --apend-verify the file is (correctly) transferred. (rsync version 3.1.3 protocol version 31 on ubuntu 20.04)
So is this a bug in rsync, in that it does the wrong thing with --checksum and --append-verify, or a bug in unison, that it uses --append-verify which is unsafe, or ?
Bug in unison.
Adding --append-verify will miss cases where a file is changed but not increased in size.
At worst the documentation should match the behaviour, with a significant caveat.
Thanks for bearing with me; I think the ticket history now explains thing well enough for me to follow.
The code change here is trivial. I can help and open a PR if someone could explain what are the correct rsync options.
You propose removing --partial
because it is redundant. This seems to be ok (if there's no different behavior between rsync versions).
You propose adding --archive
but that does not match Unison default behavior. I don't think it can be added as a default.
You propose adding --sparse
but rsync manual has this:
Note that versions of rsync older than 3.1.3 will reject the combination of --sparse and --inplace.
I don't think it can be added as a default.
Adding --protect-args
seems reasonable but is also seen as a bit of a security risk (in case of untrusted source, which is probably not the case for Unison users).
--no-whole-file
is difficult to judge for me but as I understand you are basically proposing it as a better (no false negatives) alternative for --append-verify
and not for its intended purpose (but you do find that as an added benefit).
Changing rsync args is complicated and I'm averse to a big rototill without a test plan. But, it seems like dropping --append-verify might be a good step (and fixing the docs to match reality). @HaleTom can you comment on the incremental wisdom of just dropping --append-verify in terms of correctness and performance? Have you tested this? Might you create a PR?
Per list discussion, the scope of this ticket is now just "remove --append-verify
".
1) Feedback timeout on asking about just dropping --append-verify
.
2) The plan is to address #871. Once done, then this ticket no longer matters.
I saw the following output in
ps aux | grep unison
:From the above (and issue #220) it seems that unison is using
--append-verify
by default.This doesn't match what the unison manual says are the defaults:
--append-verify
has an "interesting" edge case.From the
rsync
manual (emphasis mine):The above looks fine... but digging deeper, the entry for
--append
:The upshot is that any files changed to be the same size or smaller will be ignored with the default rsync flags.
Given
--append-verify
(when it works) does a checksum after the transfer, why not just use--no-whole-file
which will also checksum the whole file? Another advantage of--no-whole-file
is where reading data from disk quicker than writing it (which is generally the case).Work around: