Link to previous blames in file blame page #16259

zeripath · 2021-06-26T08:38:02Z

noerw commented

2021-06-26 08:38:02 +00:00

(Migrated from github.com)

Adds a link to each blame hunk, to view the blame of an earlier version of the file, similar to GitHub. Also refactors the blame render from fmtstring based to template based.

This supersedes #15171: I copied that PR, resolved conflicts, and adressed my code review concerns, as the original author seems unresponsive.

old demo gif:

Fixes #15109
Closes #15171

Adds a link to each blame hunk, to view the blame of an earlier version of the file, similar to GitHub. Also refactors the blame render from fmtstring based to template based. This supersedes #15171: I copied that PR, resolved conflicts, and adressed my code review concerns, as the original author seems unresponsive. old demo gif: ![](https://user-images.githubusercontent.com/5260711/112640319-3028c800-8e7c-11eb-8ea1-b257a4044b88.gif) Fixes #15109 Closes #15171

🎉 1

zeripath reviewed 2021-06-26 09:21:43 +00:00

routers/web/repo/blame.go

						
				@ -163,0 +174,4 @@

						// find parent commit

						if commit.ParentCount() > 0 {

							psha := commit.Parents[0]

zeripath commented

2021-06-26 09:21:43 +00:00

Argh!

commit.HasPreviousCommit is not free and is potentially computationally very expensive. It involves a call to git merge-base --is-ancestor. The results of this need to be cached at least within the loop.

Further I don't think:

haz, _ := commit.HasPreviousCommit(previousCommit.ID) can ever be false as it is a 0-level parent by line 182: previousCommit, err := commit.Parent(0).

haz1, _ := previousCommit.HasFile(ctx.Repo.TreePath); haz1 - is also quite expensive and should be cached.

Argh! commit.HasPreviousCommit is not free and is potentially computationally very expensive. It involves a call to `git merge-base --is-ancestor`. The results of this need to be cached at least within the loop. Further I don't think: `haz, _ := commit.HasPreviousCommit(previousCommit.ID)` can ever be false as it is a 0-level parent by line 182: `previousCommit, err := commit.Parent(0)`. `haz1, _ := previousCommit.HasFile(ctx.Repo.TreePath); haz1` - is also quite expensive and should be cached.

noerw (Migrated from github.com) reviewed 2021-06-26 09:29:15 +00:00

routers/web/repo/blame.go

						
				@ -163,0 +174,4 @@

						// find parent commit

						if commit.ParentCount() > 0 {

							psha := commit.Parents[0]

noerw (Migrated from github.com) commented

2021-06-26 09:29:15 +00:00

Oh ok. ~~Is there a common cache facility to use for this? Or should I just do an adhoc map[string]bool cache?~~

I assume you mean to implement something analog to LastCommitCache like CommitHasPathCache?

Oh ok. ~~Is there a common cache facility to use for this? Or should I just do an adhoc `map[string]bool` cache?~~ I assume you mean to implement something analog to `LastCommitCache` like `CommitHasPathCache`?

zeripath reviewed 2021-06-26 10:08:12 +00:00

routers/web/repo/blame.go

						
				@ -163,0 +174,4 @@

						// find parent commit

						if commit.ParentCount() > 0 {

							psha := commit.Parents[0]

zeripath commented

2021-06-26 10:08:12 +00:00

probably just use a just out-of-loop cache for the moment. In this case I don't think there's likely to be benefit pushing up to modules/cache or using an *lru.TwoQueue/*lru.Cache directly.

We should consider whether to cache the results of the internal git.Commit.GetFiles in modules/cache.Cache but at present the default cache is unlimited unless we merge my PR.

probably just use a just out-of-loop cache for the moment. In this case I don't think there's likely to be benefit pushing up to `modules/cache` or using an `*lru.TwoQueue`/`*lru.Cache` directly. We should consider whether to cache the results of the internal `git.Commit.GetFiles` in modules/cache.Cache but at present the default cache is unlimited unless we merge my PR.

zeripath reviewed 2021-06-26 18:34:19 +00:00

routers/web/repo/blame.go

						
				@ -163,0 +174,4 @@

						// find parent commit

						if commit.ParentCount() > 0 {

							psha := commit.Parents[0]

zeripath commented

2021-06-26 18:34:19 +00:00

diff --git a/routers/web/repo/blame.go b/routers/web/repo/blame.go
index 9efb868c8..79612f0d5 100644
--- a/routers/web/repo/blame.go
+++ b/routers/web/repo/blame.go
@@ -161,6 +161,8 @@ func RefBlame(ctx *context.Context) {
 	commitNames := make(map[string]models.UserCommit)
 	previousCommits := make(map[string]string)
 	commits := list.New()
+	commitCache := map[string]*git.Commit{}
+	commitCache[commitID] = commit
 
 	for _, part := range blameParts {
 		sha := part.Sha
@@ -168,21 +170,30 @@ func RefBlame(ctx *context.Context) {
 			continue
 		}
 
-		commit, err := ctx.Repo.GitRepo.GetCommit(sha)
-		if err != nil {
-			if git.IsErrNotExist(err) {
-				ctx.NotFound("Repo.GitRepo.GetCommit", err)
-			} else {
-				ctx.ServerError("Repo.GitRepo.GetCommit", err)
+		commit, ok := commitCache[sha]
+		if !ok {
+			commit, err = ctx.Repo.GitRepo.GetCommit(sha)
+			if err != nil {
+				if git.IsErrNotExist(err) {
+					ctx.NotFound("Repo.GitRepo.GetCommit", err)
+				} else {
+					ctx.ServerError("Repo.GitRepo.GetCommit", err)
+				}
+				return
 			}
-			return
+			commitCache[sha] = commit
 		}
 
-		// Get previous closest commit sha from the commit
-		previousCommit, err := commit.Parent(0)
-		if err == nil {
-			// Verify the commit
-			if haz, _ := commit.HasPreviousCommit(previousCommit.ID); haz {
+		if len(commit.Parents) > 0 {
+			psha := commit.Parents[0]
+			previousCommit, ok := commitCache[psha.String()]
+			if !ok {
+				previousCommit, _ = commit.Parent(0)
+				if previousCommit != nil {
+					commitCache[psha.String()] = previousCommit
+				}
+			}
+			if previousCommit != nil {
 				if haz1, _ := previousCommit.HasFile(ctx.Repo.TreePath); haz1 {
 					previousCommits[commit.ID.String()] = previousCommit.ID.String()
 				}

```diff diff --git a/routers/web/repo/blame.go b/routers/web/repo/blame.go index 9efb868c8..79612f0d5 100644 --- a/routers/web/repo/blame.go +++ b/routers/web/repo/blame.go @@ -161,6 +161,8 @@ func RefBlame(ctx *context.Context) { commitNames := make(map[string]models.UserCommit) previousCommits := make(map[string]string) commits := list.New() + commitCache := map[string]*git.Commit{} + commitCache[commitID] = commit for _, part := range blameParts { sha := part.Sha @@ -168,21 +170,30 @@ func RefBlame(ctx *context.Context) { continue } - commit, err := ctx.Repo.GitRepo.GetCommit(sha) - if err != nil { - if git.IsErrNotExist(err) { - ctx.NotFound("Repo.GitRepo.GetCommit", err) - } else { - ctx.ServerError("Repo.GitRepo.GetCommit", err) + commit, ok := commitCache[sha] + if !ok { + commit, err = ctx.Repo.GitRepo.GetCommit(sha) + if err != nil { + if git.IsErrNotExist(err) { + ctx.NotFound("Repo.GitRepo.GetCommit", err) + } else { + ctx.ServerError("Repo.GitRepo.GetCommit", err) + } + return } - return + commitCache[sha] = commit } - // Get previous closest commit sha from the commit - previousCommit, err := commit.Parent(0) - if err == nil { - // Verify the commit - if haz, _ := commit.HasPreviousCommit(previousCommit.ID); haz { + if len(commit.Parents) > 0 { + psha := commit.Parents[0] + previousCommit, ok := commitCache[psha.String()] + if !ok { + previousCommit, _ = commit.Parent(0) + if previousCommit != nil { + commitCache[psha.String()] = previousCommit + } + } + if previousCommit != nil { if haz1, _ := previousCommit.HasFile(ctx.Repo.TreePath); haz1 { previousCommits[commit.ID.String()] = previousCommit.ID.String() } ```

noerw (Migrated from github.com) reviewed 2021-06-27 11:51:54 +00:00

routers/web/repo/blame.go

						
				@ -163,0 +174,4 @@

						// find parent commit

						if commit.ParentCount() > 0 {

							psha := commit.Parents[0]

noerw (Migrated from github.com) commented

2021-06-27 11:51:54 +00:00

thanks, done

zeripath approved these changes 2021-06-27 16:23:54 +00:00

6543 (Migrated from github.com) approved these changes 2021-06-27 21:48:24 +00:00

This repo is archived. You cannot comment on pull requests.

No reviewers

6543

No Label

hacktoberfest-accepted

kind/upstream-related

status/needs-feedback

No Milestone

No project

No Assignees

2 Participants

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: lunny/gitea#16259