Repository: go-git/go-git

Issue:
Lost data due to clone logic
Hi there,

I was just working on a project using this library and I made the unfortunate mistake of cloning to my desktop thinking that a folder would be created based on the repo name, like the git cli does.   However, what ended up happening was that all the files on my Desktop were deleted and the git repo was clone directly to the desktop.  I partially blame myself for being more careful, but I wouldn't expect clone to clear a folder out.  I feel like the default logic should not clear an existing directory, and you could add a flag to the clone options to delete if you need it, then error if the directory already exists and isn't empty.  If that's not acceptable, I'd like to request that this deletion logic is boldly documented.


Discussion:
@github-actions: To help us keep things tidy and focus on the active tasks, we've introduced a stale bot to spot issues/PRs that haven't had any activity in a while.

This particular issue hasn't had any updates or activity in the past 90 days, so it's been labeled as 'stale'. If it remains inactive for the next 30 days, it'll be automatically closed.

We understand everyone's busy, but if this issue is still important to you, please feel free to add a comment or make an update to keep it active. 

Thanks for your understanding and cooperation!@onee-only: Since #1594 is merged, this issue can be closed.@AriehSchneier: Closing as per comment above

Solution patch:
diff --git a/go.mod b/go.mod
index f0b9eb10..122c5cd1 100644
--- a/go.mod
+++ b/go.mod
@@ -18,7 +18,7 @@ require (
 	github.com/go-git/go-git-fixtures/v5 v5.1.0
 	github.com/golang/groupcache v0.0.0-20241129210726-2c02b8208cf8
 	github.com/kevinburke/ssh_config v1.2.0
-	github.com/pjbgf/sha1cd v0.3.2
+	github.com/pjbgf/sha1cd v0.4.0
 	github.com/sergi/go-diff v1.4.0
 	github.com/stretchr/testify v1.10.0
 	golang.org/x/crypto v0.39.0
diff --git a/go.sum b/go.sum
index 351edaef..824ec4a3 100644
--- a/go.sum
+++ b/go.sum
@@ -40,8 +40,8 @@ github.com/kr/pty v1.1.1/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ=
 github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI=
 github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY=
 github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE=
-github.com/pjbgf/sha1cd v0.3.2 h1:a9wb0bp1oC2TGwStyn0Umc/IGKQnEgF0vVaZ8QF8eo4=
-github.com/pjbgf/sha1cd v0.3.2/go.mod h1:zQWigSxVmsHEZow5qaLtPYxpcKMMQpa09ixqBxuCS6A=
+github.com/pjbgf/sha1cd v0.4.0 h1:NXzbL1RvjTUi6kgYZCX3fPwwl27Q1LJndxtUDVfJGRY=
+github.com/pjbgf/sha1cd v0.4.0/go.mod h1:zQWigSxVmsHEZow5qaLtPYxpcKMMQpa09ixqBxuCS6A=
 github.com/pkg/diff v0.0.0-20210226163009-20ebb0f2a09e/go.mod h1:pJLUxLENpZxwdsKMEsNbx1VGcRFpLqf3715MtcvvzbA=
 github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
 github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
diff --git a/plumbing/transport/pack.go b/plumbing/transport/pack.go
index 9937fa87..0e3cbd3c 100644
--- a/plumbing/transport/pack.go
+++ b/plumbing/transport/pack.go
@@ -95,6 +95,7 @@ func (p *PackSession) Handshake(ctx context.Context, service Service, params ...
 	defer func() { checkError(c.stderr(), &err) }()
 
 	if err := cmd.Start(); err != nil {
+		_ = cmd.Close()
 		return nil, err
 	}
 
diff --git a/repository.go b/repository.go
index dae956f0..d584fcda 100644
--- a/repository.go
+++ b/repository.go
@@ -17,7 +17,6 @@ import (
 	"github.com/ProtonMail/go-crypto/openpgp"
 	"github.com/go-git/go-billy/v6"
 	"github.com/go-git/go-billy/v6/osfs"
-	"github.com/go-git/go-billy/v6/util"
 	"github.com/go-git/go-git/v6/config"
 	"github.com/go-git/go-git/v6/internal/path_util"
 	"github.com/go-git/go-git/v6/internal/revision"
@@ -53,7 +52,6 @@ var (
 	ErrInvalidReference            = errors.New("invalid reference, should be a tag or a branch")
 	ErrRepositoryNotExists         = errors.New("repository does not exist")
 	ErrRepositoryIncomplete        = errors.New("repository's commondir path does not exist")
-	ErrRepositoryAlreadyExists     = errors.New("repository already exists")
 	ErrRemoteNotFound              = errors.New("remote not found")
 	ErrRemoteExists                = errors.New("remote already exists")
 	ErrAnonymousRemoteName         = errors.New("anonymous remote name must be 'anonymous'")
@@ -65,6 +63,7 @@ var (
 	ErrAlternatePathNotSupported   = errors.New("alternate path must use the file scheme")
 	ErrUnsupportedMergeStrategy    = errors.New("unsupported merge strategy")
 	ErrFastForwardMergeNotPossible = errors.New("not possible to fast-forward merge changes")
+	ErrTargetDirNotEmpty           = errors.New("destination path already exists and is not empty")
 )
 
 // Repository represents a git repository
@@ -115,7 +114,7 @@ func WithObjectFormat(of formatcfg.ObjectFormat) InitOption {
 
 // 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
+// the given storer is not empty ErrTargetDirNotEmpty is returned
 func Init(s storage.Storer, opts ...InitOption) (*Repository, error) {
 	options := newInitOptions()
 	for _, oFn := range opts {
@@ -137,7 +136,7 @@ func Init(s storage.Storer, opts ...InitOption) (*Repository, error) {
 	switch err {
 	case plumbing.ErrReferenceNotFound:
 	case nil:
-		return nil, ErrRepositoryAlreadyExists
+		return nil, ErrTargetDirNotEmpty
 	default:
 		return nil, err
 	}
@@ -244,14 +243,14 @@ func Open(s storage.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.
+// storer is not empty ErrTargetDirNotEmpty is returned.
 func Clone(s storage.Storer, worktree billy.Filesystem, o *CloneOptions) (*Repository, error) {
 	return CloneContext(context.Background(), s, worktree, o)
 }
 
 // CloneContext 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.
+// given storer is not empty ErrTargetDirNotEmpty is returned.
 //
 // The provided Context must be non-nil. If the context expires before the
 // operation is complete, an error is returned. The context only affects the
@@ -280,7 +279,7 @@ func CloneContext(
 
 // PlainInit create an empty git repository at the given path. isBare defines
 // if the repository will have worktree (non-bare) or not (bare), if the path
-// is not empty ErrRepositoryAlreadyExists is returned.
+// is not empty ErrTargetDirNotEmpty is returned.
 func PlainInit(path string, isBare bool, options ...InitOption) (*Repository, error) {
 	var wt, dot billy.Filesystem
 	var initFn func(s *filesystem.Storage) (*Repository, error)
@@ -306,7 +305,6 @@ func PlainInit(path string, isBare bool, options ...InitOption) (*Repository, er
 	}
 	s := filesystem.NewStorage(dot, cache.NewObjectLRUDefault())
 	r, err := initFn(s)
-
 	if err != nil {
 		return nil, err
 	}
@@ -491,21 +489,26 @@ func dotGitCommonDirectory(fs billy.Filesystem) (commonDir billy.Filesystem, err
 
 // PlainClone a repository into the path with the given options, isBare defines
 // if the new repository will be bare or normal. If the path is not empty
-// ErrRepositoryAlreadyExists is returned.
+// ErrTargetDirNotEmpty is returned.
 func PlainClone(path string, o *CloneOptions) (*Repository, error) {
 	return PlainCloneContext(context.Background(), path, o)
 }
 
 // PlainCloneContext a repository into the path with the given options, isBare
 // defines if the new repository will be bare or normal. If the path is not empty
-// ErrRepositoryAlreadyExists is returned.
+// ErrTargetDirNotEmpty is returned.
 //
 // The provided Context must be non-nil. If the context expires before the
 // operation is complete, an error is returned. The context only affects the
 // transport operations.
-//
-// TODO(smola): refuse upfront to clone on a non-empty directory in v5, see #1027
 func PlainCloneContext(ctx context.Context, path string, o *CloneOptions) (*Repository, error) {
+	empty, err := checkTargetDirIsEmpty(path)
+	if err != nil {
+		return nil, err
+	}
+	if !empty {
+		return nil, fmt.Errorf("%w %s", ErrTargetDirNotEmpty, path)
+	}
 	start := time.Now()
 	defer func() {
 		url := ""
@@ -515,11 +518,6 @@ func PlainCloneContext(ctx context.Context, path string, o *CloneOptions) (*Repo
 		trace.Performance.Printf("performance: %.9f s: git command: git clone %s", time.Since(start).Seconds(), url)
 	}()
 
-	cleanup, cleanupParent, err := checkIfCleanupIsNeeded(path)
-	if err != nil {
-		return nil, err
-	}
-
 	isBare := o.Bare
 	if o.Mirror {
 		isBare = true
@@ -530,12 +528,6 @@ func PlainCloneContext(ctx context.Context, path string, o *CloneOptions) (*Repo
 	}
 
 	err = r.clone(ctx, o)
-	if err != nil && err != ErrRepositoryAlreadyExists {
-		if cleanup {
-			_ = cleanUpDir(path, cleanupParent)
-		}
-	}
-
 	return r, err
 }
 
@@ -547,49 +539,30 @@ func newRepository(s storage.Storer, worktree billy.Filesystem) *Repository {
 	}
 }
 
-func checkIfCleanupIsNeeded(path string) (cleanup bool, cleanParent bool, err error) {
+func checkTargetDirIsEmpty(path string) (empty bool, err error) {
 	fi, err := osfs.Default.Stat(path)
 	if err != nil {
 		if os.IsNotExist(err) {
-			return true, true, nil
+			return true, nil
 		}
 
-		return false, false, err
+		return false, err
 	}
 
 	if !fi.IsDir() {
-		return false, false, fmt.Errorf("path is not a directory: %s", path)
+		return false, fmt.Errorf("path is not a directory: %s", path)
 	}
 
 	files, err := osfs.Default.ReadDir(path)
 	if err != nil {
-		return false, false, err
+		return false, err
 	}
 
 	if len(files) == 0 {
-		return true, false, nil
-	}
-
-	return false, false, nil
-}
-
-func cleanUpDir(path string, all bool) error {
-	if all {
-		return util.RemoveAll(osfs.Default, path)
-	}
-
-	files, err := osfs.Default.ReadDir(path)
-	if err != nil {
-		return err
-	}
-
-	for _, fi := range files {
-		if err := util.RemoveAll(osfs.Default, osfs.Default.Join(path, fi.Name())); err != nil {
-			return err
-		}
+		return true, nil
 	}
 
-	return err
+	return false, nil
 }
 
 // Config return the repository config. In a filesystem backed repository this



Test patch:
diff --git a/plumbing/transport/pack_test.go b/plumbing/transport/pack_test.go
new file mode 100644
index 00000000..4c7f4f89
--- /dev/null
+++ b/plumbing/transport/pack_test.go
@@ -0,0 +1,71 @@
+package transport
+
+import (
+	"bytes"
+	"context"
+	"io"
+	"testing"
+
+	"github.com/go-git/go-git/v6/utils/ioutil"
+	"github.com/stretchr/testify/suite"
+)
+
+func TestCmdStartEOFSuite(t *testing.T) {
+	suite.Run(t, new(CmdStartEOFSuite))
+}
+
+type CmdStartEOFSuite struct {
+	suite.Suite
+}
+
+type mockStartEOFCommand struct {
+	stdin         bytes.Buffer
+	stdout        bytes.Buffer
+	stderr        bytes.Buffer
+	sessionOpened bool
+}
+
+func (c *mockStartEOFCommand) StderrPipe() (io.Reader, error) {
+	return &c.stderr, nil
+}
+
+func (c *mockStartEOFCommand) StdinPipe() (io.WriteCloser, error) {
+	return ioutil.WriteNopCloser(&c.stdin), nil
+}
+
+func (c *mockStartEOFCommand) StdoutPipe() (io.Reader, error) {
+	return &c.stdout, nil
+}
+
+func (c *mockStartEOFCommand) Start() error {
+	c.sessionOpened = true
+	return io.EOF
+}
+
+func (c *mockStartEOFCommand) Close() error {
+	c.sessionOpened = false
+	return nil
+}
+
+type mockStartEOFCommander struct {
+	mockCmd *mockStartEOFCommand
+}
+
+func (c *mockStartEOFCommander) Command(_ context.Context, cmd string, ep *Endpoint, auth AuthMethod, _ ...string) (Command, error) {
+	c.mockCmd = &mockStartEOFCommand{}
+	return c.mockCmd, nil
+}
+
+func (s *CmdStartEOFSuite) TestCmdStartEOFConnectionLeakError() {
+	client := NewPackTransport(&mockStartEOFCommander{})
+	sess, err := client.NewSession(nil, nil, nil)
+	if err != nil {
+		s.T().Fatalf("unexpected error: %s", err)
+	}
+
+	_, err = sess.Handshake(context.TODO(), UploadPackService)
+	s.ErrorIs(err, io.EOF)
+	cmdrInterface := sess.(*PackSession).cmdr
+	cmdr := cmdrInterface.(*mockStartEOFCommander)
+	s.False(cmdr.mockCmd.sessionOpened)
+}
diff --git a/repository_test.go b/repository_test.go
index d7738051..b566f3a7 100644
--- a/repository_test.go
+++ b/repository_test.go
@@ -180,7 +180,7 @@ func (s *RepositorySuite) TestInitAlreadyExists() {
 	s.NotNil(r)
 
 	r, err = Init(st)
-	s.ErrorIs(err, ErrRepositoryAlreadyExists)
+	s.ErrorIs(err, ErrTargetDirNotEmpty)
 	s.Nil(r)
 }
 
@@ -684,7 +684,7 @@ func (s *RepositorySuite) TestPlainInitAlreadyExists() {
 	s.NotNil(r)
 
 	r, err = PlainInit(dir, true)
-	s.ErrorIs(err, ErrRepositoryAlreadyExists)
+	s.ErrorIs(err, ErrTargetDirNotEmpty)
 	s.Nil(r)
 }
 
@@ -1044,7 +1044,7 @@ func (s *RepositorySuite) TestPlainCloneOverExistingGitDirectory() {
 		URL: s.GetBasicLocalRepositoryURL(),
 	})
 	s.Nil(r)
-	s.ErrorIs(err, ErrRepositoryAlreadyExists)
+	s.ErrorIs(err, ErrTargetDirNotEmpty)
 }
 
 func (s *RepositorySuite) TestPlainCloneContextCancel() {
@@ -1132,7 +1132,7 @@ func (s *RepositorySuite) TestPlainCloneContextNonExistentWithNotDir() {
 	s.False(fi.IsDir())
 }
 
-func (s *RepositorySuite) TestPlainCloneContextNonExistentWithNotEmptyDir() {
+func (s *RepositorySuite) TestPlainCloneContextFailedClonePreservesExistingDir() {
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 
@@ -1152,8 +1152,8 @@ func (s *RepositorySuite) TestPlainCloneContextNonExistentWithNotEmptyDir() {
 	r, err := PlainCloneContext(ctx, fs.Join(fs.Root(), repoDir), &CloneOptions{
 		URL: "incorrectOnPurpose",
 	})
-	s.NotNil(r)
-	s.ErrorIs(err, transport.ErrRepositoryNotFound)
+	s.Nil(r)
+	s.ErrorIs(err, ErrTargetDirNotEmpty)
 
 	_, err = fs.Stat(dummyFile)
 	s.NoError(err)
@@ -1174,7 +1174,7 @@ func (s *RepositorySuite) TestPlainCloneContextNonExistingOverExistingGitDirecto
 		URL: "incorrectOnPurpose",
 	})
 	s.Nil(r)
-	s.ErrorIs(err, ErrRepositoryAlreadyExists)
+	s.ErrorIs(err, ErrTargetDirNotEmpty)
 }
 
 func (s *RepositorySuite) TestPlainCloneWithRecurseSubmodules() {
@@ -1301,8 +1301,8 @@ func (s *RepositorySuite) TestFetchWithFilters() {
 		Filter: packp.FilterBlobNone(),
 	})
 	s.ErrorIs(err, transport.ErrFilterNotSupported)
-
 }
+
 func (s *RepositorySuite) TestFetchWithFiltersReal() {
 	r, _ := Init(memory.NewStorage())
 	_, err := r.CreateRemote(&config.RemoteConfig{
@@ -1317,8 +1317,8 @@ func (s *RepositorySuite) TestFetchWithFiltersReal() {
 	blob, err := r.BlobObject(plumbing.NewHash("9a48f23120e880dfbe41f7c9b7b708e9ee62a492"))
 	s.NotNil(err)
 	s.Nil(blob)
-
 }
+
 func (s *RepositorySuite) TestCloneWithProgress() {
 	s.T().Skip("Currently, go-git server-side implementation does not support writing" +
 		"progress and sideband messages to the client. This means any tests that" +