Scille / parsec-cloud

Open source Dropbox-like file sharing with full client encryption !
https://parsec.cloud
Other
270 stars 40 forks source link

Move `LocalFolderManifest::from_remote_with_local_context` logic directly into `merge_local_folder_manifest` #7738

Open FirelightFlagboy opened 3 months ago

FirelightFlagboy commented 3 months ago

This is not related to the PR, but I find this method poorly designed: we are both applying remote sync pattern (which is normal since we are dealing with remote data) AND restoring local sync pattern (which by definition reference files that are in local only...)

I'm wondering how this behave in corner cases (e.g. if a new file in the remote manifest match the local prevent sync pattern, and a file with the same name is also in local)

So my guess here is LocalFolderManifest::from_remote_with_local_context shouldn't exist in the first place: it is the job of merge_local_folder_manifest to provide a new local folder manifest when given a remote and a local.

_Originally posted by @touilleMan in https://github.com/Scille/parsec-cloud/pull/7730#discussion_r1682095470_

vxgmichel commented 3 months ago

I'm fine with inlining this method, since it's only called in a single place and it has a very simple implementation:

    pub fn from_remote_with_local_context(
        remote: FolderManifest,
        prevent_sync_pattern: Option<&Regex>,
        local_manifest: &Self,
        timestamp: DateTime,
    ) -> Self {
        Self::from_remote(remote, prevent_sync_pattern).restore_local_confinement_points(
            local_manifest,
            prevent_sync_pattern,
            timestamp,
        )
    }

However, the logic in restore_local_confinement_points has to stay in the local_manifest module to stay consistent with the other methods such as restore_remote_confinement_points, filter_local_confinement_points and such.