fix git repo

wxiaoguang · wxiaoguang · commit 396f6db6f7e8 · 2024-12-23T02:31:13.000+08:00
diff --git a/modules/gitrepo/gitrepo.go b/modules/gitrepo/gitrepo.go
@@ -55,7 +55,7 @@ func RepositoryFromContextOrOpen(ctx context.Context, repo Repository) (*git.Rep
 
 // RepositoryFromRequestContextOrOpen opens the repository at the given relative path in the provided request context
 // The repo will be automatically closed when the request context is done
-func RepositoryFromRequestContextOrOpen(ctx *reqctx.RequestContext, repo Repository) (*git.Repository, error) {
+func RepositoryFromRequestContextOrOpen(ctx reqctx.RequestContext, repo Repository) (*git.Repository, error) {
 	ck := contextKey{repoPath: repoPath(repo)}
 	if gitRepo, ok := ctx.Value(ck).(*git.Repository); ok {
 		return gitRepo, nil
diff --git a/modules/reqctx/reqctx.go b/modules/reqctx/reqctx.go
@@ -5,6 +5,7 @@ package reqctx
 
 import (
 	"context"
+	"io"
 	"sync"
 	"time"
 
@@ -15,8 +16,16 @@ type RequestContextKeyType struct{}
 
 var RequestContextKey RequestContextKeyType
 
+type RequestContext interface {
+	context.Context
+	SetContextValue(k, v any)
+	GetData() ContextData
+	AddCleanUp(f func())
+	AddCloser(c io.Closer)
+}
+
 // RequestContext is a short-lived context that is used to store request-specific data.
-type RequestContext struct {
+type requestContext struct {
 	ctx  context.Context
 	data ContextData
 
@@ -26,19 +35,19 @@ type RequestContext struct {
 	cleanUpFuncs []func()
 }
 
-func (r *RequestContext) Deadline() (deadline time.Time, ok bool) {
+func (r *requestContext) Deadline() (deadline time.Time, ok bool) {
 	return r.ctx.Deadline()
 }
 
-func (r *RequestContext) Done() <-chan struct{} {
+func (r *requestContext) Done() <-chan struct{} {
 	return r.ctx.Done()
 }
 
-func (r *RequestContext) Err() error {
+func (r *requestContext) Err() error {
 	return r.ctx.Err()
 }
 
-func (r *RequestContext) Value(key any) any {
+func (r *requestContext) Value(key any) any {
 	if key == RequestContextKey {
 		return r
 	}
@@ -51,42 +60,46 @@ func (r *RequestContext) Value(key any) any {
 	return r.ctx.Value(key)
 }
 
-func (r *RequestContext) SetContextValue(k, v any) {
+func (r *requestContext) SetContextValue(k, v any) {
 	r.mu.Lock()
 	r.values[k] = v
 	r.mu.Unlock()
 }
 
 // GetData and the underlying ContextData are not thread-safe, callers should ensure thread-safety.
-func (r *RequestContext) GetData() ContextData {
+func (r *requestContext) GetData() ContextData {
 	if r.data == nil {
 		r.data = make(ContextData)
 	}
 	return r.data
 }
 
-func (r *RequestContext) AddCleanUp(f func()) {
+func (r *requestContext) AddCleanUp(f func()) {
 	r.mu.Lock()
 	r.cleanUpFuncs = append(r.cleanUpFuncs, f)
 	r.mu.Unlock()
 }
 
-func (r *RequestContext) cleanUp() {
+func (r *requestContext) AddCloser(c io.Closer) {
+	r.AddCleanUp(func() { _ = c.Close() })
+}
+
+func (r *requestContext) cleanUp() {
 	for _, f := range r.cleanUpFuncs {
 		f()
 	}
 }
 
-func GetRequestContext(ctx context.Context) *RequestContext {
-	if req, ok := ctx.Value(RequestContextKey).(*RequestContext); ok {
+func GetRequestContext(ctx context.Context) RequestContext {
+	if req, ok := ctx.Value(RequestContextKey).(*requestContext); ok {
 		return req
 	}
 	return nil
 }
 
-func NewRequestContext(parentCtx context.Context, profDesc string) (_ *RequestContext, finished func()) {
+func NewRequestContext(parentCtx context.Context, profDesc string) (_ RequestContext, finished func()) {
 	ctx, _, processFinished := process.GetManager().AddTypedContext(parentCtx, profDesc, process.RequestProcessType, true)
-	reqCtx := &RequestContext{ctx: ctx, values: make(map[any]any)}
+	reqCtx := &requestContext{ctx: ctx, values: make(map[any]any)}
 	return reqCtx, func() {
 		reqCtx.cleanUp()
 		processFinished()
@@ -95,7 +108,7 @@ func NewRequestContext(parentCtx context.Context, profDesc string) (_ *RequestCo
 
 // NewRequestContextForTest creates a new RequestContext for testing purposes
 // It doesn't add the context to the process manager, nor do cleanup
-func NewRequestContextForTest(parentCtx context.Context) *RequestContext {
-	reqCtx := &RequestContext{ctx: parentCtx, values: make(map[any]any)}
+func NewRequestContextForTest(parentCtx context.Context) RequestContext {
+	reqCtx := &requestContext{ctx: parentCtx, values: make(map[any]any)}
 	return reqCtx
 }
diff --git a/routers/api/v1/repo/branch.go b/routers/api/v1/repo/branch.go
@@ -729,15 +729,11 @@ func CreateBranchProtection(ctx *context.APIContext) {
 	} else {
 		if !isPlainRule {
 			if ctx.Repo.GitRepo == nil {
-				ctx.Repo.GitRepo, err = gitrepo.OpenRepository(ctx, ctx.Repo.Repository)
+				ctx.Repo.GitRepo, err = gitrepo.RepositoryFromRequestContextOrOpen(ctx, ctx.Repo.Repository)
 				if err != nil {
 					ctx.Error(http.StatusInternalServerError, "OpenRepository", err)
 					return
 				}
-				defer func() {
-					ctx.Repo.GitRepo.Close()
-					ctx.Repo.GitRepo = nil
-				}()
 			}
 			// FIXME: since we only need to recheck files protected rules, we could improve this
 			matchedBranches, err := git_model.FindAllMatchedBranches(ctx, ctx.Repo.Repository.ID, ruleName)
@@ -1066,10 +1062,6 @@ func EditBranchProtection(ctx *context.APIContext) {
 					ctx.Error(http.StatusInternalServerError, "OpenRepository", err)
 					return
 				}
-				defer func() {
-					ctx.Repo.GitRepo.Close()
-					ctx.Repo.GitRepo = nil
-				}()
 			}
 
 			// FIXME: since we only need to recheck files protected rules, we could improve this
diff --git a/routers/api/v1/repo/compare.go b/routers/api/v1/repo/compare.go
@@ -44,13 +44,12 @@ func CompareDiff(ctx *context.APIContext) {
 	//     "$ref": "#/responses/notFound"
 
 	if ctx.Repo.GitRepo == nil {
-		gitRepo, err := gitrepo.OpenRepository(ctx, ctx.Repo.Repository)
+		var err error
+		ctx.Repo.GitRepo, err = gitrepo.RepositoryFromRequestContextOrOpen(ctx, ctx.Repo.Repository)
 		if err != nil {
 			ctx.Error(http.StatusInternalServerError, "OpenRepository", err)
 			return
 		}
-		ctx.Repo.GitRepo = gitRepo
-		defer gitRepo.Close()
 	}
 
 	infoPath := ctx.PathParam("*")
diff --git a/routers/api/v1/repo/download.go b/routers/api/v1/repo/download.go
@@ -28,13 +28,12 @@ func DownloadArchive(ctx *context.APIContext) {
 	}
 
 	if ctx.Repo.GitRepo == nil {
-		gitRepo, err := gitrepo.OpenRepository(ctx, ctx.Repo.Repository)
+		var err error
+		ctx.Repo.GitRepo, err = gitrepo.RepositoryFromRequestContextOrOpen(ctx, ctx.Repo.Repository)
 		if err != nil {
 			ctx.Error(http.StatusInternalServerError, "OpenRepository", err)
 			return
 		}
-		ctx.Repo.GitRepo = gitRepo
-		defer gitRepo.Close()
 	}
 
 	r, err := archiver_service.NewRequest(ctx.Repo.Repository.ID, ctx.Repo.GitRepo, ctx.PathParam("*"), tp)
diff --git a/routers/api/v1/repo/file.go b/routers/api/v1/repo/file.go
@@ -287,13 +287,12 @@ func GetArchive(ctx *context.APIContext) {
 	//     "$ref": "#/responses/notFound"
 
 	if ctx.Repo.GitRepo == nil {
-		gitRepo, err := gitrepo.OpenRepository(ctx, ctx.Repo.Repository)
+		var err error
+		ctx.Repo.GitRepo, err = gitrepo.RepositoryFromRequestContextOrOpen(ctx, ctx.Repo.Repository)
 		if err != nil {
 			ctx.Error(http.StatusInternalServerError, "OpenRepository", err)
 			return
 		}
-		ctx.Repo.GitRepo = gitRepo
-		defer gitRepo.Close()
 	}
 
 	archiveDownload(ctx)
diff --git a/routers/api/v1/repo/repo.go b/routers/api/v1/repo/repo.go
@@ -726,12 +726,11 @@ func updateBasicProperties(ctx *context.APIContext, opts api.EditRepoOption) err
 
 	if ctx.Repo.GitRepo == nil && !repo.IsEmpty {
 		var err error
-		ctx.Repo.GitRepo, err = gitrepo.OpenRepository(ctx, repo)
+		ctx.Repo.GitRepo, err = gitrepo.RepositoryFromRequestContextOrOpen(ctx, repo)
 		if err != nil {
 			ctx.Error(http.StatusInternalServerError, "Unable to OpenRepository", err)
 			return err
 		}
-		defer ctx.Repo.GitRepo.Close()
 	}
 
 	// Default branch only updated if changed and exist or the repository is empty
diff --git a/routers/api/v1/repo/transfer.go b/routers/api/v1/repo/transfer.go
@@ -100,7 +100,7 @@ func Transfer(ctx *context.APIContext) {
 	}
 
 	if ctx.Repo.GitRepo != nil {
-		ctx.Repo.GitRepo.Close()
+		_ = ctx.Repo.GitRepo.Close()
 		ctx.Repo.GitRepo = nil
 	}
 
diff --git a/routers/private/internal_repo.go b/routers/private/internal_repo.go
@@ -4,7 +4,6 @@
 package private
 
 import (
-	"context"
 	"fmt"
 	"net/http"
 
@@ -18,14 +17,14 @@ import (
 // This file contains common functions relating to setting the Repository for the internal routes
 
 // RepoAssignment assigns the repository and gitrepository to the private context
-func RepoAssignment(ctx *gitea_context.PrivateContext) context.CancelFunc {
+func RepoAssignment(ctx *gitea_context.PrivateContext) {
 	ownerName := ctx.PathParam(":owner")
 	repoName := ctx.PathParam(":repo")
 
 	repo := loadRepository(ctx, ownerName, repoName)
 	if ctx.Written() {
 		// Error handled in loadRepository
-		return nil
+		return
 	}
 
 	gitRepo, err := gitrepo.OpenRepository(ctx, repo)
@@ -34,23 +33,14 @@ func RepoAssignment(ctx *gitea_context.PrivateContext) context.CancelFunc {
 		ctx.JSON(http.StatusInternalServerError, private.Response{
 			Err: fmt.Sprintf("Failed to open repository: %s/%s Error: %v", ownerName, repoName, err),
 		})
-		return nil
+		return
 	}
 
 	ctx.Repo = &gitea_context.Repository{
 		Repository: repo,
 		GitRepo:    gitRepo,
 	}
-
-	// We opened it, we should close it
-	cancel := func() {
-		// If it's been set to nil then assume someone else has closed it.
-		if ctx.Repo.GitRepo != nil {
-			ctx.Repo.GitRepo.Close()
-		}
-	}
-
-	return cancel
+	ctx.AddCloser(gitRepo) // We opened it, we should close it
 }
 
 func loadRepository(ctx *gitea_context.PrivateContext, ownerName, repoName string) *repo_model.Repository {
diff --git a/routers/web/web.go b/routers/web/web.go
@@ -4,7 +4,6 @@
 package web
 
 import (
-	gocontext "context"
 	"net/http"
 	"strings"
 
@@ -1521,24 +1520,24 @@ func registerRoutes(m *web.Router) {
 
 		m.Group("/blob_excerpt", func() {
 			m.Get("/{sha}", repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.ExcerptBlob)
-		}, func(ctx *context.Context) gocontext.CancelFunc {
+		}, func(ctx *context.Context) {
 			// FIXME: refactor this function, use separate routes for wiki/code
 			if ctx.FormBool("wiki") {
 				ctx.Data["PageIsWiki"] = true
 				repo.MustEnableWiki(ctx)
-				return nil
+				return
 			}
 
 			if ctx.Written() {
-				return nil
+				return
 			}
-			cancel := context.RepoRef()(ctx)
+			context.RepoRef()(ctx)
 			if ctx.Written() {
-				return cancel
+				return
 			}
 
 			repo.MustBeNotEmpty(ctx)
-			return cancel
+			return
 		})
 
 		m.Group("/media", func() {
diff --git a/services/context/api.go b/services/context/api.go
@@ -5,7 +5,6 @@
 package context
 
 import (
-	"context"
 	"fmt"
 	"net/http"
 	"net/url"
@@ -265,31 +264,24 @@ func (ctx *APIContext) NotFound(objs ...any) {
 
 // ReferencesGitRepo injects the GitRepo into the Context
 // you can optional skip the IsEmpty check
-func ReferencesGitRepo(allowEmpty ...bool) func(ctx *APIContext) (cancel context.CancelFunc) {
-	return func(ctx *APIContext) (cancel context.CancelFunc) {
+func ReferencesGitRepo(allowEmpty ...bool) func(ctx *APIContext) {
+	return func(ctx *APIContext) {
 		// Empty repository does not have reference information.
 		if ctx.Repo.Repository.IsEmpty && !(len(allowEmpty) != 0 && allowEmpty[0]) {
-			return nil
+			return
 		}
 
 		// For API calls.
 		if ctx.Repo.GitRepo == nil {
-			gitRepo, err := gitrepo.OpenRepository(ctx, ctx.Repo.Repository)
+			var err error
+			ctx.Repo.GitRepo, err = gitrepo.RepositoryFromRequestContextOrOpen(ctx, ctx.Repo.Repository)
 			if err != nil {
 				ctx.Error(http.StatusInternalServerError, fmt.Sprintf("Open Repository %v failed", ctx.Repo.Repository.FullName()), err)
-				return cancel
-			}
-			ctx.Repo.GitRepo = gitRepo
-			// We opened it, we should close it
-			return func() {
-				// If it's been set to nil then assume someone else has closed it.
-				if ctx.Repo.GitRepo != nil {
-					_ = ctx.Repo.GitRepo.Close()
-				}
+				return
 			}
 		}
 
-		return cancel
+		return
 	}
 }
 
diff --git a/services/context/base.go b/services/context/base.go
diff --git a/services/context/repo.go b/services/context/repo.go

Original file line number	Diff line number	Diff line change
`@@ -44,13 +44,12 @@ func CompareDiff(ctx *context.APIContext) {`
`44`	`44`	`// "$ref": "#/responses/notFound"`
`45`	`45`
`46`	`46`	`if ctx.Repo.GitRepo == nil {`
`47`		`- gitRepo, err := gitrepo.OpenRepository(ctx, ctx.Repo.Repository)`
	`47`	`+ var err error`
	`48`	`+ ctx.Repo.GitRepo, err = gitrepo.RepositoryFromRequestContextOrOpen(ctx, ctx.Repo.Repository)`
`48`	`49`	`if err != nil {`
`49`	`50`	`ctx.Error(http.StatusInternalServerError, "OpenRepository", err)`
`50`	`51`	`return`
`51`	`52`	`}`
`52`		`- ctx.Repo.GitRepo = gitRepo`
`53`		`- defer gitRepo.Close()`
`54`	`53`	`}`
`55`	`54`
`56`	`55`	`infoPath := ctx.PathParam("*")`
Original file line number	Diff line number	Diff line change
`@@ -28,13 +28,12 @@ func DownloadArchive(ctx *context.APIContext) {`
`28`	`28`	`}`
`29`	`29`
`30`	`30`	`if ctx.Repo.GitRepo == nil {`
`31`		`- gitRepo, err := gitrepo.OpenRepository(ctx, ctx.Repo.Repository)`
	`31`	`+ var err error`
	`32`	`+ ctx.Repo.GitRepo, err = gitrepo.RepositoryFromRequestContextOrOpen(ctx, ctx.Repo.Repository)`
`32`	`33`	`if err != nil {`
`33`	`34`	`ctx.Error(http.StatusInternalServerError, "OpenRepository", err)`
`34`	`35`	`return`
`35`	`36`	`}`
`36`		`- ctx.Repo.GitRepo = gitRepo`
`37`		`- defer gitRepo.Close()`
`38`	`37`	`}`
`39`	`38`
`40`	`39`	`r, err := archiver_service.NewRequest(ctx.Repo.Repository.ID, ctx.Repo.GitRepo, ctx.PathParam("*"), tp)`
Original file line number	Diff line number	Diff line change
`@@ -287,13 +287,12 @@ func GetArchive(ctx *context.APIContext) {`
`287`	`287`	`// "$ref": "#/responses/notFound"`
`288`	`288`
`289`	`289`	`if ctx.Repo.GitRepo == nil {`
`290`		`- gitRepo, err := gitrepo.OpenRepository(ctx, ctx.Repo.Repository)`
	`290`	`+ var err error`
	`291`	`+ ctx.Repo.GitRepo, err = gitrepo.RepositoryFromRequestContextOrOpen(ctx, ctx.Repo.Repository)`
`291`	`292`	`if err != nil {`
`292`	`293`	`ctx.Error(http.StatusInternalServerError, "OpenRepository", err)`
`293`	`294`	`return`
`294`	`295`	`}`
`295`		`- ctx.Repo.GitRepo = gitRepo`
`296`		`- defer gitRepo.Close()`
`297`	`296`	`}`
`298`	`297`
`299`	`298`	`archiveDownload(ctx)`
Original file line number	Diff line number	Diff line change
`@@ -726,12 +726,11 @@ func updateBasicProperties(ctx *context.APIContext, opts api.EditRepoOption) err`
`726`	`726`
`727`	`727`	`if ctx.Repo.GitRepo == nil && !repo.IsEmpty {`
`728`	`728`	`var err error`
`729`		`- ctx.Repo.GitRepo, err = gitrepo.OpenRepository(ctx, repo)`
	`729`	`+ ctx.Repo.GitRepo, err = gitrepo.RepositoryFromRequestContextOrOpen(ctx, repo)`
`730`	`730`	`if err != nil {`
`731`	`731`	`ctx.Error(http.StatusInternalServerError, "Unable to OpenRepository", err)`
`732`	`732`	`return err`
`733`	`733`	`}`
`734`		`- defer ctx.Repo.GitRepo.Close()`
`735`	`734`	`}`
`736`	`735`
`737`	`736`	`// Default branch only updated if changed and exist or the repository is empty`
Original file line number	Diff line number	Diff line change
`@@ -100,7 +100,7 @@ func Transfer(ctx *context.APIContext) {`
`100`	`100`	`}`
`101`	`101`
`102`	`102`	`if ctx.Repo.GitRepo != nil {`
`103`		`- ctx.Repo.GitRepo.Close()`
	`103`	`+ _ = ctx.Repo.GitRepo.Close()`
`104`	`104`	`ctx.Repo.GitRepo = nil`
`105`	`105`	`}`
`106`	`106`