-
Notifications
You must be signed in to change notification settings - Fork 68
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
dsync/dcmp: support symlinks targets changes #618
base: main
Are you sure you want to change the base?
Conversation
Is there anything I can do to help you merge this PR? |
Salut Rémi, Eric |
Salut Eric, Oops, I missed that requirement, I just force-pushed the commit signed with my GPG key. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this contribution!
I have a couple of questions about the code.
Have you done any testing to determine the performance impact of this change? I agree with the change generally, but if the impact is significant we should warn users in the release notes.
88f2fb5
to
45e558b
Compare
Hello Eric, Olaf, Thank you for this review! I pushed an update with some changes to address your concerns. Let me know if there is anything wrong left!
TBH, no I didn't because personally I don't have access to an infrastructure where I can run significant performance tests… |
Update dsync and dcmp to detect a symlinks targets changes. In this case, the symlinks are reported to be different by dcmp and are updated by dsync. New function mfu_compare_symlinks() is introduced in library API to factorize the logic for all commands. Signed-off-by: Rémi Palancher <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
Eric, please do some brief before/after performance testing. Thanks! |
Update
dsync
anddcmp
to detect a symlinks targets changes. In this case, the symlinks are reported to be different bydcmp
and are updated bydsync
.Note this works for
dsync
both with or withoutc, --contents
.New function
mfu_compare_symlinks()
is introduced in library API to factorize the logic for all commands.Method
In order to reproduce the bug and validate the patch, I wrote this little script
symlink.sh
:Before
Script output:
The content in files tree:
You can see that
link2
still points tofile2
despite the seconddsync
run.After
Script output:
The content in files tree:
You can see that
link2
now points tofile1
after the seconddsync
run.Note
I would like to emphasize that this work is sponsored by @cea-hpc.
fix #412