Re: [pbs-devel] [PATCH proxmox-backup v2] client/pull: log snapshots that are skipped because of time

[Dominik Csapak <d.csapak@proxmox.com>]

On 6/4/21 15:54, Dietmar Maurer wrote:
> 
>> +impl std::fmt::Display for SkipInfo {
>> +    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
>> +        if self.count > 1 {
>> +            write!(
>> +                f,
>> +                "{} snapshots ({}..{}) that are older than the newest local snapshot",
>> +                self.count,
>> +                proxmox::tools::time::epoch_to_rfc3339_utc(self.oldest)
>> +                    .map_err(|_| std::fmt::Error)?,
>> +                proxmox::tools::time::epoch_to_rfc3339_utc(self.newest)
>> +                    .map_err(|_| std::fmt::Error)?,
>> +            )
> 
> what is the purpose of this complex message (why we want to show self.oldest and self.newest)?
> Its confusing me more than it helps...

was the suggestion from Fabian, and i did like the idea to tell the user
*which* snapshots were skipped (and if we only have one line,
there is not many ways to represent that)

> 
>> +        } else if self.count == 1 {
>> +            write!(
>> +                f,
>> +                "1 snapshot ({}) that is older than the newest local snapshot",
>> +                proxmox::tools::time::epoch_to_rfc3339_utc(self.oldest)
>> +                    .map_err(|_| std::fmt::Error)?,
>> +            )
> 
> do we really need this special case?

if we want to keep the info which snapshots are skipped, then
yes imho, otherwise we have line such as

1 snapshots (X..X) that are older than the newest local snapshot

which is grammatically wrong (1 snapshots are older)
and contains redundant info (X..X)


> 
>> +        } else {
>> +            write!(f, "0 snapshots")
> 
> Instead, I would avoid to call this function if count is 0 ...

i already avoid it calling below, but i wanted to implement this,
in case we reuse that struct somewhere else

> 
>> +        }
>> +    }
>> +}
>> +
>>   pub async fn pull_group(