Refine Log.

Signed-off-by: kuba-- <kuba@sourced.tech>

Author: kuba--

plumbing/object/commit_walker.go

@@ -186,98 +186,104 @@ func (w *commitPostIterator) Close() {}
 
 // commitAllIterator stands for commit iterator for all refs.
 type commitAllIterator struct {
-	// el points to the current commit.
-	el *list.Element
+	// currCommit points to the current commit.
+	currCommit *list.Element
 }
 
 // NewCommitAllIter returns a new commit iterator for all refs.
-// s is a repo Storer used to get commits and references.
-// fn is a commit iterator function, used to iterate through ref commits in chosen order
-func NewCommitAllIter(s storage.Storer, fn func(*Commit) CommitIter) (CommitIter, error) {
-	l := list.New()
-	m := make(map[plumbing.Hash]*list.Element)
-
-	// ...along with the HEAD
-	head, err := storer.ResolveReference(s, plumbing.HEAD)
+// repoStorer is a repo Storer used to get commits and references.
+// commitIterFunc is a commit iterator function, used to iterate through ref commits in chosen order
+func NewCommitAllIter(repoStorer storage.Storer, commitIterFunc func(*Commit) CommitIter) (CommitIter, error) {
+	commitsPath := list.New()
+	commitsLookup := make(map[plumbing.Hash]*list.Element)
+	head, err := storer.ResolveReference(repoStorer, plumbing.HEAD)
 	if err != nil {
 		return nil, err
 	}
-	headCommit, err := GetCommit(s, head.Hash())
-	if err != nil {
+
+	// add all references along with the HEAD
+	if err = addReference(repoStorer, commitIterFunc, head, commitsPath, commitsLookup); err != nil {
 		return nil, err
 	}
-	err = fn(headCommit).ForEach(func(c *Commit) error {
-		el := l.PushBack(c)
-		m[c.Hash] = el
-		return nil
-	})
+	refIter, err := repoStorer.IterReferences()
 	if err != nil {
 		return nil, err
 	}
-
-	refIter, err := s.IterReferences()
+	defer refIter.Close()
+	err = refIter.ForEach(
+		func(ref *plumbing.Reference) error {
+			return addReference(repoStorer, commitIterFunc, ref, commitsPath, commitsLookup)
+		},
+	)
 	if err != nil {
 		return nil, err
 	}
-	defer refIter.Close()
-	err = refIter.ForEach(func(r *plumbing.Reference) error {
-		if r.Hash() == head.Hash() {
-			// we already have the HEAD
-			return nil
-		}
 
-		el, ok := m[r.Hash()]
-		if ok {
-			return nil
-		}
+	return &commitAllIterator{commitsPath.Front()}, nil
+}
+
+func addReference(
+	repoStorer storage.Storer,
+	commitIterFunc func(*Commit) CommitIter,
+	ref *plumbing.Reference,
+	commitsPath *list.List,
+	commitsLookup map[plumbing.Hash]*list.Element) error {
+
+	_, exists := commitsLookup[ref.Hash()]
+	if exists {
+		// we already have it - skip the reference.
+		return nil
+	}
 
-		var refCommits []*Commit
-		c, _ := GetCommit(s, r.Hash())
+	refCommit, _ := GetCommit(repoStorer, ref.Hash())
+	if refCommit == nil {
 		// if it's not a commit - skip it.
-		if c == nil {
-			return nil
+		return nil
+	}
+
+	var (
+		refCommits []*Commit
+		parent     *list.Element
+	)
+	// collect all ref commits to add
+	commitIter := commitIterFunc(refCommit)
+	for c, e := commitIter.Next(); e == nil; {
+		parent, exists = commitsLookup[c.Hash]
+		if exists {
+			break
 		}
-		cit := fn(c)
-		for c, e := cit.Next(); e == nil; {
-			el, ok = m[c.Hash]
-			if ok {
-				break
-			}
-			refCommits = append(refCommits, c)
-			c, e = cit.Next()
+		refCommits = append(refCommits, c)
+		c, e = commitIter.Next()
+	}
+	commitIter.Close()
+
+	if parent == nil {
+		// common parent - not found
+		// add all commits to the path from this ref (maybe it's a HEAD and we don't have anything, yet)
+		for _, c := range refCommits {
+			parent = commitsPath.PushBack(c)
+			commitsLookup[c.Hash] = parent
 		}
-		cit.Close()
-
-		if el == nil {
-			// push back all commits from this ref.
-			for _, c := range refCommits {
-				el = l.PushBack(c)
-				m[c.Hash] = el
-			}
-		} else {
-			// insert ref's commits into the list
-			for i := len(refCommits) - 1; i >= 0; i-- {
-				c := refCommits[i]
-				el = l.InsertBefore(c, el)
-				m[c.Hash] = el
-			}
+	} else {
+		// add ref's commits to the path in reverse order (from the latest)
+		for i := len(refCommits) - 1; i >= 0; i-- {
+			c := refCommits[i]
+			// insert before found common parent
+			parent = commitsPath.InsertBefore(c, parent)
+			commitsLookup[c.Hash] = parent
 		}
-		return nil
-	})
-	if err != nil {
-		return nil, err
 	}
 
-	return &commitAllIterator{l.Front()}, nil
+	return nil
 }
 
 func (it *commitAllIterator) Next() (*Commit, error) {
-	if it.el == nil {
+	if it.currCommit == nil {
 		return nil, io.EOF
 	}
 
-	c := it.el.Value.(*Commit)
-	it.el = it.el.Next()
+	c := it.currCommit.Value.(*Commit)
+	it.currCommit = it.currCommit.Next()
 
 	return c, nil
 }
@@ -305,5 +311,5 @@ func (it *commitAllIterator) ForEach(cb func(*Commit) error) error {
 }
 
 func (it *commitAllIterator) Close() {
-	it.el = nil
+	it.currCommit = nil
 }

plumbing/object/commit_walker_file.go

@@ -3,24 +3,28 @@ package object
 import (
 	"io"
 
+	"gopkg.in/src-d/go-git.v4/plumbing"
+
 	"gopkg.in/src-d/go-git.v4/plumbing/storer"
 )
 
 type commitFileIter struct {
 	fileName      string
 	sourceIter    CommitIter
 	currentCommit *Commit
-	all           bool
+	checkParent   bool
 }
 
 // NewCommitFileIterFromIter returns a commit iterator which performs diffTree between
 // successive trees returned from the commit iterator from the argument. The purpose of this is
 // to find the commits that explain how the files that match the path came to be.
-func NewCommitFileIterFromIter(fileName string, commitIter CommitIter, all bool) CommitIter {
+// If checkParent is true then the function double checks if potential parent (next commit in a path)
+// is one of the parents in the tree (it's used by `git log --all`).
+func NewCommitFileIterFromIter(fileName string, commitIter CommitIter, checkParent bool) CommitIter {
 	iterator := new(commitFileIter)
 	iterator.sourceIter = commitIter
 	iterator.fileName = fileName
-	iterator.all = all
+	iterator.checkParent = checkParent
 	return iterator
 }
 
@@ -74,36 +78,14 @@ func (c *commitFileIter) getNextFileCommit() (*Commit, error) {
 			return nil, diffErr
 		}
 
-		foundChangeForFile := false
-		for _, change := range changes {
-			if change.name() != c.fileName {
-				continue
-			}
-
-			// filename matches, now check if source iterator contains all commits (from all refs)
-			if c.all {
-				// for `git log --all` also check if the next commit comes from the same parent
-				for _, h := range c.currentCommit.ParentHashes {
-					if h == parentCommit.Hash {
-						foundChangeForFile = true
-						break
-					}
-				}
-			} else {
-				foundChangeForFile = true
-			}
-
-			if foundChangeForFile {
-				break
-			}
-		}
+		found := c.hasFileChange(changes, parentCommit)
 
 		// Storing the current-commit in-case a change is found, and
 		// Updating the current-commit for the next-iteration
 		prevCommit := c.currentCommit
 		c.currentCommit = parentCommit
 
-		if foundChangeForFile == true {
+		if found {
 			return prevCommit, nil
 		}
 
@@ -114,6 +96,35 @@ func (c *commitFileIter) getNextFileCommit() (*Commit, error) {
 	}
 }
 
+func (c *commitFileIter) hasFileChange(changes Changes, parent *Commit) bool {
+	for _, change := range changes {
+		if change.name() != c.fileName {
+			continue
+		}
+
+		// filename matches, now check if source iterator contains all commits (from all refs)
+		if c.checkParent {
+			if parent != nil && isParentHash(parent.Hash, c.currentCommit) {
+				return true
+			}
+			continue
+		}
+
+		return true
+	}
+
+	return false
+}
+
+func isParentHash(hash plumbing.Hash, commit *Commit) bool {
+	for _, h := range commit.ParentHashes {
+		if h == hash {
+			return true
+		}
+	}
+	return false
+}
+
 func (c *commitFileIter) ForEach(cb func(*Commit) error) error {
 	for {
 		commit, nextErr := c.Next()

repository.go

@@ -1027,64 +1027,83 @@ func (r *Repository) PushContext(ctx context.Context, o *PushOptions) error {
 
 // Log returns the commit history from the given LogOptions.
 func (r *Repository) Log(o *LogOptions) (object.CommitIter, error) {
+	fn := commitIterFunc(o.Order)
+	if fn == nil {
+		return nil, fmt.Errorf("invalid Order=%v", o.Order)
+	}
+
 	var (
-		err            error
-		commitIterFunc func(*object.Commit) object.CommitIter
-		commitIter     object.CommitIter
+		it  object.CommitIter
+		err error
 	)
-	switch o.Order {
+	if o.All {
+		it, err = r.logAll(fn)
+	} else {
+		it, err = r.log(o.From, fn)
+	}
+
+	if err != nil {
+		return nil, err
+	}
+
+	if o.FileName != nil {
+		// for `git log --all` also check parent (if the next commit comes from the real parent)
+		it = r.logWithFile(*o.FileName, it, o.All)
+	}
+
+	return it, nil
+}
+
+func (r *Repository) log(from plumbing.Hash, commitIterFunc func(*object.Commit) object.CommitIter) (object.CommitIter, error) {
+	h := from
+	if from == plumbing.ZeroHash {
+		head, err := r.Head()
+		if err != nil {
+			return nil, err
+		}
+
+		h = head.Hash()
+	}
+
+	commit, err := r.CommitObject(h)
+	if err != nil {
+		return nil, err
+	}
+	return commitIterFunc(commit), nil
+}
+
+func (r *Repository) logAll(commitIterFunc func(*object.Commit) object.CommitIter) (object.CommitIter, error) {
+	return object.NewCommitAllIter(r.Storer, commitIterFunc)
+}
+
+func (*Repository) logWithFile(fileName string, commitIter object.CommitIter, checkParent bool) object.CommitIter {
+	return object.NewCommitFileIterFromIter(fileName, commitIter, checkParent)
+}
+
+func commitIterFunc(order LogOrder) func(c *object.Commit) object.CommitIter {
+	switch order {
 	case LogOrderDefault:
-		commitIterFunc = func(c *object.Commit) object.CommitIter {
+		return func(c *object.Commit) object.CommitIter {
 			return object.NewCommitPreorderIter(c, nil, nil)
 		}
 	case LogOrderDFS:
-		commitIterFunc = func(c *object.Commit) object.CommitIter {
+		return func(c *object.Commit) object.CommitIter {
 			return object.NewCommitPreorderIter(c, nil, nil)
 		}
 	case LogOrderDFSPost:
-		commitIterFunc = func(c *object.Commit) object.CommitIter {
+		return func(c *object.Commit) object.CommitIter {
 			return object.NewCommitPostorderIter(c, nil)
 		}
 	case LogOrderBSF:
-		commitIterFunc = func(c *object.Commit) object.CommitIter {
+		return func(c *object.Commit) object.CommitIter {
 			return object.NewCommitIterBSF(c, nil, nil)
 		}
 	case LogOrderCommitterTime:
-		commitIterFunc = func(c *object.Commit) object.CommitIter {
+		return func(c *object.Commit) object.CommitIter {
 			return object.NewCommitIterCTime(c, nil, nil)
 		}
-	default:
-		return nil, fmt.Errorf("invalid Order=%v", o.Order)
 	}
-
-	if o.All {
-		commitIter, err = object.NewCommitAllIter(r.Storer, commitIterFunc)
-		if err != nil {
-			return nil, err
-		}
-	} else {
-		h := o.From
-		if o.From == plumbing.ZeroHash {
-			head, err := r.Head()
-			if err != nil {
-				return nil, err
-			}
-
-			h = head.Hash()
-		}
-
-		commit, err := r.CommitObject(h)
-		if err != nil {
-			return nil, err
-		}
-		commitIter = commitIterFunc(commit)
-	}
-
-	if o.FileName != nil {
-		commitIter = object.NewCommitFileIterFromIter(*o.FileName, commitIter, o.All)
-	}
-
-	return commitIter, nil
+	return nil
 }
 
 // Tags returns all the tag References in a repository.