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
6 changes: 3 additions & 3 deletions _examples/clone/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,11 @@ func main() {
directory := os.Args[2]

// Clone the given repository to the given directory
Info("git clone %s %s", url, directory)
Info("git clone %s %s --recursive", url, directory)

r, err := git.PlainClone(directory, false, &git.CloneOptions{
URL: url,
Depth: 1,
URL: url,
RecurseSubmodules: git.DefaultRecursivity,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would change the name from DefaultRecursivity to DefaultRecurseModules or DefaultSubmoduleRecursivity or something like that, so that it does not get confused with other parameters related with recursivite of non-submodule stuff (e.g. reference resolution).

})

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
56 changes: 48 additions & 8 deletions config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,13 @@ 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 list of repository remotes
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 sentence with a full stop and add a verb.

Remotes map[string]*RemoteConfig
// Submodules list of repository submodules
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 sentence with a full stop and add a verb.

Copy link
Contributor

Choose a reason for hiding this comment

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

Explain what is the key to the map.

Submodules map[string]*Submodule

// contains the raw information of a config file, the main goal is preserve
// the parsed information from the original format, to avoid missing
Expand All @@ -50,8 +54,9 @@ type Config struct {
// NewConfig returns a new empty Config
func NewConfig() *Config {
return &Config{
Remotes: make(map[string]*RemoteConfig, 0),
raw: format.New(),
Remotes: make(map[string]*RemoteConfig, 0),
Submodules: make(map[string]*Submodule, 0),
raw: format.New(),
}
}

Expand All @@ -71,11 +76,13 @@ func (c *Config) Validate() error {
}

const (
remoteSection = "remote"
coreSection = "core"
fetchKey = "fetch"
urlKey = "url"
bareKey = "bare"
remoteSection = "remote"
submoduleSection = "submodule"
coreSection = "core"
fetchKey = "fetch"
urlKey = "url"
bareKey = "bare"
worktreeKey = "worktree"
)

// Unmarshal parses a git-config file and stores it
Expand All @@ -89,6 +96,7 @@ func (c *Config) Unmarshal(b []byte) error {
}

c.unmarshalCore()
c.unmarshalSubmodules()
return c.unmarshalRemotes()
}

Expand All @@ -97,6 +105,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 All @@ -113,10 +123,21 @@ func (c *Config) unmarshalRemotes() error {
return nil
}

func (c *Config) unmarshalSubmodules() {
s := c.raw.Section(submoduleSection)
for _, sub := range s.Subsections {
m := &Submodule{}
m.unmarshal(sub)

Copy link
Contributor

Choose a reason for hiding this comment

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

remove empty line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know why, I don't understand the coding standards we use in the project, I'm just trying to make your code look like the rest of the code in the project. We usually only insert empty lines before a return or to separate "code paragraphs".

c.Submodules[m.Name] = m
}
}

// Marshal returns Config encoded as a git-config file
func (c *Config) Marshal() ([]byte, error) {
c.marshalCore()
c.marshalRemotes()
c.marshalSubmodules()

buf := bytes.NewBuffer(nil)
if err := format.NewEncoder(buf).Encode(c.raw); err != nil {
Expand All @@ -129,6 +150,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 All @@ -142,6 +167,21 @@ func (c *Config) marshalRemotes() {
}
}

func (c *Config) marshalSubmodules() {
s := c.raw.Section(submoduleSection)
s.Subsections = make(format.Subsections, len(c.Submodules))

var i int
for _, r := range c.Submodules {
section := r.marshal()
// the submodule section at config is a subset of the .gitmodule file
// we should remove the non-valid options for the config file.
section.RemoveOption(pathKey)
s.Subsections[i] = section
i++
}
}

// RemoteConfig contains the configuration for a given remote repository
type RemoteConfig struct {
// Name of the remote
Expand Down
41 changes: 41 additions & 0 deletions config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,14 @@ 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/*
[submodule "qux"]
path = qux
url = https://github.com/foo/qux.git
branch = bar
[branch "master"]
remote = origin
merge = refs/heads/master
Expand All @@ -22,15 +27,51 @@ 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/*"})
c.Assert(cfg.Submodules, HasLen, 1)
c.Assert(cfg.Submodules["qux"].Name, Equals, "qux")
c.Assert(cfg.Submodules["qux"].URL, Equals, "https://github.com/foo/qux.git")
c.Assert(cfg.Submodules["qux"].Branch, Equals, "bar")

}

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

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

cfg.Submodules["qux"] = &Submodule{
Name: "qux",
URL: "https://github.com/foo/qux.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
5 changes: 2 additions & 3 deletions config/modules.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,8 @@ func NewModules() *Modules {
}

const (
submoduleSection = "submodule"
pathKey = "path"
branchKey = "branch"
pathKey = "path"
branchKey = "branch"
)

// Unmarshal parses a git-config file and stores it
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

29 changes: 29 additions & 0 deletions options.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,18 @@ import (
"srcd.works/go-git.v4/plumbing/transport"
)

// SubmoduleRescursivity defines how depth will affect any submodule recursive
// operation
type SubmoduleRescursivity int
Copy link
Contributor

Choose a reason for hiding this comment

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

alternative name: SubmoduleRecursionDepth, explain what negative values will do and how to make it infinite.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just change the time to uint, infinity by itself is not allowed, a number should be given always, even if is very high


const (
// DefaultRemoteName name of the default Remote, just like git command
DefaultRemoteName = "origin"

// NoRecursivity disables the recursion for a submodule operation
NoRecursivity SubmoduleRescursivity = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

alternative: NonRecursive.

// DefaultRecursivity allow recursion in a submodule operation
DefaultRecursivity SubmoduleRescursivity = 10
Copy link
Contributor

Choose a reason for hiding this comment

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

alternative: DefaultSubmoduleRecursionDepth.

)

var (
Expand All @@ -32,6 +41,10 @@ type CloneOptions struct {
SingleBranch bool
// Limit fetching to the specified number of commits
Depth int
// RecurseSubmodules 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.

RecurseSubmodules SubmoduleRescursivity
// 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 Expand Up @@ -67,6 +80,9 @@ type PullOptions struct {
Depth int
// Auth credentials, if required, to use with the remote repository
Auth transport.AuthMethod
// RecurseSubmodules controls if new commits of all populated submodules
// should be fetched too
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 sentence with a full stop.

RecurseSubmodules SubmoduleRescursivity
// 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 Expand Up @@ -148,3 +164,16 @@ func (o *PushOptions) Validate() error {

return nil
}

// SubmoduleUpdateOptions describes how a submodule update should be performed
Copy link
Contributor

Choose a reason for hiding this comment

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

Add full stop.

type SubmoduleUpdateOptions struct {
// Init initializes the submodules recorded in the index
Copy link
Contributor

Choose a reason for hiding this comment

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

Add full stop.

Copy link
Contributor

Choose a reason for hiding this comment

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

This needs much better explanation. It is a bool, but does not make sense with the comment.

Init bool
// NoFetch tell to the update command to don’t fetch new objects from the
Copy link
Collaborator

Choose a reason for hiding this comment

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

to don’t -> to not

// remote site.
NoFetch bool
// RecurseSubmodules the update is performed not only in the submodules of
// the current repository but also in any nested submodules inside those
// submodules (and so on). Until the SubmoduleRescursivity is reached.
RecurseSubmodules SubmoduleRescursivity
}
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
Loading