047b6a7429e3d98070a1d475ba66e068fcd1bce2

Author
Ayman Bagabas <ayman.bagabas@gmail.com>
Committer
GitHub <noreply@github.com>
Date

Message

fix: invalid error on empty repo collabs (#466)

Propagate a more informative error when adding an existing collaborator.
Ignore webhooks default branch git reference not found errors because
the repo won't have a default branch when it's empty.

Fixes: https://github.com/charmbracelet/soft-serve/issues/464

Diff

  1diff --git a/pkg/backend/collab.go b/pkg/backend/collab.go
  2index c9635ae3712b50c160c9455c31bb510a04bfaac6..ade58b239cea3172bf24044d204bb14e55afe652 100644
  3--- a/pkg/backend/collab.go
  4+++ b/pkg/backend/collab.go
  5@@ -33,6 +33,10 @@ func (d *Backend) AddCollaborator(ctx context.Context, repo string, username str
  6 			return d.store.AddCollabByUsernameAndRepo(ctx, tx, username, repo, level)
  7 		}),
  8 	); err != nil {
  9+		if errors.Is(err, db.ErrDuplicateKey) {
 10+			return proto.ErrCollaboratorExist
 11+		}
 12+
 13 		return err
 14 	}
 15 
 16diff --git a/pkg/proto/errors.go b/pkg/proto/errors.go
 17index cc48b6f78741247d1eb48f4585490b21cf5f77c6..fa4bc6126a253e972c4fe328193fc46ac03eba6c 100644
 18--- a/pkg/proto/errors.go
 19+++ b/pkg/proto/errors.go
 20@@ -21,4 +21,6 @@ var (
 21 	ErrTokenExpired = errors.New("token expired")
 22 	// ErrCollaboratorNotFound is returned when a collaborator is not found.
 23 	ErrCollaboratorNotFound = errors.New("collaborator not found")
 24+	// ErrCollaboratorExist is returned when a collaborator already exists.
 25+	ErrCollaboratorExist = errors.New("collaborator already exists")
 26 )
 27diff --git a/pkg/webhook/branch_tag.go b/pkg/webhook/branch_tag.go
 28index 89771e9fea0dea4f90dd426274d9f997aaeb7343..5e545879ee71749b6bd48f20d6b37723b09de570 100644
 29--- a/pkg/webhook/branch_tag.go
 30+++ b/pkg/webhook/branch_tag.go
 31@@ -77,7 +77,7 @@ func NewBranchTagEvent(ctx context.Context, user proto.User, repo proto.Reposito
 32 
 33 	payload.Repository.Owner.ID = owner.ID
 34 	payload.Repository.Owner.Username = owner.Username
 35-	payload.Repository.DefaultBranch, err = proto.RepositoryDefaultBranch(repo)
 36+	payload.Repository.DefaultBranch, err = getDefaultBranch(repo)
 37 	if err != nil {
 38 		return BranchTagEvent{}, err
 39 	}
 40diff --git a/pkg/webhook/collaborator.go b/pkg/webhook/collaborator.go
 41index e7b737b161d0983bd2dbde293a044c09ece4e20d..67a42aed9cd634877370a3d816aa964a0da395cb 100644
 42--- a/pkg/webhook/collaborator.go
 43+++ b/pkg/webhook/collaborator.go
 44@@ -65,7 +65,7 @@ func NewCollaboratorEvent(ctx context.Context, user proto.User, repo proto.Repos
 45 
 46 	payload.Repository.Owner.ID = owner.ID
 47 	payload.Repository.Owner.Username = owner.Username
 48-	payload.Repository.DefaultBranch, err = proto.RepositoryDefaultBranch(repo)
 49+	payload.Repository.DefaultBranch, err = getDefaultBranch(repo)
 50 	if err != nil {
 51 		return CollaboratorEvent{}, err
 52 	}
 53diff --git a/pkg/webhook/push.go b/pkg/webhook/push.go
 54index 6ef6062cdd9e4acd499ec09421e18fb273535802..a25d793797781e515bdf8cae883c86c81f9bce6c 100644
 55--- a/pkg/webhook/push.go
 56+++ b/pkg/webhook/push.go
 57@@ -2,7 +2,6 @@ package webhook
 58 
 59 import (
 60 	"context"
 61-	"errors"
 62 	"fmt"
 63 
 64 	gitm "github.com/aymanbagabas/git-module"
 65@@ -75,12 +74,8 @@ func NewPushEvent(ctx context.Context, user proto.User, repo proto.Repository, r
 66 		return PushEvent{}, err
 67 	}
 68 
 69-	payload.Repository.DefaultBranch, err = proto.RepositoryDefaultBranch(repo)
 70-	// XXX: we check for ErrReferenceNotExist here because we don't want to
 71-	// return an error if the repo is an empty repo.
 72-	// This means that the repo doesn't have a default branch yet and this is
 73-	// the first push to it.
 74-	if err != nil && !errors.Is(err, git.ErrReferenceNotExist) {
 75+	payload.Repository.DefaultBranch, err = getDefaultBranch(repo)
 76+	if err != nil {
 77 		return PushEvent{}, err
 78 	}
 79 
 80diff --git a/pkg/webhook/repository.go b/pkg/webhook/repository.go
 81index 3cad5c39faa0afef813ea570a7e54530bf32bb71..99fad6eec333ca883e459a56e3faccefe9260e92 100644
 82--- a/pkg/webhook/repository.go
 83+++ b/pkg/webhook/repository.go
 84@@ -76,7 +76,10 @@ func NewRepositoryEvent(ctx context.Context, user proto.User, repo proto.Reposit
 85 
 86 	payload.Repository.Owner.ID = owner.ID
 87 	payload.Repository.Owner.Username = owner.Username
 88-	payload.Repository.DefaultBranch, _ = proto.RepositoryDefaultBranch(repo)
 89+	payload.Repository.DefaultBranch, err = getDefaultBranch(repo)
 90+	if err != nil {
 91+		return RepositoryEvent{}, err
 92+	}
 93 
 94 	return payload, nil
 95 }
 96diff --git a/pkg/webhook/webhook.go b/pkg/webhook/webhook.go
 97index e5c4d2bb28443316530d00af3ef032feeebd95c3..8bd28e9c19663e3f7034a99213c3009f3486e391 100644
 98--- a/pkg/webhook/webhook.go
 99+++ b/pkg/webhook/webhook.go
100@@ -7,12 +7,15 @@ import (
101 	"crypto/sha256"
102 	"encoding/hex"
103 	"encoding/json"
104+	"errors"
105 	"fmt"
106 	"io"
107 	"net/http"
108 
109+	"github.com/charmbracelet/soft-serve/git"
110 	"github.com/charmbracelet/soft-serve/pkg/db"
111 	"github.com/charmbracelet/soft-serve/pkg/db/models"
112+	"github.com/charmbracelet/soft-serve/pkg/proto"
113 	"github.com/charmbracelet/soft-serve/pkg/store"
114 	"github.com/charmbracelet/soft-serve/pkg/utils"
115 	"github.com/charmbracelet/soft-serve/pkg/version"
116@@ -142,3 +145,16 @@ func SendEvent(ctx context.Context, payload EventPayload) error {
117 func repoURL(publicURL string, repo string) string {
118 	return fmt.Sprintf("%s/%s.git", publicURL, utils.SanitizeRepo(repo))
119 }
120+
121+func getDefaultBranch(repo proto.Repository) (string, error) {
122+	branch, err := proto.RepositoryDefaultBranch(repo)
123+	// XXX: we check for ErrReferenceNotExist here because we don't want to
124+	// return an error if the repo is an empty repo.
125+	// This means that the repo doesn't have a default branch yet and this is
126+	// the first push to it.
127+	if err != nil && !errors.Is(err, git.ErrReferenceNotExist) {
128+		return "", err
129+	}
130+
131+	return branch, nil
132+}
133diff --git a/testscript/testdata/repo-collab.txtar b/testscript/testdata/repo-collab.txtar
134index d692831bbe59933d6069331182407e069308bc99..d2960693a7a5df941a0aac1a875dc22065972179 100644
135--- a/testscript/testdata/repo-collab.txtar
136+++ b/testscript/testdata/repo-collab.txtar
137@@ -23,6 +23,18 @@ soft repo collab remove test foo
138 soft repo collab list test
139 ! stdout .
140 
141+# create empty repo
142+soft repo create empty '-d "empty repo"'
143+
144+# add collab
145+soft repo collab add empty foo
146+# add collab again
147+# test issue #464 https://github.com/charmbracelet/soft-serve/issues/464
148+! soft repo collab add empty foo
149+stderr '.*already exists.*'
150+# a placeholder to reset stderr
151+soft help
152+
153 # stop the server
154 [windows] stopserver
155 [windows] ! stderr .
156diff --git a/testscript/testdata/soft-browse.txtar b/testscript/testdata/soft-browse.txtar
157index 2ab46f333db06b69cf8bab98066e8faededb5dec..7514f770d8f2ef5135107a0c265f2a809e7146fb 100644
158--- a/testscript/testdata/soft-browse.txtar
159+++ b/testscript/testdata/soft-browse.txtar
160@@ -3,7 +3,7 @@
161 [windows] skip
162 
163 # clone repo
164-git clone https://github.com/charmbracelet/catwalk.git catwalk
165+#git clone https://github.com/charmbracelet/catwalk.git catwalk
166 
167 # run soft browse
168 # disable this temporarily