From c2ff0328eee1565804313b07be2b20f91556bb71 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 24 Apr 2024 13:26:50 +0800 Subject: [PATCH 1/5] Fix a panic bug when head repository deleting (#30674) When visiting a pull request files which head repository has been deleted, it will panic because headrepo is nil. --- routers/web/repo/pull.go | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index a0a8e5410cf15..acd05b6af5aab 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -865,21 +865,21 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi if pull.HeadRepo != nil { ctx.Data["SourcePath"] = pull.HeadRepo.Link() + "/src/branch/" + util.PathEscapeSegments(pull.HeadBranch) - } - if !pull.HasMerged && ctx.Doer != nil { - perm, err := access_model.GetUserRepoPermission(ctx, pull.HeadRepo, ctx.Doer) - if err != nil { - ctx.ServerError("GetUserRepoPermission", err) - return - } + if !pull.HasMerged && ctx.Doer != nil { + perm, err := access_model.GetUserRepoPermission(ctx, pull.HeadRepo, ctx.Doer) + if err != nil { + ctx.ServerError("GetUserRepoPermission", err) + return + } - if perm.CanWrite(unit.TypeCode) || issues_model.CanMaintainerWriteToBranch(ctx, perm, pull.HeadBranch, ctx.Doer) { - ctx.Data["CanEditFile"] = true - ctx.Data["EditFileTooltip"] = ctx.Tr("repo.editor.edit_this_file") - ctx.Data["HeadRepoLink"] = pull.HeadRepo.Link() - ctx.Data["HeadBranchName"] = pull.HeadBranch - ctx.Data["BackToLink"] = setting.AppSubURL + ctx.Req.URL.RequestURI() + if perm.CanWrite(unit.TypeCode) || issues_model.CanMaintainerWriteToBranch(ctx, perm, pull.HeadBranch, ctx.Doer) { + ctx.Data["CanEditFile"] = true + ctx.Data["EditFileTooltip"] = ctx.Tr("repo.editor.edit_this_file") + ctx.Data["HeadRepoLink"] = pull.HeadRepo.Link() + ctx.Data["HeadBranchName"] = pull.HeadBranch + ctx.Data["BackToLink"] = setting.AppSubURL + ctx.Req.URL.RequestURI() + } } } } From 2fb5cbf3ee56c9be390549b7ef1806725d83562f Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 24 Apr 2024 14:35:45 +0800 Subject: [PATCH 2/5] Add test for #30675 --- tests/integration/pull_compare_test.go | 35 ++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/tests/integration/pull_compare_test.go b/tests/integration/pull_compare_test.go index 5ce8ea3031e13..fa7a960ae83b9 100644 --- a/tests/integration/pull_compare_test.go +++ b/tests/integration/pull_compare_test.go @@ -5,8 +5,14 @@ package integration import ( "net/http" + "net/url" "testing" + "code.gitea.io/gitea/models/db" + repo_model "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/models/unittest" + user_model "code.gitea.io/gitea/models/user" + repo_service "code.gitea.io/gitea/services/repository" "code.gitea.io/gitea/tests" "github.com/stretchr/testify/assert" @@ -32,4 +38,33 @@ func TestPullCompare(t *testing.T) { doc := NewHTMLParser(t, resp.Body) editButtonCount := doc.doc.Find(".diff-file-header-actions a[href*='/_edit/']").Length() assert.Greater(t, editButtonCount, 0, "Expected to find a button to edit a file in the PR diff view but there were none") + + onGiteaRun(t, func(t *testing.T, u *url.URL) { + defer tests.PrepareTestEnv(t)() + + session := loginUser(t, "user1") + testRepoFork(t, session, "user2", "repo1", "user1", "repo1") + testCreateBranch(t, session, "user1", "repo1", "branch/master", "master1", http.StatusSeeOther) + testEditFile(t, session, "user1", "repo1", "master1", "README.md", "Hello, World (Edited)\n") + resp := testPullCreate(t, session, "user1", "repo1", false, "master", "master1", "This is a pull title") + + // the max value on issue_index.yml for repo_id=1 is 5 + req = NewRequest(t, "GET", "/user2/repo1/pulls/6/files") + resp = session.MakeRequest(t, req, http.StatusOK) + doc := NewHTMLParser(t, resp.Body) + editButtonCount := doc.doc.Find(".diff-file-header-actions a[href*='/_edit/']").Length() + assert.Greater(t, editButtonCount, 0, "Expected to find a button to edit a file in the PR diff view but there were none") + + user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + repoForked := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user1", Name: "repo1"}) + // delete the head repository and revisit the PR diff view + err := repo_service.DeleteRepositoryDirectly(db.DefaultContext, user2, repoForked.ID) + assert.NoError(t, err) + + req = NewRequest(t, "GET", "/user2/repo1/pulls/6/files") + resp = session.MakeRequest(t, req, http.StatusOK) + doc = NewHTMLParser(t, resp.Body) + editButtonCount = doc.doc.Find(".diff-file-header-actions a[href*='/_edit/']").Length() + assert.EqualValues(t, editButtonCount, 0, "Expected not to find a button to edit a file in the PR diff view because head repository has been deleted") + }) } From 88f6c5883fa5dd681f19bd91f7ff9a264266e87a Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 24 Apr 2024 14:52:41 +0800 Subject: [PATCH 3/5] Fix lint --- tests/integration/pull_compare_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/pull_compare_test.go b/tests/integration/pull_compare_test.go index fa7a960ae83b9..b814207b2fe22 100644 --- a/tests/integration/pull_compare_test.go +++ b/tests/integration/pull_compare_test.go @@ -46,7 +46,7 @@ func TestPullCompare(t *testing.T) { testRepoFork(t, session, "user2", "repo1", "user1", "repo1") testCreateBranch(t, session, "user1", "repo1", "branch/master", "master1", http.StatusSeeOther) testEditFile(t, session, "user1", "repo1", "master1", "README.md", "Hello, World (Edited)\n") - resp := testPullCreate(t, session, "user1", "repo1", false, "master", "master1", "This is a pull title") + resp = testPullCreate(t, session, "user1", "repo1", false, "master", "master1", "This is a pull title") // the max value on issue_index.yml for repo_id=1 is 5 req = NewRequest(t, "GET", "/user2/repo1/pulls/6/files") From 37353eb0b81af921b138da2f7207d5d86313e147 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 24 Apr 2024 22:41:44 +0800 Subject: [PATCH 4/5] Improve tests --- tests/integration/pull_compare_test.go | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/tests/integration/pull_compare_test.go b/tests/integration/pull_compare_test.go index b814207b2fe22..befbac8bbdf85 100644 --- a/tests/integration/pull_compare_test.go +++ b/tests/integration/pull_compare_test.go @@ -4,11 +4,13 @@ package integration import ( + "fmt" "net/http" "net/url" "testing" "code.gitea.io/gitea/models/db" + issues_model "code.gitea.io/gitea/models/issues" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" @@ -46,22 +48,24 @@ func TestPullCompare(t *testing.T) { testRepoFork(t, session, "user2", "repo1", "user1", "repo1") testCreateBranch(t, session, "user1", "repo1", "branch/master", "master1", http.StatusSeeOther) testEditFile(t, session, "user1", "repo1", "master1", "README.md", "Hello, World (Edited)\n") - resp = testPullCreate(t, session, "user1", "repo1", false, "master", "master1", "This is a pull title") + testPullCreate(t, session, "user1", "repo1", false, "master", "master1", "This is a pull title") - // the max value on issue_index.yml for repo_id=1 is 5 - req = NewRequest(t, "GET", "/user2/repo1/pulls/6/files") + repoForked := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user1", Name: "repo1"}) + issueIndex := unittest.AssertExistsAndLoadBean(t, &issues_model.IssueIndex{GroupID: repoForked.ID}) + prFilesURL := fmt.Sprintf("/user2/repo1/pulls/%d/files", issueIndex.MaxIndex) + req = NewRequest(t, "GET", prFilesURL) resp = session.MakeRequest(t, req, http.StatusOK) doc := NewHTMLParser(t, resp.Body) editButtonCount := doc.doc.Find(".diff-file-header-actions a[href*='/_edit/']").Length() assert.Greater(t, editButtonCount, 0, "Expected to find a button to edit a file in the PR diff view but there were none") user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) - repoForked := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user1", Name: "repo1"}) + // delete the head repository and revisit the PR diff view err := repo_service.DeleteRepositoryDirectly(db.DefaultContext, user2, repoForked.ID) assert.NoError(t, err) - req = NewRequest(t, "GET", "/user2/repo1/pulls/6/files") + req = NewRequest(t, "GET", prFilesURL) resp = session.MakeRequest(t, req, http.StatusOK) doc = NewHTMLParser(t, resp.Body) editButtonCount = doc.doc.Find(".diff-file-header-actions a[href*='/_edit/']").Length() From 853b656beac59f77bd22a36c720f58cc5ed05214 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 24 Apr 2024 22:56:02 +0800 Subject: [PATCH 5/5] Fix test --- tests/integration/pull_compare_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/integration/pull_compare_test.go b/tests/integration/pull_compare_test.go index befbac8bbdf85..39d9103dfd9f7 100644 --- a/tests/integration/pull_compare_test.go +++ b/tests/integration/pull_compare_test.go @@ -50,8 +50,8 @@ func TestPullCompare(t *testing.T) { testEditFile(t, session, "user1", "repo1", "master1", "README.md", "Hello, World (Edited)\n") testPullCreate(t, session, "user1", "repo1", false, "master", "master1", "This is a pull title") - repoForked := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user1", Name: "repo1"}) - issueIndex := unittest.AssertExistsAndLoadBean(t, &issues_model.IssueIndex{GroupID: repoForked.ID}) + repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user2", Name: "repo1"}) + issueIndex := unittest.AssertExistsAndLoadBean(t, &issues_model.IssueIndex{GroupID: repo1.ID}, unittest.OrderBy("group_id ASC")) prFilesURL := fmt.Sprintf("/user2/repo1/pulls/%d/files", issueIndex.MaxIndex) req = NewRequest(t, "GET", prFilesURL) resp = session.MakeRequest(t, req, http.StatusOK) @@ -59,6 +59,7 @@ func TestPullCompare(t *testing.T) { editButtonCount := doc.doc.Find(".diff-file-header-actions a[href*='/_edit/']").Length() assert.Greater(t, editButtonCount, 0, "Expected to find a button to edit a file in the PR diff view but there were none") + repoForked := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user1", Name: "repo1"}) user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) // delete the head repository and revisit the PR diff view