# net/rsync — "File system flags" patch: 3.5.0-test11 port review

Review of the port of the FreeBSD **File system flags** patch (`--file-flags`,
`--force-change`) from rsync 3.4.4 to rsync 3.5.0-test11, plus the corrections
made.

Reference for intended behaviour: `files/split/*.orig.patch` (the 3.4.4 form).
Pristine sources compared against: `/root/rsync-3.4.4`, `/root/rsync-v3.5.0-test11`.

---

## 1. Summary of the file inventory

### Added (5)

| File | Purpose |
|---|---|
| `files/patch-sender.c.orig.patch` | `--force-change` for `--remove-source-files` on the sender's fd path |
| `files/patch-rsync.1.md.orig.patch` | documents the new options; drives `--help` via `help-from-md.awk` |
| `files/patch-rsync.1.orig.patch` | the pre-generated nroff man page (see §5 — it is *not* regenerated at build time) |
| `files/patch-testsuite_file-flags__test.py` | new regression test `testsuite/file-flags_test.py` (creates the file: `@@ -0,0 +1,274 @@`) |
| `rsync_ff_changes.md` | this document |

### Updated (19)

All 19 pre-existing `files/patch-*.orig.patch` were regenerated from the work
tree. **6 have real content changes:**

`patch-syscall.c`, `patch-rsync.c`, `patch-receiver.c`, `patch-delete.c`,
`patch-generator.c`, `patch-util1.c`

**13 are byte-regenerated only — no semantic change:**

`patch-backup.c`, `patch-compat.c`, `patch-flist.c`, `patch-log.c`,
`patch-main.c`, `patch-options.c`, `patch-rsync.fns`, `patch-rsync.h`,
`patch-t_chmod_secure.c`, `patch-t_rename_secure.c`, `patch-t_stub.c`,
`patch-usage.c`, `patch-xattrs.c`

These 13 differ on disk only in the diff headers: regeneration used plain
`diff -u`, so the `.orig` timestamps are gone and the `@@` hunk headers no
longer carry the trailing function name. The source files they patch were
never edited, so the applied result is identical. If you prefer to minimise
the diff, these 13 can be reverted to their previous contents with no effect
on the build.

### Deleted

**None.** `files/split/` (15 files) and the four `prepatch-*` files in the port
root are untouched — the `prepatch-*` files were excluded from this review as
instructed and were not used as input.

---

## 2. Background: the reference patch is itself incomplete

`files/split/` cannot compile on its own. After applying all 15 of its patches
to pristine 3.4.4, `delete.c`, `generator.c` and `receiver.c` still call
`do_chmod_at()` / `do_rename_at()` with the pre-patch arity, while the patched
`syscall.c` has changed those signatures. The split set has no hunks for those
three files.

That is why the port had to invent `patch-delete.c`, `patch-generator.c` and
`patch-receiver.c` from scratch, and it is the origin of most of the defects
below: those three were written as minimal arity fix-ups, so the behaviour the
patch is supposed to provide was never wired into them.

---

## 3. Defects found and fixed

### 3.1 `syscall.c` — `do_chmod_at()` retry followed symlinks

Both force-change retry paths re-issued the chmod as:

```c
-	ret = fchmodat(dfd, bname, mode & CHMOD_BITS, 0);
+	ret = do_fchmodat_nofollow(dfd, bname, mode);
```

The primary attempt uses `do_fchmodat_nofollow()`, which refuses a leaf
symlink (`ELOOP`). The retry used `fchmodat(..., 0)`, which **follows** it.
Under `--force-change` that reopened the leaf-symlink TOCTOU that 3.5.0 added
`do_fchmodat_nofollow()` — and `t_chmod_secure.c` scenario E — to close.

Also dropped the port's added `& CHMOD_BITS` on the two *primary* calls:
`do_fchmodat_nofollow()` already does `mode &= CHMOD_BITS` internally, so it
was redundant noise.

### 3.2 `syscall.c` — `do_rename_at()` missed a call site

3.5.0 has two `renameat()` call sites: one in the `operator_path_resolve`
branch and one in the general branch. Only the second received the
force-change retry, so `--force-change` renames over an immutable destination
(`--backup-dir`, `--temp-dir`, `--partial-dir`) still failed with `EPERM`.
Added the retry to the operator branch.

### 3.3 Literal `0` where `NO_FFLAGS` was required

`delete.c`, `generator.c`, `receiver.c` passed `0` as the `file_flags`
argument. This is not a harmless placeholder — it silently disables
force-change at that call site:

* `NO_FFLAGS` → the wrapper `lstat()`s the target and discovers its real flags.
* `0` → skips the lookup, then `make_mutable(path, mode, 0, force_change)`
  evaluates `!(0 & iflags)` and returns 0, so no retry ever happens.

### 3.4 Whitespace / formatting regressions

