From 6b3aa8c5ebe98890c2b17690e74520f3aaf095ba Mon Sep 17 00:00:00 2001
From: "robert.richardson" <robert.richardson@suse.com>
Date: Fri, 16 Aug 2024 15:30:50 +0200
Subject: [PATCH] Avoid array recreation with _run_cmd helper method

- Minor fixes
---
 lib/OpenQA/Git.pm            | 49 +++++++++++++++++-------------------
 lib/OpenQA/Task/Git/Clone.pm |  2 +-
 t/14-grutasks.t              |  2 --
 3 files changed, 24 insertions(+), 29 deletions(-)

diff --git a/lib/OpenQA/Git.pm b/lib/OpenQA/Git.pm
index 0194ace11209..013e71616038 100644
--- a/lib/OpenQA/Git.pm
+++ b/lib/OpenQA/Git.pm
@@ -6,7 +6,6 @@ use Mojo::Base -base, -signatures;
 use Mojo::Util 'trim';
 use Cwd 'abs_path';
 use OpenQA::Utils qw(run_cmd_with_log_return_error);
-use Mojo::File;
 
 has 'app';
 has 'dir';
@@ -22,8 +21,9 @@ sub config ($self, $args = undef) {
     return $app->config->{'scm git'};
 }
 
-sub _prepare_git_command ($self, $args = undef) {
-    my $dir = $args->{dir} // $self->dir;
+sub _prepare_git_command ($self) {
+    my $dir = $self->dir;
+
     if ($dir !~ /^\//) {
         my $absolute_path = abs_path($dir);
         $dir = $absolute_path if ($absolute_path);
@@ -32,10 +32,9 @@ sub _prepare_git_command ($self, $args = undef) {
 }
 
 sub _format_git_error ($self, $result, $error_message) {
-
     my $path = $self->dir;
     if ($result->{stderr} or $result->{stdout}) {
-        $error_message .= "($path): " . $result->{stdout} . $result->{stderr};
+        $error_message .= ': ' . $result->{stdout} . $result->{stderr};
     }
     return $error_message;
 }
@@ -46,22 +45,22 @@ sub _validate_attributes ($self) {
     }
 }
 
+sub _run_cmd ($self, $args) { run_cmd_with_log_return_error([$self->_prepare_git_command, @$args]) }
+
 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]);
+        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']);
+            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]);
+        my $res = $self->_run_cmd(['rebase', $update_branch]);
         return $self->_format_git_error($res, 'Unable to reset repository to origin/master') unless $res->{status};
     }
 
@@ -71,47 +70,45 @@ 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}}]);
+        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]);
+    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']);
+        $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) {    #
+sub get_current_branch ($self) {
     my @git = $self->_prepare_git_command();
-    my $r = run_cmd_with_log_return_error([@git, 'branch', '--show-current']);
-    die $self->_format_git_error($r, "Error detecting current branch") unless $r->{status};
+    my $r = $self->_run_cmd(['branch', '--show-current']);
+    die $self->_format_git_error($r, 'Error detecting current branch') unless $r->{status};
     return Mojo::Util::trim($r->{stdout});
 }
 
-sub _ssh_git_cmd ($self, $git_args) {
-    my @git = $self->_prepare_git_command();
-    return ['env', 'GIT_SSH_COMMAND="ssh -oBatchMode=yes"', 'git', @$git_args];
+sub _ssh_git_cmd ($git_args) {
+    return ['env', 'GIT_SSH_COMMAND=ssh -oBatchMode=yes', 'git', @$git_args];
 }
 
 sub get_remote_default_branch ($self, $url) {
     my @git = $self->_prepare_git_command();
-    my $r = run_cmd_with_log_return_error($self->_ssh_git_cmd(['ls-remote', '--symref', $url, 'HEAD']));
+    my $r = run_cmd_with_log_return_error(_ssh_git_cmd(['ls-remote', '--symref', $url, 'HEAD']));
     die $self->_format_git_error($r, "Error detecting remote default branch name for '$url'")
       unless $r->{status} && $r->{stdout} =~ /refs\/heads\/(\S+)\s+HEAD/;
     return $1;
@@ -119,26 +116,26 @@ sub get_remote_default_branch ($self, $url) {
 
 sub git_clone_url_to_path ($self, $url, $path) {
     my @git = $self->_prepare_git_command();
-    my $r = run_cmd_with_log_return_error($self->_ssh_git_cmd(['clone', $url, $path]));
+    my $r = $self->_run_cmd(_ssh_git_cmd(['clone', $url, $path]));
     die $self->_format_git_error($r, "Failed to clone $url into '$path'") unless $r->{status};
 }
 
 sub git_get_origin_url ($self) {
     my @git = $self->_prepare_git_command();
-    my $r = run_cmd_with_log_return_error([@git, 'remote', 'get-url', 'origin']);
-    die $self->_format_git_error($r, "Failed to get origin url") unless $r->{status};
+    my $r = $self->_run_cmd(['remote', 'get-url', 'origin']);
+    die $self->_format_git_error($r, 'Failed to get origin url') unless $r->{status};
     return Mojo::Util::trim($r->{stdout});
 }
 
 sub git_fetch ($self, $branch_arg) {
     my @git = $self->_prepare_git_command();
-    my $r = run_cmd_with_log_return_error($self->_ssh_git_cmd([@git, 'fetch', 'origin', $branch_arg]));
+    my $r = $self->_run_cmd(_ssh_git_cmd([@git, 'fetch', 'origin', $branch_arg]));
     die $self->_format_git_error($r, "Failed to fetch from '$branch_arg'") unless $r->{status};
 }
 
 sub git_reset_hard ($self, $branch) {
     my @git = $self->_prepare_git_command();
-    my $r = run_cmd_with_log_return_error([@git, 'reset', '--hard', "origin/$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};
 }
 
diff --git a/lib/OpenQA/Task/Git/Clone.pm b/lib/OpenQA/Task/Git/Clone.pm
index 8fdd733428d1..78abb8dc337b 100644
--- a/lib/OpenQA/Task/Git/Clone.pm
+++ b/lib/OpenQA/Task/Git/Clone.pm
@@ -1,5 +1,6 @@
 # Copyright SUSE LLC
 # SPDX-License-Identifier: GPL-2.0-or-later
+
 package OpenQA::Task::Git::Clone;
 use Mojo::Base 'Mojolicious::Plugin', -signatures;
 use OpenQA::Git;
@@ -16,7 +17,6 @@ sub _git_clone_all ($job, $clones) {
     my $app = $job->app;
     my $job_id = $job->id;
 
-
     # Prevent multiple git clone tasks for the same path to run in parallel
     my @guards;
     my $retry_delay = {delay => 30 + int(rand(10))};
diff --git a/t/14-grutasks.t b/t/14-grutasks.t
index 197f94cadded..f9230470d22e 100644
--- a/t/14-grutasks.t
+++ b/t/14-grutasks.t
@@ -645,8 +645,6 @@ subtest 'handling dying GRU task' => sub {
     like $associated_job->reason, qr/^preparation failed: Thrown fail at/, 'reason of associated job set';
 };
 
-
-
 subtest 'git clone' => sub {
     my $openqa_git = Test::MockModule->new('OpenQA::Git');
     my @mocked_git_calls;