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

merge_local_folder_manifest should expect local to respect the prevent_sync_pattern #7885

Open
touilleMan opened this issue Aug 2, 2024 · 0 comments · May be fixed by #8710
Open

merge_local_folder_manifest should expect local to respect the prevent_sync_pattern #7885

touilleMan opened this issue Aug 2, 2024 · 0 comments · May be fixed by #8710
Labels
I-Rust Impact: Rust-related stuff

Comments

@touilleMan
Copy link
Member

In parsec v2, merge_local_folder_manifest accept local manifest with arbitrary prevent sync pattern.

The idea was it was a very unlikely event (a new prevent sync pattern have been set and is not fully applied), and in this case a flag could be passed to tell merge_local_folder_manifest to manually apply the new prevent sync pattern to local

# Start by re-applying pattern (idempotent)
if force_apply_pattern and isinstance(
local_manifest, (LocalFolderManifest, LocalWorkspaceManifest)
):

In practice however this was not needed since the prevent sync pattern is only set when starting the WorskpaceFS, which in turn ensure the prevent sync pattern is fully applied as part of it initialization routine

# Apply the current "prevent sync" pattern
await workspace.apply_prevent_sync_pattern()

On top of that, having merge_local_folder_manifest accepting outdated local makes the function significantly harder to test (see https://github.com/Scille/parsec-cloud/pull/7756/files#diff-4e496067608e0a1d8d2067e4121b7465f612c1007dc9f35128cf0054ab3adc99R605-R625)

So we could simplify this by just considering merge_local_folder_manifest only accept local manifest in sync with prevent_sync_pattern (and adding a docstring to this fonction to document this !)

@mmmarcos mmmarcos added the I-Rust Impact: Rust-related stuff label Aug 2, 2024
@vxgmichel vxgmichel linked a pull request Oct 12, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-Rust Impact: Rust-related stuff
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants