Support git_remote_create_with_opts #733

2021-02-01T07:18:01-06:00

bc-lee commented

2021-02-01 07:18:01 -06:00

(Migrated from github.com)

Closes #645

lhchavez (Migrated from github.com) reviewed 2021-02-01 09:24:13 -06:00

lhchavez (Migrated from github.com) left a comment

nice, thanks for doing this!

remote.go

lhchavez (Migrated from github.com) commented

2021-02-01 09:01:45 -06:00

could this be

type RemoteCreateOptionsFlag uint

for consistency with https://godoc.org/github.com/libgit2/git2go#BlameOptionsFlag ? bonus points if a docstring is added.

could this be ```suggestion type RemoteCreateOptionsFlag uint ``` for consistency with https://godoc.org/github.com/libgit2/git2go#BlameOptionsFlag ? bonus points if a docstring is added.

lhchavez (Migrated from github.com) commented

2021-02-01 09:12:22 -06:00

it's probably better to not have this be passed in through the Options object, since the pointer to the C repository can be obtained from within CreateWithOptions, which has a strong affinity to a pre-existing git.Repository and would be confusing (API-wise) that one would be able to call myOneRepository.Remotes.CreateWithOptions(...) and the resulting remote would be associated with a repository that is not myOneRepository.

in order to support detached remotes, we could introduce func NewDetachedRemote(url string, options *RemoteCreateOptions) (*Remote, error).

```suggestion ``` it's probably better to not have this be passed in through the `Options` object, since the pointer to the C repository can be obtained from within `CreateWithOptions`, which has a strong affinity to a pre-existing `git.Repository` and would be confusing (API-wise) that one would be able to call `myOneRepository.Remotes.CreateWithOptions(...)` and the resulting remote would be associated with a repository that is not `myOneRepository`. in order to support detached remotes, we could introduce `func NewDetachedRemote(url string, options *RemoteCreateOptions) (*Remote, error)`.

remote.go

lhchavez (Migrated from github.com) commented

2021-02-01 09:13:30 -06:00

func (c *RemoteCollection) CreateWithOptions(url string, option *RemoteCreateOptions) (*Remote, error) {

for consistency with the _with_ part of the C function.

also, can there be a docstring for this function?

```suggestion func (c *RemoteCollection) CreateWithOptions(url string, option *RemoteCreateOptions) (*Remote, error) { ``` for consistency with the `_with_` part of the C function. also, can there be a docstring for this function?

lhchavez (Migrated from github.com) commented

2021-02-01 09:23:19 -06:00

	ret := C.git_remote_create_with_opts(&remote.ptr, curl, opts)
	runtime.KeepAlive(c.repo)

otherwise, the go runtime is completely free to free the repository under us while the Cgo call is being made.

```suggestion ret := C.git_remote_create_with_opts(&remote.ptr, curl, opts) runtime.KeepAlive(c.repo) ``` otherwise, the go runtime is completely free to free the repository under us while the Cgo call is being made.

remote.go

lhchavez (Migrated from github.com) commented

2021-02-01 09:18:25 -06:00

	return &RemoteCreateOptions{
		Flags:      RemoteCreateOptionsFlag(opts.flags),
	}, nil

otherwise opts is unused!

```suggestion return &RemoteCreateOptions{ Flags: RemoteCreateOptionsFlag(opts.flags), }, nil ``` otherwise `opts` is unused!

lhchavez (Migrated from github.com) commented

2021-02-01 09:21:07 -06:00

could this follow the populateXxxOptions convention found in other files? e.g. 4b2ac7c998/revert.go (L18)

could this follow the `populateXxxOptions` convention found in other files? e.g. https://github.com/libgit2/git2go/blob/4b2ac7c998be677d865367908787f17fb570c679/revert.go#L18

bc-lee (Migrated from github.com) reviewed 2021-02-03 08:27:05 -06:00

remote.go

bc-lee (Migrated from github.com) commented

2021-02-03 08:27:04 -06:00

In my opinion, NewDetachedRemote is not needed right now. That was my design mistake. (Due to a dead copy of the C interface).

In my opinion, `NewDetachedRemote` is not needed right now. That was my design mistake. (Due to a dead copy of the C interface).

lhchavez (Migrated from github.com) reviewed 2021-02-03 09:21:04 -06:00

lhchavez (Migrated from github.com) left a comment

just a few more style requests and we're good!

remote.go

lhchavez (Migrated from github.com) commented

2021-02-03 09:17:19 -06:00

func populateRemoteCreateOptions(copts *C.git_remote_create_options, opts *RemoteCreateOptionsrepository *Repository) *C.git_remote_create_options {

for consistency with all the other populateXxxOptions() functions.

```suggestion func populateRemoteCreateOptions(copts *C.git_remote_create_options, opts *RemoteCreateOptionsrepository *Repository) *C.git_remote_create_options { ``` for consistency with all the other `populateXxxOptions()` functions.

lhchavez (Migrated from github.com) commented

2021-02-03 09:18:40 -06:00