From 05c414a169a75ca933402e5be19a5c4304aa4f00 Mon Sep 17 00:00:00 2001 From: Javi Fontan Date: Mon, 26 Mar 2018 23:18:54 +0200 Subject: [PATCH 1/3] *: Use CheckClose with named returns Previously some close errors were losts. This is specially problematic in go-git as lots of work is done here like generating indexes and moving packfiles. Signed-off-by: Javi Fontan --- plumbing/format/packfile/common.go | 3 +-- plumbing/format/packfile/scanner.go | 5 +++-- plumbing/object/blob.go | 2 +- plumbing/object/commit.go | 2 +- plumbing/object/file.go | 2 +- plumbing/object/tag.go | 5 +++-- plumbing/object/tree.go | 4 ++-- plumbing/transport/http/common.go | 6 +++--- remote.go | 12 ++++++------ repository.go | 6 ++---- storage/filesystem/config.go | 8 ++++---- storage/filesystem/index.go | 4 ++-- storage/filesystem/internal/dotgit/dotgit.go | 8 ++++---- storage/filesystem/internal/dotgit/dotgit_setref.go | 2 +- storage/filesystem/object.go | 8 ++++---- 15 files changed, 38 insertions(+), 39 deletions(-) diff --git a/plumbing/format/packfile/common.go b/plumbing/format/packfile/common.go index 7dad1f6d6..beb015d3e 100644 --- a/plumbing/format/packfile/common.go +++ b/plumbing/format/packfile/common.go @@ -40,8 +40,7 @@ func UpdateObjectStorage(s storer.EncodedObjectStorer, packfile io.Reader) error return err } -func writePackfileToObjectStorage(sw storer.PackfileWriter, packfile io.Reader) error { - var err error +func writePackfileToObjectStorage(sw storer.PackfileWriter, packfile io.Reader) (err error) { w, err := sw.PackfileWriter() if err != nil { return err diff --git a/plumbing/format/packfile/scanner.go b/plumbing/format/packfile/scanner.go index 8c216f11b..6fc183b94 100644 --- a/plumbing/format/packfile/scanner.go +++ b/plumbing/format/packfile/scanner.go @@ -279,14 +279,15 @@ func (s *Scanner) NextObject(w io.Writer) (written int64, crc32 uint32, err erro // from it zlib stream in an object entry in the packfile. func (s *Scanner) copyObject(w io.Writer) (n int64, err error) { if s.zr == nil { - zr, err := zlib.NewReader(s.r) + var zr io.ReadCloser + zr, err = zlib.NewReader(s.r) if err != nil { return 0, fmt.Errorf("zlib initialization error: %s", err) } s.zr = zr.(readerResetter) } else { - if err := s.zr.Reset(s.r, nil); err != nil { + if err = s.zr.Reset(s.r, nil); err != nil { return 0, fmt.Errorf("zlib reset error: %s", err) } } diff --git a/plumbing/object/blob.go b/plumbing/object/blob.go index 2608477a8..f376baa65 100644 --- a/plumbing/object/blob.go +++ b/plumbing/object/blob.go @@ -67,7 +67,7 @@ func (b *Blob) Decode(o plumbing.EncodedObject) error { } // Encode transforms a Blob into a plumbing.EncodedObject. -func (b *Blob) Encode(o plumbing.EncodedObject) error { +func (b *Blob) Encode(o plumbing.EncodedObject) (err error) { o.SetType(plumbing.BlobObject) w, err := o.Writer() diff --git a/plumbing/object/commit.go b/plumbing/object/commit.go index a3177144e..c9a4c0ee8 100644 --- a/plumbing/object/commit.go +++ b/plumbing/object/commit.go @@ -226,7 +226,7 @@ func (b *Commit) Encode(o plumbing.EncodedObject) error { return b.encode(o, true) } -func (b *Commit) encode(o plumbing.EncodedObject, includeSig bool) error { +func (b *Commit) encode(o plumbing.EncodedObject, includeSig bool) (err error) { o.SetType(plumbing.CommitObject) w, err := o.Writer() if err != nil { diff --git a/plumbing/object/file.go b/plumbing/object/file.go index 40b5206b5..1c5fdbb38 100644 --- a/plumbing/object/file.go +++ b/plumbing/object/file.go @@ -44,7 +44,7 @@ func (f *File) Contents() (content string, err error) { } // IsBinary returns if the file is binary or not -func (f *File) IsBinary() (bool, error) { +func (f *File) IsBinary() (bin bool, err error) { reader, err := f.Reader() if err != nil { return false, err diff --git a/plumbing/object/tag.go b/plumbing/object/tag.go index 19e55cfe7..905206bda 100644 --- a/plumbing/object/tag.go +++ b/plumbing/object/tag.go @@ -95,7 +95,8 @@ func (t *Tag) Decode(o plumbing.EncodedObject) (err error) { r := bufio.NewReader(reader) for { - line, err := r.ReadBytes('\n') + var line []byte + line, err = r.ReadBytes('\n') if err != nil && err != io.EOF { return err } @@ -168,7 +169,7 @@ func (t *Tag) Encode(o plumbing.EncodedObject) error { return t.encode(o, true) } -func (t *Tag) encode(o plumbing.EncodedObject, includeSig bool) error { +func (t *Tag) encode(o plumbing.EncodedObject, includeSig bool) (err error) { o.SetType(plumbing.TagObject) w, err := o.Writer() if err != nil { diff --git a/plumbing/object/tree.go b/plumbing/object/tree.go index 2fcd979f5..c2399f8cd 100644 --- a/plumbing/object/tree.go +++ b/plumbing/object/tree.go @@ -233,7 +233,7 @@ func (t *Tree) Decode(o plumbing.EncodedObject) (err error) { } // Encode transforms a Tree into a plumbing.EncodedObject. -func (t *Tree) Encode(o plumbing.EncodedObject) error { +func (t *Tree) Encode(o plumbing.EncodedObject) (err error) { o.SetType(plumbing.TreeObject) w, err := o.Writer() if err != nil { @@ -242,7 +242,7 @@ func (t *Tree) Encode(o plumbing.EncodedObject) error { defer ioutil.CheckClose(w, &err) for _, entry := range t.Entries { - if _, err := fmt.Fprintf(w, "%o %s", entry.Mode, entry.Name); err != nil { + if _, err = fmt.Fprintf(w, "%o %s", entry.Mode, entry.Name); err != nil { return err } diff --git a/plumbing/transport/http/common.go b/plumbing/transport/http/common.go index 24e63a4d4..2c337b77a 100644 --- a/plumbing/transport/http/common.go +++ b/plumbing/transport/http/common.go @@ -31,7 +31,7 @@ func applyHeadersToRequest(req *http.Request, content *bytes.Buffer, host string const infoRefsPath = "/info/refs" -func advertisedReferences(s *session, serviceName string) (*packp.AdvRefs, error) { +func advertisedReferences(s *session, serviceName string) (ref *packp.AdvRefs, err error) { url := fmt.Sprintf( "%s%s?service=%s", s.endpoint.String(), infoRefsPath, serviceName, @@ -52,12 +52,12 @@ func advertisedReferences(s *session, serviceName string) (*packp.AdvRefs, error s.ModifyEndpointIfRedirect(res) defer ioutil.CheckClose(res.Body, &err) - if err := NewErr(res); err != nil { + if err = NewErr(res); err != nil { return nil, err } ar := packp.NewAdvRefs() - if err := ar.Decode(res.Body); err != nil { + if err = ar.Decode(res.Body); err != nil { if err == packp.ErrEmptyAdvRefs { err = transport.ErrEmptyRemoteRepository } diff --git a/remote.go b/remote.go index 8db645c83..666c9f5eb 100644 --- a/remote.go +++ b/remote.go @@ -73,7 +73,7 @@ func (r *Remote) Push(o *PushOptions) error { // The provided Context must be non-nil. If the context expires before the // operation is complete, an error is returned. The context only affects to the // transport operations. -func (r *Remote) PushContext(ctx context.Context, o *PushOptions) error { +func (r *Remote) PushContext(ctx context.Context, o *PushOptions) (err error) { if err := o.Validate(); err != nil { return err } @@ -243,12 +243,12 @@ func (r *Remote) Fetch(o *FetchOptions) error { return r.FetchContext(context.Background(), o) } -func (r *Remote) fetch(ctx context.Context, o *FetchOptions) (storer.ReferenceStorer, error) { +func (r *Remote) fetch(ctx context.Context, o *FetchOptions) (sto storer.ReferenceStorer, err error) { if o.RemoteName == "" { o.RemoteName = r.c.Name } - if err := o.Validate(); err != nil { + if err = o.Validate(); err != nil { return nil, err } @@ -295,7 +295,7 @@ func (r *Remote) fetch(ctx context.Context, o *FetchOptions) (storer.ReferenceSt return nil, err } - if err := r.fetchPack(ctx, o, s, req); err != nil { + if err = r.fetchPack(ctx, o, s, req); err != nil { return nil, err } } @@ -354,7 +354,7 @@ func (r *Remote) fetchPack(ctx context.Context, o *FetchOptions, s transport.Upl defer ioutil.CheckClose(reader, &err) - if err := r.updateShallow(o, reader); err != nil { + if err = r.updateShallow(o, reader); err != nil { return err } @@ -872,7 +872,7 @@ func (r *Remote) buildFetchedTags(refs memory.ReferenceStorage) (updated bool, e } // List the references on the remote repository. -func (r *Remote) List(o *ListOptions) ([]*plumbing.Reference, error) { +func (r *Remote) List(o *ListOptions) (rfs []*plumbing.Reference, err error) { s, err := newUploadPackSession(r.c.URLs[0], o.Auth) if err != nil { return nil, err diff --git a/repository.go b/repository.go index 98558d925..4ea51f5c3 100644 --- a/repository.go +++ b/repository.go @@ -270,9 +270,7 @@ func dotGitToOSFilesystems(path string) (dot, wt billy.Filesystem, err error) { return dot, fs, nil } -func dotGitFileToOSFilesystem(path string, fs billy.Filesystem) (billy.Filesystem, error) { - var err error - +func dotGitFileToOSFilesystem(path string, fs billy.Filesystem) (bfs billy.Filesystem, err error) { f, err := fs.Open(".git") if err != nil { return nil, err @@ -1092,7 +1090,7 @@ func (r *Repository) createNewObjectPack(cfg *RepackConfig) (h plumbing.Hash, er if los, ok := r.Storer.(storer.LooseObjectStorer); ok { err = los.ForEachObjectHash(func(hash plumbing.Hash) error { if ow.isSeen(hash) { - err := los.DeleteLooseObject(hash) + err = los.DeleteLooseObject(hash) if err != nil { return err } diff --git a/storage/filesystem/config.go b/storage/filesystem/config.go index a2cc17378..85feaf0a6 100644 --- a/storage/filesystem/config.go +++ b/storage/filesystem/config.go @@ -13,7 +13,7 @@ type ConfigStorage struct { dir *dotgit.DotGit } -func (c *ConfigStorage) Config() (*config.Config, error) { +func (c *ConfigStorage) Config() (conf *config.Config, err error) { cfg := config.NewConfig() f, err := c.dir.Config() @@ -32,15 +32,15 @@ func (c *ConfigStorage) Config() (*config.Config, error) { return nil, err } - if err := cfg.Unmarshal(b); err != nil { + if err = cfg.Unmarshal(b); err != nil { return nil, err } return cfg, err } -func (c *ConfigStorage) SetConfig(cfg *config.Config) error { - if err := cfg.Validate(); err != nil { +func (c *ConfigStorage) SetConfig(cfg *config.Config) (err error) { + if err = cfg.Validate(); err != nil { return err } diff --git a/storage/filesystem/index.go b/storage/filesystem/index.go index 14ab09a96..092edecf6 100644 --- a/storage/filesystem/index.go +++ b/storage/filesystem/index.go @@ -12,7 +12,7 @@ type IndexStorage struct { dir *dotgit.DotGit } -func (s *IndexStorage) SetIndex(idx *index.Index) error { +func (s *IndexStorage) SetIndex(idx *index.Index) (err error) { f, err := s.dir.IndexWriter() if err != nil { return err @@ -25,7 +25,7 @@ func (s *IndexStorage) SetIndex(idx *index.Index) error { return err } -func (s *IndexStorage) Index() (*index.Index, error) { +func (s *IndexStorage) Index() (i *index.Index, err error) { idx := &index.Index{ Version: 2, } diff --git a/storage/filesystem/internal/dotgit/dotgit.go b/storage/filesystem/internal/dotgit/dotgit.go index 027ef83b0..60d122fde 100644 --- a/storage/filesystem/internal/dotgit/dotgit.go +++ b/storage/filesystem/internal/dotgit/dotgit.go @@ -375,7 +375,7 @@ func (d *DotGit) findPackedRefsInFile(f billy.File) ([]*plumbing.Reference, erro return refs, s.Err() } -func (d *DotGit) findPackedRefs() ([]*plumbing.Reference, error) { +func (d *DotGit) findPackedRefs() (r []*plumbing.Reference, error error) { f, err := d.fs.Open(packedRefsPath) if err != nil { if os.IsNotExist(err) { @@ -676,7 +676,7 @@ func (d *DotGit) PackRefs() (err error) { // Gather all refs using addRefsFromRefDir and addRefsFromPackedRefs. var refs []*plumbing.Reference seen := make(map[plumbing.ReferenceName]bool) - if err := d.addRefsFromRefDir(&refs, seen); err != nil { + if err = d.addRefsFromRefDir(&refs, seen); err != nil { return err } if len(refs) == 0 { @@ -684,7 +684,7 @@ func (d *DotGit) PackRefs() (err error) { return nil } numLooseRefs := len(refs) - if err := d.addRefsFromPackedRefsFile(&refs, f, seen); err != nil { + if err = d.addRefsFromPackedRefsFile(&refs, f, seen); err != nil { return err } @@ -701,7 +701,7 @@ func (d *DotGit) PackRefs() (err error) { w := bufio.NewWriter(tmp) for _, ref := range refs { - _, err := w.WriteString(ref.String() + "\n") + _, err = w.WriteString(ref.String() + "\n") if err != nil { return err } diff --git a/storage/filesystem/internal/dotgit/dotgit_setref.go b/storage/filesystem/internal/dotgit/dotgit_setref.go index c732c9fa9..d27c1a303 100644 --- a/storage/filesystem/internal/dotgit/dotgit_setref.go +++ b/storage/filesystem/internal/dotgit/dotgit_setref.go @@ -9,7 +9,7 @@ import ( "gopkg.in/src-d/go-git.v4/utils/ioutil" ) -func (d *DotGit) setRef(fileName, content string, old *plumbing.Reference) error { +func (d *DotGit) setRef(fileName, content string, old *plumbing.Reference) (err error) { // If we are not checking an old ref, just truncate the file. mode := os.O_RDWR | os.O_CREATE if old == nil { diff --git a/storage/filesystem/object.go b/storage/filesystem/object.go index 9f1c5efa6..26190fda3 100644 --- a/storage/filesystem/object.go +++ b/storage/filesystem/object.go @@ -55,7 +55,7 @@ func (s *ObjectStorage) requireIndex() error { return nil } -func (s *ObjectStorage) loadIdxFile(h plumbing.Hash) error { +func (s *ObjectStorage) loadIdxFile(h plumbing.Hash) (err error) { f, err := s.dir.ObjectPackIdx(h) if err != nil { return err @@ -94,7 +94,7 @@ func (s *ObjectStorage) PackfileWriter() (io.WriteCloser, error) { } // SetEncodedObject adds a new object to the storage. -func (s *ObjectStorage) SetEncodedObject(o plumbing.EncodedObject) (plumbing.Hash, error) { +func (s *ObjectStorage) SetEncodedObject(o plumbing.EncodedObject) (h plumbing.Hash, err error) { if o.Type() == plumbing.OFSDeltaObject || o.Type() == plumbing.REFDeltaObject { return plumbing.ZeroHash, plumbing.ErrInvalidType } @@ -113,11 +113,11 @@ func (s *ObjectStorage) SetEncodedObject(o plumbing.EncodedObject) (plumbing.Has defer ioutil.CheckClose(or, &err) - if err := ow.WriteHeader(o.Type(), o.Size()); err != nil { + if err = ow.WriteHeader(o.Type(), o.Size()); err != nil { return plumbing.ZeroHash, err } - if _, err := io.Copy(ow, or); err != nil { + if _, err = io.Copy(ow, or); err != nil { return plumbing.ZeroHash, err } From 7a02aee903bebd1f5023793c35171953c1d0a7cf Mon Sep 17 00:00:00 2001 From: Javi Fontan Date: Tue, 27 Mar 2018 17:48:58 +0200 Subject: [PATCH 2/3] plumbing: transport, make target repo writeable in tests Some tests write to an already existent repository retrieved from fixtures. The permissions of these files are read only and make receive pack fail. This was shadowed before as close errors were lost. Signed-off-by: Javi Fontan --- plumbing/transport/test/receive_pack.go | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/plumbing/transport/test/receive_pack.go b/plumbing/transport/test/receive_pack.go index a68329e77..6179850ab 100644 --- a/plumbing/transport/test/receive_pack.go +++ b/plumbing/transport/test/receive_pack.go @@ -8,6 +8,8 @@ import ( "context" "io" "io/ioutil" + "os" + "path/filepath" "gopkg.in/src-d/go-git.v4/plumbing" "gopkg.in/src-d/go-git.v4/plumbing/format/packfile" @@ -225,6 +227,25 @@ func (s *ReceivePackSuite) receivePackNoCheck(c *C, ep *transport.Endpoint, ep.String(), url, callAdvertisedReferences, ) + // Set write permissions to endpoint directory files. By default + // fixtures are generated with read only permissions, this casuses + // errors deleting or modifying files. + rootPath := ep.Path + println("STAT", rootPath) + stat, err := os.Stat(ep.Path) + + if rootPath != "" && err == nil && stat.IsDir() { + objectPath := filepath.Join(rootPath, "objects/pack") + files, err := ioutil.ReadDir(objectPath) + c.Assert(err, IsNil) + + for _, file := range files { + path := filepath.Join(objectPath, file.Name()) + err = os.Chmod(path, 0644) + c.Assert(err, IsNil) + } + } + r, err := s.Client.NewReceivePackSession(ep, s.EmptyAuth) c.Assert(err, IsNil, comment) defer func() { c.Assert(r.Close(), IsNil, comment) }() From 6e07548d9078505ca2945f09d11729b14abcc907 Mon Sep 17 00:00:00 2001 From: Javi Fontan Date: Wed, 28 Mar 2018 10:59:33 +0200 Subject: [PATCH 3/3] storage: filesystem/dotgit, fix typo in return param Signed-off-by: Javi Fontan --- storage/filesystem/internal/dotgit/dotgit.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/storage/filesystem/internal/dotgit/dotgit.go b/storage/filesystem/internal/dotgit/dotgit.go index 60d122fde..6f0f1a593 100644 --- a/storage/filesystem/internal/dotgit/dotgit.go +++ b/storage/filesystem/internal/dotgit/dotgit.go @@ -375,7 +375,7 @@ func (d *DotGit) findPackedRefsInFile(f billy.File) ([]*plumbing.Reference, erro return refs, s.Err() } -func (d *DotGit) findPackedRefs() (r []*plumbing.Reference, error error) { +func (d *DotGit) findPackedRefs() (r []*plumbing.Reference, err error) { f, err := d.fs.Open(packedRefsPath) if err != nil { if os.IsNotExist(err) {