Fix reinstall not detecting changed permissions by itself

[MetRonnie]

This fixes a bug where cylc reinstall in interactive mode would not pick up on permissions changes for files.

The reason for this is that rsync does not report files as included to be sent if only the permissions have been changed, unless %i is added to the --out-format option. %i is an itemized changes summary.

https://linux.die.net/man/1/rsync, https://man.freebsd.org/cgi/man.cgi?query=rsync

The "%i" escape has a cryptic output that is 11 letters long. The general format is like the string YXcstpoguax, where Y is replaced by the type of update being done, X is replaced by the file-type, and the other letters represent attributes that may be output if they are being modified.

I have hidden this itemized changes summary unless using cylc reinstall -v or even higher verbosity.

Check List

• I have read CONTRIBUTING.md and added my name as a Code Contributor.
• Contains logically grouped changes (else tidy your branch by rebase).
• Does not contain off-topic changes (use other PRs for other changes).
• No dependency changes
• Tests are included
• Changelog entry included if this is a change that can affect users
• No docs needed
• If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

[MetRonnie]

This seemed to be duplicating the work of format_rsync_output(), but removing all cannot delete non-empty directory: lines, not just the opt dir

[oliver-sanders]

The point of the +++ prefix/suffix will have been to separate intended rsync output from any unexpected "junk" that might be output by the command / shell, ensuring that we only display what we need, even if the output differs from expectation in other ways.

Why did you remove this? Just didn't like the look of it?

Remember that this code is used for more than just the dry-run check.

[MetRonnie]

From the context I could gather in #5125, the only unwanted output that was being targeted was cannot delete non-empty directory:. This is implemented in format_reinstall_output()

[oliver-sanders]

This rsync command doesn't just apply to the dry-run check (which is passed throughformat_rsync_output). And this prefix stripped out other potential output.

[MetRonnie]

cylc-flow/cylc/flow/install.py

Lines 226 to 227 in 48d3d38

	# Strip unwanted output.
	stdout = ('\n'.join(re.findall(r'\+\+\+(.*)\+\+\+', stdout))).strip()

is at odds with

cylc-flow/cylc/flow/scripts/reinstall.py

Lines 346 to 348 in 48d3d38

	else:
	# other uncategorised log line
	lines.append(line)

The former seems to have been implemented to cover cannot delete non-empty directory:, there was no mention of any other potential rsync output in #5125. And the latter seems to have been implemented to deliberately include other potential rsync output in case the rsync implementation results in non-standard output format.

Hence I am not sure we should be stripping other potential rsync output.

[oliver-sanders]

It's not at odds.

• The +++ prefix ensures that only the intended output lines get through.
• The else handles intended output lines that don't match send or del..

[MetRonnie]

cannot delete non-empty directory: does not have +++ pre/suffixed to it (it is orthogonal to the %o %n%L output format). It is always being stripped. Only lines that match send or del. have +++ pre/suffixed anyway.

[MetRonnie]

The only case I can think of where some other potential output would make it through both bits of code is if the %o output format resulted in something other than send or del.. But that seems unlikely

[wxtim]

If you don't think it should happen, then perhaps a message warning that this is an unexpected rsync implementation, please contact the dev team might be appropriate.

[wxtim]

[should not block/nice to have]
If nothing has changed except permissions can get this info from the rsync output and pass it on to the user?

[wxtim]

• Tested manually
• Read
• Made 2 non-blocking suggestions

[oliver-sanders]

Do we want to include %i in the logs?

• If this is only really relevant to the dry-run functionality, then continue to override the CLI argument as we did before (no need to change here).
• If we think we should have this in the logs, then to keep them readable it might be better to keep the operation at the start of the line?

[MetRonnie]

The rsync output isn't included in the reinstall logs. The only thing we do with the rsync output is print it to stdout in the dry-run case.

[oliver-sanders]

I think it is, install a workflow, make changes, then check ~/cylc-run/<workflow>/log/install/*.

[MetRonnie]

Ah ok - it's only included in the 01-install.log. So I should make this option with %i only apply to reinstall. But it is not included in the reinstall logs regardless.