* `rsync.c`: `make_mutable()` and `undo_make_mutable()` were indented with
  spaces where the file uses tabs; the `set_file_flags()` call in
  `finish_transfer()` was not indented under its `if`.
* `syscall.c`: mangled indentation in the `do_unlink_at()` operator branch and
  the `do_rmdir_at()` general branch; stray blank lines in `do_rmdir()` and
  `do_lchown_at()`; `#  ifdef`/`#  endif` normalised to `#ifdef`/`#endif`
  where the nesting depth did not call for the indented form.

---

## 4. Incomplete work completed

### 4.1 3.5.0's fd fast paths had no force-change support

3.5.0 routes most receiver work through descriptor-relative wrappers that the
3.4.4 patch never knew about, and whose signatures did **not** change — so the
compiler could not flag the omission:

`do_chmod_atfd`, `do_lchown_atfd`, `do_rename_atfd`, `do_unlink_atfd`,
`do_fchmod`, `do_fchown`, `set_times_at`

Whenever a held dirfd was available — the common case for an in-tree transfer
— `--force-change` silently did nothing.

These wrappers only receive `(dfd, name)`, so they have no path to hand to
`chflags(2)`. Rather than add an `fchflags` autoconf probe and an
`openat()`-based flag flip (which would need special handling for FIFOs and
devices), the fix retries through the corresponding **full-path** wrapper,
which already carries the tested force-change logic, and only on
`EPERM` with `force_change` set:

| File | Site | Falls back to |
|---|---|---|
| `rsync.c` | `set_file_attrs()` chown (`do_fchown` / `do_lchown_atfd`) | `do_lchown_at()` |
| `rsync.c` | `set_file_attrs()` chmod (`do_fchmod` / `do_chmod_atfd`) | `do_chmod_at()` |
| `delete.c` | `del_chmod()` | `do_chmod_at()` |
| `delete.c` | `delete_item()` rmdir | `do_rmdir_at()` |
| `generator.c` | `gen_entry_chmod()` | `do_chmod_at()` |
| `generator.c` | `gen_entry_rename()` | `do_rename_at()` |
| `generator.c` | `gen_entry_unlink()` | `do_unlink_at()` |
| `generator.c` | `gen_entry_set_times()` | `set_times()` (returns `-2` to trigger the tier walk) |
| `util1.c` | `robust_rename()` | `do_rename_at()` |
| `sender.c` | `--remove-source-files` unlink | `do_unlink()` |

`delete.c`'s `del_unlink()` needed no change: it already falls back to
`robust_unlink()`, which calls `do_unlink_at()`.

`extern int force_change;` was added to `rsync.c`, `delete.c`, `generator.c`
and `sender.c`; `extern int preserve_file_flags;` to `generator.c`.

### 4.2 The itemize flags column was dead code

`rsync.h` defines `ITEM_REPORT_FFLAGS`, `log.c` prints an `f` column for it,
and the testsuite's `all_plus`/`allspace`/`dots` were widened to match — but
**nothing ever set the bit**, and `unchanged_attrs()` never compared
`st_flags`. Consequences:

* a flags-only difference was not detected at all, so `--file-flags` could not
  correct the flags on an already-synced file;
* the new `f` column could never be anything but `.`.

The generator hunk that sets it was lost when the patch was split. Added to
`generator.c`:

```c
/* unchanged_attrs() */
#ifdef SUPPORT_FILE_FLAGS
	if (preserve_file_flags && sxp->st.st_flags != F_FFLAGS(file))
		return 0;
#endif

/* itemize() */
#ifdef SUPPORT_FILE_FLAGS
	if (preserve_file_flags && !S_ISLNK(file->mode)
	 && sxp->st.st_flags != F_FFLAGS(file))
		iflags |= ITEM_REPORT_FFLAGS;
#endif
```

This was found by the new test, not by reading the patch.

### 4.3 Protocol / bit-allocation check (no change needed)

Verified that 3.5.0 has not reused the bits the patch claims:
`XMIT_RESERVED_16` is still reserved-for-fileflags, `ITEM_REPORT_*` bit
`(1<<9)` and `ATTRS_*` bit `(1<<4)` are both still free. `patch-rsync.h` and
`patch-flist.c`/`patch-compat.c` are correct as ported.

---

## 5. Documentation

`--file-flags` and the `--force-*` family appeared in no man page and no
`--help` output. `files/split/` has no `rsync.1.md` hunk either, so this was
not a port regression — but the options were undiscoverable.

**Two files are patched, deliberately.** The port passes `--disable-md2man`,
and `maybe-make-man` probes `md-convert`, which cannot run on a normal build
host (no `python3` in `PATH`; no `cmarkgfm`/`commonmark` module; and
`PYTHON_NO_DEPENDS=yes` means python is only a `TEST_DEPENDS`). So `rsync.1`
is **not** regenerated from `rsync.1.md` at build time:

* `patch-rsync.1.md` → feeds `help-rsync.h` via `help-from-md.awk`, so it is
  what changes `rsync --help`.
