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

Submodules init and update #270

Merged
merged 12 commits into from
Feb 21, 2017
5 changes: 3 additions & 2 deletions _examples/clone/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,9 @@ func main() {
Info("git clone %s %s", url, directory)

r, err := git.PlainClone(directory, false, &git.CloneOptions{
URL: url,
Depth: 1,
URL: url,
RecursiveSubmodules: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Change name to imperative: RecurseSubmodules.

Depth: 1,
})

CheckIfError(err)
Expand Down
19 changes: 2 additions & 17 deletions common.go
Original file line number Diff line number Diff line change
@@ -1,23 +1,8 @@
package git

import (
"strings"
import "strings"

"srcd.works/go-git.v4/config"
"srcd.works/go-git.v4/plumbing/storer"
)

// Storer is a generic storage of objects, references and any information
// related to a particular repository. The package srcd.works/go-git.v4/storage
// contains two implementation a filesystem base implementation (such as `.git`)
// and a memory implementations being ephemeral
type Storer interface {
storer.EncodedObjectStorer
storer.ReferenceStorer
storer.ShallowStorer
storer.IndexStorer
config.ConfigStorer
}
const defaultDotGitPath = ".git"

// countLines returns the number of lines in a string à la git, this is
// The newline character is assumed to be '\n'. The empty string
Expand Down
9 changes: 9 additions & 0 deletions config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ type Config struct {
// IsBare if true this repository is assumed to be bare and has no
// working directory associated with it
IsBare bool
// Worktree is the path to the root of the working tree
Worktree string
Copy link
Contributor

Choose a reason for hiding this comment

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

change name to WorkTtree.

Add full stop at the end of the phrase.

We should call it WorkTreePath.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

worktree is a single word, is a concept. I think that is enough to explained that is a path, since a worktree and is a string.

}
// Remote list of repository remotes
Remotes map[string]*RemoteConfig
Expand Down Expand Up @@ -76,6 +78,7 @@ const (
fetchKey = "fetch"
urlKey = "url"
bareKey = "bare"
worktreeKey = "worktree"
Copy link
Contributor

Choose a reason for hiding this comment

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

Change this to workTreeKey.

)

// Unmarshal parses a git-config file and stores it
Expand All @@ -97,6 +100,8 @@ func (c *Config) unmarshalCore() {
if s.Options.Get(bareKey) == "true" {
c.Core.IsBare = true
}

c.Core.Worktree = s.Options.Get(worktreeKey)
}

func (c *Config) unmarshalRemotes() error {
Expand Down Expand Up @@ -129,6 +134,10 @@ func (c *Config) Marshal() ([]byte, error) {
func (c *Config) marshalCore() {
s := c.raw.Section(coreSection)
s.SetOption(bareKey, fmt.Sprintf("%t", c.Core.IsBare))

if c.Core.Worktree != "" {
s.SetOption(worktreeKey, c.Core.Worktree)
}
}

func (c *Config) marshalRemotes() {
Expand Down
25 changes: 25 additions & 0 deletions config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ var _ = Suite(&ConfigSuite{})
func (s *ConfigSuite) TestUnmarshall(c *C) {
input := []byte(`[core]
bare = true
worktree = foo
[remote "origin"]
url = [email protected]:mcuadros/go-git.git
fetch = +refs/heads/*:refs/remotes/origin/*
Expand All @@ -22,15 +23,39 @@ func (s *ConfigSuite) TestUnmarshall(c *C) {
c.Assert(err, IsNil)

c.Assert(cfg.Core.IsBare, Equals, true)
c.Assert(cfg.Core.Worktree, Equals, "foo")
c.Assert(cfg.Remotes, HasLen, 1)
c.Assert(cfg.Remotes["origin"].Name, Equals, "origin")
c.Assert(cfg.Remotes["origin"].URL, Equals, "[email protected]:mcuadros/go-git.git")
c.Assert(cfg.Remotes["origin"].Fetch, DeepEquals, []RefSpec{"+refs/heads/*:refs/remotes/origin/*"})
}

func (s *ConfigSuite) TestMarshall(c *C) {
output := []byte(`[core]
bare = true
worktree = bar
[remote "origin"]
url = [email protected]:mcuadros/go-git.git
`)

cfg := NewConfig()
cfg.Core.IsBare = true
cfg.Core.Worktree = "bar"
cfg.Remotes["origin"] = &RemoteConfig{
Name: "origin",
URL: "[email protected]:mcuadros/go-git.git",
}

b, err := cfg.Marshal()
c.Assert(err, IsNil)

c.Assert(string(b), Equals, string(output))
}

func (s *ConfigSuite) TestUnmarshallMarshall(c *C) {
input := []byte(`[core]
bare = true
worktree = foo
custom = ignored
[remote "origin"]
url = [email protected]:mcuadros/go-git.git
Expand Down
2 changes: 1 addition & 1 deletion doc.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,4 @@
// It is highly extensible, we have been following the open/close principle in
// its design to facilitate extensions, mainly focusing the efforts on the
// persistence of the objects.
package git
package git // import "srcd.works/go-git.v4"
Copy link
Contributor

Choose a reason for hiding this comment

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

leftover?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup

4 changes: 4 additions & 0 deletions options.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@ type CloneOptions struct {
SingleBranch bool
// Limit fetching to the specified number of commits
Depth int
// RecursiveSubmodules after the clone is created, initialize all submodules
// within, using their default settings. This option is ignored if the
// cloned repository does not have a worktree
Copy link
Contributor

Choose a reason for hiding this comment

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

end the paragraph with a full stop.

RecursiveSubmodules bool
// Progress is where the human readable information sent by the server is
// stored, if nil nothing is stored and the capability (if supported)
// no-progress, is sent to the server to avoid send this information
Expand Down
9 changes: 9 additions & 0 deletions plumbing/format/index/encoder.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"errors"
"hash"
"io"
"sort"
"time"

"srcd.works/go-git.v4/utils/binary"
Expand Down Expand Up @@ -61,6 +62,8 @@ func (e *Encoder) encodeHeader(idx *Index) error {
}

func (e *Encoder) encodeEntries(idx *Index) error {
sort.Sort(ByName(idx.Entries))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why? please explain and add tests if this is a bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is already covered by the tests, I introduced another entry in the tests


for _, entry := range idx.Entries {
if err := e.encodeEntry(&entry); err != nil {
return err
Expand Down Expand Up @@ -139,3 +142,9 @@ func (e *Encoder) padEntry(wrote int) error {
func (e *Encoder) encodeFooter() error {
return binary.Write(e.w, e.hash.Sum(nil))
}

type ByName []Entry
Copy link
Collaborator

Choose a reason for hiding this comment

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

private type?


func (l ByName) Len() int { return len(l) }
func (l ByName) Swap(i, j int) { l[i], l[j] = l[j], l[i] }
func (l ByName) Less(i, j int) bool { return l[i].Name < l[j].Name }
10 changes: 10 additions & 0 deletions plumbing/format/index/encoder_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,11 @@ func (s *IndexSuite) TestEncode(c *C) {
Stage: TheirMode,
Hash: plumbing.NewHash("e25b29c8946e0e192fae2edc1dabf7be71e8ecf3"),
Name: "foo",
}, {
CreatedAt: time.Now(),
ModifiedAt: time.Now(),
Name: "bar",
Size: 82,
}, {
CreatedAt: time.Now(),
ModifiedAt: time.Now(),
Expand All @@ -42,6 +47,11 @@ func (s *IndexSuite) TestEncode(c *C) {
c.Assert(err, IsNil)

c.Assert(idx, DeepEquals, output)

c.Assert(output.Entries[0].Name, Equals, strings.Repeat(" ", 20))
c.Assert(output.Entries[1].Name, Equals, "bar")
c.Assert(output.Entries[2].Name, Equals, "foo")

}

func (s *IndexSuite) TestEncodeUnsuportedVersion(c *C) {
Expand Down
13 changes: 9 additions & 4 deletions plumbing/format/index/index.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,14 @@ const (
// in the worktree, having information about the working files. Changes in
// worktree are detected using this Index. The Index is also used during merges
type Index struct {
Version uint32
Entries []Entry
Cache *Tree
// Version is index version
Version uint32
// Entries collection of entries represented by this Index. The order of
// this collection is not guaranteed
Entries []Entry
// Cache represents the 'Cached tree' extension
Cache *Tree
// ResolveUndo represents the 'Resolve undo' extension
ResolveUndo *ResolveUndo
}

Expand Down Expand Up @@ -84,7 +89,7 @@ type TreeEntry struct {
// Path component (relative to its parent directory)
Path string
// Entries is the number of entries in the index that is covered by the tree
// this entry represents
// this entry represents.
Entries int
// Trees is the number that represents the number of subtrees this tree has
Trees int
Expand Down
2 changes: 1 addition & 1 deletion plumbing/object/file.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ func (iter *FileIter) Next() (*File, error) {
return nil, err
}

if entry.Mode.IsDir() {
if entry.Mode.IsDir() || entry.Mode == SubmoduleMode {
Copy link
Contributor

Choose a reason for hiding this comment

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

Invert the logic here. Just continue if it is not a file, otherwise we are always adding exceptions as they come along.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you elaborate?

Copy link
Contributor

@alcortesm alcortesm Feb 21, 2017

Choose a reason for hiding this comment

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

Now, this is how it works:

The FileIter.Next method returns Files by going over all the elements in the tree, ignoring Dirs, and now (added in this PR) Submodules.

This is what I propose, instead:

The FileIter.Next method returns Files by going over all the elements in the tree, ignoring everything that is not a file (without naming explicitly what are not files).

This way we don't have to change the code everytime we support a new TreeEntry mode.

So instead of:

if entry.Mode.IsDir() || entry.Mode == SubmoduleMode {
    continue
}

I suggest:

if !entry.Mode.IsFile() && !entry.Mode.IsFileDeprecatedGroupWriteable() {
    continue
}

continue
}

Expand Down
26 changes: 26 additions & 0 deletions plumbing/object/file_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -247,3 +247,29 @@ func (s *FileSuite) TestFileIter(c *C) {

c.Assert(count, Equals, 1)
}

func (s *FileSuite) TestFileIterSubmodule(c *C) {
st, err := filesystem.NewStorage(fixtures.ByTag("submodule").One().DotGit())
Copy link
Contributor

Choose a reason for hiding this comment

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

please, comment what repository are you using for this test, it is very hard to know what should be inside the commit below otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true, if an object is referenced, we should use the url and not a tag

c.Assert(err, IsNil)

hash := plumbing.NewHash("a692ec699bff9117c1ed91752afbb7d9d272ebef")
commit, err := GetCommit(st, hash)
c.Assert(err, IsNil)

tree, err := commit.Tree()
c.Assert(err, IsNil)

expected := []string{
".gitmodules",
}

var count int
i := tree.Files()
i.ForEach(func(f *File) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

There are several things that are wrong with this test:

  • it is too complex to test that the obtained must be a single file.
  • If Files return an empty list of file, I'm pretty sure the test will pass, so we will never know if submodules are ignored.
  • the name of the test does not explain what it is doing (check that submodules are ignored)
  • the test is not calling the function that has the logic under test (FileIter.Next)

Copy link
Contributor Author

@mcuadros mcuadros Feb 20, 2017

Choose a reason for hiding this comment

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

it is too complex to test that the obtained must be a single file.

It's asserting the full content of the commit, in this case is only one file that is not a submodule

If Files return an empty list of file, I'm pretty sure the test will pass, so we will never know if submodules are ignored.

Nope, we have that is returned only one file, not less or more.

the name of the test does not explain what it is doing (check that submodules are ignored)

I can change the name, but usually for me is explaining that is walking in a commit with submodule, I am not want to explain the logic behind.

the test is not calling the function that has the logic under test (FileIter.Next)

ForEach calls Next

c.Assert(f.Name, Equals, expected[count])
count++
return nil
})

c.Assert(count, Equals, 1)
}
5 changes: 0 additions & 5 deletions plumbing/object/tree.go
Original file line number Diff line number Diff line change
Expand Up @@ -375,11 +375,6 @@ func (w *TreeWalker) Next() (name string, entry TreeEntry, err error) {
return
}

if entry.Mode == SubmoduleMode {
err = nil
continue
}

if entry.Mode.IsDir() {
obj, err = GetTree(w.s, entry.Hash)
}
Expand Down
38 changes: 38 additions & 0 deletions plumbing/object/tree_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,10 @@ import (
"io"
"os"

fixtures "github.com/src-d/go-git-fixtures"

"srcd.works/go-git.v4/plumbing"
"srcd.works/go-git.v4/storage/filesystem"

. "gopkg.in/check.v1"
"srcd.works/go-git.v4/plumbing/storer"
Expand Down Expand Up @@ -262,6 +265,41 @@ func (s *TreeSuite) TestTreeWalkerNextNonRecursive(c *C) {
c.Assert(count, Equals, 8)
}

func (s *TreeSuite) TestTreeWalkerNextSubmodule(c *C) {
st, err := filesystem.NewStorage(fixtures.ByTag("submodule").One().DotGit())
Copy link
Contributor

Choose a reason for hiding this comment

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

please, comment what repository are you using for this test, it is very hard to know what should be inside the commit below otherwise.

c.Assert(err, IsNil)

hash := plumbing.NewHash("a692ec699bff9117c1ed91752afbb7d9d272ebef")
commit, err := GetCommit(st, hash)
c.Assert(err, IsNil)

tree, err := commit.Tree()
c.Assert(err, IsNil)

expected := []string{
".gitmodules",
"basic",
"itself",
}

var count int
walker := NewTreeWalker(tree, true)
Copy link
Contributor

Choose a reason for hiding this comment

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

close the walker, or make NewTreeWalker return the close closure so we never forget about it again.

for {
name, entry, err := walker.Next()
if err == io.EOF {
break
}

c.Assert(err, IsNil)
c.Assert(entry, NotNil)
c.Assert(name, Equals, expected[count])

count++
}

c.Assert(count, Equals, 3)
}

var treeWalkerExpects = []struct {
Path, Mode, Name, Hash, Tree string
}{{
Expand Down
9 changes: 6 additions & 3 deletions remote.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
"srcd.works/go-git.v4/plumbing/storer"
"srcd.works/go-git.v4/plumbing/transport"
"srcd.works/go-git.v4/plumbing/transport/client"
"srcd.works/go-git.v4/storage"
"srcd.works/go-git.v4/storage/memory"
"srcd.works/go-git.v4/utils/ioutil"
)
Expand All @@ -25,10 +26,10 @@ var NoErrAlreadyUpToDate = errors.New("already up-to-date")
// Remote represents a connection to a remote repository
type Remote struct {
c *config.RemoteConfig
s Storer
s storage.Storer
}

func newRemote(s Storer, c *config.RemoteConfig) *Remote {
func newRemote(s storage.Storer, c *config.RemoteConfig) *Remote {
return &Remote{s: s, c: c}
}

Expand Down Expand Up @@ -321,7 +322,9 @@ func getHaves(localRefs storer.ReferenceStorer) ([]plumbing.Hash, error) {
return haves, nil
}

func getWants(spec []config.RefSpec, localStorer Storer, remoteRefs storer.ReferenceStorer) ([]plumbing.Hash, error) {
func getWants(
spec []config.RefSpec, localStorer storage.Storer, remoteRefs storer.ReferenceStorer,
) ([]plumbing.Hash, error) {
wantTags := true
for _, s := range spec {
if !s.IsWildcard() {
Expand Down
3 changes: 2 additions & 1 deletion remote_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"srcd.works/go-git.v4/config"
"srcd.works/go-git.v4/plumbing"
"srcd.works/go-git.v4/plumbing/storer"
"srcd.works/go-git.v4/storage"
"srcd.works/go-git.v4/storage/filesystem"
"srcd.works/go-git.v4/storage/memory"

Expand Down Expand Up @@ -126,7 +127,7 @@ func (s *RemoteSuite) TestFetchWithProgress(c *C) {
}

type mockPackfileWriter struct {
Storer
storage.Storer
PackfileWriterCalled bool
}

Expand Down
Loading