diff --git a/hgitaly/service/commit.py b/hgitaly/service/commit.py index 2c7eb9ca7e25f55a5c497ee19739c61669175277_aGdpdGFseS9zZXJ2aWNlL2NvbW1pdC5weQ==..296cf2eab316f25682f483390445b71de33e15f8_aGdpdGFseS9zZXJ2aWNlL2NvbW1pdC5weQ== 100644 --- a/hgitaly/service/commit.py +++ b/hgitaly/service/commit.py @@ -350,8 +350,7 @@ def GetTreeEntries(self, request: GetTreeEntriesRequest, context) -> GetTreeEntriesResponse: - logger = LoggerAdapter(base_logger, context) repo = self.load_repo(request.repository, context) revision = request.revision changeset = gitlab_revision_changeset(repo, revision) if changeset is None: @@ -354,10 +353,9 @@ repo = self.load_repo(request.repository, context) revision = request.revision changeset = gitlab_revision_changeset(repo, revision) if changeset is None: - logger.warning("Revision not found in %r", - message.Logging(request)) - return GetTreeEntriesResponse() + context.abort(StatusCode.INVALID_ARGUMENT, + "invalid revision or path") repo = repo.unfiltered() sha = changeset.hex().decode('ascii') @@ -635,7 +633,8 @@ revision, path = request.revision, request.path ctx = gitlab_revision_changeset(repo, revision) if ctx is None: - return LastCommitForPathResponse() + context.abort(StatusCode.INTERNAL, + "logging last commit for path: exit status 128") changeset = latest_changeset_for_path(path, ctx) # TODO global_options.literal_pathspecs diff --git a/hgitaly/service/tests/test_commit.py b/hgitaly/service/tests/test_commit.py index 2c7eb9ca7e25f55a5c497ee19739c61669175277_aGdpdGFseS9zZXJ2aWNlL3Rlc3RzL3Rlc3RfY29tbWl0LnB5..296cf2eab316f25682f483390445b71de33e15f8_aGdpdGFseS9zZXJ2aWNlL3Rlc3RzL3Rlc3RfY29tbWl0LnB5 100644 --- a/hgitaly/service/tests/test_commit.py +++ b/hgitaly/service/tests/test_commit.py @@ -709,8 +709,10 @@ wrapper.commit_file('foo2') assert do_rpc(revision=b'branch/other', path=b'sub') == ctx_rm.hex() - # empty response for unresolvable revision - assert do_rpc(revision=b'unknown', path=b'foo') == b'' + # error response for unresolvable revision + with pytest.raises(grpc.RpcError) as exc_info: + do_rpc(revision=b'unknown', path=b'foo') == b'' + assert exc_info.value.code() == grpc.StatusCode.INTERNAL def test_list_commits_by_oid(grpc_channel, server_repos_root): @@ -972,9 +974,10 @@ commit_oid=ctx1_oid, ) - # unknown revision gives empty response instead of an error, - # (check Gitaly Comparison tests for evidence). - assert get_tree_entries(b'.', revision=b'unknown') == ([], None) + with pytest.raises(grpc.RpcError) as exc_info: + get_tree_entries(b'.', revision=b'unknown') + assert exc_info.value.code() == grpc.StatusCode.INVALID_ARGUMENT + assert 'invalid revision' in exc_info.value.details() assert get_tree_entries(b'sub', revision=sha1) == ([ba2_default, bar_default], '') diff --git a/rust/rhgitaly/src/service/commit.rs b/rust/rhgitaly/src/service/commit.rs index 2c7eb9ca7e25f55a5c497ee19739c61669175277_cnVzdC9yaGdpdGFseS9zcmMvc2VydmljZS9jb21taXQucnM=..296cf2eab316f25682f483390445b71de33e15f8_cnVzdC9yaGdpdGFseS9zcmMvc2VydmljZS9jb21taXQucnM= 100644 --- a/rust/rhgitaly/src/service/commit.rs +++ b/rust/rhgitaly/src/service/commit.rs @@ -456,10 +456,7 @@ .await .map_err(|e| Status::internal(format!("Error resolving revision: {:?}", e)))? { - None => { - info!("Revision not resolved"); - empty_response_stream() - } + None => Err(Status::invalid_argument("invalid revision or path")), Some(node_prefix) => { info!("Revision resolved as {:x}", &node_prefix); diff --git a/tests_with_gitaly/test_blob_tree.py b/tests_with_gitaly/test_blob_tree.py index 2c7eb9ca7e25f55a5c497ee19739c61669175277_dGVzdHNfd2l0aF9naXRhbHkvdGVzdF9ibG9iX3RyZWUucHk=..296cf2eab316f25682f483390445b71de33e15f8_dGVzdHNfd2l0aF9naXRhbHkvdGVzdF9ibG9iX3RyZWUucHk= 100644 --- a/tests_with_gitaly/test_blob_tree.py +++ b/tests_with_gitaly/test_blob_tree.py @@ -194,6 +194,7 @@ normalizer=gte_normalizer, ) assert_compare = rpc_helper.assert_compare + assert_compare_errors = rpc_helper.assert_compare_errors for path in (b'.', b'sub'): for recursive in (False, True): @@ -202,7 +203,8 @@ skip_flat_paths=skip_flat, recursive=recursive) - assert_compare(path=b'.', revision=b'unknown') + assert_compare_errors(path=b'.', revision=b'unknown', + skip_structured_errors_issue=156) # case when path is actually a file assert_compare(path=b'sub/bar') @@ -306,6 +308,7 @@ rpc_helper.request_kwargs_to_git = request_kwargs_to_git assert_compare = rpc_helper.assert_compare + assert_compare_errors = rpc_helper.assert_compare_errors assert_compare_aggregated = rpc_helper.assert_compare_aggregated # asking more than the expected Gitaly first chunk size (2888 entries) @@ -345,8 +348,9 @@ ) # case of unknown revision and limit=0 - assert_compare(path=b'.', revision=b'unknown', - pagination_params=PaginationParameter(limit=0)) + assert_compare_errors(path=b'.', revision=b'unknown', + pagination_params=PaginationParameter(limit=0), + skip_structured_errors_issue=156) # case of no pagination params assert_compare_aggregated(path=b'sub', diff --git a/tests_with_gitaly/test_commit.py b/tests_with_gitaly/test_commit.py index 2c7eb9ca7e25f55a5c497ee19739c61669175277_dGVzdHNfd2l0aF9naXRhbHkvdGVzdF9jb21taXQucHk=..296cf2eab316f25682f483390445b71de33e15f8_dGVzdHNfd2l0aF9naXRhbHkvdGVzdF9jb21taXQucHk= 100644 --- a/tests_with_gitaly/test_commit.py +++ b/tests_with_gitaly/test_commit.py @@ -131,7 +131,7 @@ # case when revision does not exist for path in (b'', b'foo'): - assert_compare(revision=b'be0123ef' * 5, path=path) + assert_compare_errors(revision=b'be0123ef' * 5, path=path) def test_compare_raw_blame(gitaly_comparison):