-
Notifications
You must be signed in to change notification settings - Fork 535
Submodules init and update #270
Changes from 3 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 |
---|---|---|
|
@@ -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 |
---|---|---|
|
@@ -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) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" | ||
|
@@ -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()) | ||
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. |
||
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) | ||
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. 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 | ||
}{{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ import ( | |
"srcd.works/go-git.v4/plumbing" | ||
"srcd.works/go-git.v4/plumbing/object" | ||
"srcd.works/go-git.v4/plumbing/storer" | ||
"srcd.works/go-git.v4/storage" | ||
"srcd.works/go-git.v4/storage/filesystem" | ||
|
||
"srcd.works/go-billy.v1" | ||
|
@@ -29,7 +30,7 @@ var ( | |
|
||
// Repository represents a git repository | ||
type Repository struct { | ||
Storer Storer | ||
Storer storage.Storer | ||
|
||
r map[string]*Remote | ||
wt billy.Filesystem | ||
|
@@ -38,7 +39,7 @@ type Repository struct { | |
// Init creates an empty git repository, based on the given Storer and worktree. | ||
// The worktree Filesystem is optional, if nil a bare repository is created. If | ||
// the given storer is not empty ErrRepositoryAlreadyExists is returned | ||
func Init(s Storer, worktree billy.Filesystem) (*Repository, error) { | ||
func Init(s storage.Storer, worktree billy.Filesystem) (*Repository, error) { | ||
r := newRepository(s, worktree) | ||
_, err := r.Reference(plumbing.HEAD, false) | ||
switch err { | ||
|
@@ -66,7 +67,7 @@ func Init(s Storer, worktree billy.Filesystem) (*Repository, error) { | |
// The worktree can be nil when the repository being opened is bare, if the | ||
// repository is a normal one (not bare) and worktree is nil the err | ||
// ErrWorktreeNotProvided is returned | ||
func Open(s Storer, worktree billy.Filesystem) (*Repository, error) { | ||
func Open(s storage.Storer, worktree billy.Filesystem) (*Repository, error) { | ||
_, err := s.Reference(plumbing.HEAD) | ||
if err == plumbing.ErrReferenceNotFound { | ||
return nil, ErrRepositoryNotExists | ||
|
@@ -91,7 +92,7 @@ func Open(s Storer, worktree billy.Filesystem) (*Repository, error) { | |
// Clone a repository into the given Storer and worktree Filesystem with the | ||
// given options, if worktree is nil a bare repository is created. If the given | ||
// storer is not empty ErrRepositoryAlreadyExists is returned | ||
func Clone(s Storer, worktree billy.Filesystem, o *CloneOptions) (*Repository, error) { | ||
func Clone(s storage.Storer, worktree billy.Filesystem, o *CloneOptions) (*Repository, error) { | ||
r, err := Init(s, worktree) | ||
if err != nil { | ||
return nil, err | ||
|
@@ -159,7 +160,7 @@ func PlainClone(path string, isBare bool, o *CloneOptions) (*Repository, error) | |
return r, r.clone(o) | ||
} | ||
|
||
func newRepository(s Storer, worktree billy.Filesystem) *Repository { | ||
func newRepository(s storage.Storer, worktree billy.Filesystem) *Repository { | ||
return &Repository{ | ||
Storer: s, | ||
wt: worktree, | ||
|
@@ -247,12 +248,6 @@ func (r *Repository) clone(o *CloneOptions) error { | |
return err | ||
} | ||
|
||
// marks the repository as bare in the config, until we have Worktree, all | ||
// the repository are bare | ||
if err := r.setIsBare(true); err != nil { | ||
return err | ||
} | ||
|
||
c := &config.RemoteConfig{ | ||
Name: o.RemoteName, | ||
URL: o.URL, | ||
|
@@ -270,11 +265,13 @@ func (r *Repository) clone(o *CloneOptions) error { | |
Progress: o.Progress, | ||
}) | ||
if err != nil { | ||
|
||
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. |
||
return err | ||
} | ||
|
||
head, err := storer.ResolveReference(remoteRefs, o.ReferenceName) | ||
if err != nil { | ||
|
||
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. |
||
return err | ||
} | ||
|
||
|
@@ -283,12 +280,33 @@ func (r *Repository) clone(o *CloneOptions) error { | |
} | ||
|
||
if err := r.updateWorktree(); err != nil { | ||
fmt.Println("q", err) | ||
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? |
||
return err | ||
} | ||
|
||
if o.RecursiveSubmodules && r.wt != nil { | ||
if err := r.initSubmodules(); err != nil { | ||
return err | ||
} | ||
} | ||
|
||
return r.updateRemoteConfig(remote, o, c, head) | ||
} | ||
|
||
func (r *Repository) initSubmodules() error { | ||
w, err := r.Worktree() | ||
if err != nil { | ||
return err | ||
} | ||
|
||
s, err := w.Submodules() | ||
if err != nil { | ||
return err | ||
} | ||
|
||
return s.Init() | ||
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. Should this have a recursive flag, so the init can be done recursively? |
||
} | ||
|
||
func (r *Repository) cloneRefSpec(o *CloneOptions, | ||
c *config.RemoteConfig) []config.RefSpec { | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,10 +20,10 @@ const ( | |
configPath = "config" | ||
indexPath = "index" | ||
shallowPath = "shallow" | ||
|
||
objectsPath = "objects" | ||
packPath = "pack" | ||
refsPath = "refs" | ||
modulePath = "module" | ||
objectsPath = "objects" | ||
packPath = "pack" | ||
refsPath = "refs" | ||
|
||
packExt = ".pack" | ||
idxExt = ".idx" | ||
|
@@ -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 { | ||
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. Add tests. 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. done |
||
return d.fs.Dir(d.fs.Join(modulePath, name)) | ||
} | ||
|
||
func isHex(s string) bool { | ||
for _, b := range []byte(s) { | ||
if isNum(b) { | ||
|
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
.