Re: [pbs-devel] [PATCH proxmox-backup v3 03/13] add protected info of snapshots to api and task logs

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

more minor nitpicking

On Wed, Oct 27, 2021 at 01:22:28PM +0200, Dominik Csapak wrote:
> adds the info that a snapshot is protected to:
> * snapshot list
> * manual pruning (also dry-run)
> * prune jobs
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  pbs-api-types/src/datastore.rs |  2 ++
>  pbs-datastore/src/prune.rs     | 40 +++++++++++++++++++++++++++-------
>  src/api2/admin/datastore.rs    | 15 ++++++++-----
>  src/server/prune_job.rs        |  6 ++---
>  tests/prune.rs                 |  4 ++--
>  5 files changed, 49 insertions(+), 18 deletions(-)
> 
> diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
> index 462081e4..77c1258f 100644
> --- a/pbs-api-types/src/datastore.rs
> +++ b/pbs-api-types/src/datastore.rs
> @@ -390,6 +390,8 @@ pub struct SnapshotListItem {
>      /// The owner of the snapshots group
>      #[serde(skip_serializing_if = "Option::is_none")]
>      pub owner: Option<Authid>,
> +    /// Protection from prunes
> +    pub protected: bool,
>  }
>  
>  #[api(
> diff --git a/pbs-datastore/src/prune.rs b/pbs-datastore/src/prune.rs
> index 0eaa8acd..e66ed408 100644
> --- a/pbs-datastore/src/prune.rs
> +++ b/pbs-datastore/src/prune.rs
> @@ -7,7 +7,30 @@ use pbs_api_types::PruneOptions;
>  
>  use super::BackupInfo;
>  
> -enum PruneMark { Protected, Keep, KeepPartial, Remove }
> +#[derive(Clone, Copy, PartialEq, Eq)]
> +pub enum PruneMark { Protected, Keep, KeepPartial, Remove }
> +
> +impl PruneMark {
> +    pub fn keep(&self) -> bool {

It's a tiny `Copy` type, you can drop the `&` above and `*` below in
boht methods.

> +        *self != PruneMark::Remove
> +    }
> +
> +    pub fn protected(&self) -> bool {
> +        *self == PruneMark::Protected
> +    }
> +}
> +
> +impl std::fmt::Display for PruneMark {
> +    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
> +        let txt = match self {

    f.write_str(match self {

> +            PruneMark::Protected => "protected",
> +            PruneMark::Keep => "keep",
> +            PruneMark::KeepPartial => "keep-partial",
> +            PruneMark::Remove => "remove",
> +        };

    })
drop the `write!` usage

> +        write!(f, "{}", txt)
> +    }
> +}
>  
>  fn mark_selections<F: Fn(&BackupInfo) -> Result<String, Error>> (
>      mark: &mut HashMap<PathBuf, PruneMark>,
> @@ -125,7 +148,7 @@ pub fn cli_options_string(options: &PruneOptions) -> String {
>  pub fn compute_prune_info(
>      mut list: Vec<BackupInfo>,
>      options: &PruneOptions,
> -) -> Result<Vec<(BackupInfo, bool)>, Error> {
> +) -> Result<Vec<(BackupInfo, PruneMark)>, Error> {
>  
>      let mut mark = HashMap::new();
>  
> @@ -173,15 +196,16 @@ pub fn compute_prune_info(
>          })?;
>      }
>  
> -    let prune_info: Vec<(BackupInfo, bool)> = list.into_iter()
> +    let prune_info: Vec<(BackupInfo, PruneMark)> = list.into_iter()
>          .map(|info| {
>              let backup_id = info.backup_dir.relative_path();
> -            let keep = match mark.get(&backup_id) {
> -                Some(PruneMark::Keep) => true,
> -                Some(PruneMark::KeepPartial) => true,
> -               _ => false,
> +            let mark = if info.protected {
> +                PruneMark::Protected
> +            } else {
> +                *mark.get(&backup_id).unwrap_or(&PruneMark::Remove)

(could use `.copied()` instead of `*` and `&`)

>              };
> -            (info, keep)
> +
> +            (info, mark)
>          })
>          .collect();
>