Improvements to git_transport extensibility

git_remote_set_transport now takes a transport factory rather than a transport
git_clone_options now allows the caller to specify a remote creation callback

10 files changed, 177 insertions, 68 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 561df946a..3938dcbcf 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -12,3 +12,16 @@ v0.21 + 1
 
 * LF -> CRLF filter now runs when * text = auto (with Git for Windows 1.9.4)
 
+* The git_remote_set_transport function now sets a transport factory function,
+  rather than a pre-existing transport instance.
+
+* The git_clone_options struct no longer provides the ignore_cert_errors or
+  remote_name members for remote customization.
+
+  Instead, the git_clone_options struct has two new members, remote_cb and
+  remote_cb_payload, which allow the caller to completely override the remote
+  creation process. If needed, the caller can use this callback to give their
+  remote a name other than the default (origin) or disable cert checking.
+
+  The remote_callbacks member has been preserved for convenience, although it
+  is not used when a remote creation callback is supplied.
diff --git a/include/git2/clone.h b/include/git2/clone.h
index 05b7522ce..c07928add 100644
--- a/include/git2/clone.h
+++ b/include/git2/clone.h
@@ -12,6 +12,7 @@
 #include "indexer.h"
 #include "checkout.h"
 #include "remote.h"
+#include "transport.h"
 
 
 /**
@@ -52,6 +53,27 @@ typedef enum {
 } git_clone_local_t;
 
 /**
+ * The signature of a function matching git_remote_create, with an additional
+ * void* as a callback payload.
+ *
+ * Callers of git_clone may provide a function matching this signature to override
+ * the remote creation and customization process during a clone operation.
+ *
+ * @param out the resulting remote
+ * @param repo the repository in which to create the remote
+ * @param name the remote's name
+ * @param url the remote's url
+ * @param payload an opaque payload
+ * @return 0, GIT_EINVALIDSPEC, GIT_EEXISTS or an error code
+ */
+typedef int (*git_remote_create_cb)(
+	git_remote **out,
+	git_repository *repo,
+	const char *name,
+	const char *url,
+	void *payload);
+
+/**
  * Clone options structure
  *
  * Use the GIT_CLONE_OPTIONS_INIT to get the default settings, like this:
@@ -72,7 +94,11 @@ typedef struct git_clone_options {
 	git_checkout_options checkout_opts;
 
 	/**
-	 * Callbacks to use for reporting fetch progress.
+	 * Callbacks to use for reporting fetch progress, and for acquiring
+	 * credentials in the event they are needed. This parameter is ignored if
+	 * the remote_cb parameter is set; if you provide a remote creation
+	 * callback, then you have the opportunity to configure remote callbacks in
+	 * provided function.
 	 */
 	git_remote_callbacks remote_callbacks;
 
