bug: fix assignees double load bug

[a1012112796]

Because the assigness has been loaded in

compare.go 416:
RetrieveRepoMetas(ctx, ctx.Repo.Repository, true)
then
issue.go 381
RetrieveRepoMilestonesAndAssignees(ctx, repo)
then

issue.go 361 -- 365 , they are load assignees

So the code on compare.go 425 -- 427 is double work,
and which is the reason of #10853

close #10853

[zeripath]

Ok so the function you're looking at is:

gitea/routers/repo/compare.go

Lines 374 to 442 in 90919bb

	// CompareDiff show different from one commit to another commit
	func CompareDiff(ctx *context.Context) {
	headUser, headRepo, headGitRepo, compareInfo, baseBranch, headBranch := ParseCompareInfo(ctx)
	if ctx.Written() {
	return
	}
	defer headGitRepo.Close()
	var err error
	if err = parseBaseRepoInfo(ctx, headRepo); err != nil {
	ctx.ServerError("parseBaseRepoInfo", err)
	return
	}
	nothingToCompare := PrepareCompareDiff(ctx, headUser, headRepo, headGitRepo, compareInfo, baseBranch, headBranch)
	if ctx.Written() {
	return
	}
	if ctx.Data["PageIsComparePull"] == true {
	headBranches, err := headGitRepo.GetBranches()
	if err != nil {
	ctx.ServerError("GetBranches", err)
	return
	}
	ctx.Data["HeadBranches"] = headBranches
	pr, err := models.GetUnmergedPullRequest(headRepo.ID, ctx.Repo.Repository.ID, headBranch, baseBranch)
	if err != nil {
	if !models.IsErrPullRequestNotExist(err) {
	ctx.ServerError("GetUnmergedPullRequest", err)
	return
	}
	} else {
	ctx.Data["HasPullRequest"] = true
	ctx.Data["PullRequest"] = pr
	ctx.HTML(200, tplCompareDiff)
	return
	}
	if !nothingToCompare {
	// Setup information for new form.
	RetrieveRepoMetas(ctx, ctx.Repo.Repository, true)
	if ctx.Written() {
	return
	}
	}
	}
	beforeCommitID := ctx.Data["BeforeCommitID"].(string)
	afterCommitID := ctx.Data["AfterCommitID"].(string)
	if ctx.Data["Assignees"], err = ctx.Repo.Repository.GetAssignees(); err != nil {
	ctx.ServerError("GetAssignees", err)
	return
	}
	ctx.Data["Title"] = "Comparing " + base.ShortSha(beforeCommitID) + "..." + base.ShortSha(afterCommitID)
	ctx.Data["IsRepoToolbarCommits"] = true
	ctx.Data["IsDiffCompare"] = true
	ctx.Data["RequireHighlightJS"] = true
	ctx.Data["RequireTribute"] = true
	ctx.Data["RequireSimpleMDE"] = true
	ctx.Data["PullRequestWorkInProgressPrefixes"] = setting.Repository.PullRequest.WorkInProgressPrefixes
	setTemplateIfExists(ctx, pullRequestTemplateKey, pullRequestTemplateCandidates)
	renderAttachmentSettings(ctx)
	ctx.HTML(200, tplCompare)
	}

In particular you suggest removing lines:

gitea/routers/repo/compare.go

Lines 425 to 428 in 90919bb

	if ctx.Data["Assignees"], err = ctx.Repo.Repository.GetAssignees(); err != nil {
	ctx.ServerError("GetAssignees", err)
	return
	}

As they are duplicated by the call here:

gitea/routers/repo/compare.go

Line 416 in 90919bb

RetrieveRepoMetas(ctx, ctx.Repo.Repository, true)

I.e.

gitea/routers/repo/issue.go

Lines 368 to 397 in 9d3e69e

	// RetrieveRepoMetas find all the meta information of a repository
	func RetrieveRepoMetas(ctx *context.Context, repo *models.Repository, isPull bool) []*models.Label {
	if !ctx.Repo.CanWriteIssuesOrPulls(isPull) {
	return nil
	}
	labels, err := models.GetLabelsByRepoID(repo.ID, "", models.ListOptions{})
	if err != nil {
	ctx.ServerError("GetLabelsByRepoID", err)
	return nil
	}
	ctx.Data["Labels"] = labels
	RetrieveRepoMilestonesAndAssignees(ctx, repo)
	if ctx.Written() {
	return nil
	}
	brs, err := ctx.Repo.GitRepo.GetBranches()
	if err != nil {
	ctx.ServerError("GetBranches", err)
	return nil
	}
	ctx.Data["Branches"] = brs
	// Contains true if the user can create issue dependencies
	ctx.Data["CanCreateIssueDependencies"] = ctx.Repo.CanCreateIssueDependencies(ctx.User, isPull)
	return labels
	}

Which calls:

gitea/routers/repo/issue.go

