-
Notifications
You must be signed in to change notification settings - Fork 534
Submodules init and update #270
Changes from 9 commits
498dbf7
b3b6e51
b18d649
c551c29
940a16c
f8b5557
65351f8
7e990a8
09110d8
d6a6dec
ed288b3
790fbda
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. change name to Add full stop at the end of the phrase. We should call it There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. End the sentence with a full stop and add a verb. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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(), | ||
} | ||
} | ||
|
||
|
@@ -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 | ||
|
@@ -89,6 +96,7 @@ func (c *Config) Unmarshal(b []byte) error { | |
} | ||
|
||
c.unmarshalCore() | ||
c.unmarshalSubmodules() | ||
return c.unmarshalRemotes() | ||
} | ||
|
||
|
@@ -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 { | ||
|
@@ -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) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remove empty line. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
|
@@ -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() { | ||
|
@@ -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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. leftover? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yup |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ import ( | |
"errors" | ||
"hash" | ||
"io" | ||
"sort" | ||
"time" | ||
|
||
"srcd.works/go-git.v4/utils/binary" | ||
|
@@ -61,6 +62,8 @@ func (e *Encoder) encodeHeader(idx *Index) error { | |
} | ||
|
||
func (e *Encoder) encodeEntries(idx *Index) error { | ||
sort.Sort(ByName(idx.Entries)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why? please explain and add tests if this is a bug. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 } |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -81,7 +81,7 @@ func (iter *FileIter) Next() (*File, error) { | |
return nil, err | ||
} | ||
|
||
if entry.Mode.IsDir() { | ||
if entry.Mode.IsDir() || entry.Mode == SubmoduleMode { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you elaborate? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now, this is how it works: The This is what I propose, instead: The This way we don't have to change the code everytime we support a new TreeEntry mode. So instead of:
I suggest:
|
||
continue | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are several things that are wrong with this test:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It's asserting the full content of the commit, in this case is only one file that is not a submodule
Nope, we have that is returned only one file, not less or more.
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.
ForEach calls Next |
||
c.Assert(f.Name, Equals, expected[count]) | ||
count++ | ||
return nil | ||
}) | ||
|
||
c.Assert(count, Equals, 1) | ||
} |
There was a problem hiding this comment.
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
.