Improve submodule relative path handling

modules/git/commit_info.go

@@ -3,9 +3,20 @@
 
 package git
 
+import "path"
+
 // CommitInfo describes the first commit with the provided entry
 type CommitInfo struct {
 	Entry         *TreeEntry
 	Commit        *Commit
 	SubmoduleFile *CommitSubmoduleFile
 }
+
+func getCommitInfoSubmoduleFile(repoLink string, entry *TreeEntry, commit *Commit, treePathDir string) (*CommitSubmoduleFile, error) {
+	fullPath := path.Join(treePathDir, entry.Name())
+	submodule, err := commit.GetSubModule(fullPath)
+	if err != nil {
+		return nil, err
+	}
+	return NewCommitSubmoduleFile(repoLink, fullPath, submodule.URL, entry.ID.String()), nil
+}

modules/git/commit_info_gogit.go

@@ -16,7 +16,7 @@ import (
 )
 
 // GetCommitsInfo gets information of all commits that are corresponding to these entries
-func (tes Entries) GetCommitsInfo(ctx context.Context, commit *Commit, treePath string) ([]CommitInfo, *Commit, error) {
+func (tes Entries) GetCommitsInfo(ctx context.Context, repoLink string, commit *Commit, treePath string) ([]CommitInfo, *Commit, error) {
 	entryPaths := make([]string, len(tes)+1)
 	// Get the commit for the treePath itself
 	entryPaths[0] = ""
@@ -71,22 +71,12 @@ func (tes Entries) GetCommitsInfo(ctx context.Context, commit *Commit, treePath
 			commitsInfo[i].Commit = entryCommit
 		}
 
-		// If the entry is a submodule add a submodule file for this
+		// If the entry is a submodule, add a submodule file for this
 		if entry.IsSubModule() {
-			subModuleURL := ""
-			var fullPath string
-			if len(treePath) > 0 {
-				fullPath = treePath + "/" + entry.Name()
-			} else {
-				fullPath = entry.Name()
-			}
-			if subModule, err := commit.GetSubModule(fullPath); err != nil {
+			commitsInfo[i].SubmoduleFile, err = getCommitInfoSubmoduleFile(repoLink, entry, commit, treePath)
+			if err != nil {
 				return nil, nil, err
-			} else if subModule != nil {
-				subModuleURL = subModule.URL
 			}
-			subModuleFile := NewCommitSubmoduleFile(subModuleURL, entry.ID.String())
-			commitsInfo[i].SubmoduleFile = subModuleFile
 		}
 	}
 

modules/git/commit_info_nogogit.go

@@ -15,7 +15,7 @@ import (
 )
 
 // GetCommitsInfo gets information of all commits that are corresponding to these entries
-func (tes Entries) GetCommitsInfo(ctx context.Context, commit *Commit, treePath string) ([]CommitInfo, *Commit, error) {
+func (tes Entries) GetCommitsInfo(ctx context.Context, repoLink string, commit *Commit, treePath string) ([]CommitInfo, *Commit, error) {
 	entryPaths := make([]string, len(tes)+1)
 	// Get the commit for the treePath itself
 	entryPaths[0] = ""
@@ -62,22 +62,12 @@ func (tes Entries) GetCommitsInfo(ctx context.Context, commit *Commit, treePath
 			log.Debug("missing commit for %s", entry.Name())
 		}
 
-		// If the entry is a submodule add a submodule file for this
+		// If the entry is a submodule, add a submodule file for this
 		if entry.IsSubModule() {
-			subModuleURL := ""
-			var fullPath string
-			if len(treePath) > 0 {
-				fullPath = treePath + "/" + entry.Name()
-			} else {
-				fullPath = entry.Name()
-			}
-			if subModule, err := commit.GetSubModule(fullPath); err != nil {
+			commitsInfo[i].SubmoduleFile, err = getCommitInfoSubmoduleFile(repoLink, entry, commit, treePath)
+			if err != nil {
 				return nil, nil, err
-			} else if subModule != nil {
-				subModuleURL = subModule.URL
 			}
-			subModuleFile := NewCommitSubmoduleFile(subModuleURL, entry.ID.String())
-			commitsInfo[i].SubmoduleFile = subModuleFile
 		}
 	}
 

modules/git/commit_info_test.go

@@ -82,7 +82,7 @@ func testGetCommitsInfo(t *testing.T, repo1 *Repository) {
 		}
 
 		// FIXME: Context.TODO() - if graceful has started we should use its Shutdown context otherwise use install signals in TestMain.
-		commitsInfo, treeCommit, err := entries.GetCommitsInfo(t.Context(), commit, testCase.Path)
+		commitsInfo, treeCommit, err := entries.GetCommitsInfo(t.Context(), "/any/repo-link", commit, testCase.Path)
 		assert.NoError(t, err, "Unable to get commit information for entries of subtree: %s in commit: %s from testcase due to error: %v", testCase.Path, testCase.CommitID, err)
 		if err != nil {
 			t.FailNow()
@@ -159,7 +159,7 @@ func BenchmarkEntries_GetCommitsInfo(b *testing.B) {
 		b.ResetTimer()
 		b.Run(benchmark.name, func(b *testing.B) {
 			for b.Loop() {
-				_, _, err := entries.GetCommitsInfo(b.Context(), commit, "")
+				_, _, err := entries.GetCommitsInfo(b.Context(), "/any/repo-link", commit, "")
 				if err != nil {
 					b.Fatal(err)
 				}

modules/git/commit_submodule_file.go

@@ -6,57 +6,61 @@ package git
 
 import (
 	"context"
+	"path"
 	"strings"
 
 	giturl "code.gitea.io/gitea/modules/git/url"
+	"code.gitea.io/gitea/modules/util"
 )
 
 // CommitSubmoduleFile represents a file with submodule type.
 type CommitSubmoduleFile struct {
-	refURL string
-	refID  string
+	repoLink string
+	fullPath string
+	refURL   string
+	refID    string
 
-	parsed         bool
-	targetRepoLink string
+	parsed           bool
+	parsedTargetLink string
 }
 
 // NewCommitSubmoduleFile create a new submodule file
-func NewCommitSubmoduleFile(refURL, refID string) *CommitSubmoduleFile {
-	return &CommitSubmoduleFile{refURL: refURL, refID: refID}
+func NewCommitSubmoduleFile(repoLink, fullPath, refURL, refID string) *CommitSubmoduleFile {
+	return &CommitSubmoduleFile{repoLink: repoLink, fullPath: fullPath, refURL: refURL, refID: refID}
 }
 
 func (sf *CommitSubmoduleFile) RefID() string {
-	return sf.refID // this function is only used in templates
+	return sf.refID
 }
 
-// SubmoduleWebLink tries to make some web links for a submodule, it also works on "nil" receiver
-func (sf *CommitSubmoduleFile) SubmoduleWebLink(ctx context.Context, optCommitID ...string) *SubmoduleWebLink {
+func (sf *CommitSubmoduleFile) getWebLinkInTargetRepo(ctx context.Context, moreLinkPath string) *SubmoduleWebLink {
 	if sf == nil {
 		return nil
 	}
+	if strings.HasPrefix(sf.refURL, "../") {
+		targetLink := path.Join(sf.repoLink, path.Dir(sf.fullPath), sf.refURL)
+		return &SubmoduleWebLink{RepoWebLink: targetLink, CommitWebLink: targetLink + moreLinkPath}
+	}
 	if !sf.parsed {
 		sf.parsed = true
-		if strings.HasPrefix(sf.refURL, "../") {
-			// FIXME: when handling relative path, this logic is not right. It needs to:
-			// 1. Remember the submodule's full path and its commit's repo home link
-			// 2. Resolve the relative path: targetRepoLink = path.Join(repoHomeLink, path.Dir(submoduleFullPath), refURL)
-			// Not an easy task and need to refactor related code a lot.
-			sf.targetRepoLink = sf.refURL
-		} else {
-			parsedURL, err := giturl.ParseRepositoryURL(ctx, sf.refURL)
-			if err != nil {
-				return nil
-			}
-			sf.targetRepoLink = giturl.MakeRepositoryWebLink(parsedURL)
+		parsedURL, err := giturl.ParseRepositoryURL(ctx, sf.refURL)
+		if err != nil {
+			return nil
 		}
+		sf.parsedTargetLink = giturl.MakeRepositoryWebLink(parsedURL)
 	}
-	var commitLink string
-	if len(optCommitID) == 2 {
-		commitLink = sf.targetRepoLink + "/compare/" + optCommitID[0] + "..." + optCommitID[1]
-	} else if len(optCommitID) == 1 {
-		commitLink = sf.targetRepoLink + "/tree/" + optCommitID[0]
-	} else {
-		commitLink = sf.targetRepoLink + "/tree/" + sf.refID
+	return &SubmoduleWebLink{RepoWebLink: sf.parsedTargetLink, CommitWebLink: sf.parsedTargetLink + moreLinkPath}
+}
+
+// SubmoduleWebLinkTree tries to make the submodule's tree link in its own repo, it also works on "nil" receiver
+func (sf *CommitSubmoduleFile) SubmoduleWebLinkTree(ctx context.Context, optCommitID ...string) *SubmoduleWebLink {
+	if sf == nil {
+		return nil
 	}
-	return &SubmoduleWebLink{RepoWebLink: sf.targetRepoLink, CommitWebLink: commitLink}
+	return sf.getWebLinkInTargetRepo(ctx, "/tree/"+util.OptionalArg(optCommitID, sf.refID))
+}
+
+// SubmoduleWebLinkCompare tries to make the submodule's compare link in its own repo, it also works on "nil" receiver
+func (sf *CommitSubmoduleFile) SubmoduleWebLinkCompare(ctx context.Context, commitID1, commitID2 string) *SubmoduleWebLink {
+	return sf.getWebLinkInTargetRepo(ctx, "/compare/"+commitID1+"..."+commitID2)
 }

modules/git/commit_submodule_file_test.go

@@ -10,29 +10,29 @@ import (
 )
 
 func TestCommitSubmoduleLink(t *testing.T) {
-	wl := (*CommitSubmoduleFile)(nil).SubmoduleWebLink(t.Context())
-	assert.Nil(t, wl)
+	assert.Nil(t, (*CommitSubmoduleFile)(nil).SubmoduleWebLinkTree(t.Context()))
+	assert.Nil(t, (*CommitSubmoduleFile)(nil).SubmoduleWebLinkCompare(t.Context(), "", ""))
 
 	t.Run("GitHubRepo", func(t *testing.T) {
-		sf := NewCommitSubmoduleFile("[email protected]:user/repo.git", "aaaa")
-
-		wl := sf.SubmoduleWebLink(t.Context())
+		sf := NewCommitSubmoduleFile("/any/repo-link", "full-path", "[email protected]:user/repo.git", "aaaa")
+		wl := sf.SubmoduleWebLinkTree(t.Context())
 		assert.Equal(t, "https://github.com/user/repo", wl.RepoWebLink)
 		assert.Equal(t, "https://github.com/user/repo/tree/aaaa", wl.CommitWebLink)
 
-		wl = sf.SubmoduleWebLink(t.Context(), "1111")
-		assert.Equal(t, "https://github.com/user/repo", wl.RepoWebLink)
-		assert.Equal(t, "https://github.com/user/repo/tree/1111", wl.CommitWebLink)
-
-		wl = sf.SubmoduleWebLink(t.Context(), "1111", "2222")
+		wl = sf.SubmoduleWebLinkCompare(t.Context(), "1111", "2222")
 		assert.Equal(t, "https://github.com/user/repo", wl.RepoWebLink)
 		assert.Equal(t, "https://github.com/user/repo/compare/1111...2222", wl.CommitWebLink)
 	})
 
 	t.Run("RelativePath", func(t *testing.T) {
-		sf := NewCommitSubmoduleFile("../../user/repo", "aaaa")
-		wl := sf.SubmoduleWebLink(t.Context())
-		assert.Equal(t, "../../user/repo", wl.RepoWebLink)
-		assert.Equal(t, "../../user/repo/tree/aaaa", wl.CommitWebLink)
+		sf := NewCommitSubmoduleFile("/subpath/any/repo-home-link", "full-path", "../../user/repo", "aaaa")
+		wl := sf.SubmoduleWebLinkTree(t.Context())
+		assert.Equal(t, "/subpath/user/repo", wl.RepoWebLink)
+		assert.Equal(t, "/subpath/user/repo/tree/aaaa", wl.CommitWebLink)
+
+		sf = NewCommitSubmoduleFile("/subpath/any/repo-home-link", "dir/submodule", "../../../user/repo", "aaaa")
+		wl = sf.SubmoduleWebLinkCompare(t.Context(), "1111", "2222")
+		assert.Equal(t, "/subpath/user/repo", wl.RepoWebLink)
+		assert.Equal(t, "/subpath/user/repo/compare/1111...2222", wl.CommitWebLink)
 	})
 }

routers/web/repo/commit.go

@@ -311,7 +311,7 @@ func Diff(ctx *context.Context) {
 		maxLines, maxFiles = -1, -1
 	}
 
-	diff, err := gitdiff.GetDiffForRender(ctx, gitRepo, &gitdiff.DiffOptions{
+	diff, err := gitdiff.GetDiffForRender(ctx, ctx.Repo.RepoLink, gitRepo, &gitdiff.DiffOptions{
 		AfterCommitID:      commitID,
 		SkipTo:             ctx.FormString("skip-to"),
 		MaxLines:           maxLines,

routers/web/repo/compare.go

@@ -614,7 +614,7 @@ func PrepareCompareDiff(
 
 	fileOnly := ctx.FormBool("file-only")
 
-	diff, err := gitdiff.GetDiffForRender(ctx, ci.HeadGitRepo,
+	diff, err := gitdiff.GetDiffForRender(ctx, ci.HeadRepo.Link(), ci.HeadGitRepo,
 		&gitdiff.DiffOptions{
 			BeforeCommitID:     beforeCommitID,
 			AfterCommitID:      headCommitID,

routers/web/repo/pull.go

@@ -750,7 +750,7 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi
 		diffOptions.BeforeCommitID = startCommitID
 	}
 
-	diff, err := gitdiff.GetDiffForRender(ctx, gitRepo, diffOptions, files...)
+	diff, err := gitdiff.GetDiffForRender(ctx, ctx.Repo.RepoLink, gitRepo, diffOptions, files...)
 	if err != nil {
 		ctx.ServerError("GetDiff", err)
 		return

routers/web/repo/treelist.go

@@ -144,7 +144,7 @@ func transformDiffTreeForWeb(renderedIconPool *fileicon.RenderedIconPool, diffTr
 
 func TreeViewNodes(ctx *context.Context) {
 	renderedIconPool := fileicon.NewRenderedIconPool()
-	results, err := files_service.GetTreeViewNodes(ctx, renderedIconPool, ctx.Repo.Commit, ctx.Repo.TreePath, ctx.FormString("sub_path"))
+	results, err := files_service.GetTreeViewNodes(ctx, ctx.Repo.RepoLink, renderedIconPool, ctx.Repo.Commit, ctx.Repo.TreePath, ctx.FormString("sub_path"))
 	if err != nil {
 		ctx.ServerError("GetTreeViewNodes", err)
 		return

routers/web/repo/view.go

@@ -305,7 +305,7 @@ func renderDirectoryFiles(ctx *context.Context, timeout time.Duration) git.Entri
 		defer cancel()
 	}
 
-	files, latestCommit, err := allEntries.GetCommitsInfo(commitInfoCtx, ctx.Repo.Commit, ctx.Repo.TreePath)
+	files, latestCommit, err := allEntries.GetCommitsInfo(commitInfoCtx, ctx.Repo.RepoLink, ctx.Repo.Commit, ctx.Repo.TreePath)
 	if err != nil {
 		ctx.ServerError("GetCommitsInfo", err)
 		return nil

routers/web/repo/wiki.go

@@ -6,7 +6,6 @@ package repo
 
 import (
 	"bytes"
-	gocontext "context"
 	"html/template"
 	"io"
 	"net/http"
@@ -569,7 +568,7 @@ func WikiPages(ctx *context.Context) {
 	}
 	allEntries.CustomSort(base.NaturalSortLess)
 
-	entries, _, err := allEntries.GetCommitsInfo(gocontext.Context(ctx), commit, treePath)
+	entries, _, err := allEntries.GetCommitsInfo(ctx, ctx.Repo.RepoLink, commit, treePath)
 	if err != nil {
 		ctx.ServerError("GetCommitsInfo", err)
 		return

services/gitdiff/gitdiff.go

@@ -1185,7 +1185,7 @@ func GetDiffForAPI(ctx context.Context, gitRepo *git.Repository, opts *DiffOptio
 	return diff, err
 }
 
-func GetDiffForRender(ctx context.Context, gitRepo *git.Repository, opts *DiffOptions, files ...string) (*Diff, error) {
+func GetDiffForRender(ctx context.Context, repoLink string, gitRepo *git.Repository, opts *DiffOptions, files ...string) (*Diff, error) {
 	diff, beforeCommit, afterCommit, err := getDiffBasic(ctx, gitRepo, opts, files...)
 	if err != nil {
 		return nil, err
@@ -1211,7 +1211,7 @@ func GetDiffForRender(ctx context.Context, gitRepo *git.Repository, opts *DiffOp
 
 		// Populate Submodule URLs
 		if diffFile.SubmoduleDiffInfo != nil {
-			diffFile.SubmoduleDiffInfo.PopulateURL(diffFile, beforeCommit, afterCommit)
+			diffFile.SubmoduleDiffInfo.PopulateURL(repoLink, diffFile, beforeCommit, afterCommit)
 		}
 
 		if !isVendored.Has() {

services/gitdiff/submodule.go

@@ -20,7 +20,7 @@ type SubmoduleDiffInfo struct {
 	PreviousRefID string
 }
 
-func (si *SubmoduleDiffInfo) PopulateURL(diffFile *DiffFile, leftCommit, rightCommit *git.Commit) {
+func (si *SubmoduleDiffInfo) PopulateURL(repoLink string, diffFile *DiffFile, leftCommit, rightCommit *git.Commit) {
 	si.SubmoduleName = diffFile.Name
 	submoduleCommit := rightCommit // If the submodule is added or updated, check at the right commit
 	if diffFile.IsDeleted {
@@ -30,34 +30,35 @@ func (si *SubmoduleDiffInfo) PopulateURL(diffFile *DiffFile, leftCommit, rightCo
 		return
 	}
 
-	submodule, err := submoduleCommit.GetSubModule(diffFile.GetDiffFileName())
+	submoduleFullPath := diffFile.GetDiffFileName()
+	submodule, err := submoduleCommit.GetSubModule(submoduleFullPath)
 	if err != nil {
-		log.Error("Unable to PopulateURL for submodule %q: GetSubModule: %v", diffFile.GetDiffFileName(), err)
+		log.Error("Unable to PopulateURL for submodule %q: GetSubModule: %v", submoduleFullPath, err)
 		return // ignore the error, do not cause 500 errors for end users
 	}
 	if submodule != nil {
-		si.SubmoduleFile = git.NewCommitSubmoduleFile(submodule.URL, submoduleCommit.ID.String())
+		si.SubmoduleFile = git.NewCommitSubmoduleFile(repoLink, submoduleFullPath, submodule.URL, submoduleCommit.ID.String())
 	}
 }
 
 func (si *SubmoduleDiffInfo) CommitRefIDLinkHTML(ctx context.Context, commitID string) template.HTML {
-	webLink := si.SubmoduleFile.SubmoduleWebLink(ctx, commitID)
+	webLink := si.SubmoduleFile.SubmoduleWebLinkTree(ctx, commitID)
 	if webLink == nil {
 		return htmlutil.HTMLFormat("%s", base.ShortSha(commitID))
 	}
 	return htmlutil.HTMLFormat(`<a href="%s">%s</a>`, webLink.CommitWebLink, base.ShortSha(commitID))
 }
 
 func (si *SubmoduleDiffInfo) CompareRefIDLinkHTML(ctx context.Context) template.HTML {
-	webLink := si.SubmoduleFile.SubmoduleWebLink(ctx, si.PreviousRefID, si.NewRefID)
+	webLink := si.SubmoduleFile.SubmoduleWebLinkCompare(ctx, si.PreviousRefID, si.NewRefID)
 	if webLink == nil {
 		return htmlutil.HTMLFormat("%s...%s", base.ShortSha(si.PreviousRefID), base.ShortSha(si.NewRefID))
 	}
 	return htmlutil.HTMLFormat(`<a href="%s">%s...%s</a>`, webLink.CommitWebLink, base.ShortSha(si.PreviousRefID), base.ShortSha(si.NewRefID))
 }
 
 func (si *SubmoduleDiffInfo) SubmoduleRepoLinkHTML(ctx context.Context) template.HTML {
-	webLink := si.SubmoduleFile.SubmoduleWebLink(ctx)
+	webLink := si.SubmoduleFile.SubmoduleWebLinkTree(ctx)
 	if webLink == nil {
 		return htmlutil.HTMLFormat("%s", si.SubmoduleName)
 	}

services/gitdiff/submodule_test.go

@@ -227,7 +227,7 @@ func TestSubmoduleInfo(t *testing.T) {
 	assert.EqualValues(t, "aaaa...bbbb", sdi.CompareRefIDLinkHTML(ctx))
 	assert.EqualValues(t, "name", sdi.SubmoduleRepoLinkHTML(ctx))
 
-	sdi.SubmoduleFile = git.NewCommitSubmoduleFile("https://github.com/owner/repo", "1234")
+	sdi.SubmoduleFile = git.NewCommitSubmoduleFile("/any/repo-link", "fullpath", "https://github.com/owner/repo", "1234")
 	assert.EqualValues(t, `<a href="https://github.com/owner/repo/tree/1111">1111</a>`, sdi.CommitRefIDLinkHTML(ctx, "1111"))
 	assert.EqualValues(t, `<a href="https://github.com/owner/repo/compare/aaaa...bbbb">aaaa...bbbb</a>`, sdi.CompareRefIDLinkHTML(ctx))
 	assert.EqualValues(t, `<a href="https://github.com/owner/repo">name</a>`, sdi.SubmoduleRepoLinkHTML(ctx))