Skip to content
This repository was archived by the owner on Sep 11, 2020. It is now read-only.

rev-list command implementation for objects #135

Merged
merged 3 commits into from
Nov 25, 2016
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
115 changes: 115 additions & 0 deletions revlist.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
package git

import (
"io"

"gopkg.in/src-d/go-git.v4/plumbing"
)

// RevListObjects gets all the hashes from all the reachable objects from the
// given commits. Ignore param are object hashes that we want to ignore on the
// result. All that objects must be accessible from the Repository.
func RevListObjects(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the operation performed by this function a complementary set?, maybe we should call it just like that.

Copy link
Contributor

@alcortesm alcortesm Nov 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or at least mention it in the comment instead of explaining what a complementary set operation is without mention it.

r *Repository,
commits []*Commit,
ignore []plumbing.Hash) ([]plumbing.Hash, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it will make more sense to pass a set of commits and a set of ignores instead of lists. What do you think?


seen := hashListToSet(ignore)
result := make(map[plumbing.Hash]bool)
Copy link
Contributor

@alcortesm alcortesm Nov 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this a set but then return a list. Lets use a list instead directly or return a set.

for _, c := range commits {
err := iterateAll(r, c, seen, func(h plumbing.Hash) error {
if !seen[h] {
result[h] = true
seen[h] = true
}

return nil
})

if err != nil {
return nil, err
}
}

return hashSetToList(result), nil
}

func hashSetToList(hashes map[plumbing.Hash]bool) []plumbing.Hash {
var result []plumbing.Hash
for key := range hashes {
result = append(result, key)
}

return result
}

func hashListToSet(hashes []plumbing.Hash) map[plumbing.Hash]bool {
result := make(map[plumbing.Hash]bool)
for _, h := range hashes {
result[h] = true
}

return result
}

func iterateCommits(commit *Commit, cb func(c *Commit) error) error {
Copy link
Contributor

@alcortesm alcortesm Nov 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got lost pretty quickly with this, it should be easy to read in a single pass.

Please add comments explaining what iterateCommits, iterateCommitTrees and iterateAll do and refactor for legibility. For example iterateAll is just calling iterateCommits with some arguments, usually you don't write a new function for something so simple unless the name is very descriptive, but in this case it is not ('All' does not transmit any meaning, is iterateAll iterating over more thing than iterateCommits, then why call it All?). Why not just substitute iterateAll with a call to iterateCommits with well explained and named arguments?

if err := cb(commit); err != nil {
return err
}

return WalkCommitHistory(commit, func(c *Commit) error {
return cb(c)
})
}

func iterateCommitTrees(
repository *Repository,
commit *Commit,
cb func(h plumbing.Hash) error) error {

tree, err := commit.Tree()
if err != nil {
return err
}
if err := cb(tree.Hash); err != nil {
return err
}

treeWalker := NewTreeWalker(repository, tree, true)

for {
_, e, err := treeWalker.Next()
if err == io.EOF {
break
}
if err != nil {
return err
}
if err := cb(e.Hash); err != nil {
return err
}
}

return nil
}

func iterateAll(
r *Repository,
commit *Commit,
seen map[plumbing.Hash]bool,
cb func(h plumbing.Hash) error) error {

return iterateCommits(commit, func(commit *Commit) error {
if seen[commit.Hash] {
return nil
}

if err := cb(commit.Hash); err != nil {
return err
}

return iterateCommitTrees(r, commit, func(h plumbing.Hash) error {
return cb(h)
})
})
}
145 changes: 145 additions & 0 deletions revlist_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,145 @@
package git

import (
"gopkg.in/src-d/go-git.v4/fixtures"
"gopkg.in/src-d/go-git.v4/plumbing"

. "gopkg.in/check.v1"
)

type RevListSuite struct {
BaseSuite
r *Repository
}

var _ = Suite(&RevListSuite{})

const (
initialCommit = "b029517f6300c2da0f4b651b8642506cd6aaf45d"
secondCommit = "b8e471f58bcbca63b07bda20e428190409c2db47"

someCommit = "918c48b83bd081e863dbe1b80f8998f058cd8294"
someCommitBranch = "e8d3ffab552895c19b9fcf7aa264d277cde33881"
someCommitOtherBranch = "6ecf0ef2c2dffb796033e5a02219af86ec6584e5"
)

