Refactor git functions into git module

- Make path variable and ssh batch mode usage method parameters - Fix merge conflicts after rebase to master branch - Avoid array recreation with _run_cmd helper method - Minor fixes
r-richardson · Aug 30, 2024 · e2a6dfe · e2a6dfe
1 parent aa42e73
commit e2a6dfe
Show file tree

Hide file tree

Showing 4 changed files with 119 additions and 96 deletions.
diff --git a/lib/OpenQA/Git.pm b/lib/OpenQA/Git.pm
@@ -4,6 +4,7 @@
 package OpenQA::Git;
 
 use Mojo::Base -base, -signatures;
+use Mojo::Util 'trim';
 use Cwd 'abs_path';
 use OpenQA::Utils qw(run_cmd_with_log_return_error);
 
@@ -21,45 +22,68 @@ sub config ($self, $args = undef) {
     return $app->config->{'scm git'};
 }
 
-sub _prepare_git_command ($self, $args = undef) {
-    my $dir = $args->{dir} // $self->dir;
-    if ($dir !~ /^\//) {
-        my $absolute_path = abs_path($dir);
-        $dir = $absolute_path if ($absolute_path);
+sub _validate_attributes ($self) {
+    for my $mandatory_property (qw(app dir user)) {
+        die "no $mandatory_property specified" unless $self->$mandatory_property();
     }
-    return ('git', '-C', $dir);
 }
 
-sub _format_git_error ($result, $error_message) {
-    if ($result->{stderr} or $result->{stdout}) {
-        $error_message .= ': ' . $result->{stdout} . $result->{stderr};
+sub _run_cmd ($self, $args, $options = {}) {
+    my $include_git_path = $options->{include_git_path} // 1;
+    my $ssh_batchmode = $options->{ssh_batchmode} // 0;
+
+    my @cmd;
+
+    if ($ssh_batchmode) {
+        push @cmd, 'env', 'GIT_SSH_COMMAND="ssh -oBatchMode=yes"';
     }
-    return $error_message;
+
+    push @cmd, $self->_prepare_git_command($include_git_path), @$args;
+
+    my $result = run_cmd_with_log_return_error(\@cmd);
+    if (!$result->{status}) {
+        $self->app->log->error("Git command failed: @cmd - Error: $result->{stderr}");
+    }
+    return $result;
 }
 
-sub _validate_attributes ($self) {
-    for my $mandatory_property (qw(app dir user)) {
-        die "no $mandatory_property specified" unless $self->$mandatory_property();
+sub _prepare_git_command ($self, $include_git_path) {
+    if ($include_git_path) {
+        my $dir = $self->dir;
+        die 'no valid directory was found during git preparation' unless $dir;
+        if ($dir !~ /^\//) {
+            my $absolute_path = abs_path($dir);
+            $dir = $absolute_path if ($absolute_path);
+        }
+        return ('git', '-C', $dir);
+    }
+    else {
+        return 'git';
+    }
+}
+
+sub _format_git_error ($self, $result, $error_message) {
+    my $dir = $self->dir;
+    if ($result->{stderr} or $result->{stdout}) {
+        $error_message .= " ($dir): " . $result->{stdout} . $result->{stderr};
     }
+    return $error_message;
 }
 
 sub set_to_latest_master ($self, $args = undef) {
     $self->_validate_attributes;
-
-    my @git = $self->_prepare_git_command($args);
-
     if (my $update_remote = $self->config->{update_remote}) {
-        my $res = run_cmd_with_log_return_error([@git, 'remote', 'update', $update_remote]);
-        return _format_git_error($res, 'Unable to fetch from origin master') unless $res->{status};
+        my $res = $self->_run_cmd(['remote', 'update', $update_remote]);
+        return $self->_format_git_error($res, 'Unable to fetch from origin master') unless $res->{status};
     }
 
     if (my $update_branch = $self->config->{update_branch}) {
         if ($self->config->{do_cleanup} eq 'yes') {
-            my $res = run_cmd_with_log_return_error([@git, 'reset', '--hard', 'HEAD']);
-            return _format_git_error($res, 'Unable to reset repository to HEAD') unless $res->{status};
+            my $res = $self->_run_cmd(['reset', '--hard', 'HEAD']);
+            return $self->_format_git_error($res, 'Unable to reset repository to HEAD') unless $res->{status};
         }
-        my $res = run_cmd_with_log_return_error([@git, 'rebase', $update_branch]);
-        return _format_git_error($res, 'Unable to reset repository to origin/master') unless $res->{status};
+        my $res = $self->_run_cmd(['rebase', $update_branch]);
+        return $self->_format_git_error($res, 'Unable to reset repository to origin/master') unless $res->{status};
     }
 
     return undef;
@@ -68,30 +92,63 @@ sub set_to_latest_master ($self, $args = undef) {
 sub commit ($self, $args = undef) {
     $self->_validate_attributes;
 
-    my @git = $self->_prepare_git_command($args);
     my @files;
 
     # stage changes
     for my $cmd (qw(add rm)) {
         next unless $args->{$cmd};
         push(@files, @{$args->{$cmd}});
-        my $res = run_cmd_with_log_return_error([@git, $cmd, @{$args->{$cmd}}]);
-        return _format_git_error($res, "Unable to $cmd via Git") unless $res->{status};
+        my $res = $self->_run_cmd([$cmd, @{$args->{$cmd}}]);
+        return $self->_format_git_error($res, "Unable to $cmd via Git") unless $res->{status};
     }
 
     # commit changes
     my $message = $args->{message};
     my $author = sprintf('--author=%s <%s>', $self->user->fullname, $self->user->email);
-    my $res = run_cmd_with_log_return_error([@git, 'commit', '-q', '-m', $message, $author, @files]);
-    return _format_git_error($res, 'Unable to commit via Git') unless $res->{status};
+    my $res = $self->_run_cmd(['commit', '-q', '-m', $message, $author, @files]);
+    return $self->_format_git_error($res, 'Unable to commit via Git') unless $res->{status};
 
     # push changes
     if (($self->config->{do_push} || '') eq 'yes') {
-        $res = run_cmd_with_log_return_error([@git, 'push']);
-        return _format_git_error($res, 'Unable to push Git commit') unless $res->{status};
+        $res = $self->_run_cmd(['push']);
+        return $self->_format_git_error($res, 'Unable to push Git commit') unless $res->{status};
     }
 
     return undef;
 }
 
+sub get_current_branch ($self) {
+    my $r = $self->_run_cmd(['branch', '--show-current']);
+    die $self->_format_git_error($r, 'Error detecting current branch') unless $r->{status};
+    return trim($r->{stdout});
+}
+
+sub get_remote_default_branch ($self, $url) {
+    my $r = $self->_run_cmd(['ls-remote', '--symref', $url, 'HEAD'], {include_git_path => 0, ssh_batchmode => 1});
+    die "Error detecting remote default branch name for '$url': $r->{stdout} $r->{stderr}"
+      unless $r->{status} && $r->{stdout} =~ m{refs/heads/(\S+)\s+HEAD};
+    return $1;
+}
+
+sub clone_url ($self, $url) {
+    my $r = $self->_run_cmd(['clone', $url, $self->dir], {include_git_path => 0, ssh_batchmode => 1});
+    die $self->_format_git_error($r, "Failed to clone $url") unless $r->{status};
+}
+
+sub get_origin_url ($self) {
+    my $r = $self->_run_cmd(['remote', 'get-url', 'origin']);
+    die $self->_format_git_error($r, 'Failed to get origin url') unless $r->{status};
+    return trim($r->{stdout});
+}
+
+sub fetch ($self, $branch_arg) {
+    my $r = $self->_run_cmd(['fetch', 'origin', $branch_arg], {ssh_batchmode => 1});
+    die $self->_format_git_error($r, "Failed to fetch from '$branch_arg'") unless $r->{status};
+}
+
+sub reset_hard ($self, $branch) {
+    my $r = $self->_run_cmd(['reset', '--hard', "origin/$branch"]);
+    die $self->_format_git_error($r, "Failed to reset to 'origin/$branch'") unless $r->{status};
+}
+
 1;
diff --git a/lib/OpenQA/Task/Git/Clone.pm b/lib/OpenQA/Task/Git/Clone.pm
@@ -13,7 +13,6 @@ sub register ($self, $app, @) {
     $app->minion->add_task(git_clone => \&_git_clone_all);
 }
 
-
 # $clones is a hashref with paths as keys and urls to git repos as values.
 # The urls may also refer to a branch via the url fragment.
 # If no branch is set, the default branch of the remote (if target path doesn't exist yet)
@@ -45,77 +44,40 @@ sub _git_clone_all ($job, $clones) {
     for my $path (sort { length($a) <=> length($b) } keys %$clones) {
         my $url = $clones->{$path};
         die "Don't even think about putting '..' into '$path'." if $path =~ /\.\./;
-        eval { _git_clone($job, $ctx, $path, $url) };
+        eval { _git_clone($app, $job, $ctx, $path, $url) };
         next unless my $error = $@;
         my $max_retries = $ENV{OPENQA_GIT_CLONE_RETRIES} // 10;
         return $job->retry($retry_delay) if $job->retries < $max_retries;
         return $job->fail($error);
     }
 }
 
-sub _get_current_branch ($path) {
-    my $r = run_cmd_with_log_return_error(['git', '-C', $path, 'branch', '--show-current']);
-    die "Error detecting current branch for '$path': $r->{stderr}" unless $r->{status};
-    return trim($r->{stdout});
-}
-
-sub _ssh_git_cmd ($git_args) {
-    return ['env', 'GIT_SSH_COMMAND="ssh -oBatchMode=yes"', 'git', @$git_args];
-}
-
-sub _get_remote_default_branch ($url) {
-    my $r = run_cmd_with_log_return_error(_ssh_git_cmd(['ls-remote', '--symref', $url, 'HEAD']));
-    die "Error detecting remote default branch name for '$url': $r->{stdout} $r->{stderr}"
-      unless $r->{status} && $r->{stdout} =~ m{refs/heads/(\S+)\s+HEAD};
-    return $1;
-}
-
-sub _git_clone_url_to_path ($url, $path) {
-    my $r = run_cmd_with_log_return_error(_ssh_git_cmd(['clone', $url, $path]));
-    die "Failed to clone $url into '$path': $r->{stderr}" unless $r->{status};
-}
-
-sub _git_get_origin_url ($path) {
-    my $r = run_cmd_with_log_return_error(['git', '-C', $path, 'remote', 'get-url', 'origin']);
-    die "Failed to get origin url for '$path': $r->{stderr}" unless $r->{status};
-    return trim($r->{stdout});
-}
-
-sub _git_fetch ($path, $branch_arg) {
-    my $r = run_cmd_with_log_return_error(_ssh_git_cmd(['-C', $path, 'fetch', 'origin', $branch_arg]));
-    die "Failed to fetch from '$branch_arg': $r->{stderr}" unless $r->{status};
-}
-
-sub _git_reset_hard ($path, $branch) {
-    my $r = run_cmd_with_log_return_error(['git', '-C', $path, 'reset', '--hard', "origin/$branch"]);
-    die "Failed to reset to 'origin/$branch': $r->{stderr}" unless $r->{status};
-}
-
-sub _git_clone ($job, $ctx, $path, $url) {
+sub _git_clone ($app, $job, $ctx, $path, $url) {
+    my $git = OpenQA::Git->new(app => $app, dir => $path);
     $ctx->debug(qq{Updating $path to $url});
     $url = Mojo::URL->new($url);
     my $requested_branch = $url->fragment;
     $url->fragment(undef);
 
     # An initial clone fetches all refs, we are done
-    return _git_clone_url_to_path($url, $path) unless -d $path;
+    return $git->clone_url($url) unless -d $path;
 
-    my $origin_url = _git_get_origin_url($path);
+    my $origin_url = $git->get_origin_url;
     if ($url ne $origin_url) {
         $ctx->warn("Local checkout at $path has origin $origin_url but requesting to clone from $url");
         return;
     }
 
     unless ($requested_branch) {
-        my $remote_default = _get_remote_default_branch($url);
+        my $remote_default = $git->get_remote_default_branch($url);
         $requested_branch = $remote_default;
         $ctx->debug(qq{Remote default branch $remote_default});
     }
 
-    my $current_branch = _get_current_branch($path);
+    my $current_branch = $git->get_current_branch;
     # updating default branch (including checkout)
-    _git_fetch($path, $requested_branch);
-    _git_reset_hard($path, $requested_branch) if ($requested_branch eq $current_branch);
+    $git->fetch($requested_branch);
+    $git->reset_hard($requested_branch) if ($requested_branch eq $current_branch);
 }
 
 1;
diff --git a/t/14-grutasks.t b/t/14-grutasks.t
@@ -645,14 +645,14 @@ subtest 'handling dying GRU task' => sub {
 };
 
 subtest 'git clone' => sub {
-    my $openqa_utils = Test::MockModule->new('OpenQA::Task::Git::Clone');
+    my $openqa_git = Test::MockModule->new('OpenQA::Git');
     my @mocked_git_calls;
     my $clone_dirs = {
         '/etc/' => 'http://localhost/foo.git',
         '/root/' => 'http://localhost/foo.git#foobranch',
         '/this_directory_does_not_exist/' => 'http://localhost/bar.git',
     };
-    $openqa_utils->redefine(
+    $openqa_git->redefine(
         run_cmd_with_log_return_error => sub ($cmd) {
             push @mocked_git_calls, "@$cmd";
             my $stdout = '';
@@ -675,21 +675,21 @@ subtest 'git clone' => sub {
     is $res->{result}, 'Job successfully executed', 'minion job result indicates success';
     #<<< no perltidy
     my $expected_calls = [
-        # /etc/
-        ['get-url' => 'git -C /etc/ remote get-url origin'],
-        ['default remote' => 'env GIT_SSH_COMMAND="ssh -oBatchMode=yes" git ls-remote --symref http://localhost/foo.git HEAD'],
-        ['current branch' => 'git -C /etc/ branch --show-current'],
-        ['fetch default' => 'env GIT_SSH_COMMAND="ssh -oBatchMode=yes" git -C /etc/ fetch origin master'],
-        ['reset' => 'git -C /etc/ reset --hard origin/master'],
-
-        # /root
-        ['get-url' => 'git -C /root/ remote get-url origin'],
-        ['current branch' => 'git -C /root/ branch --show-current'],
-        ['fetch branch' => 'env GIT_SSH_COMMAND="ssh -oBatchMode=yes" git -C /root/ fetch origin foobranch'],
-
-        # /this_directory_does_not_exist/
-        ['clone' => 'env GIT_SSH_COMMAND="ssh -oBatchMode=yes" git clone http://localhost/bar.git /this_directory_does_not_exist/'],
-    ];
+    # /etc/
+    ['get-url' => 'git -C /etc/ remote get-url origin'],
+    ['default remote' => 'env GIT_SSH_COMMAND="ssh -oBatchMode=yes" git ls-remote --symref http://localhost/foo.git HEAD'],
+    ['current branch' => 'git -C /etc/ branch --show-current'],
+    ['fetch default' => 'env GIT_SSH_COMMAND="ssh -oBatchMode=yes" git -C /etc/ fetch origin master'],
+    ['reset' => 'git -C /etc/ reset --hard origin/master'],
+
+    # /root
+    ['get-url' => 'git -C /root/ remote get-url origin'],
+    ['current branch' => 'git -C /root/ branch --show-current'],
+    ['fetch branch' => 'env GIT_SSH_COMMAND="ssh -oBatchMode=yes" git -C /root/ fetch origin foobranch'],
+
+    # /this_directory_does_not_exist/
+    ['clone' => 'env GIT_SSH_COMMAND="ssh -oBatchMode=yes" git clone http://localhost/bar.git /this_directory_does_not_exist/'],
+];
     #>>> no perltidy
     for my $i (0 .. $#$expected_calls) {
         my $test = $expected_calls->[$i];
@@ -706,13 +706,16 @@ subtest 'git clone' => sub {
 
     subtest 'git clone retried on failure' => sub {
         $ENV{OPENQA_GIT_CLONE_RETRIES} = 1;
-        $openqa_utils->redefine(_git_clone => sub (@) { die "fake error\n" });
+        my $openqa_clone = Test::MockModule->new('OpenQA::Task::Git::Clone');
+        $openqa_clone->redefine(_git_clone => sub (@) { die "fake error\n" });
         $res = run_gru_job($t->app, 'git_clone', $clone_dirs, {priority => 10});
         is $res->{retries}, 1, 'job retries incremented';
         is $res->{state}, 'inactive', 'job set back to inactive';
     };
     subtest 'git clone fails when all retry attempts exhausted' => sub {
         $ENV{OPENQA_GIT_CLONE_RETRIES} = 0;
+        my $openqa_clone = Test::MockModule->new('OpenQA::Task::Git::Clone');
+        $openqa_clone->redefine(_git_clone => sub (@) { die "fake error\n" });
         $res = run_gru_job($t->app, 'git_clone', $clone_dirs, {priority => 10});
         is $res->{retries}, 0, 'job retries not incremented';
         is $res->{state}, 'failed', 'job considered failed';

diff --git a/t/16-utils-runcmd.t b/t/16-utils-runcmd.t
@@ -53,7 +53,8 @@ subtest 'make git commit (error handling)' => sub {
     my $res;
     stdout_like { $res = $git->commit({cmd => 'status', message => 'test'}) }
     qr/.*\[warn\].*fatal: Not a git repository/i, 'git message found';
-    like $res, qr'^Unable to commit via Git: fatal: (N|n)ot a git repository \(or any', 'Git error message returned';
+    like $res, qr"^Unable to commit via Git \($empty_tmp_dir\): fatal: (N|n)ot a git repository \(or any",
+      'Git error message returned';
 };
 
 # setup mocking
@@ -117,7 +118,7 @@ subtest 'git commands with mocked run_cmd_with_log_return_error' => sub {
     $mock_return_value{stdout} = '';
     is(
         $git->set_to_latest_master,
-        'Unable to fetch from origin master: mocked error',
+        'Unable to fetch from origin master (foo/bar): mocked error',
         'an error occurred on remote update'
     );
     is_deeply(\@executed_commands, [[qw(git -C foo/bar remote update origin)],], 'git reset not attempted',)