* `patch-rsync.1` → the pre-generated nroff that is actually installed,
  hand-written to match the generator's own style (`.IP "\fB\-\-opt\fP"`,
  `\fB…\fP`, `\-` escapes, bare `.IP` between paragraphs of one entry).

⚠️ **These two are now maintained in parallel, not derived.** A future edit to
one must be mirrored in the other until `md-convert` can run at build time.

Documented (matching what `options.c` actually implements, including the
`--force-delete` alias the patch adds):

```
--file-flags             preserve file flags (aka chflags)
--force, --force-delete  force deletion of directories even if not empty
--force-change           affect user-/system-immutable files/dirs
--force-uchange          affect user-immutable files/dirs
--force-schange          affect system-immutable files/dirs
```

Full entries were written for `--file-flags`, `--force-change`,
`--force-uchange` and `--force-schange`; `--force` gained a paragraph
explaining the `--force-delete` synonym; `--archive` gained a note that `-a`
does not imply `--file-flags`. `--force` was kept as the primary heading name
so the two existing `[--force](#opt)` cross-references keep resolving.

The entries state the two real constraints: system-immutable flags need root
*and* securelevel < 1 (so `--force-uchange` is the variant usable in
multi-user mode), and the immutable-parent-directory limitation of §7.1.

---

## 6. New regression test

`testsuite/file-flags_test.py`, shipped as
`files/patch-testsuite_file-flags__test.py`. `runtests.py` discovers tests by
globbing `*_test.py`, so no registration is needed.

Gating — the test skips rather than fails when it cannot run:

1. no `os.chflags` → skip;
2. `rsync -VV` lacks `"file_flags": true` → skip;
3. filesystem probe: a flag must be settable **and clearable**. This matters —
   this host's ZFS stores `schg`/`sappnd` but rejects `uchg`/`uappnd`, and at
   securelevel ≥ 1 the system flags are one-way, which would strand the
   scratch tree. Flags are also cleared via `atexit`.

Coverage:

| # | Assertion |
|---|---|
| 1 | `--file-flags` propagates the immutable flag to the receiver |
| 2 | plain `-a` does not (opt-in) |
| 3 | an immutable destination blocks an update without `--force-change` |
| 4 | `--force-change` pushes the update through |
| 5 | `--delete` alone leaves an immutable file; `--force-change --delete` removes it |
| 6 | an immutable *parent directory* still blocks deletion (see §7.1) |
| 7 | a flags-only change is itemized with an `f` column |
| 8 | a second `--file-flags` pass is a no-op |

Test 6 pins a known limitation rather than asserting a fix. If someone teaches
the patch to handle immutable parents, test 6 fails and should be rewritten
into a positive assertion — the failure message says so.

---

## 7. Known limitations and open items (not changed)

### 7.1 `--force-change` cannot descend into an immutable directory

The patch clears the immutable flag on the object it is about to touch, but
unlinking a file also needs write permission on its **parent**, and nothing
makes an immutable parent mutable first. This is explicit in the 3.4.4
original, in `do_unlink()`:

```c
/* TODO: handle immutable directories */
```

Left as-is: fixing it would add behaviour the reference patch never had.
Documented in the man page and pinned by test 6.

---

## 8. Verification

All runs are full `make clean && make test` cycles in `/usr/ports/net/rsync`.

| Check | Baseline (as received) | After changes |
|---|---|---|
| `make test` | 267 passed, 78 skipped, exit 0 | **268 passed, 78 skipped, exit 0** |
| compiler warnings | 11 | 10 |
| failed / fuzzy hunks | — | 0 |
| `rsync --help` lists new options | no | yes |
| `mandoc -T lint rsync.1` | 282 | no new warnings attributable to the patch¹ |

¹ The single extra lint line (`input text line longer than 80 bytes:
/usr/local/etc/rsync…`) comes from the port's own `post-patch` `ETCDIR`
substitution lengthening a pre-existing line; the man page patch touches no
path lines.

The eliminated warning is the `receiver.c` comma-operator bug of §3.1. No new
warnings were introduced in any patched file. The remaining `./syscall.c`
warnings (`unused function 'abspath_step'`, etc.) come from the
`t_chmod_secure`/`t_rename_secure` harness compilation units and are
pre-existing upstream.

Testing is functional, not merely a compile check: this host is FreeBSD 15.0
with a working `chflags(2)`, so the new test exercises real immutable-flag
behaviour end to end.

### Not verified here

* `uchg`/`uappnd` (user-immutable) behaviour on a filesystem that stores them —
  this host's ZFS rejects them, so the test selected `schg`. The code path is
  identical (`--force-uchange` differs only in the flag mask it sets).
* The rsync 3.4.4 reference could not be built for a side-by-side behavioural
  comparison: its `configure` fails in this environment while generating
  `config.status`. The comparison was therefore made against the patch text
  and the pristine sources.