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

Adding usage of git_create_remote_detached #755

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ralucas
Copy link

@ralucas ralucas commented Mar 2, 2021

This creates a RemoteDetached type that is instantiable with a remote uri string. It has only one method, LsDetached for emulating git ls-remote <remote-uri>.

The reason for this is that it looks like there is no other way to simply do a git ls-remote type request without creating a repository reference locally.

Thank you for your time in reviewing this.

@ralucas ralucas force-pushed the remote-detached branch 3 times, most recently from f61dc59 to db7fcff Compare March 2, 2021 21:47
@ralucas ralucas changed the title initial commit on adding usage of git_create_remote_detached Adding usage of git_create_remote_detached Mar 3, 2021
Copy link
Contributor

@lhchavez lhchavez left a comment

Choose a reason for hiding this comment

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

thanks for the change!

}

func TestRemoteDetached_LsDetached(t *testing.T) {
r, _ := NewRemoteDetached(TestGitRepoUri)
Copy link
Contributor

@lhchavez lhchavez Mar 8, 2021

Choose a reason for hiding this comment

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

Suggested change
r, _ := NewRemoteDetached(TestGitRepoUri)
remote, err := NewRemoteDetached(TestGitRepoUri)
checkFatal(t, err)
defer remote.Free()

for consistency with other tests that create remotes (and to avoid memory leaks)

return nil, MakeGitError(ret)
}

return &RemoteDetached{r: Remote{ptr: ptr}}, nil
Copy link
Contributor

@lhchavez lhchavez Mar 8, 2021

Choose a reason for hiding this comment

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

would it be possible for this to return the *Remote directly? That way the RemoteDetached and the LsDetached shouldn't be needed, since callers can interact with the remote using the normal API, which will avoid having to implement more APIs on RemoteDetached that would only forward arguments to the underlying Remote. It's totally fine if the repo property is nil: it's just there to avoid the GC from deleting the Repository when the Remote is still around, but here it will be nil.

also, the finalizer for the Remote object must be set prior to returning to avoid memory leaks:

git2go/remote.go

Lines 586 to 587 in 75708ee

runtime.SetFinalizer(remote, (*Remote).Free)
return remote, nil

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants