Ensure Written is set in GZIP ProxyResponseWriter (#9018) (#9026)

Fix #9001

The GZIP ProxyReponseWriter doesn't currently respond correctly
to requests about its Written status - leading to #9001.

This PR properly reimplements these methods.

CHANGELOG.md

@@ -4,6 +4,38 @@ This changelog goes through all the changes that have been made in each release
 without substantial changes to our git log; to see the highlights of what has
 been added to each release, please refer to the [blog](https://blog.gitea.io).
 
+## [1.9.6](https://github.com/go-gitea/gitea/releases/tag/v1.9.6) - 2019-11-13
+* BUGFIXES
+  * Allow to merge if file path contains " or \ (#8629) (#8772)
+  * Fix 500 when edit hook (#8782) (#8790)
+  * Fix issue with user.fullname (#8904)
+  * Update Github Migration Test (#8897) (#8946)
+  * Add Close() method to gogitRepository (#8901) (#8958)
+
+## [1.9.5](https://github.com/go-gitea/gitea/releases/tag/v1.9.5) - 2019-10-30
+* BREAKING
+  * Hide some user information via API if user doesn't have enough permission (#8655) (#8658)
+* BUGFIXES
+  * Fix milestone close timestamp (#8728) (#8731)
+  * Fix deadline on update issue or PR via API (#8699)
+  * Fix 'New Issue Missing Milestone Comment' (#8678) (#8682)
+  * Fix 500 when getting user as unauthenticated user (#8653) (#8662)
+  * Use AppSubUrl for more redirections (#8647) (#8652)
+  * Add SubURL to redirect path (#8632) (#8634) (#8640)
+  * Fix #8582 by handling empty repos (#8587) (#8593)
+  * Fix bug on pull requests when transfer head repository (#8571)
+  * Add missed close in ServeBlobLFS (#8527) (#8543)
+  * Return false if provided branch name is empty for IsBranchExist (#8485) (#8492)
+  * Create .ssh dir as necessary (#8369) (#8486) (#8489)
+  * Restore functionality for early gits (#7775) (#8476)
+  * Add check for empty set when dropping indexes during migration (#8475)
+  * Ensure Request Body Readers are closed in LFS server (#8454) (#8459)
+  * Ensure that LFS files are relative to the LFS content path (#8455) (#8458)
+* SECURITY
+  * Ignore mentions for users with no access (#8395) (#8484)
+* TESTING
+  * Update heatmap fixtures to restore tests (#8615) (#8617)
+
 ## [1.9.4](https://github.com/go-gitea/gitea/releases/tag/v1.9.4) - 2019-10-08
 * BUGFIXES
   * Highlight issue references (#8101) (#8404)

cmd/admin.go

@@ -374,17 +374,20 @@ func runRepoSyncReleases(c *cli.Context) error {
 
 			if err = models.SyncReleasesWithTags(repo, gitRepo); err != nil {
 				log.Warn(" SyncReleasesWithTags: %v", err)
+				gitRepo.Close()
 				continue
 			}
 
 			count, err = getReleaseCount(repo.ID)
 			if err != nil {
 				log.Warn(" GetReleaseCountByRepoID: %v", err)
+				gitRepo.Close()
 				continue
 			}
 
 			log.Trace(" repo %s releases synchronized to tags: from %d to %d",
 				repo.FullName(), oldnum, count)
+			gitRepo.Close()
 		}
 	}
 

docs/content/doc/advanced/migrations.en-us.md

@@ -67,6 +67,7 @@ type Uploader interface {
 	CreateComment(issueNumber int64, comment *Comment) error
 	CreatePullRequest(pr *PullRequest) error
 	Rollback() error
+	Close()
 }
 
-```
+```

go.mod

@@ -83,8 +83,6 @@ require (
 	github.com/mattn/go-sqlite3 v1.11.0
 	github.com/mcuadros/go-version v0.0.0-20190308113854-92cdf37c5b75
 	github.com/microcosm-cc/bluemonday v0.0.0-20161012083705-f77f16ffc87a
-	github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
-	github.com/modern-go/reflect2 v1.0.1 // indirect
 	github.com/mschoch/smat v0.0.0-20160514031455-90eadee771ae // indirect
 	github.com/msteinert/pam v0.0.0-20151204160544-02ccfbfaf0cc
 	github.com/nfnt/resize v0.0.0-20160724205520-891127d8d1b5

integrations/api_releases_test.go

@@ -51,6 +51,7 @@ func TestAPICreateAndUpdateRelease(t *testing.T) {
 
 	gitRepo, err := git.OpenRepository(repo.RepoPath())
 	assert.NoError(t, err)
+	defer gitRepo.Close()
 
 	err = gitRepo.CreateTag("v0.0.1", "master")
 	assert.NoError(t, err)
@@ -112,6 +113,7 @@ func TestAPICreateReleaseToDefaultBranchOnExistingTag(t *testing.T) {
 
 	gitRepo, err := git.OpenRepository(repo.RepoPath())
 	assert.NoError(t, err)
+	defer gitRepo.Close()
 
 	err = gitRepo.CreateTag("v0.0.1", "master")
 	assert.NoError(t, err)

integrations/api_repo_file_create_test.go

@@ -139,6 +139,7 @@ func TestAPICreateFile(t *testing.T) {
 			assert.EqualValues(t, expectedFileResponse.Commit.HTMLURL, fileResponse.Commit.HTMLURL)
 			assert.EqualValues(t, expectedFileResponse.Commit.Author.Email, fileResponse.Commit.Author.Email)
 			assert.EqualValues(t, expectedFileResponse.Commit.Author.Name, fileResponse.Commit.Author.Name)
+			gitRepo.Close()
 		}
 
 		// Test creating a file in a new branch

integrations/api_repo_file_update_test.go

@@ -143,6 +143,7 @@ func TestAPIUpdateFile(t *testing.T) {
 			assert.EqualValues(t, expectedFileResponse.Commit.HTMLURL, fileResponse.Commit.HTMLURL)
 			assert.EqualValues(t, expectedFileResponse.Commit.Author.Email, fileResponse.Commit.Author.Email)
 			assert.EqualValues(t, expectedFileResponse.Commit.Author.Name, fileResponse.Commit.Author.Name)
+			gitRepo.Close()
 		}
 
 		// Test updating a file in a new branch

integrations/api_repo_get_contents_list_test.go

@@ -74,6 +74,8 @@ func testAPIGetContentsList(t *testing.T, u *url.URL) {
 	repo1.CreateNewBranch(user2, repo1.DefaultBranch, newBranch)
 	// Get the commit ID of the default branch
 	gitRepo, _ := git.OpenRepository(repo1.RepoPath())
+	defer gitRepo.Close()
+
 	commitID, _ := gitRepo.GetBranchCommitID(repo1.DefaultBranch)
 	// Make a new tag in repo1
 	newTag := "test_tag"

integrations/api_repo_get_contents_test.go

@@ -75,6 +75,8 @@ func testAPIGetContents(t *testing.T, u *url.URL) {
 	repo1.CreateNewBranch(user2, repo1.DefaultBranch, newBranch)
 	// Get the commit ID of the default branch
 	gitRepo, _ := git.OpenRepository(repo1.RepoPath())
+	defer gitRepo.Close()
+
 	commitID, _ := gitRepo.GetBranchCommitID(repo1.DefaultBranch)
 	// Make a new tag in repo1
 	newTag := "test_tag"

integrations/api_repo_git_tags_test.go

@@ -29,6 +29,8 @@ func TestAPIGitTags(t *testing.T) {
 	git.NewCommand("config", "user.email", user.Email).RunInDir(repo.RepoPath())
 
 	gitRepo, _ := git.OpenRepository(repo.RepoPath())
+	defer gitRepo.Close()
+
 	commit, _ := gitRepo.GetBranchCommit("master")
 	lTagName := "lightweightTag"
 	gitRepo.CreateTag(lTagName, commit.ID.String())

integrations/api_team_user_test.go

@@ -29,7 +29,6 @@ func TestAPITeamUser(t *testing.T) {
 	var user2 *api.User
 	DecodeJSON(t, resp, &user2)
 	user2.Created = user2.Created.In(time.Local)
-	user2.LastLogin = user2.LastLogin.In(time.Local)
 	user := models.AssertExistsAndLoadBean(t, &models.User{Name: "user2"}).(*models.User)
 
 	assert.Equal(t, convert.ToUser(user, true, false), user2)

integrations/api_user_heatmap_test.go

@@ -24,7 +24,7 @@ func TestUserHeatmap(t *testing.T) {
 	var heatmap []*models.UserHeatmapData
 	DecodeJSON(t, resp, &heatmap)
 	var dummyheatmap []*models.UserHeatmapData
-	dummyheatmap = append(dummyheatmap, &models.UserHeatmapData{Timestamp: 1540080000, Contributions: 1})
+	dummyheatmap = append(dummyheatmap, &models.UserHeatmapData{Timestamp: 1571616000, Contributions: 1})
 
 	assert.Equal(t, dummyheatmap, heatmap)
 }

integrations/git_helper_for_declarative_test.go

@@ -12,7 +12,9 @@ import (
 	"net/http"
 	"net/url"
 	"os"
+	"path"
 	"path/filepath"
+	"strings"
 	"testing"
 	"time"
 
@@ -36,7 +38,12 @@ func withKeyFile(t *testing.T, keyname string, callback func(string)) {
 	err = ssh.GenKeyPair(keyFile)
 	assert.NoError(t, err)
 
+	err = ioutil.WriteFile(path.Join(tmpDir, "ssh"), []byte("#!/bin/bash\n"+
+		"ssh -o \"UserKnownHostsFile=/dev/null\" -o \"StrictHostKeyChecking=no\" -o \"IdentitiesOnly=yes\" -i \""+keyFile+"\" \"$@\""), 0700)
+	assert.NoError(t, err)
+
 	//Setup ssh wrapper
+	os.Setenv("GIT_SSH", path.Join(tmpDir, "ssh"))
 	os.Setenv("GIT_SSH_COMMAND",
 		"ssh -o \"UserKnownHostsFile=/dev/null\" -o \"StrictHostKeyChecking=no\" -o \"IdentitiesOnly=yes\" -i \""+keyFile+"\"")
 	os.Setenv("GIT_SSH_VARIANT", "ssh")
@@ -53,6 +60,24 @@ func createSSHUrl(gitPath string, u *url.URL) *url.URL {
 	return &u2
 }
 
+func allowLFSFilters() []string {
+	// Now here we should explicitly allow lfs filters to run
+	globalArgs := git.GlobalCommandArgs
+	filteredLFSGlobalArgs := make([]string, len(git.GlobalCommandArgs))
+	j := 0
+	for _, arg := range git.GlobalCommandArgs {
+		if strings.Contains(arg, "lfs") {
+			j--
+		} else {
+			filteredLFSGlobalArgs[j] = arg
+			j++
+		}
+	}
+	filteredLFSGlobalArgs = filteredLFSGlobalArgs[:j]
+	git.GlobalCommandArgs = filteredLFSGlobalArgs
+	return globalArgs
+}
+
 func onGiteaRun(t *testing.T, callback func(*testing.T, *url.URL)) {
 	prepareTestEnv(t, 1)
 	s := http.Server{
@@ -78,7 +103,9 @@ func onGiteaRun(t *testing.T, callback func(*testing.T, *url.URL)) {
 
 func doGitClone(dstLocalPath string, u *url.URL) func(*testing.T) {
 	return func(t *testing.T) {
+		oldGlobals := allowLFSFilters()
 		assert.NoError(t, git.Clone(u.String(), dstLocalPath, git.CloneRepoOptions{}))
+		git.GlobalCommandArgs = oldGlobals
 		assert.True(t, com.IsExist(filepath.Join(dstLocalPath, "README.md")))
 	}
 }
@@ -139,7 +166,9 @@ func doGitCreateBranch(dstPath, branch string) func(*testing.T) {
 
 func doGitCheckoutBranch(dstPath string, args ...string) func(*testing.T) {
 	return func(t *testing.T) {
+		oldGlobals := allowLFSFilters()
 		_, err := git.NewCommand(append([]string{"checkout"}, args...)...).RunInDir(dstPath)
+		git.GlobalCommandArgs = oldGlobals
 		assert.NoError(t, err)
 	}
 }
@@ -153,7 +182,9 @@ func doGitMerge(dstPath string, args ...string) func(*testing.T) {
 
 func doGitPull(dstPath string, args ...string) func(*testing.T) {
 	return func(t *testing.T) {
+		oldGlobals := allowLFSFilters()
 		_, err := git.NewCommand(append([]string{"pull"}, args...)...).RunInDir(dstPath)
+		git.GlobalCommandArgs = oldGlobals
 		assert.NoError(t, err)
 	}
 }

integrations/git_test.go

@@ -19,6 +19,7 @@ import (
 
 	"code.gitea.io/gitea/models"
 	"code.gitea.io/gitea/modules/git"
+	"code.gitea.io/gitea/modules/setting"
 	api "code.gitea.io/gitea/modules/structs"
 
 	"github.com/stretchr/testify/assert"
@@ -135,6 +136,11 @@ func standardCommitAndPushTest(t *testing.T, dstPath string) (little, big string
 func lfsCommitAndPushTest(t *testing.T, dstPath string) (littleLFS, bigLFS string) {
 	t.Run("LFS", func(t *testing.T) {
 		PrintCurrentTest(t)
+		setting.CheckLFSVersion()
+		if !setting.LFS.StartServer {
+			t.Skip()
+			return
+		}
 		prefix := "lfs-data-file-"
 		_, err := git.NewCommand("lfs").AddArguments("install").RunInDir(dstPath)
 		assert.NoError(t, err)
@@ -142,6 +148,21 @@ func lfsCommitAndPushTest(t *testing.T, dstPath string) (littleLFS, bigLFS strin
 		assert.NoError(t, err)
 		err = git.AddChanges(dstPath, false, ".gitattributes")
 		assert.NoError(t, err)
+		oldGlobals := allowLFSFilters()
+		err = git.CommitChanges(dstPath, git.CommitChangesOptions{
+			Committer: &git.Signature{
+				Email: "user2@example.com",
+				Name:  "User Two",
+				When:  time.Now(),
+			},
+			Author: &git.Signature{
+				Email: "user2@example.com",
+				Name:  "User Two",
+				When:  time.Now(),
+			},
+			Message: fmt.Sprintf("Testing commit @ %v", time.Now()),
+		})
+		git.GlobalCommandArgs = oldGlobals
 
 		littleLFS, bigLFS = commitAndPushTest(t, dstPath, prefix)
 
@@ -185,20 +206,25 @@ func rawTest(t *testing.T, ctx *APITestContext, little, big, littleLFS, bigLFS s
 		resp := session.MakeRequest(t, req, http.StatusOK)
 		assert.Equal(t, littleSize, resp.Body.Len())
 
-		req = NewRequest(t, "GET", path.Join("/", username, reponame, "/raw/branch/master/", littleLFS))
-		resp = session.MakeRequest(t, req, http.StatusOK)
-		assert.NotEqual(t, littleSize, resp.Body.Len())
-		assert.Contains(t, resp.Body.String(), models.LFSMetaFileIdentifier)
+		setting.CheckLFSVersion()
+		if setting.LFS.StartServer {
+			req = NewRequest(t, "GET", path.Join("/", username, reponame, "/raw/branch/master/", littleLFS))
+			resp = session.MakeRequest(t, req, http.StatusOK)
+			assert.NotEqual(t, littleSize, resp.Body.Len())
+			assert.Contains(t, resp.Body.String(), models.LFSMetaFileIdentifier)
+		}
 
 		if !testing.Short() {
 			req = NewRequest(t, "GET", path.Join("/", username, reponame, "/raw/branch/master/", big))
 			resp = session.MakeRequest(t, req, http.StatusOK)
 			assert.Equal(t, bigSize, resp.Body.Len())
 
-			req = NewRequest(t, "GET", path.Join("/", username, reponame, "/raw/branch/master/", bigLFS))
-			resp = session.MakeRequest(t, req, http.StatusOK)
-			assert.NotEqual(t, bigSize, resp.Body.Len())
-			assert.Contains(t, resp.Body.String(), models.LFSMetaFileIdentifier)
+			if setting.LFS.StartServer {
+				req = NewRequest(t, "GET", path.Join("/", username, reponame, "/raw/branch/master/", bigLFS))
+				resp = session.MakeRequest(t, req, http.StatusOK)
+				assert.NotEqual(t, bigSize, resp.Body.Len())
+				assert.Contains(t, resp.Body.String(), models.LFSMetaFileIdentifier)
+			}
 		}
 	})
 }
@@ -217,18 +243,23 @@ func mediaTest(t *testing.T, ctx *APITestContext, little, big, littleLFS, bigLFS
 		resp := session.MakeRequestNilResponseRecorder(t, req, http.StatusOK)
 		assert.Equal(t, littleSize, resp.Length)
 
-		req = NewRequest(t, "GET", path.Join("/", username, reponame, "/media/branch/master/", littleLFS))
-		resp = session.MakeRequestNilResponseRecorder(t, req, http.StatusOK)
-		assert.Equal(t, littleSize, resp.Length)
+		setting.CheckLFSVersion()
+		if setting.LFS.StartServer {
+			req = NewRequest(t, "GET", path.Join("/", username, reponame, "/media/branch/master/", littleLFS))
+			resp = session.MakeRequestNilResponseRecorder(t, req, http.StatusOK)
+			assert.Equal(t, littleSize, resp.Length)
+		}
 
 		if !testing.Short() {
 			req = NewRequest(t, "GET", path.Join("/", username, reponame, "/media/branch/master/", big))
 			resp = session.MakeRequestNilResponseRecorder(t, req, http.StatusOK)
 			assert.Equal(t, bigSize, resp.Length)
 
-			req = NewRequest(t, "GET", path.Join("/", username, reponame, "/media/branch/master/", bigLFS))
-			resp = session.MakeRequestNilResponseRecorder(t, req, http.StatusOK)
-			assert.Equal(t, bigSize, resp.Length)
+			if setting.LFS.StartServer {
+				req = NewRequest(t, "GET", path.Join("/", username, reponame, "/media/branch/master/", bigLFS))
+				resp = session.MakeRequestNilResponseRecorder(t, req, http.StatusOK)
+				assert.Equal(t, bigSize, resp.Length)
+			}
 		}
 	})
 }
@@ -274,6 +305,8 @@ func generateCommitWithNewData(size int, repoPath, email, fullName, prefix strin
 	}
 
 	//Commit
+	// Now here we should explicitly allow lfs filters to run
+	oldGlobals := allowLFSFilters()
 	err = git.AddChanges(repoPath, false, filepath.Base(tmpFile.Name()))
 	if err != nil {
 		return "", err
@@ -291,6 +324,7 @@ func generateCommitWithNewData(size int, repoPath, email, fullName, prefix strin
 		},
 		Message: fmt.Sprintf("Testing commit @ %v", time.Now()),
 	})
+	git.GlobalCommandArgs = oldGlobals
 	return filepath.Base(tmpFile.Name()), err
 }
 

integrations/lfs_getobject_test.go

@@ -58,6 +58,11 @@ func storeObjectInRepo(t *testing.T, repositoryID int64, content *[]byte) string
 
 func doLfs(t *testing.T, content *[]byte, expectGzip bool) {
 	prepareTestEnv(t)
+	setting.CheckLFSVersion()
+	if !setting.LFS.StartServer {
+		t.Skip()
+		return
+	}
 	repo, err := models.GetRepositoryByOwnerAndName("user2", "repo1")
 	assert.NoError(t, err)
 	oid := storeObjectInRepo(t, repo.ID, content)

integrations/repofiles_delete_test.go

@@ -73,6 +73,7 @@ func testDeleteRepoFile(t *testing.T, u *url.URL) {
 	test.LoadRepoCommit(t, ctx)
 	test.LoadUser(t, ctx, 2)
 	test.LoadGitRepo(t, ctx)
+	defer ctx.Repo.GitRepo.Close()
 	repo := ctx.Repo.Repository
 	doer := ctx.User
 	opts := getDeleteRepoFileOptions(repo)
@@ -111,6 +112,8 @@ func testDeleteRepoFileWithoutBranchNames(t *testing.T, u *url.URL) {
 	test.LoadRepoCommit(t, ctx)
 	test.LoadUser(t, ctx, 2)
 	test.LoadGitRepo(t, ctx)
+	defer ctx.Repo.GitRepo.Close()
+
 	repo := ctx.Repo.Repository
 	doer := ctx.User
 	opts := getDeleteRepoFileOptions(repo)
@@ -139,6 +142,8 @@ func TestDeleteRepoFileErrors(t *testing.T) {
 	test.LoadRepoCommit(t, ctx)
 	test.LoadUser(t, ctx, 2)
 	test.LoadGitRepo(t, ctx)
+	defer ctx.Repo.GitRepo.Close()
+
 	repo := ctx.Repo.Repository
 	doer := ctx.User
 

integrations/repofiles_update_test.go

@@ -191,6 +191,8 @@ func TestCreateOrUpdateRepoFileForCreate(t *testing.T) {
 		test.LoadRepoCommit(t, ctx)
 		test.LoadUser(t, ctx, 2)
 		test.LoadGitRepo(t, ctx)
+		defer ctx.Repo.GitRepo.Close()
+
 		repo := ctx.Repo.Repository
 		doer := ctx.User
 		opts := getCreateRepoFileOptions(repo)
@@ -201,6 +203,8 @@ func TestCreateOrUpdateRepoFileForCreate(t *testing.T) {
 		// asserts
 		assert.Nil(t, err)
 		gitRepo, _ := git.OpenRepository(repo.RepoPath())
+		defer gitRepo.Close()
+
 		commitID, _ := gitRepo.GetBranchCommitID(opts.NewBranch)
 		expectedFileResponse := getExpectedFileResponseForRepofilesCreate(commitID)
 		assert.EqualValues(t, expectedFileResponse.Content, fileResponse.Content)
@@ -220,6 +224,8 @@ func TestCreateOrUpdateRepoFileForUpdate(t *testing.T) {
 		test.LoadRepoCommit(t, ctx)
 		test.LoadUser(t, ctx, 2)
 		test.LoadGitRepo(t, ctx)
+		defer ctx.Repo.GitRepo.Close()
+
 		repo := ctx.Repo.Repository
 		doer := ctx.User
 		opts := getUpdateRepoFileOptions(repo)
@@ -230,6 +236,8 @@ func TestCreateOrUpdateRepoFileForUpdate(t *testing.T) {
 		// asserts
 		assert.Nil(t, err)
 		gitRepo, _ := git.OpenRepository(repo.RepoPath())
+		defer gitRepo.Close()
+
 		commitID, _ := gitRepo.GetBranchCommitID(opts.NewBranch)
 		expectedFileResponse := getExpectedFileResponseForRepofilesUpdate(commitID, opts.TreePath)
 		assert.EqualValues(t, expectedFileResponse.Content, fileResponse.Content)
@@ -249,6 +257,8 @@ func TestCreateOrUpdateRepoFileForUpdateWithFileMove(t *testing.T) {
 		test.LoadRepoCommit(t, ctx)
 		test.LoadUser(t, ctx, 2)
 		test.LoadGitRepo(t, ctx)
+		defer ctx.Repo.GitRepo.Close()
+
 		repo := ctx.Repo.Repository
 		doer := ctx.User
 		opts := getUpdateRepoFileOptions(repo)
@@ -261,6 +271,8 @@ func TestCreateOrUpdateRepoFileForUpdateWithFileMove(t *testing.T) {
 		// asserts
 		assert.Nil(t, err)
 		gitRepo, _ := git.OpenRepository(repo.RepoPath())
+		defer gitRepo.Close()
+
 		commit, _ := gitRepo.GetBranchCommit(opts.NewBranch)
 		expectedFileResponse := getExpectedFileResponseForRepofilesUpdate(commit.ID.String(), opts.TreePath)
 		// assert that the old file no longer exists in the last commit of the branch
@@ -288,6 +300,8 @@ func TestCreateOrUpdateRepoFileWithoutBranchNames(t *testing.T) {
 		test.LoadRepoCommit(t, ctx)
 		test.LoadUser(t, ctx, 2)
 		test.LoadGitRepo(t, ctx)
+		defer ctx.Repo.GitRepo.Close()
+
 		repo := ctx.Repo.Repository
 		doer := ctx.User
 		opts := getUpdateRepoFileOptions(repo)
@@ -300,6 +314,8 @@ func TestCreateOrUpdateRepoFileWithoutBranchNames(t *testing.T) {
 		// asserts
 		assert.Nil(t, err)
 		gitRepo, _ := git.OpenRepository(repo.RepoPath())
+		defer gitRepo.Close()
+
 		commitID, _ := gitRepo.GetBranchCommitID(repo.DefaultBranch)
 		expectedFileResponse := getExpectedFileResponseForRepofilesUpdate(commitID, opts.TreePath)
 		assert.EqualValues(t, expectedFileResponse.Content, fileResponse.Content)
@@ -315,6 +331,8 @@ func TestCreateOrUpdateRepoFileErrors(t *testing.T) {
 		test.LoadRepoCommit(t, ctx)
 		test.LoadUser(t, ctx, 2)
 		test.LoadGitRepo(t, ctx)
+		defer ctx.Repo.GitRepo.Close()
+
 		repo := ctx.Repo.Repository
 		doer := ctx.User
 

models/action.go

@@ -762,6 +762,7 @@ func CommitRepoAction(opts CommitRepoActionOptions) error {
 			if err != nil {
 				log.Error("GetBranchCommitID[%s]: %v", opts.RefFullName, err)
 			}
+			gitRepo.Close()
 			if err = PrepareWebhooks(repo, HookEventCreate, &api.CreatePayload{
 				Ref:     refName,
 				Sha:     shaSum,
@@ -797,6 +798,8 @@ func CommitRepoAction(opts CommitRepoActionOptions) error {
 		if err != nil {
 			log.Error("GetTagCommitID[%s]: %v", opts.RefFullName, err)
 		}
+		gitRepo.Close()
+
 		if err = PrepareWebhooks(repo, HookEventCreate, &api.CreatePayload{
 			Ref:     refName,
 			Sha:     shaSum,

models/fixtures/action.yml

@@ -5,7 +5,7 @@
   act_user_id: 2
   repo_id: 2
   is_private: true
-  created_unix: 1540139562
+  created_unix: 1571686356
 
 -
   id: 2

models/git_diff.go

@@ -675,6 +675,7 @@ func GetDiffRangeWithWhitespaceBehavior(repoPath, beforeCommitID, afterCommitID
 	if err != nil {
 		return nil, err
 	}
+	defer gitRepo.Close()
 
 	commit, err := gitRepo.GetCommit(afterCommitID)
 	if err != nil {
@@ -747,6 +748,7 @@ func GetRawDiffForFile(repoPath, startCommit, endCommit string, diffType RawDiff
 	if err != nil {
 		return fmt.Errorf("OpenRepository: %v", err)
 	}
+	defer repo.Close()
 
 	commit, err := repo.GetCommit(endCommit)
 	if err != nil {

models/graph_test.go

@@ -17,6 +17,7 @@ func BenchmarkGetCommitGraph(b *testing.B) {
 	if err != nil {
 		b.Error("Could not open repository")
 	}
+	defer currentRepo.Close()
 
 	for i := 0; i < b.N; i++ {
 		graph, err := GetCommitGraph(currentRepo)

models/issue.go

@@ -1102,6 +1102,10 @@ func newIssue(e *xorm.Session, doer *User, opts NewIssueOptions) (err error) {
 		if _, err = e.Exec("UPDATE `milestone` SET num_issues=num_issues+1 WHERE id=?", opts.Issue.MilestoneID); err != nil {
 			return err
 		}
+
+		if _, err = createMilestoneComment(e, doer, opts.Repo, opts.Issue, 0, opts.Issue.MilestoneID); err != nil {
+			return err
+		}
 	}
 
 	// Insert the assignees
@@ -1462,46 +1466,18 @@ func getParticipantsByIssueID(e Engine, issueID int64) ([]*User, error) {
 	return users, e.In("id", userIDs).Find(&users)
 }
 
-// UpdateIssueMentions extracts mentioned people from content and
-// updates issue-user relations for them.
-func UpdateIssueMentions(e Engine, issueID int64, mentions []string) error {
+// UpdateIssueMentions updates issue-user relations for mentioned users.
+func UpdateIssueMentions(e Engine, issueID int64, mentions []*User) error {
 	if len(mentions) == 0 {
 		return nil
 	}
-
-	for i := range mentions {
-		mentions[i] = strings.ToLower(mentions[i])
+	ids := make([]int64, len(mentions))
+	for i, u := range mentions {
+		ids[i] = u.ID
 	}
-	users := make([]*User, 0, len(mentions))
-
-	if err := e.In("lower_name", mentions).Asc("lower_name").Find(&users); err != nil {
-		return fmt.Errorf("find mentioned users: %v", err)
-	}
-
-	ids := make([]int64, 0, len(mentions))
-	for _, user := range users {
-		ids = append(ids, user.ID)
-		if !user.IsOrganization() || user.NumMembers == 0 {
-			continue
-		}
-
-		memberIDs := make([]int64, 0, user.NumMembers)
-		orgUsers, err := getOrgUsersByOrgID(e, user.ID)
-		if err != nil {
-			return fmt.Errorf("GetOrgUsersByOrgID [%d]: %v", user.ID, err)
-		}
-
-		for _, orgUser := range orgUsers {
-			memberIDs = append(memberIDs, orgUser.ID)
-		}
-
-		ids = append(ids, memberIDs...)
-	}
-
 	if err := UpdateIssueUsersByMentions(e, issueID, ids); err != nil {
 		return fmt.Errorf("UpdateIssueUsersByMentions: %v", err)
 	}
-
 	return nil
 }
 
@@ -1854,3 +1830,120 @@ func (issue *Issue) updateClosedNum(e Engine) (err error) {
 	}
 	return
 }
+
+// ResolveMentionsByVisibility returns the users mentioned in an issue, removing those that
+// don't have access to reading it. Teams are expanded into their users, but organizations are ignored.
+func (issue *Issue) ResolveMentionsByVisibility(e Engine, doer *User, mentions []string) (users []*User, err error) {
+	if len(mentions) == 0 {
+		return
+	}
+	if err = issue.loadRepo(e); err != nil {
+		return
+	}
+	resolved := make(map[string]bool, 20)
+	names := make([]string, 0, 20)
+	resolved[doer.LowerName] = true
+	for _, name := range mentions {
+		name := strings.ToLower(name)
+		if _, ok := resolved[name]; ok {
+			continue
+		}
+		resolved[name] = false
+		names = append(names, name)
+	}
+
+	if err := issue.Repo.getOwner(e); err != nil {
+		return nil, err
+	}
+
+	if issue.Repo.Owner.IsOrganization() {
+		// Since there can be users with names that match the name of a team,
+		// if the team exists and can read the issue, the team takes precedence.
+		teams := make([]*Team, 0, len(names))
+		if err := e.
+			Join("INNER", "team_repo", "team_repo.team_id = team.id").
+			Where("team_repo.repo_id=?", issue.Repo.ID).
+			In("team.lower_name", names).
+			Find(&teams); err != nil {
+			return nil, fmt.Errorf("find mentioned teams: %v", err)
+		}
+		if len(teams) != 0 {
+			checked := make([]int64, 0, len(teams))
+			unittype := UnitTypeIssues
+			if issue.IsPull {
+				unittype = UnitTypePullRequests
+			}
+			for _, team := range teams {
+				if team.Authorize >= AccessModeOwner {
+					checked = append(checked, team.ID)
+					resolved[team.LowerName] = true
+					continue
+				}
+				has, err := e.Get(&TeamUnit{OrgID: issue.Repo.Owner.ID, TeamID: team.ID, Type: unittype})
+				if err != nil {
+					return nil, fmt.Errorf("get team units (%d): %v", team.ID, err)
+				}
+				if has {
+					checked = append(checked, team.ID)
+					resolved[team.LowerName] = true
+				}
+			}
+			if len(checked) != 0 {
+				teamusers := make([]*User, 0, 20)
+				if err := e.
+					Join("INNER", "team_user", "team_user.uid = `user`.id").
+					In("`team_user`.team_id", checked).
+					And("`user`.is_active = ?", true).
+					And("`user`.prohibit_login = ?", false).
+					Find(&teamusers); err != nil {
+					return nil, fmt.Errorf("get teams users: %v", err)
+				}
+				if len(teamusers) > 0 {
+					users = make([]*User, 0, len(teamusers))
+					for _, user := range teamusers {
+						if already, ok := resolved[user.LowerName]; !ok || !already {
+							users = append(users, user)
+							resolved[user.LowerName] = true
+						}
+					}
+				}
+			}
+		}
+
+		// Remove names already in the list to avoid querying the database if pending names remain
+		names = make([]string, 0, len(resolved))
+		for name, already := range resolved {
+			if !already {
+				names = append(names, name)
+			}
+		}
+		if len(names) == 0 {
+			return
+		}
+	}
+
+	unchecked := make([]*User, 0, len(names))
+	if err := e.
+		Where("`user`.is_active = ?", true).
+		And("`user`.prohibit_login = ?", false).
+		In("`user`.lower_name", names).
+		Find(&unchecked); err != nil {
+		return nil, fmt.Errorf("find mentioned users: %v", err)
+	}
+	for _, user := range unchecked {
+		if already := resolved[user.LowerName]; already || user.IsOrganization() {
+			continue
+		}
+		// Normal users must have read access to the referencing issue
+		perm, err := getUserRepoPermission(e, issue.Repo, user)
+		if err != nil {
+			return nil, fmt.Errorf("getUserRepoPermission [%d]: %v", user.ID, err)
+		}
+		if !perm.CanReadIssuesOrPulls(issue.IsPull) {
+			continue
+		}
+		users = append(users, user)
+	}
+
+	return
+}

models/issue_comment.go

@@ -387,11 +387,18 @@ func (c *Comment) MailParticipants(opType ActionType, issue *Issue) (err error)
 }
 
 func (c *Comment) mailParticipants(e Engine, opType ActionType, issue *Issue) (err error) {
-	mentions := markup.FindAllMentions(c.Content)
-	if err = UpdateIssueMentions(e, c.IssueID, mentions); err != nil {
+	rawMentions := markup.FindAllMentions(c.Content)
+	userMentions, err := issue.ResolveMentionsByVisibility(e, c.Poster, rawMentions)
+	if err != nil {
+		return fmt.Errorf("ResolveMentionsByVisibility [%d]: %v", c.IssueID, err)
+	}
+	if err = UpdateIssueMentions(e, c.IssueID, userMentions); err != nil {
 		return fmt.Errorf("UpdateIssueMentions [%d]: %v", c.IssueID, err)
 	}
-
+	mentions := make([]string, len(userMentions))
+	for i, u := range userMentions {
+		mentions[i] = u.LowerName
+	}
 	if len(c.Content) > 0 {
 		if err = mailIssueCommentToParticipants(e, issue, c.Poster, c.Content, c, mentions); err != nil {
 			log.Error("mailIssueCommentToParticipants: %v", err)
@@ -886,6 +893,7 @@ func CreateCodeComment(doer *User, repo *Repository, issue *Issue, content, tree
 	if err != nil {
 		return nil, fmt.Errorf("OpenRepository: %v", err)
 	}
+	defer gitRepo.Close()
 
 	// FIXME validate treePath
 	// Get latest commit referencing the commented line

models/issue_mail.go

@@ -123,11 +123,18 @@ func (issue *Issue) MailParticipants(doer *User, opType ActionType) (err error)
 }
 
 func (issue *Issue) mailParticipants(e Engine, doer *User, opType ActionType) (err error) {
-	mentions := markup.FindAllMentions(issue.Content)
-
-	if err = UpdateIssueMentions(e, issue.ID, mentions); err != nil {
+	rawMentions := markup.FindAllMentions(issue.Content)
+	userMentions, err := issue.ResolveMentionsByVisibility(e, doer, rawMentions)
+	if err != nil {
+		return fmt.Errorf("ResolveMentionsByVisibility [%d]: %v", issue.ID, err)
+	}
+	if err = UpdateIssueMentions(e, issue.ID, userMentions); err != nil {
 		return fmt.Errorf("UpdateIssueMentions [%d]: %v", issue.ID, err)
 	}
+	mentions := make([]string, len(userMentions))
+	for i, u := range userMentions {
+		mentions[i] = u.LowerName
+	}
 
 	if len(issue.Content) > 0 {
 		if err = mailIssueCommentToParticipants(e, issue, doer, issue.Content, nil, mentions); err != nil {

models/issue_milestone.go

@@ -6,6 +6,7 @@ package models
 
 import (
 	"fmt"
+	"time"
 
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/setting"
@@ -290,6 +291,10 @@ func ChangeMilestoneStatus(m *Milestone, isClosed bool) (err error) {
 	}
 
 	m.IsClosed = isClosed
+	if isClosed {
+		m.ClosedDateUnix = util.TimeStamp(time.Now().Unix())
+	}
+
 	if err = updateMilestone(sess, m); err != nil {
 		return err
 	}

models/issue_test.go

@@ -320,3 +320,35 @@ func TestIssue_SearchIssueIDsByKeyword(t *testing.T) {
 	assert.EqualValues(t, 1, total)
 	assert.EqualValues(t, []int64{1}, ids)
 }
+
+func TestIssue_ResolveMentions(t *testing.T) {
+	assert.NoError(t, PrepareTestDatabase())
+
+	testSuccess := func(owner, repo, doer string, mentions []string, expected []int64) {
+		o := AssertExistsAndLoadBean(t, &User{LowerName: owner}).(*User)
+		r := AssertExistsAndLoadBean(t, &Repository{OwnerID: o.ID, LowerName: repo}).(*Repository)
+		issue := &Issue{RepoID: r.ID}
+		d := AssertExistsAndLoadBean(t, &User{LowerName: doer}).(*User)
+		resolved, err := issue.ResolveMentionsByVisibility(x, d, mentions)
+		assert.NoError(t, err)
+		ids := make([]int64, len(resolved))
+		for i, user := range resolved {
+			ids[i] = user.ID
+		}
+		sort.Slice(ids, func(i, j int) bool { return ids[i] < ids[j] })
+		assert.EqualValues(t, expected, ids)
+	}
+
+	// Public repo, existing user
+	testSuccess("user2", "repo1", "user1", []string{"user5"}, []int64{5})
+	// Public repo, non-existing user
+	testSuccess("user2", "repo1", "user1", []string{"nonexisting"}, []int64{})
+	// Public repo, doer
+	testSuccess("user2", "repo1", "user1", []string{"user1"}, []int64{})
+	// Private repo, team member
+	testSuccess("user17", "big_test_private_4", "user20", []string{"user2"}, []int64{2})
+	// Private repo, not a team member
+	testSuccess("user17", "big_test_private_4", "user20", []string{"user5"}, []int64{})
+	// Private repo, whole team
+	testSuccess("user17", "big_test_private_4", "user15", []string{"owners"}, []int64{18})
+}

models/migrations/migrations.go

@@ -384,9 +384,11 @@ func dropTableColumns(sess *xorm.Session, tableName string, columnNames ...strin
 		}
 		for _, index := range res {
 			indexName := index["column_name"]
-			_, err := sess.Exec(fmt.Sprintf("DROP INDEX `%s` ON `%s`", indexName, tableName))
-			if err != nil {
-				return err
+			if len(indexName) > 0 {
+				_, err := sess.Exec(fmt.Sprintf("DROP INDEX `%s` ON `%s`", indexName, tableName))
+				if err != nil {
+					return err
+				}
 			}
 		}
 

models/migrations/v39.go

@@ -47,6 +47,7 @@ func releaseAddColumnIsTagAndSyncTags(x *xorm.Engine) error {
 			if err = models.SyncReleasesWithTags(repo, gitRepo); err != nil {
 				log.Warn("SyncReleasesWithTags: %v", err)
 			}
+			gitRepo.Close()
 		}
 		if len(repos) < pageSize {
 			break

models/migrations/v82.go

@@ -91,6 +91,7 @@ func fixReleaseSha1OnReleaseTable(x *xorm.Engine) error {
 				if err != nil {
 					return err
 				}
+				defer gitRepo.Close()
 				gitRepoCache[release.RepoID] = gitRepo
 			}
 

models/org_team.go

@@ -252,7 +252,7 @@ func (t *Team) UnitEnabled(tp UnitType) bool {
 
 func (t *Team) unitEnabled(e Engine, tp UnitType) bool {
 	if err := t.getUnits(e); err != nil {
-		log.Warn("Error loading repository (ID: %d) units: %s", t.ID, err.Error())
+		log.Warn("Error loading team (ID: %d) units: %s", t.ID, err.Error())
 	}
 
 	for _, unit := range t.Units {