[pve-devel] [PATCH v2 storage] storage/plugin: factoring out regex for backup extension rey

[Wolfgang Bumiller]

On Mon, Aug 02, 2021 at 12:52:36PM +0200, Lorenz Stechauner wrote:
> Signed-off-by: Lorenz Stechauner <l.stechauner at proxmox.com>
> ---
> changes to v1:
> * factored $compressor_extension_re out of $backup_extension_re
>     should now be less confusing

not sure about less confusing... but I suppose it'll have to do

> 
>  PVE/Storage.pm        | 14 +++++++++-----
>  PVE/Storage/Plugin.pm |  4 ++--
>  2 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
> index c04b5a2..942246f 100755
> --- a/PVE/Storage.pm
> +++ b/PVE/Storage.pm
> @@ -105,6 +105,10 @@ our $iso_extension_re = qr/\.(?:iso|img)/i;
>  
>  our $vztmpl_extension_re = qr/\.tar\.([gx]z)/i;
>  
> +our $compressor_extension_re = qr/\.(${\PVE::Storage::Plugin::COMPRESSOR_RE})/i;
> +
> +our $backup_extension_re = qr/\.(tgz|(?:tar|vma)$compressor_extension_re?)/i;

The reason I don't find it less confusing is that both of these are
globals, and one introduces 1 capture group, the other introduces 2
capture groups.

I'd say "let's just use named capture groups everywhere", but then if we
ever add branch reset patterns (`(?|a|b|c)`) it all falls apart...

I think we should probably add doc comments at least... also maybe
introduce a naming scheme?

$COMPRESSOR_EXTENSION_RE_1
$BACKUP_EXTENSION_AND_COMPRESSION_RE_2

or something? Not sure the numbering is a good idea, but at least naming
one "X_AND_Y" shows there are 2 things involved, and might hint future
editors that adding more groups should also update the name.