@@ -83,23 +109,11 @@ typedef struct git_clone_options {
 	int bare;
 
 	/**
-	 * Set to 1 if errors validating the remote host's certificate
-	 * should be ignored.
-	 */
-	int ignore_cert_errors;
-
-	/**
 	 * Whether to use a fetch or copy the object database.
 	 */
 	git_clone_local_t local;
 
 	/**
-	 * The name to be given to the remote that will be
-	 * created. The default is "origin".
-	 */
-	const char *remote_name;
-
-	/**
 	 * The name of the branch to checkout. NULL means use the
 	 * remote's default branch.
 	 */
@@ -110,6 +124,20 @@ typedef struct git_clone_options {
 	 * use the default signature using the config.
 	 */
 	git_signature *signature;
+
+	/**
+	 * A callback used to create the git_remote, prior to its being
+	 * used to perform the clone operation. See the documentation for
+	 * git_remote_create_cb for details. This parameter may be NULL,
+	 * indicating that git_clone should provide default behavior.
+	 */
+	git_remote_create_cb remote_cb;
+
+	/**
+	 * An opaque payload to pass to the git_remote creation callback.
+	 * This parameter is ignored unless remote_cb is non-NULL.
+	 */
+	void *remote_cb_payload;
 } git_clone_options;
 
 #define GIT_CLONE_OPTIONS_VERSION 1
diff --git a/include/git2/remote.h b/include/git2/remote.h
index c72c9c8cc..c8b6ac97a 100644
--- a/include/git2/remote.h
+++ b/include/git2/remote.h
@@ -419,20 +419,19 @@ GIT_EXTERN(int) git_remote_list(git_strarray *out, git_repository *repo);
 GIT_EXTERN(void) git_remote_check_cert(git_remote *remote, int check);
 
 /**
- * Sets a custom transport for the remote. The caller can use this function
- * to bypass the automatic discovery of a transport by URL scheme (i.e.
- * http://, https://, git://) and supply their own transport to be used
- * instead. After providing the transport to a remote using this function,
- * the transport object belongs exclusively to that remote, and the remote will
- * free it when it is freed with git_remote_free.
+ * Sets a custom transport factory for the remote. The caller can use this
+ * function to override the transport used for this remote when performing
+ * network operations.
  *
  * @param remote the remote to configure
- * @param transport the transport object for the remote to use
+ * @param transport_cb the function to use to create a transport
+ * @param payload opaque parameter passed to transport_cb
  * @return 0 or an error code
  */
 GIT_EXTERN(int) git_remote_set_transport(
 	git_remote *remote,
-	git_transport *transport);
+	git_transport_cb transport_cb,
+	void *payload);
 
 /**
  * Argument to the completion callback which tells it which operation
diff --git a/src/clone.c b/src/clone.c
index 6c4fb6727..a4ed1a29c 100644
--- a/src/clone.c
+++ b/src/clone.c
@@ -229,6 +229,22 @@ cleanup:
 	return retcode;
 }
 
+static int default_remote_create(
+		git_remote **out,
+		git_repository *repo,
+		const char *name,
+		const char *url,
+		void *payload)
+{
+	int error;
+	git_remote_callbacks *callbacks = payload;
+
+	if ((error = git_remote_create(out, repo, name, url)) < 0)
+		return error;
+
+	return git_remote_set_callbacks(*out, callbacks);
+}
+
 /*
  * submodules?
  */
