Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

git: add support for cloning empty repos #393

Merged
merged 1 commit into from
Nov 8, 2022
Merged

Conversation

aryan9600
Copy link
Member

Add support for cloning empty repos using go-git and libgit2.
go-git returns an error when cloning an empty repository directly, which is handled by using the advice provided here: go-git/go-git#118 (comment). libgit2 doesn't return an error and thus the change is much simpler.
The go-git package gets test coverage for this scenario but the libgit2 package does not because of the following weird behavior:

_, err := git2go.InitRepository(filepath.Join(server.Root(), emptyRepoPath), true)

err = l.remote.Fetch([]string{branch},
	&git2go.FetchOptions{
		DownloadTags:    git2go.DownloadTagsNone,
		RemoteCallbacks: remoteCallBacks,
	},
	"")
if err != nil {
	return nil, fmt.Errorf("unable to fetch remote '%s': %w", url, gitutil.LibGit2Error(err))
}

isEmpty, err := l.repository.IsEmpty()
fmt.Println(isEmpty) // prints false

To make sure the changes to the libgit2 package have the intended effect, it was tested (and confirmed to have the correct behavior) using empty repos on GitHub and GitLab.(same goes for the go-git package as well).
Signed-off-by: Sanskar Jaiswal [email protected]

@aryan9600 aryan9600 requested a review from pjbgf November 8, 2022 00:59
@pjbgf pjbgf added the area/git Git and SSH related issues and pull requests label Nov 8, 2022
@pjbgf pjbgf added this to the Bootstrap GA milestone Nov 8, 2022
git/gogit/clone.go Outdated Show resolved Hide resolved
Copy link
Member

@pjbgf pjbgf left a comment

Choose a reason for hiding this comment

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

LGTM

thanks @aryan9600! please rebase.

@hiddeco
Copy link
Member

hiddeco commented Feb 7, 2023

Wouldn't it have been better to return a typed error to avoid panics? This would be easy to ignore where applicable using errors.Is. For example:

c, err := r.Clone(context.Background(), "https://github.com/...")
if err != nil && !errors.Is(err, git.ErrEmptyRepository) {
	return err
}

if c == nil {
	// Do something to init repository...
}

// ...Continue

Where as the current solution results in unsafe default behavior, as has become apparent in fluxcd/source-controller#1021

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/git Git and SSH related issues and pull requests
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants