fix

Author: wxiaoguang

modules/fileicon/entry.go

@@ -6,17 +6,17 @@ package fileicon
 import "code.gitea.io/gitea/modules/git"
 
 type EntryInfo struct {
-	FullName      string
+	BaseName      string
 	EntryMode     git.EntryMode
 	SymlinkToMode git.EntryMode
 	IsOpen        bool
 }
 
-func EntryInfoFromGitTreeEntry(gitEntry *git.TreeEntry) *EntryInfo {
-	ret := &EntryInfo{FullName: gitEntry.Name(), EntryMode: gitEntry.Mode()}
+func EntryInfoFromGitTreeEntry(commit *git.Commit, fullPath string, gitEntry *git.TreeEntry) *EntryInfo {
+	ret := &EntryInfo{BaseName: gitEntry.Name(), EntryMode: gitEntry.Mode()}
 	if gitEntry.IsLink() {
-		if te, err := gitEntry.FollowLink(); err == nil && te.IsDir() {
-			ret.SymlinkToMode = te.Mode()
+		if res, err := git.EntryFollowLink(commit, fullPath, gitEntry); err == nil && res.TargetEntry.IsDir() {
+			ret.SymlinkToMode = res.TargetEntry.Mode()
 		}
 	}
 	return ret

modules/fileicon/material.go

@@ -134,7 +134,7 @@ func (m *MaterialIconProvider) FindIconName(entry *EntryInfo) string {
 		return "folder-git"
 	}
 
-	fileNameLower := strings.ToLower(path.Base(entry.FullName))
+	fileNameLower := strings.ToLower(path.Base(entry.BaseName))
 	if entry.EntryMode.IsDir() {
 		if s, ok := m.rules.FolderNames[fileNameLower]; ok {
 			return s

modules/fileicon/material_test.go

@@ -20,8 +20,8 @@ func TestMain(m *testing.M) {
 func TestFindIconName(t *testing.T) {
 	unittest.PrepareTestEnv(t)
 	p := fileicon.DefaultMaterialIconProvider()
-	assert.Equal(t, "php", p.FindIconName(&fileicon.EntryInfo{FullName: "foo.php", EntryMode: git.EntryModeBlob}))
-	assert.Equal(t, "php", p.FindIconName(&fileicon.EntryInfo{FullName: "foo.PHP", EntryMode: git.EntryModeBlob}))
-	assert.Equal(t, "javascript", p.FindIconName(&fileicon.EntryInfo{FullName: "foo.js", EntryMode: git.EntryModeBlob}))
-	assert.Equal(t, "visualstudio", p.FindIconName(&fileicon.EntryInfo{FullName: "foo.vba", EntryMode: git.EntryModeBlob}))
+	assert.Equal(t, "php", p.FindIconName(&fileicon.EntryInfo{BaseName: "foo.php", EntryMode: git.EntryModeBlob}))
+	assert.Equal(t, "php", p.FindIconName(&fileicon.EntryInfo{BaseName: "foo.PHP", EntryMode: git.EntryModeBlob}))
+	assert.Equal(t, "javascript", p.FindIconName(&fileicon.EntryInfo{BaseName: "foo.js", EntryMode: git.EntryModeBlob}))
+	assert.Equal(t, "visualstudio", p.FindIconName(&fileicon.EntryInfo{BaseName: "foo.vba", EntryMode: git.EntryModeBlob}))
 }

modules/git/error.go

@@ -32,22 +32,6 @@ func (err ErrNotExist) Unwrap() error {
 	return util.ErrNotExist
 }
 
-// ErrSymlinkUnresolved entry.FollowLink error
-type ErrSymlinkUnresolved struct {
-	Name    string
-	Message string
-}
-
-func (err ErrSymlinkUnresolved) Error() string {
-	return fmt.Sprintf("%s: %s", err.Name, err.Message)
-}
-
-// IsErrSymlinkUnresolved if some error is ErrSymlinkUnresolved
-func IsErrSymlinkUnresolved(err error) bool {
-	_, ok := err.(ErrSymlinkUnresolved)
-	return ok
-}
-
 // ErrBranchNotExist represents a "BranchNotExist" kind of error.
 type ErrBranchNotExist struct {
 	Name string

modules/git/tree_blob_nogogit.go

@@ -11,7 +11,7 @@ import (
 )
 
 // GetTreeEntryByPath get the tree entries according the sub dir
-func (t *Tree) GetTreeEntryByPath(relpath string) (*TreeEntry, error) {
+func (t *Tree) GetTreeEntryByPath(relpath string) (_ *TreeEntry, err error) {
 	if len(relpath) == 0 {
 		return &TreeEntry{
 			ptree:     t,
@@ -21,10 +21,8 @@ func (t *Tree) GetTreeEntryByPath(relpath string) (*TreeEntry, error) {
 		}, nil
 	}
 
-	// FIXME: This should probably use git cat-file --batch to be a bit more efficient
 	relpath = path.Clean(relpath)
 	parts := strings.Split(relpath, "/")
-	var err error
 
 	tree := t
 	for _, name := range parts[:len(parts)-1] {
@@ -41,7 +39,6 @@ func (t *Tree) GetTreeEntryByPath(relpath string) (*TreeEntry, error) {
 	}
 	for _, v := range entries {
 		if v.Name() == name {
-			v.fullName = relpath
 			return v, nil
 		}
 	}

modules/git/tree_entry.go

@@ -5,7 +5,7 @@
 package git
 
 import (
-	"io"
+	"path"
 	"sort"
 	"strings"
 
@@ -24,87 +24,57 @@ func (te *TreeEntry) Type() string {
 	}
 }
 
-// FollowLink returns the entry pointed to by a symlink
-func (te *TreeEntry) FollowLink() (*TreeEntry, error) {
+type EntryFollowResult struct {
+	SymlinkContent string
+	TargetFullPath string
+	TargetEntry    *TreeEntry
+}
+
+func EntryFollowLink(commit *Commit, fullPath string, te *TreeEntry) (*EntryFollowResult, error) {
 	if !te.IsLink() {
-		return nil, ErrSymlinkUnresolved{te.Name(), "not a symlink"}
+		return nil, util.ErrorWrap(util.ErrUnprocessableContent, "%q is not a symlink", fullPath)
 	}
 
-	// read the link
-	r, err := te.Blob().DataAsync()
-	if err != nil {
-		return nil, err
+	// git's filename max length is 4096, hopefully a link won't be longer than multiple of that
+	const maxSymlinkSize = 20 * 4096
+	if te.Blob().Size() > maxSymlinkSize {
+		return nil, util.ErrorWrap(util.ErrUnprocessableContent, "%q content exceeds symlink limit", fullPath)
 	}
-	closed := false
-	defer func() {
-		if !closed {
-			_ = r.Close()
-		}
-	}()
-	buf := make([]byte, te.Size())
-	_, err = io.ReadFull(r, buf)
+
+	link, err := te.Blob().GetBlobContent(maxSymlinkSize)
 	if err != nil {
 		return nil, err
 	}
-	_ = r.Close()
-	closed = true
-
-	lnk := string(buf)
-	t := te.ptree
-
-	// traverse up directories
-	for ; t != nil && strings.HasPrefix(lnk, "../"); lnk = lnk[3:] {
-		t = t.ptree
+	if strings.HasPrefix(link, "/") {
+		// It's said that absolute path will be stored as is in Git
+		return &EntryFollowResult{SymlinkContent: link}, util.ErrorWrap(util.ErrUnprocessableContent, "%q is an absolute symlink", fullPath)
 	}
 
-	if t == nil {
-		return nil, ErrSymlinkUnresolved{te.Name(), "points outside of repo"}
-	}
-
-	target, err := t.GetTreeEntryByPath(lnk)
+	targetFullPath := path.Join(path.Dir(fullPath), link)
+	targetEntry, err := commit.GetTreeEntryByPath(targetFullPath)
 	if err != nil {
-		if IsErrNotExist(err) {
-			return nil, ErrSymlinkUnresolved{te.Name(), "broken link"}
-		}
-		return nil, err
+		return &EntryFollowResult{SymlinkContent: link}, err
 	}
-	return target, nil
+	return &EntryFollowResult{SymlinkContent: link, TargetFullPath: targetFullPath, TargetEntry: targetEntry}, nil
 }
 
-// FollowLinks returns the entry ultimately pointed to by a symlink
-func (te *TreeEntry) FollowLinks(optLimit ...int) (*TreeEntry, error) {
-	if !te.IsLink() {
-		return nil, ErrSymlinkUnresolved{te.Name(), "not a symlink"}
-	}
+func EntryFollowLinks(commit *Commit, firstFullPath string, firstTreeEntry *TreeEntry, optLimit ...int) (res *EntryFollowResult, err error) {
 	limit := util.OptionalArg(optLimit, 10)
-	entry := te
+	treeEntry, fullPath := firstTreeEntry, firstFullPath
 	for range limit {
-		if !entry.IsLink() {
-			break
-		}
-		next, err := entry.FollowLink()
+		res, err = EntryFollowLink(commit, fullPath, treeEntry)
 		if err != nil {
-			return nil, err
+			return res, err
 		}
-		if next.ID == entry.ID {
-			return nil, ErrSymlinkUnresolved{entry.Name(), "recursive link"}
+		treeEntry, fullPath = res.TargetEntry, res.TargetFullPath
+		if !treeEntry.IsLink() {
+			break
 		}
-		entry = next
-	}
-	if entry.IsLink() {
-		return nil, ErrSymlinkUnresolved{te.Name(), "too many levels of symbolic links"}
 	}
-	return entry, nil
-}
-
-// TryFollowingLinks attempts to follow the symlinks of this entry to the origin
-// If that fails, it defaults to the entry itself
-func (te *TreeEntry) TryFollowingLinks() *TreeEntry {
-	newEntry, err := te.FollowLinks()
-	if err != nil {
-		return te
+	if treeEntry.IsLink() {
+		return res, util.ErrorWrap(util.ErrUnprocessableContent, "%q has too many links", fullPath)
 	}
-	return newEntry
+	return res, nil
 }
 
 // returns the Tree pointed to by this TreeEntry, or nil if this is not a tree

modules/git/tree_entry_common_test.go

@@ -6,101 +6,71 @@ package git
 import (
 	"testing"
 
+	"code.gitea.io/gitea/modules/util"
+
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestFollowLink(t *testing.T) {
 	r, err := openRepositoryWithDefaultContext("tests/repos/repo1_bare")
-	assert.NoError(t, err)
+	require.NoError(t, err)
 	defer r.Close()
 
 	commit, err := r.GetCommit("37991dec2c8e592043f47155ce4808d4580f9123")
-	assert.NoError(t, err)
+	require.NoError(t, err)
 
 	// get the symlink
-	lnk, err := commit.Tree.GetTreeEntryByPath("foo/bar/link_to_hello")
-	assert.NoError(t, err)
-	assert.True(t, lnk.IsLink())
-
-	// should be able to dereference to target
-	target, err := lnk.FollowLink()
-	assert.NoError(t, err)
-	assert.Equal(t, "hello", target.Name())
-	assert.Equal(t, "foo/nar/hello", target.FullPath())
-	assert.False(t, target.IsLink())
-	assert.Equal(t, "b14df6442ea5a1b382985a6549b85d435376c351", target.ID.String())
-
-	// should error when called on normal file
-	target, err = commit.Tree.GetTreeEntryByPath("file1.txt")
-	assert.NoError(t, err)
-	_, err = target.FollowLink()
-	assert.ErrorAs(t, err, ErrBadLink{})
-
-	// should error for broken links
-	target, err = commit.Tree.GetTreeEntryByPath("foo/broken_link")
-	assert.NoError(t, err)
-	assert.True(t, target.IsLink())
-	_, err = target.FollowLink()
-	assert.ErrorAs(t, err, ErrBadLink{})
-
-	// should error for external links
-	target, err = commit.Tree.GetTreeEntryByPath("foo/outside_repo")
-	assert.NoError(t, err)
-	assert.True(t, target.IsLink())
-	_, err = target.FollowLink()
-	assert.ErrorAs(t, err, ErrBadLink{})
-
-	// testing fix for short link bug
-	target, err = commit.Tree.GetTreeEntryByPath("foo/link_short")
-	assert.NoError(t, err)
-	_, err = target.FollowLink()
-	assert.ErrorAs(t, err, ErrBadLink{})
-}
-
-func TestTryFollowingLinks(t *testing.T) {
-	r, err := openRepositoryWithDefaultContext("tests/repos/repo1_bare")
-	assert.NoError(t, err)
-	defer r.Close()
-
-	commit, err := r.GetCommit("37991dec2c8e592043f47155ce4808d4580f9123")
-	assert.NoError(t, err)
-
-	// get the symlink
-	list, err := commit.Tree.GetTreeEntryByPath("foo/bar/link_to_hello")
-	assert.NoError(t, err)
-	assert.True(t, list.IsLink())
-
-	// should be able to dereference to target
-	target := list.TryFollowingLinks()
-	assert.NotEqual(t, target, list)
-	assert.Equal(t, "hello", target.Name())
-	assert.Equal(t, "foo/nar/hello", target.FullPath())
-	assert.False(t, target.IsLink())
-	assert.Equal(t, "b14df6442ea5a1b382985a6549b85d435376c351", target.ID.String())
-
-	// should default to original when called on normal file
-	link, err := commit.Tree.GetTreeEntryByPath("file1.txt")
-	assert.NoError(t, err)
-	target = link.TryFollowingLinks()
-	assert.Same(t, link, target)
-
-	// should default to original for broken links
-	link, err = commit.Tree.GetTreeEntryByPath("foo/broken_link")
-	assert.NoError(t, err)
-	assert.True(t, link.IsLink())
-	target = link.TryFollowingLinks()
-	assert.Same(t, link, target)
-
-	// should default to original for external links
-	link, err = commit.Tree.GetTreeEntryByPath("foo/outside_repo")
-	assert.NoError(t, err)
-	assert.True(t, link.IsLink())
-	target = link.TryFollowingLinks()
-	assert.Same(t, link, target)
-
-	// testing fix for short link bug
-	link, err = commit.Tree.GetTreeEntryByPath("foo/link_short")
-	assert.NoError(t, err)
-	target = link.TryFollowingLinks()
-	assert.Same(t, link, target)
+	{
+		lnkFullPath := "foo/bar/link_to_hello"
+		lnk, err := commit.Tree.GetTreeEntryByPath("foo/bar/link_to_hello")
+		require.NoError(t, err)
+		assert.True(t, lnk.IsLink())
+
+		// should be able to dereference to target
+		res, err := EntryFollowLink(commit, lnkFullPath, lnk)
+		require.NoError(t, err)
+		assert.Equal(t, "hello", res.TargetEntry.Name())
+		assert.Equal(t, "foo/nar/hello", res.TargetFullPath)
+		assert.False(t, res.TargetEntry.IsLink())
+		assert.Equal(t, "b14df6442ea5a1b382985a6549b85d435376c351", res.TargetEntry.ID.String())
+	}
+
+	{
+		// should error when called on a normal file
+		entry, err := commit.Tree.GetTreeEntryByPath("file1.txt")
+		require.NoError(t, err)
+		res, err := EntryFollowLink(commit, "file1.txt", entry)
+		assert.ErrorIs(t, err, util.ErrUnprocessableContent)
+		assert.Nil(t, res)
+	}
+
+	{
+		// should error for broken links
+		entry, err := commit.Tree.GetTreeEntryByPath("foo/broken_link")
+		require.NoError(t, err)
+		assert.True(t, entry.IsLink())
+		res, err := EntryFollowLink(commit, "foo/broken_link", entry)
+		assert.ErrorIs(t, err, util.ErrNotExist)
+		assert.Equal(t, "nar/broken_link", res.SymlinkContent)
+	}
+
+	{
+		// should error for external links
+		entry, err := commit.Tree.GetTreeEntryByPath("foo/outside_repo")
+		require.NoError(t, err)
+		assert.True(t, entry.IsLink())
+		res, err := EntryFollowLink(commit, "foo/outside_repo", entry)
+		assert.ErrorIs(t, err, util.ErrNotExist)
+		assert.Equal(t, "../../outside_repo", res.SymlinkContent)
+	}
+
+	{
+		// testing fix for short link bug
+		entry, err := commit.Tree.GetTreeEntryByPath("foo/link_short")
+		require.NoError(t, err)
+		res, err := EntryFollowLink(commit, "foo/link_short", entry)
+		assert.ErrorIs(t, err, util.ErrNotExist)
+		assert.Equal(t, "a", res.SymlinkContent)
+	}
 }