From 8021b753c6cf0de3505e5f510532c40e4ac84aa1 Mon Sep 17 00:00:00 2001 From: Jaydeep Das Date: Thu, 2 Jan 2025 14:50:09 -0800 Subject: [PATCH 1/2] Returning apt error codes for ResolveRevision --- internal/git/localrepo/tree.go | 3 +++ internal/gitaly/service/conflicts/resolve_conflicts.go | 3 +++ internal/gitaly/service/operations/apply_patch.go | 3 +++ internal/gitaly/service/operations/apply_patch_test.go | 2 +- internal/gitaly/service/operations/cherry_pick.go | 3 +++ internal/gitaly/service/operations/commit_files.go | 3 +++ internal/gitaly/service/operations/submodules.go | 3 +++ 7 files changed, 19 insertions(+), 1 deletion(-) diff --git a/internal/git/localrepo/tree.go b/internal/git/localrepo/tree.go index b56e172d31..471183ac24 100644 --- a/internal/git/localrepo/tree.go +++ b/internal/git/localrepo/tree.go @@ -688,6 +688,9 @@ func (repo *Repo) ReadTree(ctx context.Context, treeish git.Revision, options .. treeOID, err := repo.ResolveRevision(ctx, rev) if err != nil { + if errors.Is(err, git.ErrReferenceNotFound) { + return nil, structerr.NewInvalidArgument("could not resolve revision: %w", err) + } return nil, fmt.Errorf("getting revision: %w", err) } diff --git a/internal/gitaly/service/conflicts/resolve_conflicts.go b/internal/gitaly/service/conflicts/resolve_conflicts.go index c412efd902..8cf1633a5a 100644 --- a/internal/gitaly/service/conflicts/resolve_conflicts.go +++ b/internal/gitaly/service/conflicts/resolve_conflicts.go @@ -402,6 +402,9 @@ func (s *server) repoWithBranchCommit(ctx context.Context, sourceRepo *localrepo oid, err := targetRepo.ResolveRevision(ctx, targetRevision) if err != nil { + if errors.Is(err, git.ErrReferenceNotFound) { + return structerr.NewInvalidArgument("could not resolve target revision %q: %w", targetBranch, err) + } return fmt.Errorf("could not resolve target revision %q: %w", targetRevision, err) } diff --git a/internal/gitaly/service/operations/apply_patch.go b/internal/gitaly/service/operations/apply_patch.go index 2bd3886a51..dc758cb305 100644 --- a/internal/gitaly/service/operations/apply_patch.go +++ b/internal/gitaly/service/operations/apply_patch.go @@ -199,6 +199,9 @@ func (s *Server) userApplyPatch(ctx context.Context, header *gitalypb.UserApplyP ctx, git.Revision(fmt.Sprintf("%s^{object}", currentCommit)), ) if err != nil { + if errors.Is(err, git.ErrReferenceNotFound) { + return structerr.NewInvalidArgument("expected old object cannot be resolved: %w", err) + } return fmt.Errorf("expected old object cannot be resolved: %w", err) } } diff --git a/internal/gitaly/service/operations/apply_patch_test.go b/internal/gitaly/service/operations/apply_patch_test.go index d31d145c27..93d5d03945 100644 --- a/internal/gitaly/service/operations/apply_patch_test.go +++ b/internal/gitaly/service/operations/apply_patch_test.go @@ -402,7 +402,7 @@ func TestUserApplyPatch(t *testing.T) { expected: func(t *testing.T, repoPath string) expected { return expected{ oldOID: gittest.DefaultObjectHash.ZeroOID.String(), - err: structerr.NewInternal("expected old object cannot be resolved: reference not found"), + err: structerr.NewInvalidArgument("expected old object cannot be resolved: reference not found"), } }, }, diff --git a/internal/gitaly/service/operations/cherry_pick.go b/internal/gitaly/service/operations/cherry_pick.go index dbd12c8116..71950c908f 100644 --- a/internal/gitaly/service/operations/cherry_pick.go +++ b/internal/gitaly/service/operations/cherry_pick.go @@ -92,6 +92,9 @@ func (s *Server) UserCherryPick(ctx context.Context, req *gitalypb.UserCherryPic git.Revision(fmt.Sprintf("%s^{tree}", startRevision.String())), ) if err != nil { + if errors.Is(err, git.ErrReferenceNotFound) { + return nil, structerr.NewInvalidArgument("resolve old tree: %w", err) + } return nil, fmt.Errorf("resolve old tree: %w", err) } if oldTree == treeOID { diff --git a/internal/gitaly/service/operations/commit_files.go b/internal/gitaly/service/operations/commit_files.go index eff9487dac..38907fd76b 100644 --- a/internal/gitaly/service/operations/commit_files.go +++ b/internal/gitaly/service/operations/commit_files.go @@ -431,6 +431,9 @@ func (s *Server) userCommitFilesGit( git.Revision(fmt.Sprintf("%s^{tree}", parentCommitOID)), ) if err != nil { + if errors.Is(err, git.ErrReferenceNotFound) { + return "", structerr.NewInvalidArgument("getting tree id: %w", err) + } return "", fmt.Errorf("getting tree id: %w", err) } } diff --git a/internal/gitaly/service/operations/submodules.go b/internal/gitaly/service/operations/submodules.go index cdf9cfce26..aaaa1c7712 100644 --- a/internal/gitaly/service/operations/submodules.go +++ b/internal/gitaly/service/operations/submodules.go @@ -241,6 +241,9 @@ func (s *Server) updateSubmodule(ctx context.Context, quarantineRepo *localrepo. treeID = fullTree.OID currentBranchCommit, err := quarantineRepo.ResolveRevision(ctx, git.Revision(req.GetBranch())) if err != nil { + if errors.Is(err, git.ErrReferenceNotFound) { + return "", structerr.NewInvalidArgument("resolving submodule branch: %w", err) + } return "", fmt.Errorf("resolving submodule branch: %w", err) } -- GitLab From 3f0d5b4ba979b5015dd31d467a4ca6db4c8a5430 Mon Sep 17 00:00:00 2001 From: Jaydeep Das Date: Fri, 10 Jan 2025 21:03:11 -0800 Subject: [PATCH 2/2] made error messages more descriptive --- internal/git/localrepo/tree.go | 2 +- internal/git/localrepo/tree_test.go | 8 ++++---- internal/gitaly/service/conflicts/resolve_conflicts.go | 2 +- internal/gitaly/service/operations/apply_patch.go | 2 +- internal/gitaly/service/operations/apply_patch_test.go | 2 +- 5 files changed, 8 insertions(+), 8 deletions(-) diff --git a/internal/git/localrepo/tree.go b/internal/git/localrepo/tree.go index 471183ac24..39ac60d3d7 100644 --- a/internal/git/localrepo/tree.go +++ b/internal/git/localrepo/tree.go @@ -689,7 +689,7 @@ func (repo *Repo) ReadTree(ctx context.Context, treeish git.Revision, options .. treeOID, err := repo.ResolveRevision(ctx, rev) if err != nil { if errors.Is(err, git.ErrReferenceNotFound) { - return nil, structerr.NewInvalidArgument("could not resolve revision: %w", err) + return nil, structerr.NewInvalidArgument("revision not found").WithMetadata("revision", treeish) } return nil, fmt.Errorf("getting revision: %w", err) } diff --git a/internal/git/localrepo/tree_test.go b/internal/git/localrepo/tree_test.go index d17400f236..36d4327f64 100644 --- a/internal/git/localrepo/tree_test.go +++ b/internal/git/localrepo/tree_test.go @@ -569,7 +569,7 @@ func TestReadTree(t *testing.T) { // We get a NotExist error here because it's invalid to suffix an object ID // which resolves to a blob with a colon (":") given that it's not possible // to resolve a subpath. - expectedErr: git.ErrReferenceNotFound, + expectedErr: structerr.NewInvalidArgument("revision not found").WithMetadata("revision", blobID.Revision()), }, { desc: "valid revision with invalid path", @@ -577,7 +577,7 @@ func TestReadTree(t *testing.T) { options: []ReadTreeOption{ WithRelativePath("does-not-exist"), }, - expectedErr: git.ErrReferenceNotFound, + expectedErr: structerr.NewInvalidArgument("revision not found").WithMetadata("revision", treeWithSubtree.Revision()), }, { desc: "valid revision with path pointing to blob", @@ -590,7 +590,7 @@ func TestReadTree(t *testing.T) { { desc: "listing nonexistent object fails", treeish: "does-not-exist", - expectedErr: git.ErrReferenceNotFound, + expectedErr: structerr.NewInvalidArgument("revision not found").WithMetadata("revision", "does-not-exist"), }, { desc: "path to submodule", @@ -606,7 +606,7 @@ func TestReadTree(t *testing.T) { options: []ReadTreeOption{ WithRelativePath("submodule/foo"), }, - expectedErr: git.ErrReferenceNotFound, + expectedErr: structerr.NewInvalidArgument("revision not found").WithMetadata("revision", treeWithSubmodule.Revision()), }, } { t.Run(tc.desc, func(t *testing.T) { diff --git a/internal/gitaly/service/conflicts/resolve_conflicts.go b/internal/gitaly/service/conflicts/resolve_conflicts.go index 8cf1633a5a..2c6a3c1df1 100644 --- a/internal/gitaly/service/conflicts/resolve_conflicts.go +++ b/internal/gitaly/service/conflicts/resolve_conflicts.go @@ -403,7 +403,7 @@ func (s *server) repoWithBranchCommit(ctx context.Context, sourceRepo *localrepo oid, err := targetRepo.ResolveRevision(ctx, targetRevision) if err != nil { if errors.Is(err, git.ErrReferenceNotFound) { - return structerr.NewInvalidArgument("could not resolve target revision %q: %w", targetBranch, err) + return structerr.NewInvalidArgument("target branch %q not found", targetBranch).WithMetadata("revision", targetRevision) } return fmt.Errorf("could not resolve target revision %q: %w", targetRevision, err) } diff --git a/internal/gitaly/service/operations/apply_patch.go b/internal/gitaly/service/operations/apply_patch.go index dc758cb305..38e25de430 100644 --- a/internal/gitaly/service/operations/apply_patch.go +++ b/internal/gitaly/service/operations/apply_patch.go @@ -200,7 +200,7 @@ func (s *Server) userApplyPatch(ctx context.Context, header *gitalypb.UserApplyP ) if err != nil { if errors.Is(err, git.ErrReferenceNotFound) { - return structerr.NewInvalidArgument("expected old object cannot be resolved: %w", err) + return structerr.NewInvalidArgument("expected old object not found").WithMetadata("expected_old_oid", expectedOldOID) } return fmt.Errorf("expected old object cannot be resolved: %w", err) } diff --git a/internal/gitaly/service/operations/apply_patch_test.go b/internal/gitaly/service/operations/apply_patch_test.go index 93d5d03945..f082f055d6 100644 --- a/internal/gitaly/service/operations/apply_patch_test.go +++ b/internal/gitaly/service/operations/apply_patch_test.go @@ -402,7 +402,7 @@ func TestUserApplyPatch(t *testing.T) { expected: func(t *testing.T, repoPath string) expected { return expected{ oldOID: gittest.DefaultObjectHash.ZeroOID.String(), - err: structerr.NewInvalidArgument("expected old object cannot be resolved: reference not found"), + err: structerr.NewInvalidArgument("expected old object not found").WithMetadata("expected_old_oid", gittest.DefaultObjectHash.ZeroOID.String()), } }, }, -- GitLab