fine tune edge cases

Author: wxiaoguang

modules/git/commit_submodule_file.go

@@ -25,10 +25,14 @@ func NewCommitSubmoduleFile(refURL, refID string) *CommitSubmoduleFile {
 }
 
 func (sf *CommitSubmoduleFile) RefID() string {
-	return sf.refID
+	return sf.refID // this function is only used in templates
 }
 
+// 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 {
+	if sf == nil {
+		return nil
+	}
 	if !sf.parsed {
 		sf.parsed = true
 		parsedURL, err := giturl.ParseRepositoryURL(ctx, sf.refURL)

modules/git/commit_submodule_file_test.go

@@ -24,4 +24,7 @@ func TestCommitSubmoduleLink(t *testing.T) {
 	wl = sf.SubmoduleWebLink(context.Background(), "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)
+
+	wl = (*CommitSubmoduleFile)(nil).SubmoduleWebLink(context.Background())
+	assert.Nil(t, wl)
 }

services/gitdiff/gitdiff.go

@@ -1212,10 +1212,7 @@ func GetDiff(ctx context.Context, gitRepo *git.Repository, opts *DiffOptions, fi
 
 		// Populate Submodule URLs
 		if diffFile.SubmoduleDiffInfo != nil {
-			err := diffFile.SubmoduleDiffInfo.PopulateURL(diffFile, beforeCommit, commit)
-			if err != nil {
-				return nil, err
-			}
+			diffFile.SubmoduleDiffInfo.PopulateURL(diffFile, beforeCommit, commit)
 		}
 
 		if !isVendored.Has() {

services/gitdiff/submodule.go

@@ -15,46 +15,59 @@ import (
 
 type SubmoduleDiffInfo struct {
 	SubmoduleName string
-	SubmoduleFile *git.CommitSubmoduleFile
+	SubmoduleFile *git.CommitSubmoduleFile // it might be nil if the submodule is not found or unable to parse
 	NewRefID      string
 	PreviousRefID string
 }
 
-func (si *SubmoduleDiffInfo) PopulateURL(diffFile *DiffFile, leftCommit, rightCommit *git.Commit) error {
+func (si *SubmoduleDiffInfo) PopulateURL(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 {
 		submoduleCommit = leftCommit // If the submodule is deleted, check at the left commit
 	}
-	if submoduleCommit != nil {
-		submodule, err := submoduleCommit.GetSubModule(diffFile.GetDiffFileName())
-		if err != nil {
-			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.SubmoduleFile = git.NewCommitSubmoduleFile(submodule.URL, submoduleCommit.ID.String())
-		}
-	}
-	return nil
+	if submoduleCommit == nil {
+		return
+	}
+
+	submodule, err := submoduleCommit.GetSubModule(diffFile.GetDiffFileName())
+	if err != nil {
+		log.Error("Unable to PopulateURL for submodule %q: GetSubModule: %v", diffFile.GetDiffFileName(), 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())
+	}
 }
 
 func (si *SubmoduleDiffInfo) PreviousRefIDLinkHTML(ctx context.Context) template.HTML {
 	webLink := si.SubmoduleFile.SubmoduleWebLink(ctx, si.PreviousRefID)
+	if webLink == nil {
+		return htmlutil.HTMLFormat("%s", base.ShortSha(si.PreviousRefID))
+	}
 	return htmlutil.HTMLFormat(`<a href="%s">%s</a>`, webLink.CommitWebLink, base.ShortSha(si.PreviousRefID))
 }
 
 func (si *SubmoduleDiffInfo) NewRefIDLinkHTML(ctx context.Context) template.HTML {
 	webLink := si.SubmoduleFile.SubmoduleWebLink(ctx, si.NewRefID)
+	if webLink == nil {
+		return htmlutil.HTMLFormat("%s", base.ShortSha(si.NewRefID))
+	}
 	return htmlutil.HTMLFormat(`<a href="%s">%s</a>`, webLink.CommitWebLink, base.ShortSha(si.NewRefID))
 }
 
 func (si *SubmoduleDiffInfo) CompareRefIDLinkHTML(ctx context.Context) template.HTML {
 	webLink := si.SubmoduleFile.SubmoduleWebLink(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)
+	if webLink == nil {
+		return htmlutil.HTMLFormat("%s", si.SubmoduleName)
+	}
 	return htmlutil.HTMLFormat(`<a href="%s">%s</a>`, webLink.RepoWebLink, si.SubmoduleName)
 }

templates/repo/diff/box.tmpl

@@ -199,7 +199,7 @@
 									</div>
 								{{else if $file.SubmoduleDiffInfo}}
 									<div class="tw-p-3">{{svg "octicon-file-submodule"}} {{$submoduleDiffInfo := $file.SubmoduleDiffInfo -}}
-										{{- $submoduleName := ($submoduleDiffInfo.SubmoduleRepoLinkHTML ctx) -}}
+										{{- $submoduleName := $submoduleDiffInfo.SubmoduleRepoLinkHTML ctx -}}
 										{{- if $file.IsDeleted -}}
 											{{- ctx.Locale.Tr "repo.diff.submodule_deleted" $submoduleName ($submoduleDiffInfo.PreviousRefIDLinkHTML ctx) -}}
 										{{- else if $file.IsCreated -}}