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

bjaroszewski · 2018-07-03T09:32:18Z

This PR introduces the cleanup for the PlainClone().

While using PlainClone, if the clone will raise an error (f.e. incorrect credentials used for authentication) we have work dir left.

smola · 2018-07-06T11:59:37Z

repository.go

@@ -334,7 +335,23 @@ func dotGitFileToOSFilesystem(path string, fs billy.Filesystem) (bfs billy.Files
 // if the new repository will be bare or normal. If the path is not empty
 // ErrRepositoryAlreadyExists is returned.
 func PlainClone(path string, isBare bool, o *CloneOptions) (*Repository, error) {
-	return PlainCloneContext(context.Background(), path, isBare, o)
+	r, err := PlainCloneContext(context.Background(), path, isBare, o)
+	if err != nil {


Cleanup logic should be the same for PlainCloneContext

smola · 2018-07-06T12:00:19Z

repository.go

+			os.Remove(path)
+			return nil, err
+		}
+	}


nitpick: add a space here (we usually add a space after every if/for block.

smola · 2018-07-06T12:01:26Z

repository.go

+		case ErrRepositoryAlreadyExists:
+			return nil, err
+		default:
+			os.Remove(path)


Why the default case is Remove and auth errors are RemoveAll?

smola

Please, add some test.

smola · 2018-07-06T15:04:28Z

repository.go

@@ -334,7 +334,13 @@ func dotGitFileToOSFilesystem(path string, fs billy.Filesystem) (bfs billy.Files
 // if the new repository will be bare or normal. If the path is not empty
 // ErrRepositoryAlreadyExists is returned.
 func PlainClone(path string, isBare bool, o *CloneOptions) (*Repository, error) {
-	return PlainCloneContext(context.Background(), path, isBare, o)
+	r, err := PlainCloneContext(context.Background(), path, isBare, o)


Keep it in a single place. If you implement it in PlainCloneContext, then that's it, you don't need it here.

Signed-off-by: Bartek Jaroszewski <[email protected]>

smola · 2018-07-19T14:05:32Z

repository_test.go

+
+	_, err = os.Stat(dir)
+	dirNotExist := os.IsNotExist(err)
+	c.Assert(dirNotExist, Equals, true)


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.

Signed-off-by: Bartek Jaroszewski <[email protected]>

smola · 2018-08-01T14:13:17Z

repository_test.go

+	c.Assert(err, NotNil)
+
+	_, err = os.Stat(repoDir)
+	dirNotExist := os.IsNotExist(err)


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

mcuadros · 2018-08-07T09:23:10Z

@bjaroszewski friendly ping

bjaroszewski · 2018-08-07T09:27:58Z

@mcuadros as per discussion on the slack, I will introduce the changes as @smola suggested for this cleanup on PlainCloneContext(). I should have some free evening this week, so I'll update the PR. Sorry for the delay with it.

Signed-off-by: Bartek Jaroszewski <[email protected]>

smola

Looks good. Some style comments:

smola · 2018-09-17T14:26:29Z

repository.go

+	dirExist := false
+
+	file, err := os.Stat(path)
+	pathNotExist := os.IsNotExist(err)


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.

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

smola · 2018-09-17T14:28:29Z

repository.go

+
+		names, err := fh.Readdirnames(1)
+
+		if err == io.EOF && len(names) == 0 {


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

smola · 2018-09-17T14:30:53Z

repository.go

+					return nil, err
+				}
+			}
+		} else if !dirExist {


Avoid additional indentation by reversing condition order:

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

Meh, I confused dirExist and dirEmpty, sorry.

smola · 2018-09-17T14:31:15Z

repository_test.go

+	cancel()
+
+	tmpDir := c.MkDir()
+	repoDir := filepath.Join(tmpDir, "repoDir") //path.Join(path.Dir(tmpDir), "repoDir")


Remove commented out code.

smola · 2018-09-17T14:32:57Z

CI fails:

# gopkg.in/src-d/go-git.v4
./repository.go:373:16: undefined: ErrDirNotEmpty

Signed-off-by: Bartek Jaroszewski <[email protected]>

PaluMacil

Your typo fix still had a typo: NotNill should be NotNil

PaluMacil · 2018-09-28T20:32:34Z

repository_test.go

@@ -576,7 +576,7 @@ func (s *RepositorySuite) TestPlainCloneContextWithProperParameters(c *C) {
 		URL: s.GetBasicLocalRepositoryURL(),
 	})

-	c.Assert(r, NoNill)
+	c.Assert(r, NotNill)


still a typo here... you want NotNil, not NotNill

mcuadros requested a review from smola July 6, 2018 09:26

smola suggested changes Jul 6, 2018

View reviewed changes

bjaroszewski force-pushed the master branch from 9d2e123 to 7a491e8 Compare July 17, 2018 20:02

repository: added cleanup for the PlainCloneContext(). [Fixes src-d#741]

77f738c

Signed-off-by: Bartek Jaroszewski <[email protected]>

bjaroszewski force-pushed the master branch from 7a491e8 to 77f738c Compare July 18, 2018 09:43

smola suggested changes Jul 19, 2018

View reviewed changes

repository: added cleanup for the PlainCloneContext(). [Fixes src-d#741]

203a372

Signed-off-by: Bartek Jaroszewski <[email protected]>

smola suggested changes Aug 1, 2018

View reviewed changes

bjaroszewski added 2 commits September 17, 2018 15:42

repository: added cleanup for the PlainCloneContext(). [Fixes src-d#741]

5230acf

Signed-off-by: Bartek Jaroszewski <[email protected]>

Merge remote-tracking branch 'upstream/master'

520ddb5

smola reviewed Sep 17, 2018

View reviewed changes

repository: added cleanup for the PlainCloneContext(). [Fixes src-d#741]

4599a77

Signed-off-by: Bartek Jaroszewski <[email protected]>

bjaroszewski force-pushed the master branch from 199a321 to 4599a77 Compare September 27, 2018 10:51

repository: fixed typo in PlainCloneContext(). [Fixes src-d#741]

6d3a9c3

Signed-off-by: Bartek Jaroszewski <[email protected]>

PaluMacil suggested changes Sep 28, 2018

View reviewed changes

smola mentioned this pull request Oct 30, 2018

cleanup after failed clone #1008

Merged

mcuadros closed this Oct 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

bjaroszewski commented Jul 3, 2018 •

edited

Loading

smola Jul 6, 2018

smola Jul 6, 2018

smola Jul 6, 2018

smola left a comment

smola Jul 6, 2018

smola Jul 19, 2018

smola Aug 1, 2018

mcuadros commented Aug 7, 2018

bjaroszewski commented Aug 7, 2018

smola left a comment

smola Sep 17, 2018

smola Sep 18, 2018

smola Sep 17, 2018

smola Sep 17, 2018

smola Sep 19, 2018

smola Sep 17, 2018

smola commented Sep 17, 2018

PaluMacil left a comment

PaluMacil Sep 28, 2018


		names, err := fh.Readdirnames(1)

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

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

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

Conversation

bjaroszewski commented Jul 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smola left a 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 commented Aug 7, 2018

bjaroszewski commented Aug 7, 2018

smola left a 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

smola commented Sep 17, 2018

PaluMacil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjaroszewski commented Jul 3, 2018 •

edited

Loading