Skip to content
This repository was archived by the owner on Sep 11, 2020. It is now read-only.

repository: added cleanup for PlainClone. Fixes #741 #880

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 54 additions & 1 deletion repository.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"context"
"errors"
"fmt"
"io"
stdioutil "io/ioutil"
"os"
"path"
Expand Down Expand Up @@ -347,12 +348,64 @@ func PlainClone(path string, isBare bool, o *CloneOptions) (*Repository, error)
// operation is complete, an error is returned. The context only affects to the
// transport operations.
func PlainCloneContext(ctx context.Context, path string, isBare bool, o *CloneOptions) (*Repository, error) {
dirEmpty := false
dirExist := false

file, err := os.Stat(path)
pathNotExist := os.IsNotExist(err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can write here:

file, err := os.Stat(path)
if os.IsNotExist(err) {
  dirExist = true
} else if err != nil {
  return nil, err
}

If os.Statreturns an error and it is not a not exist error, something is wrong (e.g. permissions) and we should not continue.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, my snippet was wrong.
Leave your logic as it is if behavior with permission errors is correct.
Do not use pathNotExist variable here only to use it for the if condition. Just write if !os.IsNotExist(err).


if !pathNotExist {
dirExist = file.IsDir()
}

if dirExist {
fh, err := os.Open(path)
if err != nil {
return nil, err
}
defer fh.Close()

names, err := fh.Readdirnames(1)

if err == io.EOF && len(names) == 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here if err is not nil or io.EOF, an error should be returned.

dirEmpty = true
} else {
return nil, ErrDirNotEmpty
}
}

r, err := PlainInit(path, isBare)
if err != nil {
return nil, err
}

return r, r.clone(ctx, o)
err = r.clone(ctx, o)
if err != nil && err != ErrRepositoryAlreadyExists {
if dirEmpty {
fh, err := os.Open(path)
if err != nil {
return nil, err
}
defer fh.Close()

names, err := fh.Readdirnames(-1)
if err != nil && err != io.EOF {
return nil, err
}

for _, name := range names {
err = os.RemoveAll(filepath.Join(path, name))
if err != nil {
return nil, err
}
}
} else if !dirExist {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Avoid additional indentation by reversing condition order:

if !dirExist {
    os.RemoveAll(path)
    return nil, err
}

// continue here

Copy link
Collaborator

Choose a reason for hiding this comment

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

Meh, I confused dirExist and dirEmpty, sorry.

os.RemoveAll(path)
return nil, err
}
}

return r, err
}

func newRepository(s storage.Storer, worktree billy.Filesystem) *Repository {
Expand Down
34 changes: 33 additions & 1 deletion repository_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -568,7 +568,7 @@ func (s *RepositorySuite) TestPlainClone(c *C) {
c.Assert(cfg.Branches["master"].Name, Equals, "master")
}

func (s *RepositorySuite) TestPlainCloneContext(c *C) {
func (s *RepositorySuite) TestPlainCloneContextWithProperParameters(c *C) {
ctx, cancel := context.WithCancel(context.Background())
cancel()

Expand All @@ -579,6 +579,38 @@ func (s *RepositorySuite) TestPlainCloneContext(c *C) {
c.Assert(err, NotNil)
}

func (s *RepositorySuite) TestPlainCloneContextWithIncorrectRepo(c *C) {
ctx, cancel := context.WithCancel(context.Background())
cancel()

tmpDir := c.MkDir()
repoDir := filepath.Join(tmpDir, "repoDir") //path.Join(path.Dir(tmpDir), "repoDir")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove commented out code.

r, err := PlainCloneContext(ctx, repoDir, false, &CloneOptions{
URL: "incorrectOnPurpose",
})
c.Assert(r, IsNil)
c.Assert(err, NotNil)

_, err = os.Stat(repoDir)
dirNotExist := os.IsNotExist(err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

There should be another test to check that if the directory exists, it is not removed by PlainClone.

c.Assert(dirNotExist, Equals, true)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The behaviour does not seem correct: it removes a directory that already existed before. go-git shouldn't remove anything it didn't create during clone.

}

func (s *RepositorySuite) TestPlainCloneContextWithNotEmptyDir(c *C) {
ctx, cancel := context.WithCancel(context.Background())
cancel()

tmpDir := c.MkDir()
err := ioutil.WriteFile(filepath.Join(tmpDir, "dummyFile"), []byte(fmt.Sprint("dummyContent")), 0644)
c.Assert(err, IsNil)

r, err := PlainCloneContext(ctx, tmpDir, false, &CloneOptions{
URL: "incorrectOnPurpose",
})
c.Assert(r, IsNil)
c.Assert(err, Equals, ErrDirNotEmpty)
}

func (s *RepositorySuite) TestPlainCloneWithRecurseSubmodules(c *C) {
if testing.Short() {
c.Skip("skipping test in short mode.")
Expand Down