plumbing: blame, Complete rewrite. Fixes #603

[AriehSchneier]

No description provided.

[pjbgf]

@AriehSchneier Nice work! I noticed that there are a few optimisation TODO's. What's the performance impact of the changes?

[AriehSchneier]

@AriehSchneier Nice work! I noticed that there are a few optimisation TODO's. What's the performance impact of the changes?

@pjbgf Thats a good question, its hard to know without running tests. It would be good if someone could create a couple of benchmarks that we could run against to compare.

[AriehSchneier]

@pjbgf rebased and ready for review

[pjbgf]

I noticed that go-git's version of git blame returns Author email instead of Author name. I think it would be worth changing that as part of the rewrite:

For backwards compatibility, I would add a new AuthorName (or something similar). Then we reassess the API at v6.0.

[pjbgf]

We could short-circuit here if curItems is ever 0.

[AriehSchneier]

It is impossible for len(curItems) to be 0 otherwise the calling code would have returned the error invalid state: no items left on the blame queue

[pjbgf]

Please add an example on how to use Blame:

package main

import (
	"fmt"
	"os"

	"github.com/go-git/go-git/v5"
	. "github.com/go-git/go-git/v5/_examples"
)

// Basic example of how to blame a repository.
func main() {
	CheckArgs("<url>", "<file_to_blame>")
	url := os.Args[1]
	path := os.Args[2]

	tmp, err := os.MkdirTemp("", "go-git-blame-*")
	CheckIfError(err)

	defer os.RemoveAll(tmp)

	// Clone the given repository.
	Info("git clone %s %s", url, tmp)
	r, err := git.PlainClone(
		tmp,
		false,
		&git.CloneOptions{
			URL:  url,
			Tags: git.NoTags,
		},
	)
	CheckIfError(err)

	// Retrieve the branch's HEAD, to then get the HEAD commit.
	ref, err := r.Head()
	CheckIfError(err)

	c, err := r.CommitObject(ref.Hash())
	CheckIfError(err)

	Info("git blame %s", path)

	// Blame the given file/path.
	br, err := git.Blame(c, path)
	CheckIfError(err)

	for i, l := range br.Lines {
		fmt.Printf("%s (%s %s %d) %s\n", l.Hash.String()[:7], l.Author, l.Date.Format("2006-01-02 15:04:05 -0700"), i+1, l.Text)
	}
}

[pjbgf]

@pjbgf Thats a good question, its hard to know without running tests. It would be good if someone could create a couple of benchmarks that we could run against to compare.

The previous Blame was not working properly, so could not bench it on a large file. The results below are for a file created on single commit:

cpu: Intel(R) Core(TM) i9-10885H CPU @ 2.40GHz
         │   /tmp/old   │                /tmp/new                │
         │    sec/op    │    sec/op      vs base                 │
Blame-16   1.853m ± 10%   103.262m ± 2%  +5473.84% (p=0.002 n=6)

         │   /tmp/old   │                /tmp/new                │
         │     B/op     │     B/op       vs base                 │
Blame-16   1.115Mi ± 0%   77.680Mi ± 0%  +6868.48% (p=0.002 n=6)

         │  /tmp/old   │               /tmp/new                │
         │  allocs/op  │  allocs/op    vs base                 │
Blame-16   11.06k ± 0%   327.18k ± 0%  +2859.02% (p=0.002 n=6)

Code used:

func BenchmarkBlame(b *testing.B) {
	b.StopTimer()

	r, err := PlainClone(b.TempDir(), false, &CloneOptions{
		URL:          "https://github.com/go-git/go-git.git",
		Tags:         NoTags,
		SingleBranch: true,
	})
	if err != nil {
		b.Error(err)
	}

	ref, err := r.Head()
	if err != nil {
		b.Error(err)
	}

	c, err := r.CommitObject(ref.Hash())
	if err != nil {
		b.Error(err)
	}

	b.StartTimer()
	for i := 0; i < b.N; i++ {
		_, err = Blame(c, "SECURITY.md")
		if err != nil {
			b.Error(err)
		}
	}
}

[pjbgf]

We may be able to reuse the backing array so it does not need to be created every time.
So this line could be moved to before the for, and here you replace it with:
items = items[:0]

[pjbgf]

If you are keen on bounce some ideas on this, feel free to reach out via Slack.

[AriehSchneier]

@pjbgf all comments addressed, I didn't squash yet so you could see the diff. Once we are ready to merge I will squash.

[pjbgf]

I ran the blame benchmark from current implementation vs your PR, and got this results:

cpu: Intel(R) Core(TM) i9-10885H CPU @ 2.40GHz
         │  /tmp/old   │              /tmp/new              │
         │   sec/op    │   sec/op     vs base               │
Blame-16   442.8m ± 3%   573.3m ± 6%  +29.49% (p=0.002 n=6)

         │   /tmp/old   │              /tmp/new               │
         │     B/op     │     B/op      vs base               │
Blame-16   294.3Mi ± 0%   373.3Mi ± 0%  +26.83% (p=0.002 n=6)

         │  /tmp/old   │              /tmp/new              │
         │  allocs/op  │  allocs/op   vs base               │
Blame-16   2.689M ± 0%   3.183M ± 0%  +18.37% (p=0.002 n=6)

The test targeted plumbing/object/commit.go from your PR. This looks much better.
Although it still is slower than the current, it is not by much and the changes fixes previous inaccuracies.

So it LGTM. If there aren't more changes, please squash the commits and add a short description to the commit/PR on the reasons behind the rewrite.