Merge branch 'list-conflict-errors' into 'master'

Translate more ListConflictFiles errors into FailedPrecondition

Closes gitlab-ce#48881

See merge request gitlab-org/gitaly!797

4 files changed, 59 insertions, 45 deletions

diff --git a/changelogs/unreleased/list-conflict-errors.yml b/changelogs/unreleased/list-conflict-errors.yml
new file mode 100644
index 000000000..8991538d1
--- /dev/null
+++ b/changelogs/unreleased/list-conflict-errors.yml
@@ -0,0 +1,5 @@
+---
+title: Translate more ListConflictFiles errors into FailedPrecondition
+merge_request: 797
+author:
+type: fixed
diff --git a/internal/service/conflicts/list_conflict_files_test.go b/internal/service/conflicts/list_conflict_files_test.go
index 3454f9de7..f342abe61 100644
--- a/internal/service/conflicts/list_conflict_files_test.go
+++ b/internal/service/conflicts/list_conflict_files_test.go
@@ -90,7 +90,7 @@ end
 	require.Equal(t, expectedFiles, receivedFiles)
 }
 
-func TestFailedListConflictFilesRequestDueToConflictSideMissing(t *testing.T) {
+func TestListConflictFilesFailedPrecondition(t *testing.T) {
 	server, serverSocketPath := runConflictsServer(t)
 	defer server.Stop()
 
@@ -100,51 +100,60 @@ func TestFailedListConflictFilesRequestDueToConflictSideMissing(t *testing.T) {
 	testRepo, _, cleanupFn := testhelper.NewTestRepo(t)
 	defer cleanupFn()
 
-	ourCommitOid := "eb227b3e214624708c474bdab7bde7afc17cefcc" // conflict-missing-side
-	theirCommitOid := "824be604a34828eb682305f0d963056cfac87b2d"
-
 	ctx, cancel := testhelper.Context()
 	defer cancel()
 
-	request := &pb.ListConflictFilesRequest{
-		Repository:     testRepo,
-		OurCommitOid:   ourCommitOid,
-		TheirCommitOid: theirCommitOid,
+	testCases := []struct {
+		desc           string
+		ourCommitOid   string
+		theirCommitOid string
+	}{
+		{
+			desc:           "conflict side missing",
+			ourCommitOid:   "eb227b3e214624708c474bdab7bde7afc17cefcc",
+			theirCommitOid: "824be604a34828eb682305f0d963056cfac87b2d",
+		},
+		{
+			// These commits have a conflict on the 'VERSION' file in the test repo.
+			// The conflict is expected to raise an encoding error.
+			desc:           "encoding error",
+			ourCommitOid:   "bd493d44ae3c4dd84ce89cb75be78c4708cbd548",
+			theirCommitOid: "7df99c9ad5b8c9bfc5ae4fb7a91cc87adcce02ef",
+		},
+		{
+			desc:           "submodule object lookup error",
+			ourCommitOid:   "de78448b0b504f3f60093727bddfda1ceee42345",
+			theirCommitOid: "2f61d70f862c6a4f782ef7933e020a118282db29",
+		},
+		{
+			desc:           "invalid commit id on 'our' side",
+			ourCommitOid:   "abcdef0000000000000000000000000000000000",
+			theirCommitOid: "1a35b5a77cf6af7edf6703f88e82f6aff613666f",
+		},
+		{
+			desc:           "invalid commit id on 'their' side",
+			ourCommitOid:   "1a35b5a77cf6af7edf6703f88e82f6aff613666f",
+			theirCommitOid: "abcdef0000000000000000000000000000000000",
+		},
 	}
 
-	c, err := client.ListConflictFiles(ctx, request)
-	require.NoError(t, err)
-	testhelper.RequireGrpcError(t, drainListConflictFilesResponse(c), codes.FailedPrecondition)
-}
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
 
-func TestFailedListConflictFilesFailedPrecondition(t *testing.T) {
-	server, serverSocketPath := runConflictsServer(t)
-	defer server.Stop()
-
-	client, conn := NewConflictsClient(t, serverSocketPath)
-	defer conn.Close()
-
-	testRepo, _, cleanupFn := testhelper.NewTestRepo(t)
-	defer cleanupFn()
-
-	// These commits have a conflict on the 'VERSION' file in the test repo.
-	// The conflict is expected to raise an encoding error.
-	ourCommitOid := "bd493d44ae3c4dd84ce89cb75be78c4708cbd548"
-	theirCommitOid := "7df99c9ad5b8c9bfc5ae4fb7a91cc87adcce02ef"
+			request := &pb.ListConflictFilesRequest{
+				Repository:     testRepo,
+				OurCommitOid:   tc.ourCommitOid,
+				TheirCommitOid: tc.theirCommitOid,
+			}
 
-	ctx, cancel := testhelper.Context()
-	defer cancel()
+			c, err := client.ListConflictFiles(ctx, request)
+			if err == nil {
+				err = drainListConflictFilesResponse(c)
+			}
 
-	request := &pb.ListConflictFilesRequest{
-		Repository:     testRepo,
-		OurCommitOid:   ourCommitOid,
-		TheirCommitOid: theirCommitOid,
+			testhelper.RequireGrpcError(t, err, codes.FailedPrecondition)
+		})
 	}
-
-	c, err := client.ListConflictFiles(ctx, request)
-	require.NoError(t, err)
-
-	testhelper.RequireGrpcError(t, drainListConflictFilesResponse(c), codes.FailedPrecondition)
 }
 
 func TestFailedListConflictFilesRequestDueToValidation(t *testing.T) {
@@ -175,7 +184,7 @@ func TestFailedListConflictFilesRequestDueToValidation(t *testing.T) {
 			code: codes.InvalidArgument,
 		},
 		{
-			desc: "empty OurCommitId repo",
+			desc: "empty OurCommitId field",
 			request: &pb.ListConflictFilesRequest{
 				Repository:     testRepo,
 				OurCommitOid:   "",
@@ -184,7 +193,7 @@ func TestFailedListConflictFilesRequestDueToValidation(t *testing.T) {
 			code: codes.InvalidArgument,
 		},
 		{
-			desc: "empty TheirCommitId repo",
+			desc: "empty TheirCommitId field",
 			request: &pb.ListConflictFilesRequest{
 				Repository:     testRepo,
 				OurCommitOid:   ourCommitOid,
diff --git a/ruby/lib/gitaly_server/conflicts_service.rb b/ruby/lib/gitaly_server/conflicts_service.rb
index e44a7c7d8..d3f961d63 100644
--- a/ruby/lib/gitaly_server/conflicts_service.rb
+++ b/ruby/lib/gitaly_server/conflicts_service.rb
@@ -37,7 +37,7 @@ module GitalyServer
             # Send leftover data, if any
             y.yield(Gitaly::ListConflictFilesResponse.new(files: files)) if files.any?
           end
-        rescue Gitlab::Git::Conflict::Resolver::ConflictSideMissing => e
+        rescue Gitlab::Git::Conflict::Resolver::ListError => e
           raise GRPC::FailedPrecondition.new(e.message)
         end
       end
@@ -93,7 +93,7 @@ module GitalyServer
       conflicts.each do |file|
         yield file
       end
-    rescue Gitlab::Git::Conflict::File::UnsupportedEncoding => e
+    rescue Gitlab::Git::Conflict::File::UnsupportedEncoding, Gitlab::Git::Conflict::Resolver::ListError => e
       raise GRPC::FailedPrecondition.new(e.message)
     end
   end
diff --git a/ruby/lib/gitlab/git/conflict/resolver.rb b/ruby/lib/gitlab/git/conflict/resolver.rb
index 2d904e784..2469b2073 100644
--- a/ruby/lib/gitlab/git/conflict/resolver.rb
+++ b/ruby/lib/gitlab/git/conflict/resolver.rb
@@ -2,7 +2,7 @@ module Gitlab
   module Git
     module Conflict
       class Resolver
-        ConflictSideMissing = Class.new(StandardError)
+        ListError = Class.new(StandardError)
         ResolutionError = Class.new(StandardError)
 
         def initialize(target_repository, our_commit_oid, their_commit_oid)
@@ -13,8 +13,8 @@ module Gitlab
 
         def conflicts
           @conflicts = rugged_list_conflict_files
-        rescue Rugged::ReferenceError, Rugged::OdbError, GRPC::BadStatus => e
-          raise Gitlab::Git::CommandError.new(e)
+        rescue Rugged::ReferenceError, Rugged::OdbError => e
+          raise ListError.new(e)
         end
 
         def resolve_conflicts(source_repository, resolution, source_branch:, target_branch:)
@@ -31,7 +31,7 @@ module Gitlab
 
         def conflict_files(repository, index)
           index.conflicts.map do |conflict|
-            raise ConflictSideMissing unless conflict[:theirs] && conflict[:ours]
+            raise ListError unless conflict[:theirs] && conflict[:ours]
 
             Gitlab::Git::Conflict::File.new(
               repository,