FetchOptions: add ability to specify ProxyOptions #623

2020-07-10T11:03:53-05:00

lollipopman commented

2020-07-10 11:03:53 -05:00

(Migrated from github.com)

Prior to this change you could not specifiy proxy options on the
FetchOptions struct, which made it impossible to specify a proxy for an
initial clone. This change adds the ProxyOptions to the FetchOptions
struct so you can go through a proxy when cloning.

Prior to this change you could not specifiy proxy options on the FetchOptions struct, which made it impossible to specify a proxy for an initial clone. This change adds the ProxyOptions to the FetchOptions struct so you can go through a proxy when cloning.

lhchavez (Migrated from github.com) reviewed 2020-07-10 14:27:54 -05:00

lhchavez (Migrated from github.com) left a comment

nice, just two small requests.

remote.go

lhchavez (Migrated from github.com) commented

2020-07-10 14:23:52 -05:00

it may be better to avoid embedding ProxyOptions, and also use a value instead of a pointer type since the zero value for ProxyOptions is well-defined and matches the semantics of not specifying it (also makes it consistent with all the other fields of FetchOptions):

	// ProxyOptions are the proxy options to use for this fetch operation.
	ProxyOptions ProxyOptions

it may be better to avoid [embedding](https://golang.org/doc/effective_go.html#embedding) `ProxyOptions`, and also use a value instead of a pointer type since the zero value for `ProxyOptions` is well-defined and matches the semantics of not specifying it (also makes it consistent with all the other fields of `FetchOptions`): ```suggestion // ProxyOptions are the proxy options to use for this fetch operation. ProxyOptions ProxyOptions ```

remote.go

lhchavez (Migrated from github.com) commented

2020-07-10 14:25:02 -05:00

both git_fetch_init_options() and populateProxyOptions() take care of this, so it can be safely omitted.

both `git_fetch_init_options()` and `populateProxyOptions()` take care of this, so it can be safely omitted.

lollipopman commented

2020-07-10 14:44:17 -05:00

(Migrated from github.com)

Thanks for the detailed feedback @lhchavez, I believe I have addressed both of your good suggestions, let me know if you what to see any additional changes, thanks!