Re: [PATCH proxmox-backup] fix #7024: pxar: show archive summary for legacy and data mode

[Christian Ebner <c.ebner@proxmox.com>]

Hi,

thanks for the patch, the fix tag is however not justified for this 
single patch IMHO. It only covers the first part of the enhancements 
requested in the bugzilla issue.

On 6/2/26 3:14 PM, Shan Shaji wrote:
> earlier, the archiver summary was only being shown when the change
> detection mode is metadata. To improve this, log the total number of
> files and the amount of data re-encoded for legacy and data modes
> as well.
> 
> Signed-off-by: Shan Shaji <s.shaji@proxmox.com>
> ---
> 
>   **note**: will send follow up patches for the verbose option and logs
>   improvement.

If possible, please send them together with this as patch series. It 
would be beneficial for review to get the full picture of changes 
already, e.g. with respect to variable and struct naming. If there are 
still open questions which need to be discussed, feel free to send the 
patches as RFC series.

> 
>   pbs-client/src/pxar/create.rs | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
> 
> diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
> index 304de4d83..c828db790 100644
> --- a/pbs-client/src/pxar/create.rs
> +++ b/pbs-client/src/pxar/create.rs
> @@ -348,7 +348,20 @@ where
>               ),
>               archiver.reuse_stats.partial_chunks_count,
>           );
> +    } else {
> +        info!("Processing summary: ");

nit: no need for the extra space after the colon. Also, not sure it is 
warranted to show this output like a list, given it does contain only a 
single item. OTOH it makes it easy to find the results, so no strong 
opinion on this one.

> +
> +        let total_files =
> +            archiver.reuse_stats.files_reencoded_count + archiver.reuse_stats.files_hardlink_count;

nit: this variable is only used to display the count in the log output 
below, just inline it there (analogous to the metadata mode).

Further, `ReuseStats` for the struct and reuse_stats for the archiver 
field are now not fitting their use-case anymore. So it would be best to 
rename them to better describe what they are used for. Maybe 
`ProgressStats` or `ProcessingStats`?

> +
> +        info!(
> +            "- {} total files ({} hardlinks) with {} data",
> +            total_files,
> +            archiver.reuse_stats.files_hardlink_count,
> +            HumanByte::from(archiver.reuse_stats.total_reencoded_size),
> +        );
>       }
> +
>       Ok(())
>   }
>