Some improvements

Author: lunny

routers/api/v1/repo/pull.go

@@ -423,6 +423,9 @@ func CreatePullRequest(ctx *context.APIContext) {
 		return
 	}
 
+	// we just need to check the head repository's permission here because the base
+	// repository's permission is already checked in api.go with
+	// mustAllowPulls, reqRepoReader(unit.TypeCode)
 	if !ci.IsSameRepo() {
 		// user should have permission to read headrepo's codes
 		permHead, err := access_model.GetUserRepoPermission(ctx, ci.HeadRepo, ctx.Doer)

routers/common/compare.go

@@ -101,8 +101,6 @@ type CompareInfo struct {
 	HeadUser     *user_model.User
 	HeadRepo     *repo_model.Repository
 	HeadGitRepo  *git.Repository
-	RootRepo     *repo_model.Repository
-	OwnForkRepo  *repo_model.Repository
 	CompareInfo  *git.CompareInfo
 	close        func()
 	IsBaseCommit bool
@@ -289,38 +287,45 @@ func ParseComparePathParams(ctx context.Context, pathParam string, baseRepo *rep
 		}
 	}
 
+	ci.BaseFullRef, ci.IsBaseCommit, err = detectFullRef(ctx, baseRepo.ID, baseGitRepo, ci.BaseOriRef)
+	if err != nil {
+		ci.Close()
+		return nil, err
+	}
+
+	ci.HeadFullRef, ci.IsHeadCommit, err = detectFullRef(ctx, ci.HeadRepo.ID, ci.HeadGitRepo, ci.HeadOriRef)
+	if err != nil {
+		ci.Close()
+		return nil, err
+	}
+	return ci, nil
+}
+
+func (ci *CompareInfo) LoadRootRepoAndOwnForkRepo(ctx context.Context, baseRepo *repo_model.Repository, doer *user_model.User) (*repo_model.Repository, *repo_model.Repository, error) {
 	// find root repo
+	var rootRepo *repo_model.Repository
+	var err error
 	if !baseRepo.IsFork {
-		ci.RootRepo = baseRepo
+		rootRepo = baseRepo
 	} else {
 		if !ci.HeadRepo.IsFork {
-			ci.RootRepo = ci.HeadRepo
+			rootRepo = ci.HeadRepo
 		} else {
-			ci.RootRepo, err = getRootRepo(ctx, baseRepo)
+			rootRepo, err = getRootRepo(ctx, baseRepo)
 			if err != nil {
-				return nil, err
+				return nil, nil, err
 			}
 		}
 	}
 
 	// find ownfork repo
+	var ownForkRepo *repo_model.Repository
 	if doer != nil && ci.HeadRepo.OwnerID != doer.ID && baseRepo.OwnerID != doer.ID {
-		ci.OwnForkRepo, err = findHeadRepo(ctx, baseRepo, doer.ID)
+		ownForkRepo, err = findHeadRepo(ctx, baseRepo, doer.ID)
 		if err != nil {
-			return nil, err
+			return nil, nil, err
 		}
 	}
 
-	ci.BaseFullRef, ci.IsBaseCommit, err = detectFullRef(ctx, baseRepo.ID, baseGitRepo, ci.BaseOriRef)
-	if err != nil {
-		ci.Close()
-		return nil, err
-	}
-
-	ci.HeadFullRef, ci.IsHeadCommit, err = detectFullRef(ctx, ci.HeadRepo.ID, ci.HeadGitRepo, ci.HeadOriRef)
-	if err != nil {
-		ci.Close()
-		return nil, err
-	}
-	return ci, nil
+	return rootRepo, ownForkRepo, nil
 }

routers/web/repo/compare.go

@@ -444,13 +444,19 @@ func prepareCompareRepoBranchesTagsDropdowns(ctx *context.Context, ci *common.Co
 		ctx.Data["HeadTags"] = headTags
 	}
 
-	if ci.RootRepo != nil &&
-		ci.RootRepo.ID != ci.HeadRepo.ID &&
-		ci.RootRepo.ID != baseRepo.ID {
-		canRead := access_model.CheckRepoUnitUser(ctx, ci.RootRepo, ctx.Doer, unit.TypeCode)
+	rootRepo, ownForkRepo, err := ci.LoadRootRepoAndOwnForkRepo(ctx, baseRepo, ctx.Doer)
+	if err != nil {
+		ctx.ServerError("LoadRootRepoAndOwnForkRepo", err)
+		return
+	}
+
+	if rootRepo != nil &&
+		rootRepo.ID != ci.HeadRepo.ID &&
+		rootRepo.ID != baseRepo.ID {
+		canRead := access_model.CheckRepoUnitUser(ctx, rootRepo, ctx.Doer, unit.TypeCode)
 		if canRead {
-			ctx.Data["RootRepo"] = ci.RootRepo
-			branches, tags, err := getBranchesAndTagsForRepo(ctx, ci.RootRepo)
+			ctx.Data["RootRepo"] = rootRepo
+			branches, tags, err := getBranchesAndTagsForRepo(ctx, rootRepo)
 			if err != nil {
 				ctx.ServerError("GetBranchesForRepo", err)
 				return
@@ -460,12 +466,12 @@ func prepareCompareRepoBranchesTagsDropdowns(ctx *context.Context, ci *common.Co
 		}
 	}
 
-	if ci.OwnForkRepo != nil &&
-		ci.OwnForkRepo.ID != ci.HeadRepo.ID &&
-		ci.OwnForkRepo.ID != baseRepo.ID &&
-		(ci.RootRepo == nil || ci.OwnForkRepo.ID != ci.RootRepo.ID) {
-		ctx.Data["OwnForkRepo"] = ci.OwnForkRepo
-		branches, tags, err := getBranchesAndTagsForRepo(ctx, ci.OwnForkRepo)
+	if ownForkRepo != nil &&
+		ownForkRepo.ID != ci.HeadRepo.ID &&
+		ownForkRepo.ID != baseRepo.ID &&
+		(rootRepo == nil || ownForkRepo.ID != rootRepo.ID) {
+		ctx.Data["OwnForkRepo"] = ownForkRepo
+		branches, tags, err := getBranchesAndTagsForRepo(ctx, ownForkRepo)
 		if err != nil {
 			ctx.ServerError("GetBranchesForRepo", err)
 			return