Re: [pbs-devel] [PATCH proxmox-backup v2 1/5] api2: make Remote for SyncJob optional

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

Just minor things.

On Thu, Feb 23, 2023 at 01:55:36PM +0100, Hannes Laimer wrote:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>  pbs-api-types/src/jobs.rs         |  4 +-
>  src/api2/config/remote.rs         |  2 +-
>  src/api2/config/sync.rs           | 41 +++++++++++++------
>  src/api2/node/tasks.rs            |  4 +-
>  src/api2/pull.rs                  | 68 +++++++++++++++++++++++--------
>  src/server/email_notifications.rs | 16 ++++----
>  6 files changed, 93 insertions(+), 42 deletions(-)
> 
> diff --git a/src/api2/pull.rs b/src/api2/pull.rs
> index b2473ec8..bb8f6fe1 100644
> --- a/src/api2/pull.rs
> +++ b/src/api2/pull.rs
> @@ -64,7 +75,7 @@ impl TryFrom<&SyncJobConfig> for PullParameters {
>          PullParameters::new(
>              &sync_job.store,
>              sync_job.ns.clone().unwrap_or_default(),
> -            &sync_job.remote,
> +            sync_job.remote.clone().as_deref(),

^ unnecessary .clone()

>              &sync_job.remote_store,
>              sync_job.remote_ns.clone().unwrap_or_default(),
>              sync_job
> @@ -75,7 +86,6 @@ impl TryFrom<&SyncJobConfig> for PullParameters {
>              sync_job.remove_vanished,
>              sync_job.max_depth,
>              sync_job.group_filter.clone(),
> -            sync_job.limit.clone(),
>          )
>      }
>  }
> diff --git a/src/server/email_notifications.rs b/src/server/email_notifications.rs
> index b3298cf9..31a46b0f 100644
> --- a/src/server/email_notifications.rs
> +++ b/src/server/email_notifications.rs
> @@ -486,15 +486,15 @@ pub fn send_sync_status(
>          }
>      };
>  

You can declare a binding here to:
    let tmp_src_string;
> +    let source_str = if let Some(remote) = job.remote.clone() {

You can borrow here instead of cloning.

> +        format!("Sync remote '{}'", remote)

You can assign `tmp_src_string` here, and return &tmp_src_string from
the block.  (and remote can go into the `{}` portion)
        let tmp_src_string = format!("Sync remote '{remote}'");
        &tmp_src_string

> +    } else {
> +        format!("Sync local")

And drop the `format!()` here altogether and just use "Sync local".

With the `tmp_src_string` being outside the `if` scope this will allow
the string w/o a remote to just reference the `&'static str` instead of
allocating.

> +    };
> +
>      let subject = match result {
> -        Ok(()) => format!(
> -            "Sync remote '{}' datastore '{}' successful",
> -            job.remote, job.remote_store,
> -        ),
> -        Err(_) => format!(
> -            "Sync remote '{}' datastore '{}' failed",
> -            job.remote, job.remote_store,
> -        ),
> +        Ok(()) => format!("{} datastore '{}' successful", source_str, job.remote_store,),
> +        Err(_) => format!("{} datastore '{}' failed", source_str, job.remote_store,),
>      };
>  
>      send_job_status_mail(email, &subject, &text)?;
> -- 
> 2.30.2