From 311476604bc42dc6978c546e1658463b6427fe5e Mon Sep 17 00:00:00 2001 From: James Couball Date: Tue, 21 Feb 2023 08:36:15 -0800 Subject: [PATCH] Refactor error thrown when a git command fails Signed-off-by: James Couball --- lib/git.rb | 4 + lib/git/command_line_result.rb | 86 +++++++++++++++++++ lib/git/failed_error.rb | 51 +++++++++++ lib/git/git_execute_error.rb | 7 ++ lib/git/lib.rb | 15 ++-- lib/git/signaled_error.rb | 50 +++++++++++ tests/units/test_branch.rb | 2 +- tests/units/test_command_line_result.rb | 17 ++++ tests/units/test_commit_with_empty_message.rb | 2 +- tests/units/test_failed_error.rb | 23 +++++ tests/units/test_git_execute_error.rb | 7 ++ tests/units/test_lib.rb | 2 +- tests/units/test_log.rb | 2 +- tests/units/test_remotes.rb | 4 +- tests/units/test_signaled_error.rb | 23 +++++ tests/units/test_tags.rb | 2 +- tests/units/test_tree_ops.rb | 4 +- 17 files changed, 284 insertions(+), 17 deletions(-) create mode 100644 lib/git/command_line_result.rb create mode 100644 lib/git/failed_error.rb create mode 100644 lib/git/git_execute_error.rb create mode 100644 lib/git/signaled_error.rb create mode 100644 tests/units/test_command_line_result.rb create mode 100644 tests/units/test_failed_error.rb create mode 100644 tests/units/test_git_execute_error.rb create mode 100644 tests/units/test_signaled_error.rb diff --git a/lib/git.rb b/lib/git.rb index fe38972f..c32ef896 100644 --- a/lib/git.rb +++ b/lib/git.rb @@ -7,10 +7,13 @@ require 'git/base' require 'git/branch' require 'git/branches' +require 'git/command_line_result' require 'git/config' require 'git/diff' require 'git/encoding_utils' require 'git/escaped_path' +require 'git/failed_error' +require 'git/git_execute_error' require 'git/index' require 'git/lib' require 'git/log' @@ -18,6 +21,7 @@ require 'git/path' require 'git/remote' require 'git/repository' +require 'git/signaled_error' require 'git/status' require 'git/stash' require 'git/stashes' diff --git a/lib/git/command_line_result.rb b/lib/git/command_line_result.rb new file mode 100644 index 00000000..9194a292 --- /dev/null +++ b/lib/git/command_line_result.rb @@ -0,0 +1,86 @@ +# frozen_string_literal: true + +module Git + # The result of running a git command + # + # This object stores the Git command executed and its status, stdout, and stderr. + # + # @api public + # + class CommandLineResult + # Create a CommandLineResult object + # + # @example + # `true` + # git_cmd = %w[git version] + # status = $? + # stdout = "git version 2.39.1\n" + # stderr = "" + # result = Git::CommandLineResult.new(git_cmd, status, stdout, stderr) + # + # @param git_cmd [Array] the git command that was executed + # @param status [Process::Status] the status of the process + # @param stdout [String] the output of the process + # @param stderr [String] the error output of the process + # + def initialize(git_cmd, status, stdout, stderr) + @git_cmd = git_cmd + @status = status + @stdout = stdout + @stderr = stderr + end + + # @attribute [r] git_cmd + # + # The git command that was executed + # + # @example + # git_cmd = %w[git version] + # result = Git::CommandLineResult.new(git_cmd, $?, "", "") + # result.git_cmd #=> ["git", "version"] + # + # @return [Array] + # + attr_reader :git_cmd + + # @attribute [r] status + # + # The status of the process + # + # @example + # `true` + # status = $? + # result = Git::CommandLineResult.new(status, "", "") + # result.status #=> # + # + # @return [Process::Status] + # + attr_reader :status + + # @attribute [r] stdout + # + # The output of the process + # + # @example + # stdout = "git version 2.39.1\n" + # result = Git::CommandLineResult.new($?, stdout, "") + # result.stdout #=> "git version 2.39.1\n" + # + # @return [String] + # + attr_reader :stdout + + # @attribute [r] stderr + # + # The error output of the process + # + # @example + # stderr = "Tag not found\n" + # result = Git::CommandLineResult.new($?, "", stderr) + # result.stderr #=> "Tag not found\n" + # + # @return [String] + # + attr_reader :stderr + end +end diff --git a/lib/git/failed_error.rb b/lib/git/failed_error.rb new file mode 100644 index 00000000..244ba2ca --- /dev/null +++ b/lib/git/failed_error.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +require 'git/git_execute_error' + +module Git + # This error is raised when a git command fails + # + # The git command executed, status, stdout, and stderr are available from this + # object. The #message includes the git command, the status of the process, and + # the stderr of the process. + # + # @api public + # + class FailedError < Git::GitExecuteError + # Create a FailedError object + # + # @example + # `exit 1` # set $? appropriately for this example + # result = Git::CommandLineResult.new(%w[git status], $?, '', "failed") + # error = Git::FailedError.new(result) + # error.message #=> + # "[\"git\", \"status\"]\nstatus: pid 89784 exit 1\nstderr: \"failed\"" + # + # @param result [Git::CommandLineResult] the result of the git command including + # the git command, status, stdout, and stderr + # + def initialize(result) + super("#{result.git_cmd}\nstatus: #{result.status}\nstderr: #{result.stderr.inspect}") + @result = result + end + + # @attribute [r] result + # + # The result of the git command including the git command and its status and output + # + # @example + # `exit 1` # set $? appropriately for this example + # result = Git::CommandLineResult.new(%w[git status], $?, '', "failed") + # error = Git::FailedError.new(result) + # error.result #=> + # #, + # @stderr="failed", + # @stdout=""> + # + # @return [Git::CommandLineResult] + # + attr_reader :result + end +end diff --git a/lib/git/git_execute_error.rb b/lib/git/git_execute_error.rb new file mode 100644 index 00000000..52d2c80f --- /dev/null +++ b/lib/git/git_execute_error.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +module Git + # This error is raised when a git command fails + # + class GitExecuteError < StandardError; end +end \ No newline at end of file diff --git a/lib/git/lib.rb b/lib/git/lib.rb index 37c0529b..92172ed7 100644 --- a/lib/git/lib.rb +++ b/lib/git/lib.rb @@ -1,12 +1,9 @@ +require 'git/failed_error' require 'logger' require 'tempfile' require 'zlib' module Git - - class GitExecuteError < StandardError - end - class Lib @@semaphore = Mutex.new @@ -1128,12 +1125,12 @@ def command(*cmd, redirect: '', chomp: true, &block) command_thread = nil; - exitstatus = nil + status = nil with_custom_env_variables do command_thread = Thread.new do output = run_command(git_cmd, &block) - exitstatus = $?.exitstatus + status = $? end command_thread.join end @@ -1141,8 +1138,10 @@ def command(*cmd, redirect: '', chomp: true, &block) @logger.info(git_cmd) @logger.debug(output) - raise Git::GitExecuteError, "#{git_cmd}:#{output}" if - exitstatus > 1 || (exitstatus == 1 && output != '') + if status.exitstatus > 1 || (status.exitstatus == 1 && output != '') + result = Git::CommandLineResult.new(git_cmd, status, output, '') + raise Git::FailedError.new(result) + end output.chomp! if output && chomp && !block_given? diff --git a/lib/git/signaled_error.rb b/lib/git/signaled_error.rb new file mode 100644 index 00000000..279f0fb0 --- /dev/null +++ b/lib/git/signaled_error.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +require 'git/git_execute_error' + +module Git + # This error is raised when a git command exits because of an uncaught signal + # + # The git command executed, status, stdout, and stderr are available from this + # object. The #message includes the git command, the status of the process, and + # the stderr of the process. + # + # @api public + # + class SignaledError < Git::GitExecuteError + # Create a SignaledError object + # + # @example + # `kill -9 $$` # set $? appropriately for this example + # result = Git::CommandLineResult.new(%w[git status], $?, '', "killed") + # error = Git::SignaledError.new(result) + # error.message #=> + # "[\"git\", \"status\"]\nstatus: pid 88811 SIGKILL (signal 9)\nstderr: \"killed\"" + # + # @param result [Git::CommandLineResult] the result of the git command including the git command, status, stdout, and stderr + # + def initialize(result) + super("#{result.git_cmd}\nstatus: #{result.status}\nstderr: #{result.stderr.inspect}") + @result = result + end + + # @attribute [r] result + # + # The result of the git command including the git command, status, and output + # + # @example + # `kill -9 $$` # set $? appropriately for this example + # result = Git::CommandLineResult.new(%w[git status], $?, '', "killed") + # error = Git::SignaledError.new(result) + # error.result #=> + # #, + # @stderr="killed", + # @stdout=""> + # + # @return [Git::CommandLineResult] + # + attr_reader :result + end +end diff --git a/tests/units/test_branch.rb b/tests/units/test_branch.rb index 2c81618e..8fdc1db7 100644 --- a/tests/units/test_branch.rb +++ b/tests/units/test_branch.rb @@ -79,7 +79,7 @@ def test_branch_create_and_switch assert(git.status.untracked.assoc('test-file1')) assert(!git.status.added.assoc('test-file1')) - assert_raise Git::GitExecuteError do + assert_raise Git::FailedError do git.branch('new_branch').delete end assert_equal(1, git.branches.select { |b| b.name == 'new_branch' }.size) diff --git a/tests/units/test_command_line_result.rb b/tests/units/test_command_line_result.rb new file mode 100644 index 00000000..acec4bb6 --- /dev/null +++ b/tests/units/test_command_line_result.rb @@ -0,0 +1,17 @@ +require 'test_helper' + +class TestCommamndLineResult < Test::Unit::TestCase + def test_initialization + git_cmd = Object.new + status = Object.new + stdout = Object.new + stderr = Object.new + + result = Git::CommandLineResult.new(git_cmd, status, stdout, stderr) + + assert_equal(git_cmd, result.git_cmd) + assert_equal(status, result.status) + assert_equal(stdout, result.stdout) + assert_equal(stderr, result.stderr) + end +end diff --git a/tests/units/test_commit_with_empty_message.rb b/tests/units/test_commit_with_empty_message.rb index fb3b0bca..4bf04991 100755 --- a/tests/units/test_commit_with_empty_message.rb +++ b/tests/units/test_commit_with_empty_message.rb @@ -9,7 +9,7 @@ def setup def test_without_allow_empty_message_option Dir.mktmpdir do |dir| git = Git.init(dir) - assert_raises Git::GitExecuteError do + assert_raises Git::FailedError do git.commit('', { allow_empty: true }) end end diff --git a/tests/units/test_failed_error.rb b/tests/units/test_failed_error.rb new file mode 100644 index 00000000..d3c5485f --- /dev/null +++ b/tests/units/test_failed_error.rb @@ -0,0 +1,23 @@ +require 'test_helper' + +class TestFailedError < Test::Unit::TestCase + def test_initializer + status = Struct.new(:to_s).new('pid 89784 exit 1') + result = Git::CommandLineResult.new(%w[git status], status, '', "failed") + + error = Git::FailedError.new(result) + + assert(error.is_a?(Git::GitExecuteError)) + assert_equal(result, error.result) + end + + def test_message + status = Struct.new(:to_s).new('pid 89784 exit 1') + result = Git::CommandLineResult.new(%w[git status], status, '', "failed") + + error = Git::FailedError.new(result) + + expected_message = "[\"git\", \"status\"]\nstatus: pid 89784 exit 1\nstderr: \"failed\"" + assert_equal(expected_message, error.message) + end +end diff --git a/tests/units/test_git_execute_error.rb b/tests/units/test_git_execute_error.rb new file mode 100644 index 00000000..b675a3b3 --- /dev/null +++ b/tests/units/test_git_execute_error.rb @@ -0,0 +1,7 @@ +require 'test_helper' + +class TestGitExecuteError < Test::Unit::TestCase + def test_is_a_standard_error + assert(Git::GitExecuteError < StandardError) + end +end diff --git a/tests/units/test_lib.rb b/tests/units/test_lib.rb index 4e5b08c5..fdeeba06 100644 --- a/tests/units/test_lib.rb +++ b/tests/units/test_lib.rb @@ -64,7 +64,7 @@ def test_commit_with_no_verify @lib.add('test_file_2') # Error raised because of pre-commit hook and no use of no_verify option - assert_raise Git::GitExecuteError do + assert_raise Git::FailedError do @lib.commit('commit without no verify and pre-commit file') end diff --git a/tests/units/test_log.rb b/tests/units/test_log.rb index d2859915..dff04286 100644 --- a/tests/units/test_log.rb +++ b/tests/units/test_log.rb @@ -74,7 +74,7 @@ def test_get_log_path end def test_log_file_noexist - assert_raise Git::GitExecuteError do + assert_raise Git::FailedError do @git.log.object('no-exist.txt').size end end diff --git a/tests/units/test_remotes.rb b/tests/units/test_remotes.rb index d51451e3..ce0ed507 100644 --- a/tests/units/test_remotes.rb +++ b/tests/units/test_remotes.rb @@ -155,10 +155,10 @@ def test_fetch_command_injection origin = "--upload-pack=touch #{test_file};" begin git.fetch(origin, { ref: 'some/ref/head' }) - rescue Git::GitExecuteError + rescue Git::FailedError # This is expected else - raise 'Expected Git::GitExecuteError to be raised' + raise 'Expected Git::Failed to be raised' end vulnerability_exists = File.exist?(test_file) diff --git a/tests/units/test_signaled_error.rb b/tests/units/test_signaled_error.rb new file mode 100644 index 00000000..25922aa9 --- /dev/null +++ b/tests/units/test_signaled_error.rb @@ -0,0 +1,23 @@ +require 'test_helper' + +class TestSignaledError < Test::Unit::TestCase + def test_initializer + status = Struct.new(:to_s).new('pid 65628 SIGKILL (signal 9)') # `kill -9 $$` + result = Git::CommandLineResult.new(%w[git status], status, '', "uncaught signal") + + error = Git::SignaledError.new(result) + + assert(error.is_a?(Git::GitExecuteError)) + assert_equal(result, error.result) + end + + def test_message + status = Struct.new(:to_s).new('pid 65628 SIGKILL (signal 9)') # `kill -9 $$` + result = Git::CommandLineResult.new(%w[git status], status, '', "uncaught signal") + + error = Git::SignaledError.new(result) + + expected_message = "[\"git\", \"status\"]\nstatus: pid 65628 SIGKILL (signal 9)\nstderr: \"uncaught signal\"" + assert_equal(expected_message, error.message) + end +end diff --git a/tests/units/test_tags.rb b/tests/units/test_tags.rb index 03f8ad2f..31745bf8 100644 --- a/tests/units/test_tags.rb +++ b/tests/units/test_tags.rb @@ -43,7 +43,7 @@ def test_tags assert(r2.tags.detect{|t| t.name == 'third'}.objectish == r2.tags.detect{|t| t.name == 'fifth'}.objectish) - assert_raise Git::GitExecuteError do + assert_raise Git::FailedError do r2.add_tag('third') end diff --git a/tests/units/test_tree_ops.rb b/tests/units/test_tree_ops.rb index 8586486b..1f38cae9 100644 --- a/tests/units/test_tree_ops.rb +++ b/tests/units/test_tree_ops.rb @@ -60,8 +60,8 @@ def test_read_tree g.add rescue Exception => e # Adding nothig is now validd on Git 1.7.x - # If an error ocurres (Git 1.6.x) it MUST rise Git::GitExecuteError - assert_equal(e.class, Git::GitExecuteError) + # If an error ocurres (Git 1.6.x) it MUST raise Git::FailedError + assert_equal(e.class, Git::FailedError) end g.read_tree('testbranch1', :prefix => 'b1/') g.read_tree('testbranch3', :prefix => 'b1/b3/')