Lines 347 to 366 in 9d3e69e

	// RetrieveRepoMilestonesAndAssignees find all the milestones and assignees of a repository
	func RetrieveRepoMilestonesAndAssignees(ctx *context.Context, repo *models.Repository) {
	var err error
	ctx.Data["OpenMilestones"], err = models.GetMilestones(repo.ID, -1, false, "")
	if err != nil {
	ctx.ServerError("GetMilestones", err)
	return
	}
	ctx.Data["ClosedMilestones"], err = models.GetMilestones(repo.ID, -1, true, "")
	if err != nil {
	ctx.ServerError("GetMilestones", err)
	return
	}
	ctx.Data["Assignees"], err = repo.GetAssignees()
	if err != nil {
	ctx.ServerError("GetAssignees", err)
	return
	}
	}

In particular

gitea/routers/repo/issue.go

Line 361 in 9d3e69e

ctx.Data["Assignees"], err = repo.GetAssignees()

[zeripath]

Now the trouble is RetrieveRepoMetas is only called if:

gitea/routers/repo/compare.go

Line 393 in 90919bb

if ctx.Data["PageIsComparePull"] == true {

And

gitea/routers/repo/compare.go

Line 414 in 90919bb

if !nothingToCompare {

So it's not always a necessarily duplicate call. (Although it may be a completely unnecessary call ... I'm looking at this code likely for the first time from my phone and bed so I don't know completely.)

Would it be sufficient to add the opposite conditions on the ctx.Data['Assignees'] bit? Or is this call totally unnecessary in the first place?

[a1012112796]

Now the trouble is RetrieveRepoMetas is only called if:

gitea/routers/repo/compare.go

Line 393 in 90919bb

if ctx.Data["PageIsComparePull"] == true {

And

gitea/routers/repo/compare.go

Line 414 in 90919bb

if !nothingToCompare {

So it's not always a necessarily duplicate call. (Although it may be a completely unnecessary call ... I'm looking at this code likely for the first time from my phone and bed so I don't know completely.)

Would it be sufficient to add the opposite conditions on the ctx.Data['Assignees'] bit? Or is this call totally unnecessary in the first place?

Hello, I think this two check is all to check if it's ready to make a new pr, if not , It's not necessary to load them. because It's not useed in compare page. thanks.

you can see them in compare.tml :

gitea/templates/repo/diff/compare.tmpl

Line 6 in 7cd4704

{{if .PageIsComparePull}}

and

gitea/templates/repo/diff/compare.tmpl

Line 60 in 7cd4704

{{if .IsNothingToCompare}}

[zeripath]

OK the Assignees box is only shown if those two conditions are passed:

gitea/templates/repo/diff/compare.tmpl

Lines 60 to 83 in 7cd4704

	{{if .IsNothingToCompare}}
	<div class="ui segment">{{.i18n.Tr "repo.pulls.nothing_to_compare"}}</div>
	{{else if .PageIsComparePull}}
	{{if .HasPullRequest}}
	<div class="ui segment">
	{{.i18n.Tr "repo.pulls.has_pull_request" $.RepoLink $.RepoRelPath .PullRequest.Index | Safe}}
	</div>
	{{else}}
	{{if not .Repository.IsArchived}}
	<div class="ui info message show-form-container">
	<button class="ui button green show-form">{{.i18n.Tr "repo.pulls.new"}}</button>
	</div>
	{{ else }}
	<div class="ui warning message">
	{{.i18n.Tr "repo.archive.title"}}
	</div>
	{{ end }}
	<div class="pullrequest-form" style="display: none">
	{{template "repo/issue/new_form" .}}
	</div>
	{{template "repo/commits_table" .}}
	{{template "repo/diff/box" .}}
	{{end}}
	{{else}}

in particular the Assignees box comes from:

gitea/templates/repo/diff/compare.tmpl

Line 78 in 7cd4704

{{template "repo/issue/new_form" .}}

Therefore the get assignees call is definitely unnecessary for every other case and LGTM.

[codecov-io]

Codecov Report

Merging #10856 into master will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #10856   +/-   ##
=======================================
  Coverage   43.41%   43.41%           
=======================================
  Files         593      593           
  Lines       83265    83262    -3     
=======================================
+ Hits        36150    36152    +2     
+ Misses      42622    42619    -3     
+ Partials     4493     4491    -2     

Impacted Files	Coverage Δ
routers/repo/compare.go	40.93% <ø> (+0.49%)	⬆️
services/pull/check.go	50.00% <0.00%> (-3.05%)	⬇️
modules/git/command.go	86.95% <0.00%> (-2.61%)	⬇️
services/pull/pull.go	35.88% <0.00%> (+0.88%)	⬆️
modules/indexer/stats/db.go	50.00% <0.00%> (+9.37%)	⬆️
modules/indexer/stats/queue.go	81.25% <0.00%> (+18.75%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 828a27f...691639b. Read the comment docs.