Submodules init and update #270

mcuadros · 2017-02-13T23:36:03Z

This is the implementation of git submodule init and git submodule update, this implementation has required make some changes non direct related:

storage: git.Storer move to storage.Storer and module handling 498dbf7, due to the requirement of make reference to the storage.Storer as part of itself was moved to another package in other to avoid a cyclic dependency
plumbing/object: allow TreeIter return SubModule entries b18d649 the submodule were ignored, now are being returned without problem
format/index: sort the Entries before encode 940a16c, this was a fix since a Index entries should be save in order by Name
config: adding Config.Core.Worktree f8b5557
Repository.Init now handles non-standard .git location65351f8, it creates a.git` file in the root of the worktree and sets Config.Worktree when the .git folder is not in a standard location

Once this is merge, I will do another PR renaming config.RemoteConfig to config.Remote to mimic config.Submodule

smola · 2017-02-17T16:28:52Z

_examples/clone/main.go

-		URL:   url,
-		Depth: 1,
+		URL:               url,
+		RecurseSubmodules: git.DefaultRecursivity,


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).

smola · 2017-02-17T16:30:30Z

options.go

+type SubmoduleUpdateOptions struct {
+	// Init initializes the submodules recorded in the index
+	Init bool
+	// NoFetch tell to the update command to don’t fetch new objects from the


to don’t -> to not

smola · 2017-02-17T16:30:44Z

plumbing/format/index/encoder.go

@@ -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


private type?

alcortesm · 2017-02-20T09:48:48Z

storage/test/storage_suite.go

@@ -21,6 +22,8 @@ type Storer interface {
 	storer.ShallowStorer
 	storer.IndexStorer
 	config.ConfigStorer
+


remove extra line here.

alcortesm · 2017-02-20T09:50:34Z

storage/test/storage_suite.go

+	c.Assert(err, IsNil)
+	c.Assert(storer, NotNil)
+
+	storer, err = s.Storer.Module("foo")


What is the point in calling Module twice?
(explain this in the name of the test)

Actually what is the point of this whole test?

tests that returns a module. The function should returns a module if exists or not. The duplicate code is an error

I don't understand your answer.

Can you explain what you are testing in the name of the test or on a comment?

alcortesm · 2017-02-20T09:52:49Z

storage/memory/storage.go

+		s[name] = NewStorage()
+	}
+
+	return s[name], nil


Don't look up twice in the map, use a variable to store the results of the first lookup.

alcortesm · 2017-02-20T09:53:27Z

storage/storer.go

+	ModuleStorer
+}
+
+type ModuleStorer interface {


This needs to be explained, What if the module doesn't exists?

alcortesm · 2017-02-20T09:57:23Z

_examples/clone/main.go

-		URL:   url,
-		Depth: 1,
+		URL:                 url,
+		RecursiveSubmodules: true,


Change name to imperative: RecurseSubmodules.

alcortesm · 2017-02-20T09:58:08Z

doc.go

@@ -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"


alcortesm · 2017-02-20T09:59:47Z

options.go

@@ -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


end the paragraph with a full stop.

alcortesm · 2017-02-20T10:01:37Z

repository.go

@@ -270,11 +265,13 @@ func (r *Repository) clone(o *CloneOptions) error {
 		Progress: o.Progress,
 	})
 	if err != nil {
+


remove empty line.

alcortesm · 2017-02-20T10:01:47Z

repository.go

 		return err
 	}

 	head, err := storer.ResolveReference(remoteRefs, o.ReferenceName)
 	if err != nil {
+


remove empty line.

alcortesm · 2017-02-20T10:01:57Z

repository.go

@@ -283,12 +280,33 @@ func (r *Repository) clone(o *CloneOptions) error {
 	}

 	if err := r.updateWorktree(); err != nil {
+		fmt.Println("q", err)


alcortesm · 2017-02-20T10:06:38Z

repository.go

+		return err
+	}
+
+	return s.Init()


Should this have a recursive flag, so the init can be done recursively?

alcortesm · 2017-02-20T10:08:03Z

worktree.go

+const gitmodulesFile = ".gitmodules"
+
+func (w *Worktree) Submodules() (Submodules, error) {
+	l := make(Submodules, 0)


make this after calling readGitmodulesFile, and make it with the size of the m.Submodules,

alcortesm · 2017-02-20T10:09:14Z

worktree.go

+
+	m := config.NewModules()
+	return m, m.Unmarshal(input)
+


Remove empty line.

alcortesm · 2017-02-20T10:10:48Z

worktree.go

@@ -167,6 +170,61 @@ func (w *Worktree) getMode(fi billy.FileInfo) os.FileMode {
 	return object.FileMode
 }

+const gitmodulesFile = ".gitmodules"
+
+func (w *Worktree) Submodules() (Submodules, error) {


document it, especially why it can return (empty slice and nil) and what does it means. Is it an error if the .gitmodules is empty?

alcortesm · 2017-02-20T10:19:19Z

plumbing/object/file.go

@@ -81,7 +81,7 @@ func (iter *FileIter) Next() (*File, error) {
 			return nil, err
 		}

-		if entry.Mode.IsDir() {
+		if entry.Mode.IsDir() || entry.Mode == SubmoduleMode {


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

Can you elaborate?

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 }

alcortesm · 2017-02-20T10:27:37Z

plumbing/object/file_test.go

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


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)

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

alcortesm · 2017-02-20T10:29:15Z

plumbing/object/file_test.go

@@ -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())


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

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

alcortesm · 2017-02-20T10:29:26Z

plumbing/object/tree_test.go

@@ -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())


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

alcortesm · 2017-02-20T10:31:35Z

plumbing/object/tree_test.go

+	}
+
+	var count int
+	walker := NewTreeWalker(tree, true)


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

alcortesm · 2017-02-20T10:34:04Z

plumbing/format/index/encoder.go

@@ -61,6 +62,8 @@ func (e *Encoder) encodeHeader(idx *Index) error {
 }

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


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

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

alcortesm · 2017-02-20T10:34:30Z

storage/filesystem/internal/dotgit/dotgit.go

@@ -390,6 +390,11 @@ func (d *DotGit) readReferenceFile(refsPath, refFile string) (ref *plumbing.Refe
 	return plumbing.NewReferenceFromStrings(refFile, line), nil
 }

+// Module return a billy.Filesystem poiting to the module folder
+func (d *DotGit) Module(name string) billy.Filesystem {


alcortesm · 2017-02-20T10:34:56Z

storage/filesystem/module.go

+	dir *dotgit.DotGit
+}
+
+func (s *ModuleStorage) Module(name string) (storage.Storer, error) {


Add tests and comments.

Is already covered by the tests. And the documentation is on the interface as in the other methods

alcortesm · 2017-02-20T10:35:48Z

storage/filesystem/module.go

+	"srcd.works/go-git.v4/storage/filesystem/internal/dotgit"
+)
+
+type ModuleStorage struct {


Do we really need this type?, can we just make dotgit implement storage.ModuleStorer instead.

This is the pattern we are using. I prefer keep dotgit isolate

alcortesm · 2017-02-20T10:37:25Z

config/config.go

@@ -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


change name to WorkTtree.

Add full stop at the end of the phrase.

We should call it WorkTreePath.

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.

alcortesm · 2017-02-20T10:38:38Z

config/config.go

@@ -76,6 +78,7 @@ const (
 	fetchKey      = "fetch"
 	urlKey        = "url"
 	bareKey       = "bare"
+	worktreeKey   = "worktree"


Change this to workTreeKey.

alcortesm · 2017-02-20T10:41:26Z

repository.go

+	return r, setWorktreeAndStoragePaths(r, worktree)
+}
+
+func setWorktreeAndStoragePaths(r *Repository, worktree billy.Filesystem) error {


Capitalize tree in the function name.

alcortesm · 2017-02-20T10:43:15Z

repository.go

+	return err
+}
+
+func setConfigWorktree(r *Repository, worktree, storage billy.Filesystem) error {


Capitalize tree in function name.

alcortesm · 2017-02-20T10:49:46Z

submodule.go

-	Branch string
-	URL    string
-
+	m *config.Submodule


Why is it called m?

alcortesm · 2017-02-20T10:50:04Z

submodule.go

-	URL    string
-
+	m *config.Submodule
+	w *Worktree


Change this to WorkTree.

alcortesm · 2017-02-20T10:50:57Z

submodule.go

 	r *Repository
 }

+// Config returns the submodule config
+func (s *Submodule) Config() *config.Submodule {


Why not make m public instead of a getter?

alcortesm · 2017-02-20T10:56:02Z

worktree.go

@@ -210,6 +210,22 @@ func (w *Worktree) getMode(fi billy.FileInfo) os.FileMode {

 const gitmodulesFile = ".gitmodules"

+// Submodule returns the submodule with the given name


Explains what happens if it does not exists.

alcortesm · 2017-02-20T10:57:36Z

worktree.go

+	return w.newSubmodule(c)
+}
+
+// Submodules returns all the available submodules


Explain that this also init the submodules and why?

alcortesm · 2017-02-20T11:00:17Z

config/config.go

@@ -40,8 +40,10 @@ type Config struct {
 		// Worktree is the path to the root of the working tree
 		Worktree string
 	}
-	// Remote list of repository remotes
+	// Remotes list of repository remotes


End the sentence with a full stop and add a verb.

alcortesm · 2017-02-20T11:00:38Z

config/config.go

 	Remotes map[string]*RemoteConfig
+	// Submodules list of repository submodules


End the sentence with a full stop and add a verb.

Explain what is the key to the map.

alcortesm · 2017-02-20T11:02:00Z

config/config.go

+	for _, sub := range s.Subsections {
+		m := &Submodule{}
+		m.unmarshal(sub)
+


remove empty line.

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".

alcortesm · 2017-02-20T11:04:21Z

options.go

@@ -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


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

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

alcortesm · 2017-02-20T11:05:03Z

options.go

 const (
 	// DefaultRemoteName name of the default Remote, just like git command
 	DefaultRemoteName = "origin"
+
+	// NoRecursivity disables the recursion for a submodule operation
+	NoRecursivity SubmoduleRescursivity = 0


alternative: NonRecursive.

alcortesm · 2017-02-20T11:05:28Z

options.go

+	// NoRecursivity disables the recursion for a submodule operation
+	NoRecursivity SubmoduleRescursivity = 0
+	// DefaultRecursivity allow recursion in a submodule operation
+	DefaultRecursivity SubmoduleRescursivity = 10


alternative: DefaultSubmoduleRecursionDepth.

alcortesm · 2017-02-20T11:06:52Z

options.go

@@ -71,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


End the sentence with a full stop.

alcortesm · 2017-02-20T11:07:10Z

options.go

@@ -152,3 +164,16 @@ func (o *PushOptions) Validate() error {

 	return nil
 }
+
+// SubmoduleUpdateOptions describes how a submodule update should be performed


Add full stop.

alcortesm · 2017-02-20T11:07:18Z

options.go

+
+// SubmoduleUpdateOptions describes how a submodule update should be performed
+type SubmoduleUpdateOptions struct {
+	// Init initializes the submodules recorded in the index


Add full stop.

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

alcortesm · 2017-02-21T10:26:55Z

storage/storer.go

@@ -18,6 +18,9 @@ type Storer interface {
 	ModuleStorer
 }

+// ModuleStorer allows interact with the modules' Storers


End the sentence with a full stop.

Rewrite the sentence a little bit, it is hard to understand, suggestion:

Types that keep track of the relation between submodule names and their Storers implement this interface.

alcortesm · 2017-02-21T11:48:37Z

storage/storer.go

 type ModuleStorer interface {
+	// Module returns a Storer reprensting a submodule, if not exists returns a
+	// new empty Storer is returned


End the sentence with a full stop.

Rewrite the sentence, it is hard to understand, suggestion:

Module returns the Storer of a submodule given its name. If the name is unknown to the ModuleStorer, it will create a new empty Storer and associate it to the name.

alcortesm · 2017-02-21T11:50:28Z

config/config.go

 		Worktree string
 	}
-	// Remotes list of repository remotes
+	// Remotes list of repository remotes, the key of the map is the name
+	// of the remote, should equal to RemoteConfig.Name


End the sentence with a full stop.

alcortesm · 2017-02-21T11:50:40Z

config/config.go

 	Remotes map[string]*RemoteConfig
-	// Submodules list of repository submodules
+	// Submodules list of repository submodules, the key of the map is the name
+	// of the submodule, should equal to Submodule.Name


End the sentence with a full stop.

alcortesm · 2017-02-21T13:20:16Z

plumbing/object/file.go

@@ -81,7 +81,7 @@ func (iter *FileIter) Next() (*File, error) {
 			return nil, err
 		}

-		if entry.Mode.IsDir() {
+		if entry.Mode.IsDir() || entry.Mode == SubmoduleMode {


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 }

alcortesm · 2017-02-21T13:30:02Z

config/config.go

+	for _, sub := range s.Subsections {
+		m := &Submodule{}
+		m.unmarshal(sub)
+


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".

alcortesm

Nice fixes, I agree with most of them, still some small issues pending, though.

mcuadros added 8 commits February 12, 2017 23:03

storage: git.Storer move to storage.Storer and module handling

498dbf7

submodule init implementation

b3b6e51

plumbing/object: allow TreeIter return SubModule entries

b18d649

submodule init implementation

c551c29

format/index: sort the Entries before encode

940a16c

config: adding Config.Core.Worktree

f8b5557

Repository.Init now handles non-standard .git location

65351f8

submodule init and update implementation

7e990a8

mcuadros changed the title ~~Submodules init and update~~ Submodules init and update (wip) Feb 14, 2017

mcuadros added 2 commits February 14, 2017 15:56

config: added Config.Submodules

09110d8

submodule update implementation

d6a6dec

mcuadros changed the title ~~Submodules init and update (wip)~~ Submodules init and update Feb 15, 2017

smola suggested changes Feb 17, 2017

View reviewed changes

alcortesm suggested changes Feb 20, 2017

View reviewed changes

alcortesm reviewed Feb 21, 2017

View reviewed changes

alcortesm suggested changes Feb 21, 2017

View reviewed changes

mcuadros added 2 commits February 21, 2017 16:03

documentation and API improvements

ed288b3

rebase master

790fbda

mcuadros merged commit 867b106 into src-d:master Feb 21, 2017

		@@ -210,6 +210,22 @@ func (w *Worktree) getMode(fi billy.FileInfo) os.FileMode {

		const gitmodulesFile = ".gitmodules"

		// Submodule returns the submodule with the given name

		Remotes map[string]*RemoteConfig
		// Submodules list of repository submodules

Submodules init and update #270

Submodules init and update #270

Conversation

mcuadros commented Feb 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcuadros Feb 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alcortesm Feb 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcuadros Feb 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alcortesm Feb 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alcortesm left a comment

Choose a reason for hiding this comment

mcuadros commented Feb 13, 2017 •

edited

Loading

mcuadros Feb 20, 2017 •

edited

Loading

alcortesm Feb 21, 2017 •

edited

Loading

mcuadros Feb 20, 2017 •

edited

Loading

alcortesm Feb 21, 2017 •

edited

Loading