// Basic fixture repository commits tree:
//
// * 6ecf0ef vendor stuff
// | * e8d3ffa some code in a branch
// |/
// * 918c48b some code
// * af2d6a6 some json
// * 1669dce Merge branch 'master'
// |\
// | * a5b8b09 Merge pull request #1
// | |\
// | | * b8e471f Creating changelog
// | |/
// * | 35e8510 binary file
// |/
// * b029517 Initial commit
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is awesome, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the record: git log --graph --oneline --all

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if is for the record, then write this as a comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


func (s *RevListSuite) SetUpTest(c *C) {
r, err := NewFilesystemRepository(fixtures.Basic().One().DotGit().Base())
c.Assert(err, IsNil)
s.r = r
}

// ---
// | |\
// | | * b8e471f Creating changelog
// | |/
// * | 35e8510 binary file
// |/
// * b029517 Initial commit
func (s *RevListSuite) TestRevListObjects(c *C) {
revList := map[string]bool{
"b8e471f58bcbca63b07bda20e428190409c2db47": true, // second commit
"c2d30fa8ef288618f65f6eed6e168e0d514886f4": true, // init tree
"d3ff53e0564a9f87d8e84b6e28e5060e517008aa": true, // CHANGELOG
}

initCommit, err := s.r.Commit(plumbing.NewHash(initialCommit))
c.Assert(err, IsNil)
secondCommit, err := s.r.Commit(plumbing.NewHash(secondCommit))
c.Assert(err, IsNil)

localHist, err := RevListObjects(s.r, []*Commit{initCommit}, nil)
c.Assert(err, IsNil)

remoteHist, err := RevListObjects(s.r, []*Commit{secondCommit}, localHist)
c.Assert(err, IsNil)

for _, h := range remoteHist {
c.Assert(revList[h.String()], Equals, true)
}
c.Assert(len(remoteHist), Equals, len(revList))
}

func (s *RevListSuite) TestRevListObjectsReverse(c *C) {
initCommit, err := s.r.Commit(plumbing.NewHash(initialCommit))
c.Assert(err, IsNil)

secondCommit, err := s.r.Commit(plumbing.NewHash(secondCommit))
c.Assert(err, IsNil)

localHist, err := RevListObjects(s.r, []*Commit{secondCommit}, nil)
c.Assert(err, IsNil)

remoteHist, err := RevListObjects(s.r, []*Commit{initCommit}, localHist)
c.Assert(err, IsNil)

c.Assert(len(remoteHist), Equals, 0)
}

func (s *RevListSuite) TestRevListObjectsSameCommit(c *C) {
commit, err := s.r.Commit(plumbing.NewHash(secondCommit))
c.Assert(err, IsNil)

localHist, err := RevListObjects(s.r, []*Commit{commit}, nil)
c.Assert(err, IsNil)

remoteHist, err := RevListObjects(s.r, []*Commit{commit}, localHist)
c.Assert(err, IsNil)

c.Assert(len(remoteHist), Equals, 0)
}

// * 6ecf0ef vendor stuff
// | * e8d3ffa some code in a branch
// |/
// * 918c48b some code
// -----
func (s *RevListSuite) TestRevListObjectsNewBranch(c *C) {
someCommit, err := s.r.Commit(plumbing.NewHash(someCommit))
c.Assert(err, IsNil)

someCommitBranch, err := s.r.Commit(plumbing.NewHash(someCommitBranch))
c.Assert(err, IsNil)

someCommitOtherBranch, err := s.r.Commit(plumbing.NewHash(someCommitOtherBranch))
c.Assert(err, IsNil)

localHist, err := RevListObjects(s.r, []*Commit{someCommit}, nil)
c.Assert(err, IsNil)

remoteHist, err := RevListObjects(
s.r, []*Commit{someCommitBranch, someCommitOtherBranch}, localHist)
c.Assert(err, IsNil)

revList := map[string]bool{
"a8d315b2b1c615d43042c3a62402b8a54288cf5c": true, // init tree
"cf4aa3b38974fb7d81f367c0830f7d78d65ab86b": true, // vendor folder
"9dea2395f5403188298c1dabe8bdafe562c491e3": true, // foo.go
"e8d3ffab552895c19b9fcf7aa264d277cde33881": true, // branch commit
"dbd3641b371024f44d0e469a9c8f5457b0660de1": true, // init tree
"7e59600739c96546163833214c36459e324bad0a": true, // README
"6ecf0ef2c2dffb796033e5a02219af86ec6584e5": true, // otherBranch commit
}

for _, h := range remoteHist {
c.Assert(revList[h.String()], Equals, true)
}
c.Assert(len(remoteHist), Equals, len(revList))
}