refactor

Author: wxiaoguang

services/gitdiff/submodule.go

@@ -3,7 +3,14 @@
 
 package gitdiff
 
-import "code.gitea.io/gitea/modules/git"
+import (
+	"html/template"
+
+	"code.gitea.io/gitea/modules/base"
+	"code.gitea.io/gitea/modules/git"
+	"code.gitea.io/gitea/modules/htmlutil"
+	"code.gitea.io/gitea/modules/log"
+)
 
 type SubmoduleInfo struct {
 	SubmoduleURL  string
@@ -12,44 +19,49 @@ type SubmoduleInfo struct {
 }
 
 func (si *SubmoduleInfo) PopulateURL(diffFile *DiffFile, leftCommit, rightCommit *git.Commit) error {
-	// If the submodule is removed, we need to check it at the left commit
-	if diffFile.IsDeleted {
-		if leftCommit == nil {
-			return nil
-		}
-
-		submodule, err := leftCommit.GetSubModule(diffFile.GetDiffFileName())
+	var submoduleCommit *git.Commit
+	switch {
+	case diffFile.IsDeleted:
+		submoduleCommit = leftCommit // If the submodule is removed, we need to check it at the left commit
+	default:
+		submoduleCommit = rightCommit // If the submodule path is added or updated, we check this at the right commit
+	}
+	if submoduleCommit != nil {
+		submodule, err := submoduleCommit.GetSubModule(diffFile.GetDiffFileName())
 		if err != nil {
-			return err
+			log.Error("Unable to PopulateURL for submodule %q: GetSubModule: %v", diffFile.GetDiffFileName(), err)
+			return nil // ignore the error, do not cause 500 errors for end users
 		}
-
 		if submodule != nil {
 			si.SubmoduleURL = submodule.URL
 		}
-
-		return nil
 	}
+	return nil
+}
 
-	// Even if the submodule path is updated, we check this at the right commit
-	submodule, err := rightCommit.GetSubModule(diffFile.Name)
-	if err != nil {
-		return err
+func (si *SubmoduleInfo) NewRefIDLinkHTML() template.HTML {
+	refURL := si.refURL()
+	if si.PreviousRefID == "" {
+		return htmlutil.HTMLFormat(`<a href="%s/commit/%s"">%s</a>`, refURL, si.NewRefID, base.ShortSha(si.NewRefID))
 	}
+	return htmlutil.HTMLFormat(`<a href="%s/compare/%s...%s">%s</a>`, refURL, si.PreviousRefID, si.NewRefID, base.ShortSha(si.NewRefID))
+}
 
-	if submodule != nil {
-		si.SubmoduleURL = submodule.URL
-	}
-	return nil
+func (si *SubmoduleInfo) PreviousRefIDLinkHTML() template.HTML {
+	refURL := si.refURL()
+	return htmlutil.HTMLFormat(`<a href="%s/commit/%s">%s</a>`, refURL, si.PreviousRefID, base.ShortSha(si.PreviousRefID))
 }
 
-func (si *SubmoduleInfo) RefID() string {
-	if si.NewRefID != "" {
-		return si.NewRefID
-	}
-	return si.PreviousRefID
+func (si *SubmoduleInfo) SubmoduleRepoLinkHTML(name string) template.HTML {
+	refURL := si.refURL()
+	return htmlutil.HTMLFormat(`<a href="%s">%s</a>`, refURL, name)
 }
 
 // RefURL guesses and returns reference URL.
-func (si *SubmoduleInfo) RefURL(repoFullName string) string {
-	return git.NewCommitSubModuleFile(si.SubmoduleURL, si.RefID()).RefURL(repoFullName)
+func (si *SubmoduleInfo) refURL() string {
+	// FIXME: use unified way to handle domain and subpath
+	// FIXME: RefURL(repoFullName) is only used to provide relative path support for submodules
+	// it is not our use case because it won't work for git clone via http/ssh
+	// FIXME: the current RefURL is not right, it doesn't consider the subpath
+	return git.NewCommitSubModuleFile(si.SubmoduleURL, "").RefURL("")
 }

services/gitdiff/submodule_test.go

@@ -215,3 +215,7 @@ index 0000000..68972a9
 		})
 	}
 }
+
+func TestSubmoduleInfo(t *testing.T) {
+	// TODO: test NewRefIDLinkHTML PreviousRefIDLinkHTML SubmoduleRepoLinkHTML after we get the unifed "RefURL" function
+}

templates/repo/diff/box.tmpl

@@ -199,29 +199,13 @@
 									</div>
 								{{else if and $file.SubmoduleInfo}}
 									<div class="tw-p-3">{{svg "octicon-file-submodule"}} {{$submodule := $file.SubmoduleInfo -}}
-										{{- $refURL := $submodule.RefURL $.Repository.FullName -}}
-										{{- $submoduleName := $file.Name -}}
-										{{- $refShortID := ShortSha $submodule.RefID -}}
+										{{- $submoduleName := ($submodule.SubmoduleRepoLinkHTML $file.Name) -}}
 										{{- if $file.IsDeleted -}}
-											{{- ctx.Locale.Tr "repo.diff.submodule_deleted" $submoduleName $refShortID -}}
+											{{- ctx.Locale.Tr "repo.diff.submodule_deleted" $submoduleName $submodule.PreviousRefIDLinkHTML -}}
 										{{- else if $file.IsCreated -}}
-											{{- $refAddedSubModule := $submoduleName -}}
-											{{- $refAddedAt := $refShortID -}}
-											{{- if $refURL -}}
-												{{- $refAddedSubModule = HTMLFormat `<a href="%s">%s</a>` $refURL $submoduleName -}}
-												{{- $refAddedAt = HTMLFormat `<a href="%s/commit/%s">%s</a>` $refURL (PathEscape $submodule.RefID) $refShortID -}}
-											{{- end -}}
-											{{- ctx.Locale.Tr "repo.diff.submodule_added" $refAddedSubModule $refAddedAt -}}
+											{{- ctx.Locale.Tr "repo.diff.submodule_added" $submoduleName $submodule.NewRefIDLinkHTML -}}
 										{{- else -}}
-											{{- $refUpdatedSubModule := $submoduleName -}}
-											{{- $refUpdatedFrom := ShortSha $submodule.PreviousRefID -}}
-											{{- $refUpdatedTo := $refShortID -}}
-											{{- if $refURL -}}
-												{{- $refUpdatedSubModule = HTMLFormat `<a href="%s">%s</a>` $refURL $submoduleName -}}
-												{{- $refUpdatedFrom = HTMLFormat `<a href="%s/commit/%s">%s</a>` $refURL (PathEscape $submodule.PreviousRefID) (ShortSha $submodule.PreviousRefID) -}}
-												{{- $refUpdatedTo = HTMLFormat `<a href="%s/commit/%s">%s</a>` $refURL (PathEscape $submodule.RefID) (ShortSha $submodule.RefID) -}}
-											{{- end -}}
-											{{- ctx.Locale.Tr "repo.diff.submodule_updated" $refUpdatedSubModule $refUpdatedFrom $refUpdatedTo -}}
+											{{- ctx.Locale.Tr "repo.diff.submodule_updated" $submoduleName $submodule.PreviousRefIDLinkHTML $submodule.NewRefIDLinkHTML -}}
 										{{end}}
 									</div>
 								{{else}}