@@ -241,8 +257,9 @@ static int create_and_configure_origin(
 {
 	int error;
 	git_remote *origin = NULL;
-	const char *name;
 	char buf[GIT_PATH_MAX];
+	git_remote_create_cb remote_create = options->remote_cb;
+	void *payload = options->remote_cb_payload;
 
 	/* If the path exists and is a dir, the url should be the absolute path */
 	if (git_path_root(url) < 0 && git_path_exists(url) && git_path_isdir(url)) {
@@ -252,14 +269,12 @@ static int create_and_configure_origin(
 		url = buf;
 	}
 
-	name = options->remote_name ? options->remote_name : "origin";
-	if ((error = git_remote_create(&origin, repo, name, url)) < 0)
-		goto on_error;
-
-	if (options->ignore_cert_errors)
-		git_remote_check_cert(origin, 0);
+	if (!remote_create) {
+		remote_create = default_remote_create;
+		payload = (void *)&options->remote_callbacks;
+	}
 
-	if ((error = git_remote_set_callbacks(origin, &options->remote_callbacks)) < 0)
+	if ((error = remote_create(&origin, repo, "origin", url, payload)) < 0)
 		goto on_error;
 
 	if ((error = git_remote_save(origin)) < 0)
diff --git a/src/remote.c b/src/remote.c
index 47b61b1b1..e9dafa0ea 100644
--- a/src/remote.c
+++ b/src/remote.c
@@ -267,9 +267,11 @@ int git_remote_dup(git_remote **dest, git_remote *source)
 
 	if (source->pushurl != NULL) {
 		remote->pushurl = git__strdup(source->pushurl);
-		GITERR_CHECK_ALLOC(remote->pushurl);		
+		GITERR_CHECK_ALLOC(remote->pushurl);
 	}
 
+	remote->transport_cb = source->transport_cb;
+	remote->transport_cb_payload = source->transport_cb_payload;
 	remote->repo = source->repo;
 	remote->download_tags = source->download_tags;
 	remote->check_cert = source->check_cert;
@@ -659,8 +661,14 @@ int git_remote_connect(git_remote *remote, git_direction direction)
 		return -1;
 	}
 
-	/* A transport could have been supplied in advance with
-	 * git_remote_set_transport */
+	/* If we don't have a transport object yet, and the caller specified a
+	 * custom transport factory, use that */
+	if (!t && remote->transport_cb &&
+		(error = remote->transport_cb(&t, remote, remote->transport_cb_payload)) < 0)
+		return error;
+
+	/* If we still don't have a transport, then use the global
+	 * transport registrations which map URI schemes to transport factories */
 	if (!t && (error = git_transport_new(&t, remote, url)) < 0)
 		return error;
 
@@ -1262,18 +1270,20 @@ const git_remote_callbacks *git_remote_get_callbacks(git_remote *remote)
 	return &remote->callbacks;
 }
 
-int git_remote_set_transport(git_remote *remote, git_transport *transport)
+int git_remote_set_transport(
+	git_remote *remote,
+	git_transport_cb transport_cb,
+	void *payload)
 {
-	assert(remote && transport);
-
-	GITERR_CHECK_VERSION(transport, GIT_TRANSPORT_VERSION, "git_transport");
+	assert(remote);
 
 	if (remote->transport) {
 		giterr_set(GITERR_NET, "A transport is already bound to this remote");
 		return -1;
 	}
 
-	remote->transport = transport;
+	remote->transport_cb = transport_cb;
+	remote->transport_cb_payload = payload;
 	return 0;
 }
 
diff --git a/src/remote.h b/src/remote.h
index 4164a14b3..47c4f7221 100644
--- a/src/remote.h
+++ b/src/remote.h
@@ -22,6 +22,8 @@ struct git_remote {
 	git_vector refs;
 	git_vector refspecs;
 	git_vector active_refspecs;
+	git_transport_cb transport_cb;
+	void *transport_cb_payload;
 	git_transport *transport;
 	git_repository *repo;
 	git_remote_callbacks callbacks;
diff --git a/src/transport.c b/src/transport.c
index 2194b1864..fbcda5a53 100644
--- a/src/transport.c
+++ b/src/transport.c
@@ -133,10 +133,11 @@ int git_transport_new(git_transport **out, git_remote *owner, const char *url)
 		return -1;
 	}
 
-	error = fn(&transport, owner, param);
-	if (error < 0)
+	if ((error = fn(&transport, owner, param)) < 0)
 		return error;
 
+	GITERR_CHECK_VERSION(transport, GIT_TRANSPORT_VERSION, "git_transport");
+
 	*out = transport;
 
 	return 0;
diff --git a/tests/clone/nonetwork.c b/tests/clone/nonetwork.c
index 4bdc6e13b..ab3e8f50d 100644
--- a/tests/clone/nonetwork.c
+++ b/tests/clone/nonetwork.c
@@ -110,12 +110,25 @@ void test_clone_nonetwork__fail_with_already_existing_but_non_empty_directory(vo
 	cl_git_fail(git_clone(&g_repo, cl_git_fixture_url("testrepo.git"), "./foo", &g_options));
 }
 
+int custom_origin_name_remote_create(
+	git_remote **out,
+	git_repository *repo,
+	const char *name,
+	const char *url,
+	void *payload)
+{
+	GIT_UNUSED(name);
+	GIT_UNUSED(payload);
+
+	return git_remote_create(out, repo, "my_origin", url);
+}
+
 void test_clone_nonetwork__custom_origin_name(void)
 {
-       g_options.remote_name = "my_origin";
-       cl_git_pass(git_clone(&g_repo, cl_git_fixture_url("testrepo.git"), "./foo", &g_options));
+	g_options.remote_cb = custom_origin_name_remote_create;
+	cl_git_pass(git_clone(&g_repo, cl_git_fixture_url("testrepo.git"), "./foo", &g_options));
 
-       cl_git_pass(git_remote_load(&g_remote, g_repo, "my_origin"));
+	cl_git_pass(git_remote_load(&g_remote, g_repo, "my_origin"));
 }
 
 void test_clone_nonetwork__defaults(void)
diff --git a/tests/clone/transport.c b/tests/clone/transport.c
new file mode 100644
index 000000000..27568f228
--- /dev/null
+++ b/tests/clone/transport.c
@@ -0,0 +1,50 @@
+#include "clar_libgit2.h"
+
+#include "git2/clone.h"
+#include "git2/transport.h"
+#include "fileops.h"
+
+static int custom_transport(
+	git_transport **out,
+	git_remote *owner,
+	void *payload)
+{
+	*((int*)payload) = 1;
+
+	return git_transport_local(out, owner, payload);
+}
+
+static int custom_transport_remote_create(
+	git_remote **out,
+	git_repository *repo,
+	const char *name,
+	const char *url,
+	void *payload)
+{
+	int error;
+
+	if ((error = git_remote_create(out, repo, name, url)) < 0)
+		return error;
+
+	if ((error = git_remote_set_transport(*out, custom_transport, payload)) < 0)
+		return error;
+
+	return 0;
+}
+
+void test_clone_transport__custom_transport(void)
+{
+	git_repository *repo;
+	git_clone_options clone_opts = GIT_CLONE_OPTIONS_INIT;
+	int custom_transport_used = 0;
+
+	clone_opts.remote_cb = custom_transport_remote_create;
+	clone_opts.remote_cb_payload = &custom_transport_used;
+
+	cl_git_pass(git_clone(&repo, cl_fixture("testrepo.git"), "./custom_transport.git", &clone_opts));
+	git_repository_free(repo);
+
+	cl_git_pass(git_futils_rmdir_r("./custom_transport.git", NULL, GIT_RMDIR_REMOVE_FILES));
+
+	cl_assert(custom_transport_used == 1);
+}
diff --git a/tests/network/remote/remotes.c b/tests/network/remote/remotes.c
index 333b52a5b..21c57119a 100644
--- a/tests/network/remote/remotes.c
+++ b/tests/network/remote/remotes.c
@@ -72,18 +72,17 @@ void test_network_remote_remotes__error_when_not_found(void)
 void test_network_remote_remotes__error_when_no_push_available(void)
 {
 	git_remote *r;
-	git_transport *t;
 	git_push *p;
 
 	cl_git_pass(git_remote_create_anonymous(&r, _repo, cl_fixture("testrepo.git"), NULL));
 
-	cl_git_pass(git_transport_local(&t,r,NULL));
+	cl_git_pass(git_remote_set_transport(r, git_transport_local, NULL));
+
+	cl_git_pass(git_remote_connect(r, GIT_DIRECTION_PUSH));
 
 	/* Make sure that push is really not available */
-	t->push = NULL;
-	cl_git_pass(git_remote_set_transport(r, t));
+	r->transport->push = NULL;
 
-	cl_git_pass(git_remote_connect(r, GIT_DIRECTION_PUSH));
 	cl_git_pass(git_push_new(&p, r));
 	cl_git_pass(git_push_add_refspec(p, "refs/heads/master"));
 	cl_git_fail_with(git_push_finish(p), GIT_ERROR);
@@ -438,27 +437,6 @@ void test_network_remote_remotes__returns_ENOTFOUND_when_neither_url_nor_pushurl
 		git_remote_load(&remote, _repo, "no-remote-url"), GIT_ENOTFOUND);
 }
 
-void test_network_remote_remotes__check_structure_version(void)
-{
-	git_transport transport = GIT_TRANSPORT_INIT;
-	const git_error *err;
-
-	git_remote_free(_remote);
-	_remote = NULL;
-	cl_git_pass(git_remote_create_anonymous(&_remote, _repo, "test-protocol://localhost", NULL));
-
-	transport.version = 0;
-	cl_git_fail(git_remote_set_transport(_remote, &transport));
-	err = giterr_last();
-	cl_assert_equal_i(GITERR_INVALID, err->klass);
-
-	giterr_clear();
-	transport.version = 1024;
-	cl_git_fail(git_remote_set_transport(_remote, &transport));
-	err = giterr_last();
-	cl_assert_equal_i(GITERR_INVALID, err->klass);
-}
-
 void assert_cannot_create_remote(const char *name, int expected_error)
 {
 	git_remote *remote = NULL;