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

 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

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

 	})

-	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

repository_test.go

})

c.Assert(r, NoNill)

c.Assert(r, NotNill)

Copy link

PaluMacil Sep 28, 2018

Choose a reason for hiding this comment

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

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

smola mentioned this pull request Oct 30, 2018

cleanup after failed clone #1008

Merged

mcuadros closed this Oct 30, 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

Uh oh!

Conversation

bjaroszewski commented Jul 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smola left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcuadros commented Aug 7, 2018

Uh oh!

bjaroszewski commented Aug 7, 2018

Uh oh!

smola left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smola commented Sep 17, 2018

Uh oh!

PaluMacil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bjaroszewski commented Jul 3, 2018 •

edited

Loading