Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add independent update mode for symbolic links #198

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

twojstaryzdomu
Copy link

@twojstaryzdomu twojstaryzdomu commented Jul 3, 2021

This PR enhances rsync with an independent update mode for symbolic links. Its main purpose is to skip symlinks that are newer on the receiver. The mode is enabled independently of the update mode via the --update-links switch. To extend the behaviour of this option to empty directories, an additional --allow-link-update-dir switch may be enabled. In this mode, a less recent empty directory on the destination may be replaced with a more recent symbolic link from the source.
The PR includes a sanity test to verify the impact of the proposed modes on a set of files (relative, dangling, absolute, unsafe, devlink dirlink, empty directory, non-empty directory, fifo) under different circumstances.

@twojstaryzdomu
Copy link
Author

The test included in the PR needs further work to run on FreeBSD apparently.

@WayneD
Copy link
Member

WayneD commented Jan 3, 2022

I'll note that tweaking .github/workflows/build.yml to include coreutils on the brew install line should hopefully be enough to fix the test error on macOS.

Copy link
Author

@twojstaryzdomu twojstaryzdomu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The redundant calls to the macro were intentional, commit reverted.

@twojstaryzdomu
Copy link
Author

twojstaryzdomu commented Jan 8, 2022

I'll note that tweaking .github/workflows/build.yml to include coreutils on the brew install line should hopefully be enough to fix the test error on macOS.

Noted and added it in commit c3437c1. The workflow change now awaits approval.

@@ -231,10 +232,15 @@ static int readlink_stat(const char *path, STRUCT_STAT *stp, char *linkbuf)
int link_stat(const char *path, STRUCT_STAT *stp, int follow_dirlinks)
{
#ifdef SUPPORT_LINKS
if (copy_links)
if (copy_links && update_links == 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If they asked for --copy-links they should get --copy-links. Remember that it's only the sending side that sees this flag set, not the receiving side, so combining --copy-links with --update-links would mean that the sender doesn't send any symlinks, and the receiver could deal with any existing symlinks in the destination using the new option. (Thus, drop this change.)

Copy link
Author

@twojstaryzdomu twojstaryzdomu Jan 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two modes appear mutually exclusive from a user standpoint. Either every symlink is transformed into referent (--copy-links) or copied across if newer (--update-links).

so combining --copy-links with --update-links would mean that the sender doesn't send any symlink

From testing the PR'd code briefly now, when both modes are enabled, --update-links trumps --copy-links, by virtue of a subsequent condition at if (update_links && S_ISLNK(stp->st_mode)).

Added #274 to discuss how to handle symlink mode precedence.

STRUCT_STAT st;
if (x_stat(path, &st, NULL) == 0 && !S_ISDIR(st.st_mode))
return x_lstat(path, stp, NULL);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hunk baffles me. It does a stat of the referent file, and if it's a non-dir it re-stats the symlink's stat that is already present in stp and returns. But if it is not a dir, the following code also returns the same stat data that is already in stp. So, it seems like this can be dropped as well.

Copy link
Author

@twojstaryzdomu twojstaryzdomu Jan 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Precisely, if the symlink points at a non-directory referent, return the symlink's stat or fall through. It's only apparent looking at the surrounding code why the x_lstat call is redundant. Committed as aed6235, works on assumption lstat or lstat64 is guaranteed to return either 0 or -1 on all platforms.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants