Perl script to import flat-file versioned files into Git repositories

Question

I wrote a Perl script to import local files with "File-Ending-Versioning" (like file.1.12.pm) into a Git repository. I tried to use the concept of roles in this script, and I am not sure if I applied the concept right.

The basic idea is that the script must be able to operate in a so-called "dryrun-mode": if invoked with the dryrun option, it should act as if it were doing an import like print log and error messages, but it should not actually create anything.

As the code is quite long, I shortened it to the relevant sections.

First, I have a main script file called git-importer.

#!/usr/bin/perl 
 
use strict;
use warnings;
use version; our $VERSION = qv( '0.3.0' );
use English qw( -no_match_vars );
 
use lib '/our/special/library/path';
use Utils::Cwd qw(abs_path); # oddity of my workplaces environment
 
# use lib './lib' but make it independant of the current directory
use File::Basename;
# move local lib folder to top of @INC
BEGIN { my $prog = unshift( @INC,dirname(abs_path(0ドル))."/lib"); }
 
# those are the modules I use for my scripts behaviour
use Importer;
use Import::Git;
use Import::Git::dryrun;
use Import::Github;
use Import::Github::dryrun;
use Import::File;
use Import::File::dryrun;
 
use Getopt::Long::Descriptive;
use Getopt::Long::Descriptive::Opts;
 
 
my ( $opt, $usage ) = describe_options(
 "Import files into Github Repositories\n\n Usage: %c %o [FILE]",
 [ 'config|c=s', "path to configuration file", { default => '/common/admin/etc/git-importer' } ],
 [ 'sourcefile|s=s@', "required - path to source file. If source is a softlink, this file will be taken as a starting version. ".
 "Module names, like Utils::Test::... are supported if --modules is used", { required => 1 } ],
 [ 'targetdir|t=s', "optional - path to target directory where local repository will be created" ],
 [ 'logfile|l=s', "optional - path to logfile" ],
 [ 'remote|r=s', "optional - ssh clone url of the remote repository" ],
 [ 'dryrun|d', "optional - only perform read actions" ],
 [ 'modules|m ', "optional - Create a module repository", ],
);
 
my $importer = Importer->new();
 
 
my $params = $importer->init_config( $opt->config );
$importer->is_module($opt->modules);
# I did start a code review on a Perlmonks Meetup but we were not able to finish on time. They found it odd that I used a different object for the dryrun. 
$importer->check_permissions();
foreach my $file ( @{ $opt->sourcefile } ) { 
 $file = $importer->test_file( $file, $opt->modules );
 my $o_file;
 if ( $opt->dryrun ) { 
 $o_file = Import::File::dryrun->new( rawpath => $file, is_module => $opt->modules );
 }
 else {
 $o_file = Import::File->new( rawpath => $file, is_module => $opt->modules );
 }
 $importer->add_file( $o_file );
 $importer->add_type( $o_file->type );
}
[...] 
 
my $git; 
if ( $opt->dryrun ) { 
 $git = Import::Git::dryrun->new( git_import_user => $params->{ git_import_user }, targetdir => $params->{ targetdir }, tag => '1' );
} 
else { 
 $git = Import::Git->new( git_import_user => $params->{ git_import_user }, targetdir => $params->{ targetdir }, tag => '1' );
} 
 
[...]

My structure is as follows: I have a lib folder where I placed all my modules in like this:

.
├── Import
│  ├── File
│  │  ├── dryrun.pm
│  │  ├── Role.pm
│  │  ├── Version
│  │  │  └── dryrun.pm
│  │  └── Version.pm
│  ├── File.pm
│  ├── Git
│  │  ├── dryrun.pm
│  │  └── Role.pm
│  ├── Github
│  │  ├── dryrun.pm
│  │  └── Role.pm
│  ├── Github.pm
│  └── Git.pm
└── Importer.pm

So for all of my modules (except Importer.pm itself) there is a .pm, a dryrun.pm and a Role.pm.

I would like to look at Git.pm. The Role looks like this:

 #! /usr/bin/perl
 
 package Import::Git::Role;
 
 use strict;
 use warnings;
 
 use Mouse::Role;
 
 use Carp;
 use English qw( -no_match_vars );
 use File::Path;
 use Storable;
 use Utils::Log;
 
 # functionality that all classes using this Role must implement 
 requires qw(commit rmdir mkdir create_repository choose_repo_name push clone_empty_remote ); 
 
 # attributes of all classes that use this role 
 has 'local' => ( is => 'rw' );
 has 'remote' => ( is => 'rw' );
 has 'targetdir' => ( is => 'rw' );
 has 'msg_file' => ( is => 'rw', trigger => \&read_msg_file );
 has 'messages' => ( is => 'rw' );
 has 'author' => ( is => 'rw' );
 has 'push_default' => ( is => 'rw' );
 has 'git_import_user' => ( is => 'rw', trigger => \&build_author );
 has '_repo' => ( is => 'rw' );
 has 'name' => ( is => 'rw' );
 has 'tag' => ( is => 'rw', default => '1' );
 
 sub rollback {
 my ( $self ) = @_; 
 $self->rmdir();
 
 return;
 } 
 
 sub choose_repo_name {
 my ( $self ) = @_; 
 
 print "\nPlease enter a name for your repository: \n";
 my $chosen_option = <STDIN>;
 chomp $chosen_option;
 
 while ( $chosen_option !~ /[A-Za-z0-9_-]+/xms ) { 
 print "Invalid characters in $chosen_option. Please try again\n";
 $chosen_option = <>; 
 chomp $chosen_option;
 }
 
 report LOG_INFO, "User chose $chosen_option as repository name\n";
 return $chosen_option;
 } 
 
 sub build_author {
 my ( $self, $import_user ) = @_; 
 
 $self->author( $import_user->{ name } . ' <' . $import_user->{ email } . '>' );
 $self->push_default( $import_user->{ push_default } );
 
 return;
 } 

It specifies what functionality must be implemented by the other classes and implements common functionality for Git.pm and dryrun.pm. This functionality is performed the same way, no matter if it's a productive or a dryrun.

Then there is Git.pm, used when an actual import is performed: (only sub commit shown for brevity, there are more)

 #! /usr/bin/perl 
 
 package Import::Git;
 
 use strict;
 use warnings;
 
 use Mouse;
 
 with 'Import::Git::Role';
 
 use Carp;
 use English qw( -no_match_vars );
 use File::Path qw(:DEFAULT make_path);
 use Git;
 
 use Utils::Log;
 
 sub commit {
 my ( $self, $name, $version ) = @_; 
 
 my $message = "$name Version $version\nImported from " . $name . " in version $version";
 
 if ( $self->messages && $self->messages->{ $name }->{ $version } ) { 
 $message = $self->messages->{ $name }->{ $version }; 
 }
 
 my $rc = $self->_repo->command( 'add', '-A' );
 
 eval {
 $rc = $self->_repo->command( 'commit', '-m', $message, '--author', $self->author );
 1; 
 } or do {
 #Error 1 = Nothing to commit. We can ignore this, its two versions of our file whichcontain no differences
 unless ( $EVAL_ERROR->{'-value'} == '1' ) { 
 die "Commit failed: $EVAL_ERROR";
 } 
 }; 
 
 my $msg = "commited file $name in version $version";
 
 if ( $self->tag == 1 ) { 
 $rc = $self->_repo->command( "tag", "$version" );
 $msg = "commited and tagged file $name in version $version";
 } 
 return $msg;
 }
 [...]

And there is dryrun.pm for performing the testrun:

 #! /usr/bin/perl 
 
 package Import::Git::dryrun;
 
 use strict;
 use warnings;
 
 use Mouse;
 with 'Import::Git::Role';
 use Carp;
 use English qw( -no_match_vars );
 use File::Path;
 use Utils::Log;
 
 sub commit {
 my ( $self, $name, $version ) = @_; 
 
 my $message = "$name Version $version\nImported from " . $name . " in version $version";
 
 if ( $self->messages ) { 
 $message = $self->messages->{ $name }->{ $version } if $self->messages->{ $name }->{ $version };
 }
 
 print "git add -A\n";
 print "git commit -m $message --author " . $self->author . "\n";
 print "git tag $version\n" if $self->tag == 1;
 
 return "Commited file $name in Version $version";
 }

I wanted to use the Role to share behaviour that is the same between Git.pm and dryrun.pm and to mandate the list of functions that both of them would have to use. I am looking for feedback on my code quality, what I could improve and wheter this is a proper usage of Roles.

Answer 1

Documentation

For each of your files, you should add documentation to describe its purpose. It is recommended to use plain old documentation (POD) which gives users manpage-like help with perldoc.

For infrequent Git users like me, it would be beneficial to describe the difference between Git and GitHub and how that relates to your code.

Unused code

I see lines like this in your files:

use Carp;

However, it does not look like you use anything from the Carp module in the code. If not, the line should be removed to avoid namespace pollution. It is also misleading.

English

In the git-importer script, you use English, but then you used the 0ドル special variable whose name is a bit cryptic. You can use $PROGRAM_NAME instead, which is a more explicit name.

Note that you no longer need to use -no_match_vars with newer versions of Perl (since 2014).

Naming

Conventionally, module names begin with a capital letter. You have done so for all your modules except dryrun. Consider renaming it Dryrun for consistency.

Comments

These comments are not needed and can be deleted:

# those are the modules I use for my scripts behaviour
# I did start a code review on a Perlmonks Meetup but we were not able to finish on time. They found it odd that I used a different object for the dryrun. 

DRY

The options passed to the new function are repeated for the dryrun calls:

 $o_file = Import::File::dryrun->new( rawpath => $file, is_module => $opt->modules );
 $o_file = Import::File->new( rawpath => $file, is_module => $opt->modules );

You can create a variable to store the list.

Layout

Some of the indentation is inconsistent. You can use perltidy to automatically format your code for consistency.

Less typing

for can be used instead of foreach since they are synonyms.

Simpler

There is no need for string concatenation in the following line since the variable can be interpolated in the double quotes:

 my $message = "$name Version $version\nImported from " . $name . " in version $version";

It is simpler as:

 my $message = "$name Version $version\nImported from $name in version $version";

Also, you seldom need to add double quotes around just a scalar variable as in the following line:

 $rc = $self->_repo->command( "tag", "$version" );

It is better as:

 $rc = $self->_repo->command( 'tag', $version );

Note that single quotes are preferred for strings where interpolation is not needed.